-
Notifications
You must be signed in to change notification settings - Fork 17
Allow finding arrow function variables when arrow function is in file scope #347
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
Conversation
`findVariableScopeExceptArrowFunctions()` can return null if it finds no scope, but it can also return 0 which is the file level scope. The additional code to look for arrow function scope needs to operate even on 0.
Some tests are failing because of some dependency issue with phpcs's dev-master branch. It has nothing to do with this PR. See #348
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.
Can self::findVariableScopeExceptArrowFunctions() return false ? Would a check for is_int() instead of !is_null() be more stable ?
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.
I don't think it can return false. It returns null|int. It has only 3 return statements:
- this one which is always an integer (guarded by
is_int()). - this one which is also always an integer.
- this one which is the return value of
getStartOfTokenScope()when it's not an integer or less than or equal to 0. Since that function can return null or 0 or a key from the token conditions stack, I think it is alwaysnull|int, unless there's something I don't grok aboutconditions(can the key of aconditionsarray be false?).
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.
That's fair enough. I was mostly asking because the code within the condition seems to expect an int, while the condition doesn't actually strictly safeguard that.
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.
That's a good point. I think that is_int() is probably better for that reason, but I'm going to leave it for now since I already merged and I have to trust the types in the phpdoc, even if they're only enforced by static analysis tools.
I checked, and if I change the return type of findVariableScopeExceptArrowFunctions to int|null|false, I get this error:
Parameter #3 $enclosingScopeIndex of static method VariableAnalysis\Lib\Helpers::getContainingArrowFunctionIndex() expects int, int|false given.
So everything should be safe. Hooray for strong types!
Arrow function variables are a little bit tricky to parse because while other variables always exist in one scope or another, variables found within an arrow function could be local to the arrow function or exist in the scope outside the arrow function.
Therefore, when we find a variable, we first find its largest possible scope – normally this is the function – and then check to see if there is a closer arrow function scope that contains that variable.
However, there's a bug: if the function scope is not found, we return the file scope (token index 0). There is a guard which does not allow searching for an arrow function scope if the larger scope is falsy. I believe this was meant to guard against a
nullscope, but it was not specific enough and so also was true when it found the file scope 0. As a result, we cannot properly parse function-level variables in arrow functions at the file-level scope.In this PR, we modify the guard to explicitly check for null.
Fixes #346