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

[bradzacher]

https://github.com/eslint/eslint-scope/blob/dbddf14d5771b21b5da704213e4508c660ca1c64/tests/references.js#L211-L263

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]

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]

Interesting. Is this something that should be fixed?

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

[mysticatea]

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]

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]

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

[nzakas]

@mysticatea is this something you want to address?

[nzakas]

Porting this over from the old repo.

@eslint/eslint-tsc is this something we want to address in the v10 timeframe?

[mdjermanovic]

Based on https://github.com/estools/escope/issues/56#issuecomment-98797261, it seems the reasoning was that global function and var variables can be deleted, so the references are not guaranteed to be resolved at the time of their evaluation.

This looks like a very edge case, so I'd be in favor of resolving these references in eslint-scope.

I think this would be a breaking change in the eslint-scope package.

In the eslint package, we'd still need to keep the code that additionally resolves references to global variables (https://github.com/eslint/eslint/blob/eaf8a418af32b3190494e4a2284533353c28ccfa/lib/languages/js/source-code/source-code.js#L318-L334), because of predefined globals, so I think switching to the new version of eslint-scope wouldn't be a breaking change in the eslint package.

[fasttime]

What would be the benefit of this breaking change? I could be wrong but my impression is that @bradzacher was asking a question rather than reporting a bug, and it seems that @mdjermanovic found the answer:

Based on estools/escope#56 (comment), it seems the reasoning was that global function and var variables can be deleted, so the references are not guaranteed to be resolved at the time of their evaluation.

[bradzacher]

But eslint runs in the assumption that doesn't happen - I'm on mobile so I can't grab the lines - but IIRC at the start of each file's lint run eslint will iterate all these "unresolved" references and resolve them to the variables in the global scope.

And I would hazard a guess that all consumers do the same thing.

IMO it just makes sense to do this automatically.

[mdjermanovic]

But eslint runs in the assumption that doesn't happen - I'm on mobile so I can't grab the lines - but IIRC at the start of each file's lint run eslint will iterate all these "unresolved" references and resolve them to the variables in the global scope.

And I would hazard a guess that all consumers do the same thing.

IMO it just makes sense to do this automatically.

Yes, but that would still be needed for predefined globals (ES built-ins, languageOptions.globals, etc). We could consider adding a way to pass in a list of predefined globals to eslint-scope, so that eslint-scope, instead of the consumer, creates Variable objects and updates references, but that would be a breaking change for third-party scope managers as they'd need to provide the same interface and functionality.

[mdjermanovic]

What would be the benefit of this breaking change?

No particular benefits for ESLint, and maybe just a partial benefit for other consumers, as they'll still need to handle predefined globals (unless we also consider adding a way to pass in a list, as mentioned in my previous comment). I just thought that the current behavior for declared variables could be very surprising to consumers, while eslint-scope not knowing what predefined variables exist in an environment might be somewhat expected.

[nzakas]

But eslint runs in the assumption that doesn't happen - I'm on mobile so I can't grab the lines - but IIRC at the start of each file's lint run eslint will iterate all these "unresolved" references and resolve them to the variables in the global scope.

@bradzacher I believe these are the lines you're referring to: https://github.com/eslint/eslint/blob/fef04b5c7fea99362d67b31b8e98cd4914020ed3/lib/linter/linter.js#L232-L253

We could consider adding a way to pass in a list of predefined globals to eslint-scope, so that eslint-scope, instead of the consumer, creates Variable objects and updates references, but that would be a breaking change for third-party scope managers as they'd need to provide the same interface and functionality.

I like the idea of passing a list of globals into eslint-scope to resolve. While this would be a breaking change for other scope managers, I'm not sure if any exist outside of the typescript-eslint scope one. :smile: Even if they do, I'm guessing there aren't that many and it's a pretty easy upgrade. We could hold off on using that new feature in ESLint until v10.

[sam3k]

The TSC group has decided to add this issue to v10.

[mdjermanovic]

I'll work on this.

[eslint-github-bot[bot]]

👋 Hi! This issue is being addressed in pull request https://github.com/eslint/js/pull/682. Thanks, @mdjermanovic!