[BUGFIX] Prevent accessing an uninitialized variable in `DeclarationB… #448
Conversation
oliverklee
added this to the 9.0.0: Drop support for PHP < 7.2 and clean up things milestone
There was a problem hiding this comment.
The code change is good, as it could prevent a bug from being inadvertently introduced in future. Though it could be made much more optimal by swapping the && parameters.
However, as things stood, there wasn't actually a bug. In the loop, $oRule is always assigned (necessarily to a Rule according to the type spec of getRulesAssoc) before $aNewValues is added to. So it was actually impossible for $aNewValues to end up nonempty with $oRule not assigned.
On that basis, I would suggest changing the commit title to a [CLEANUP] or [TASK], and not having the changelog entry at all - as both would incorrectly suggest that an actual bug had been fixed.
An interesting sidenote is that this makes a good testcase for evaluating and comparing static code analysis tools. Would Psalm be able to work out that there is no bug here (I suspect it would because of the way it follows 'branches')? And would we want that? Perhaps it is better for potential future bugs due to programmer error to be reported - even though they would be picked up by such tools if and when introduced.
src/RuleSet/DeclarationBlock.php
Outdated
| @@ -585,7 +586,7 @@ public function createShorthandProperties(array $aProperties, $sShorthand) | |||
| $this->removeRule($sProperty); | |||
| } | |||
| } | |||
| if (count($aNewValues)) { | |||
| if ($oRule instanceof Rule && count($aNewValues)) { | |||
There was a problem hiding this comment.
instanceof is quite slow, whereas count is atomic. So it would be much more optimal to have &&'s parameters the other way round.
There was a problem hiding this comment.
Wouldn’t a null check suffice instead of the instanceof?
There was a problem hiding this comment.
Wouldn’t a
nullcheck suffice instead of theinstanceof?
Technically, it probably would. For the sake of communicating to the reader, I usually prefer the instanceof to reduce the cognitive load required to understand the resulting type.
There was a problem hiding this comment.
instanceofis quite slow, whereascountis atomic. So it would be much more optimal to have&&'s parameters the other way round.
Actually not that much more optimal, on reflection, since the instanceof test still needs execute whenever the array is nonempty (and the object is nonnull). Swapping the parameter order only saves the instanceof test when the object is null, when I would expect it to be quite quick.
I can imply an answer my own question. In civil and mechanical engineering, there are at least two failsafes. So if one component fails, there will not be any disaster unless two more components (that possibly check or regulate the failing part) also fail. I cannot remember the name of this or find it online (search engines are rubbish these days), though recall it being an issue in the Deepwater Horizon oil spill, when three components failed within quick succession. So I am thinking that using both PHPStan and Psalm would be beneficial. |
I'd prefer to stick to one of the two. My reasoning is that both tools are very similar in what they find/report, but they might require different PHPDoc annotations for the type information that cannot be conveyed using native type declarations alone. I don't know whether these notations would contradict/conflict with each other, and I fear that having multiple, very similar notations might increase the cognitive load for the reader. I propose we revisit this discussion once we've reached the highest PHPStan level and fixed all issues. Before that, PHPStan will have so much more to tell us. ;-) |
oliverklee
force-pushed
the
bugfix/uninitialized-1
branch
from
4dfef4f to
72bc4f5
Compare
…lock`