-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fix: api.markVariableAsUsed should check Node.js scope (fixes #2089) #2090
Conversation
|
||
found = markVariableAsUsedInScope(scope); | ||
|
||
if (!found && scope.childScopes.length) { |
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.
This is overly aggressive because there's only one time when we want to check the first child. You really want to check if scope.type
is "global"
and only do this next step if that's true.
01a2f4c
to
c921648
Compare
Has requested, I updated the condition to be less aggressive. |
c921648
to
47fdfa6
Compare
47fdfa6
to
294568f
Compare
variables[i].eslintUsed = true; | ||
return true; | ||
found = 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.
There's a much more appropriate change to make here. This could cause the wrong variable to be marked because it searches global and then node scope, which is the opposite of how variable resolution works.
Instead, this can be added to the original code just before the loop:
if (scope.type === "global" && currentConfig.ecmaFeatures.globalReturn) {
scope = scope.childScopes[0];
}
Closing in favor of #2121 |
Fix very similar to the one you've done for #2064
I also added a test to reproduce the problem and fixed the getVariable helper to also check the Node.js scope.