Fix narrowing of superglobals #3901
Conversation
| assertType('array', $GLOBALS); | ||
| assertNativeType('array<mixed>', $GLOBALS); |
There was a problem hiding this comment.
these 2 fail without the adaptions here
|
phpstan-doctrine failures are unrelated, those seemed to have appeared because of recent phpstan improvements, opened phpstan/phpstan-doctrine#650 for it |
| @@ -1001,4 +1001,9 @@ public function testHashing(): void | |||
| ]); | |||
| } | |||
|
|
|||
| public function testBug12772(): void | |||
There was a problem hiding this comment.
Please add also $_ENV['CI'] test explicitly, as I want to make sure GitHub CI environment variable is never assumed to exist on runtime - https://github.com/atk4/ui/blob/235416b31066827226e462f1916d9fc99c0d14b4/demos/init-db.php#L556. And thank you very much for looking into this issue!
Our reproducible environment is the same as here, GitHub CI, so as long as the tests passes here, our should pass then too.
There was a problem hiding this comment.
I get your point, but adaptions here are not related to any environment variables. If Ondřej is fine with this, I expect it to be merged very soon and then we should also see the effects
src/Analyser/MutatingScope.php
Outdated
|
|
||
| $superglobalType = new ArrayType(new BenevolentUnionType([new IntegerType(), new StringType()]), new MixedType(true)); | ||
| $this->expressionTypes[$varExprString] = ExpressionTypeHolder::createYes(new Variable($variableName), $superglobalType); | ||
| $this->nativeExpressionTypes[$varExprString] = ExpressionTypeHolder::createYes(new Variable($variableName), $superglobalType); |
There was a problem hiding this comment.
I don't get why the fix looks like this.
- This is polluting
$this->expressionTypeswith any superglobal in the analysed code. - It's mutating Scope in a getter method which is a big red-flag.
There was a problem hiding this comment.
good point, I don't understand how this even worked before without adding an expression tbh :P the idea of setting the expression for proper narrowing came from #3762 (comment) basically.
not sure if relevant for the bug or fix, but after the expression is created it is dealing with late resolveable types still in the same method in
phpstan-src/src/Analyser/MutatingScope.php
Line 566 in a11741d
There was a problem hiding this comment.
looks like I can't get around step-debugging this issue :D I assume the expressions are just overriding something that is incorrectly set and retained or so? let's see..
There was a problem hiding this comment.
I found a better fix, the root issue has to be addressed in mergeVariableHolder. Stay tuned.
There was a problem hiding this comment.
those variable type holders were the missing piece for me I guess
ondrejmirtes
force-pushed
the
superglobal-narrowing
branch
from
a11741d to
9710ba5
Compare
ondrejmirtes
force-pushed
the
superglobal-narrowing
branch
from
9710ba5 to
f68cc6f
Compare
|
Thank you. |
|
thank you for adapting the fix to be proper :) |
Improvements of #3871, pulling in parts of #3762 basically
Fixes phpstan/phpstan#12771
Fixes phpstan/phpstan#12772
Maybe fixes phpstan/phpstan#12776
I did not take over the changes where the expressions are passed over through classes, functions and namespaces in scopes. Those would deal with some edge cases, but not sure if it's worth.
fyi @staabm