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

fix: resolve to module scope for top level statements #291

Closed
wants to merge 3 commits into from
Closed

fix: resolve to module scope for top level statements #291

wants to merge 3 commits into from

Conversation

DMartens
Copy link

@DMartens DMartens commented Mar 6, 2023

Fixes #280.
scopeManager.acquire(node) returns null in these cases.
The fix is to only return the global scope for the root Program node (consistent with other eslint parsers) and SvelteScriptElement (as I would expect it as it is for me also a root node)
Some notes:

  • There seems to always be a module scope as in the parseForESLint method the sourceType is always overwritten to "module"
  • Are there other scenarios where scopeManager.acquire returns null?

@changeset-bot
Copy link

changeset-bot bot commented Mar 6, 2023

🦋 Changeset detected

Latest commit: 4e3686f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte-eslint-parser Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -0,0 +1,61 @@
import assert from "assert";
import { parseForESLint } from "../../../src";
import { getScopeFromNode } from "../../../src/scope";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR.

Can you replace the test with ESLint's context.getScope instead of getScopeFromNode? getScopeFromNode is not used in the actual rule implementation.

@DMartens
Copy link
Author

DMartens commented Mar 7, 2023

So I dug deeper and the problem is that the Program is mutated after analyzing the scope.
Because of that the acquire method of eslint-scope cannot look up the scope of the node as the references mismatch.
The cause of the mutation is the scriptElements = ast.body.filter(...) in parseForESLint.
As no scope is found the default is to use the global scope and nested scopes are not affected as they are not mutated.
So I implemented a workaround by extending the acquire method looking up the module scope if the parent node is SvelteScriptElement.
Feel free to close this PR if you rather get rid of the mutations.
Also now the tests are actually E2E and I used to experimental FlatESLint as I did not want to mock the file system for rule loading.

@@ -0,0 +1,69 @@
import assert from "assert";
import * as svelte from "../../../src";
import { FlatESLint } from 'eslint/use-at-your-own-risk';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change it to use the Linter class instead of FlatESLint? I believe that using the Linter class is sufficient for this test.

https://eslint.org/docs/latest/integrate/nodejs-api#linter

@ota-meshi
Copy link
Member

Hmm... Fixing this bug seems a bit difficult. I would like to close this PR and work on this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scope Manager should return module scope for top level statements
2 participants