Skip to content
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

api.markVariableAsUsed does not check Node.js scope #2089

Closed
yannickcr opened this issue Mar 17, 2015 · 9 comments
Closed

api.markVariableAsUsed does not check Node.js scope #2089

yannickcr opened this issue Mar 17, 2015 · 9 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features

Comments

@yannickcr
Copy link
Contributor

The Node.js scope is not checked by the api.markVariableAsUsed function making these variables impossible to mark as used.

"use strict";

var eslint = require("./lib/api.js").linter;

var code = "var a = 1, b = 2;";

eslint.reset();
eslint.on("Program:exit", function() {
    eslint.markVariableAsUsed("a"); // return false, the variable was not found in the scope
});

eslint.verify(code, { env: { node: true }}, "test.js", true);
@nzakas nzakas added the triage An ESLint team member will look at this issue soon label Mar 17, 2015
@nzakas
Copy link
Member

nzakas commented Mar 18, 2015

I'm not sure I'd call this a bug with the method, it seems more a bug with how it's being used. The method searches from the current scope up to the global scope. If you're calling it in "Program", then you're saying "only search the global scope".

However, in almost all cases you should not be calling markVariableAsUsed() in the global scope, you should be calling it where the identifier is actually used.

Can you show me how you're using it?

@nzakas nzakas added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Mar 18, 2015
@nzakas
Copy link
Member

nzakas commented Mar 18, 2015

Okay, I saw your other bug and I get it now. While I'd still recommend not using markVariableAsUsed() in the global scope, rather use it in the appropriate JSX element context, I can see how this would be confusing.

@yannickcr
Copy link
Contributor Author

I'm using it to mark as used the variables in the JSX code.

You can see the rule here: https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/jsx-uses-vars.js

@nzakas
Copy link
Member

nzakas commented Mar 18, 2015

That looks like it should work (since you're not in the global scope). Are you still seeing an error?

@yannickcr
Copy link
Contributor Author

Yes, there is an error when the variable is used in the global scope.

You can see some example of problematic code here and here

@nzakas
Copy link
Member

nzakas commented Mar 18, 2015

Hmm, I'm I'm looking at this correctly, I'm not sure markVariableAsUsed() is the problem. Because it searches from the current scope up to the global scope, it should still find any variable that was defined in the Node.js scope. It seems more likely the problem is in no-undef and no-unused-vars not checking the Node.js scope.

@yannickcr
Copy link
Contributor Author

Sorry but I do not see the relation with no-undef or no-unused-vars

To retake the above example:

"use strict";

var eslint = require("./lib/api.js").linter;

var code = "var a = 1, b = 2;";

eslint.reset();
eslint.on("Program:exit", function() {
    eslint.markVariableAsUsed("a"); // return false, the variable was not found in the scope
});

eslint.verify(code, { env: { node: true }}, "test.js", true);

But if we are not in the the Node.js environment:

"use strict";

var eslint = require("./lib/api.js").linter;

var code = "var a = 1, b = 2;";

eslint.reset();
eslint.on("Program:exit", function() {
    eslint.markVariableAsUsed("a"); // return true, the variable was found in the scope
});

eslint.verify(code, {}, "test.js", true);

When in a Node.js scope, the variable a and b cannot be found in scope.variables but are in scope.childScopes[0].variables which is not currently checked in the actual markVariableAsUsed(). Seems to be a bug with markVariableAsUsed() to me :)

Pull request #2090 should fix this.

@nzakas
Copy link
Member

nzakas commented Mar 20, 2015

What I meant was the original issue you reported had nothing to do with this. This is a separate issue.

@nzakas
Copy link
Member

nzakas commented Mar 21, 2015

Working on this.

@nzakas nzakas closed this as completed in 89c1e10 Mar 23, 2015
nzakas added a commit that referenced this issue Mar 23, 2015
Fix: markVariableAsUsed() should work in Node.js env (fixes #2089)
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

No branches or pull requests

2 participants