Hey Rowan,
On Mon, Mar 2, 2020 at 3:39 PM Rowan Tommins <rowan.coll...@gmail.com>
wrote:
> Hi Marco,
>
> On Mon, 2 Mar 2020 at 14:00, Marco Pivetta <ocram...@gmail.com> wrote:
>
> >
> > Overall against the RFC: `++` and `--` (prefix and suffix) are supposed
> to
> > be used with numeric values, not with other types, as that makes
> everything
> > only more confusing.
> >
>
>
> While I'm generally sympathetic to that principle, it could also be said of
> the + and - operators, which currently silently handle both nulls and
> booleans. I would be open to changing *all* arithmetic behaviour,
> particularly on booleans, but don't think a vague ambition for that
> justifies inconsistent behaviour between different arithmetic operators.
>
Yes, I'm indeed not suggesting to change them: my comment is explicitly
about turning the BC break (in this RFC) into an useful and *loud* BC break.
> > Code written (intentionally) to use `++` and `--` against non-numeric
> > values should **NOT** pass a code review, and I am sorry for those that
> > have to maintain it if that happens.
> >
>
>
> I think you're taking simplified examples too literally here, and assuming
> that all affected code would be obvious at a glance.
>
> Take for instance this innocent-looking patch:
>
> function foo($bar) {
> - $bar -= 1;
> + $bar--;
> // more code here
> }
>
> With the current behaviour, any inadvertent calls to the function passing
> null, true, false, or an array will change behaviour when this patch is
> merged. In particular, the variable $bar would previously have been
> guaranteed to be an integer, and will now retain the original type passed
> in.
>
> I completely agree that such code has a bug, but the difference in
> behaviour is still a massive WTF.
>
The code in the above example cannot be accepted without an `int` type
declaration, or an `(int)` cast (or similar number) applied before the
first arithmetic happens.
For older code that needs to respect BC due to LSP, a docblock type
declaration would suffice.
The code above is NOT going to pass a code review.
The asymmetry of ++ and -- is even more confusing. Consider this patch:
>
> function repeatThing($extraTimes) {
> - for ( $i = 0; $i <= $extraTimes ; $i ++ ) {
> + for ( $i = $extraTimes ; $i >= 0; $i -- ) {
> doThing();
> }
> }
>
> Again, this looks like a safe change - but if passed a value of NULL, the
> old code will call doThing() once, and the new code will enter an infinite
> loop.
>
Same as above: types are not optional, and `$extraTimes` is not usable in
its form.
This code cannot pass a code review either.
>
>
> > Changing the current behavior, regardless in which way, is a BC break:
> > might as well make the BC break useful:
> >
> > RFC Proposal: `$a = null; $a--; $a === -1`. Let's make this an explicit
> > TypeError instead.
> > RFC Proposal: `$a = null; --$a; $a === -1`. Let's make this an explicit
> > TypeError instead.
> >
>
>
> And what about `$a = null; $a++;` / `$a = null; ++$a;`, which currently
> has behaviour consistent with other arithmetic operations (in particular,
> is identical to $a+=1) and this RFC does not propose to change?
>
Leave unchanged. If a BC break is to be introduced, make it go with a bang
(throw error)
If we change the ++ case to throw a TypeError, would we also change null+1
> to throw one? And would we then also examine all the other operators and
> utility functions which silently coerce null to zero or an empty string?
>
Can also not change it: I'd throw an error, if I were to propose it, but
the BC break must bring active value.
Right now, psalm & phpstan catch these, luckily.
Marco Pivetta
http://twitter.com/Ocramius
http://ocramius.github.com/
