Re: [RFC] Increment/Decrement Fixes

Hey Rowan, On Mon, Mar 2, 2020 at 3:39 PM Rowan Tommins <rowan.collins@gmail.com> wrote: > Hi Marco, > > On Mon, 2 Mar 2020 at 14:00, Marco Pivetta <ocramius@gmail.com> wrote: > > > > > Overall against the RFC: ++ and @ > > z��xi�x5�-- (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/