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

Question: how come function references on the global scope are not closed #57

Open
bradzacher opened this issue May 6, 2020 · 6 comments
Labels
Projects

Comments

@bradzacher
Copy link

bradzacher commented May 6, 2020

describe("When there is a `function` declaration on global,", () => {
it("the reference on global should NOT be resolved.", () => {
const ast = espree(`
function a() {}
a();
`);
const scopeManager = analyze(ast, { ecmaVersion: 6 });
expect(scopeManager.scopes).to.have.length(2); // [global, a]
const scope = scopeManager.scopes[0];
expect(scope.variables).to.have.length(1);
expect(scope.references).to.have.length(1);
const reference = scope.references[0];
expect(reference.from).to.equal(scope);
expect(reference.identifier.name).to.equal("a");
expect(reference.resolved).to.be.null;
expect(reference.writeExpr).to.be.undefined;
expect(reference.isWrite()).to.be.false;
expect(reference.isRead()).to.be.true;
});
it("the reference in functions should NOT be resolved.", () => {
const ast = espree(`
function a() {}
function foo() {
let b = a();
}
`);
const scopeManager = analyze(ast, { ecmaVersion: 6 });
expect(scopeManager.scopes).to.have.length(3); // [global, a, foo]
const scope = scopeManager.scopes[2];
expect(scope.variables).to.have.length(2); // [arguments, b]
expect(scope.references).to.have.length(2); // [b, a]
const reference = scope.references[1];
expect(reference.from).to.equal(scope);
expect(reference.identifier.name).to.equal("a");
expect(reference.resolved).to.be.null;
expect(reference.writeExpr).to.be.undefined;
expect(reference.isWrite()).to.be.false;
expect(reference.isRead()).to.be.true;
});
});

I'm working on trying to understand this codebase better.
But I can't understand why it works this way:

function a() {}
a();

To me it seems clear that the reference that's created as part of the call should resolve directly to the function declaration, but it's not - it remains as an unresolved reference that's added to the global scope's through list.

This would cause the no-unused-vars ESLint rule to error on the function declaration, were it not for this code in the linter.

It looks like it works seemingly by chance? ESLint augments the global scope with more variables, and then forcefully resolves any through references against the global variable set, which includes the function declaration as well as the augmented variables.

Is anyone able to explain why this works this way?

Or a better question - when closing the global scope, why doesn't it attempt to resolve all through references against the variables defined in the global scope?

@mysticatea
Copy link
Member

Thank you for your question.

....but, I'm not sure why it's so. I guess that it's as-is from the original package escope.

Maybe... it couldn't assume declarations in global scope make variables, because browsers may ignore declarations.

var top = function() {}
top() // ERROR: 'top' is not a function

data:text/html,<script>var top = function(){}; top();</script>

@bradzacher
Copy link
Author

Interesting.
Is this something that should be fixed?

I believe at least in sourceType: module mode, this shouldn't work this way, right?

@mysticatea
Copy link
Member

Yes, data:text/html,<script type="module">var top = function(){}; top();</script> works fine.

Also, new knownGlobals: string[] option may be good to make variables and resolve references.

@bradzacher
Copy link
Author

is it only var declarations that shouldn't be closed?

Doing some testing it looks like every single other case works as expected and throws this error: Identifier 'top' has already been declared

It's only var top that is ignored and throws top is not a function

@nzakas
Copy link
Member

nzakas commented Jan 26, 2021

@mysticatea what's the verdict here? Is there something to fix or no?

@nzakas nzakas added this to Evaluating in Triage Jan 26, 2021
@nzakas
Copy link
Member

nzakas commented Apr 30, 2021

@mysticatea is this something you want to address?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Evaluating
Triage
Evaluating
Development

No branches or pull requests

3 participants