-
Notifications
You must be signed in to change notification settings - Fork 533
Indicates whether a variable is global #4245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.1.x
Are you sure you want to change the base?
Conversation
2671bed
to
4717d70
Compare
4717d70
to
f5cc717
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You're not testing what this should be allowing you. In the original issue you're saying you want to override variable types with ExpressionTypeResolverExtension but you're not testing that here.
- Instead of setting an attribute on
Variable
inassignVariable
, I'm thinking we could callassignExpression
herephpstan-src/src/Analyser/NodeScopeResolver.php
Line 1970 in 70a039f
$scope = $scope->assignVariable($var->name, new MixedType(), new MixedType(), TrinaryLogic::createYes()); GlobalVariableExpr
. TheScope::isGlobalVariable
could check the type of the expr node for that.
Existing virtual expr nodes are defined here: https://github.com/phpstan/phpstan-src/tree/2.1.x/src/Node/Expr
Additionally, they have to be handled here https://github.com/phpstan/phpstan-src/blob/2.1.x/src/Node/Printer/Printer.php and here
phpstan-src/src/Analyser/MutatingScope.php
Lines 739 to 753 in 70a039f
if ($node instanceof GetIterableKeyTypeExpr) { | |
return $this->getIterableKeyType($this->getType($node->getExpr())); | |
} | |
if ($node instanceof GetIterableValueTypeExpr) { | |
return $this->getIterableValueType($this->getType($node->getExpr())); | |
} | |
if ($node instanceof GetOffsetValueTypeExpr) { | |
return $this->getType($node->getVar())->getOffsetValueType($this->getType($node->getDim())); | |
} | |
if ($node instanceof ExistingArrayDimFetch) { | |
return $this->getType(new Expr\ArrayDimFetch($node->getVar(), $node->getDim())); | |
} | |
if ($node instanceof UnsetOffsetExpr) { | |
return $this->getType($node->getVar())->unsetOffset($this->getType($node->getDim())); | |
} |
f5cc717
to
db78deb
Compare
I added a test for this use case.
I struggle to find how to make it work, probably because I am not familiar with the PHPStan internal logic, so maybe I missed something. I finally found a solution with an early call to the phpstan-src/src/Analyser/MutatingScope.php Lines 4292 to 4301 in db78deb
This is the only solution I found to be able to assign the type during the |
I think you need to rebase/fix some conflict @cedric-anne |
db78deb
to
72730d1
Compare
Done. Conflicts were only related to I am not entirely satisfied with what I have proposed, but I was waiting for a review from someone who knows more about the internal logic of PHPStan to find out if I had missed something obvious, or even if I was heading in the right direction. Also, maybe this should be splitted into two distinct PRs. Indeed, being able to know whether a variable is global and being able to define its type using an extension are two distinct features. |
This will permit to easilly assign types to global variables in an extension. Without this change, it is not possible to distinguish local variables from global variables.
Related to phpstan/phpstan#13243 and following a discussion in #4233.
Here is an exemple: