Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122549
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-205-g4dbcac4545-fm-20240301.001-g4dbcac45
Precedence: bulk
MIME-Version: 1.0
Message-ID: <f0fdace7-5526-4567-b8d9-15ab4b20c64b@app.fastmail.com>
In-Reply-To: <115C4A93-120E-45A4-85E2-D91649B66E70@koalephant.com>
References: <fca97d6c-d4ec-4000-958a-72c32beb8421@app.fastmail.com>
 <e0753ec0-0b73-42f6-95e6-e8e57d90ab5b@gmail.com>
 <59619244-917d-4936-8f21-2854840a9bf8@rwec.co.uk>
 <2299271f-50ea-48c1-81fb-b64fa10c9bbb@app.fastmail.com>
 <1204BFC3-B976-4FEE-BE01-E668699C84E2@koalephant.com>
 <f2ce7ebe-9faa-432b-9300-01c5e046478b@app.fastmail.com>
 <115C4A93-120E-45A4-85E2-D91649B66E70@koalephant.com>
Date: Fri, 01 Mar 2024 16:14:22 +0000
To: "php internals" <internals@lists.php.net>
Subject: Re: [PHP-DEV] [RFC[ Property accessor hooks, take 2
Content-Type: text/plain
From: larry@garfieldtech.com ("Larry Garfield")

On Fri, Mar 1, 2024, at 6:28 AM, Stephen Reay wrote:

>>>> == Re the $value variable in set
>> 
>> *snip*
>> 
>>>> So what makes the most sense to me is to keep $value optional, but IF you specify an alternate name, you must also specify a type (which may be wider).  So these are equivalent:
>>>> 
>>>> public int $foo { set (int $value) => $value + 1 }
>>>> public int $foo { set => $value + 1 }
>>>> 
>>>> And only those forms are legal.  But you could also do this, if the situation called for it:
>>>> 
>>>> public int $foo { set(int|float $num) => floor($num) + 1; }
>>>> 
>>>> This "all or nothing" approach seems like it strikes the best balance, gives the most flexibility where needed while still having the least redundancy when not needed, and when a name/type is provided, its behavior is the same as for a method being inherited.
>>>> 
>>>> Does that sound acceptable?  (Again, question for everyone.)
>>>> 
>>> 
>>> My only question with this is the same as I had in an earlier reply 
>>> (and I'm not sure it was ever answered directly?), and you allude to 
>>> this yourself: everywhere *else*, `($var)` means a parameter with type 
>>> `mixed`. Why is the type *required* here, when you've specifically said 
>>> you want to avoid boilerplate? If we're going to assume people can 
>>> understand that `(implicit property-type $value) is implicit, surely we 
>>> can also assume that they will understand "specifying a parameter 
>>> without a type" means the parameter has no type (i.e. is `mixed`).
>>> 
>>> Again, for myself I'd be likely to type it (or regular parameters, 
>>> properties, etc) as `mixed` if that's what I want *anyway*, but the 
>>> inconsistency here seems odd, unless there's some until-now unknown 
>>> drive to deprecate type-less parameters/properties/etc.
>> 
>> If we went this route, then an untyped set param would likely imply "mixed", just like on methods.  Which, since mixed is the super type of everything, would still technically work, but would weaken the type enforcement and thus static analysis potential.  (Just as a method param can be widened in a child class all the way to mixed/omitted, and it would be unwise for all the same reasons.)
>> 
>
> Having a `mixed` param in the set hook shouldn't weaken the actual 
> backing parameter though - when the hook writes to `$this->prop`, the 
> parent type is still enforced, *surely*? If not, why not?
>
> As for how static analysis tools handle this concept - I'd have thought 
> it's too early to suggest what static analysis tools will or won't 
> support given how much they already support based on less-formal syntax 
> like docblocks. It's *already* possible to have a property that is 
> reported (to static analysis  tools/IDEs/etc) as a fixed type, but 
> accepts a wider type on write, with the current mish-mash of typed 
> properties, docblocks, magic getters and setters, and the bizarre 
> `unset` behaviour. This is simply converting that into standardised 
> syntax. The RFC itself proposes a scenario where a wider type is 
> accepted in the set hook. I find it hard to believe that a static 
> analysis tool can model "this property is `Foo` but it accepts 
> `string|Foo` on write" but not "this property is `Foo` but it accepts 
> `mixed` on write". Heck if that's such a problem what is said tool 
> going to do when someone *explicitly* widens the parameter to `mixed` 
> in a set hook? 
>
> I would argue that if the language is providing better support for 
> typed properties by adding 'hooks' like this, the need for static 
> analysis of those specific parts reduces greatly - if someone wants to 
> accept `mixed` when storing as a string, and convert it in the hook, 
> and the language can **enforce those types at runtime**, why should 
> some hypothetical static analysis be a hangup for that? 
>
>> In the RFC as currently written, omitted means "derive from the property," which is a concept that doesn't exist in methods; the closest equivalent would be if omitting a type in a child method parameter meant "use the parent's type implicitly," which is not how that works right now.
>
> For the third time: I'm well aware of how parameter types work 
> everywhere else, and that's why I'm asking why the same behaviour isn't 
> being followed here? 
>
> - You've said you want to avoid boilerplate; and
> - You've said you would expect most people to just use the implicit 
> `same-type $value` parameter; and
> - You've acknowledged that the existing 'standard' is that a parameter 
> without a type is considered `mixed`; and
> - You've acknowledged in your RFC that there is a use-case for wanting 
> to accept a wider type than what a property stores internally.
>
> So why then is it so unacceptable that the existing standard be 
> followed, such that a set hook with an "untyped" parameter would be 
> treated as `mixed` as it is everywhere else? 
>
> Yes, I know you said "widening to `mixed` is unwise". I don't seem to 
> recall amongst all the type-related previous RFCs, any that suggested 
> that child parameters widening to `mixed` (either explicitly or 
> implicitly) should be deprecated, so I'm sorry but I don't see much 
> value in that argument.

I think we're talking past each other here.  I'm not saying that implicitly widening the type to mixed would break the world.  Just that it's not a good idea in most cases.

Example:

public UnicodeString $s1;

public UnicodeString $s2 { 
    set (UnicodeString|string $value) {
        $this->s2 = $value instanceof UnicodeString ? $value : new UnicodeString($value);
    }

public UnicodeString $s3 { 
    set (mixed $value) {
        $this->s3 = $value instanceof UnicodeString ? $value : new UnicodeString($value);
    }
}

// These are all fine, of course.
$foo->s1 = new UnicodeString('beep);
$foo->s2 = new UnicodeString('beep);
$foo->s3 = new UnicodeString('beep);

// These would also be allowed by type widening:
$foo->s2 = 'beep';
$foo->s3 = 'beep';

// This would break with a type error.  SA tools could detect it right on this line and flag it.
$foo->s2 = new User();

// This would break with a type error *inside* the set body, when calling `new UnicodeString($value)`
$foo->s3 = new User();

To an SA tool's point of view, the last example is valid, even though it will certainly break.  In the worst case, it would type error when the set hook completes, so it has the same effect at runtime.  But because the type information is weaker, SA tools can do less.  The same is true for a method with a `mixed` param type: SA tools can't tell you at the call-site if it's going to be a problem or not, and where the error happens is less predictable.

Of course, if someone really wants to widen the set type to `mixed` and their code can handle it, go for it.  I've no problem there.  My issue is with designing a syntax such that the set type implicitly gets widened to mixed often.

public UnicodeString $s4 { 
    set ($name) {
        $this->s4 = $name instanceof UnicodeString ? $name : new UnicodeString($name);
    }
}

It would be very unexpected to me to now need to handle non-string|UnicodeString values of $name, or for my IDE to not flag when I try to assign it to a User object.  That's my concern.  (A concern I didn't realize was there until this thread, so I'm glad it was pointed out.)  On the flipside, it's probably very unexpected for someone else for it to *not* implicitly mean mixed, the way it does for method parameters.  Which unexpected is right?

Neither and both, I think.  Hence why I think "if you set anything, you have to set both, but you can also omit both" is the best solution: There's a very clear and self-evident default behavior for the vast-majority case, and if you need a non-default behavior, the resulting code is fully self-documenting and allows SA tools to flag issues at the callsite.

The next-best alternative is to punt on both widening and custom variable names entirely and go with the C# model: The variable is called $value, always, you can't change it, deal.  That leaves the syntax open to readd both in the future.  I'd prefer to include both features now, but I'm OK with punting on both for now.  I do think they have to come together, though, to avoid the unexpectedness described above.

--Larry Garfield