Skip to content
This repository has been archived by the owner on Aug 15, 2024. It is now read-only.

fix: Incorrect scope analysis of class name in extends expression #113

Closed
wants to merge 3 commits into from
Closed

Conversation

lizhihao132
Copy link

#112
or
#59

Copy link

linux-foundation-easycla bot commented Dec 9, 2023

CLA Missing ID CLA Not Signed

@eslint-github-bot
Copy link

Hi @lizhihao132!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

1 similar comment
@eslint-github-bot
Copy link

Hi @lizhihao132!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@eslint-github-bot
Copy link

Hi @lizhihao132!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@eslint-github-bot
Copy link

Hi @lizhihao132!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@@ -272,7 +272,8 @@ class Referencer extends esrecurse.Visitor {
));
}

this.visit(node.superClass);
let may_ref_classid = node.superClass && node.superClass.type!=='Identifier'

Choose a reason for hiding this comment

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

Suggested change
let may_ref_classid = node.superClass && node.superClass.type!=='Identifier'
let mayRefClassid = node.superClass && node.superClass.type!=='Identifier'

The naming convention for the codebase is camelCase.

@@ -272,7 +272,8 @@ class Referencer extends esrecurse.Visitor {
));
}

this.visit(node.superClass);
let may_ref_classid = node.superClass && node.superClass.type!=='Identifier'
Copy link
Member

Choose a reason for hiding this comment

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

Why is extends <Identifer> a special case?

var C = class {};

var C1 = class C extends C {}; // ReferenceError: Cannot access 'C' before initialization

It seems we should just always visit node.superClass after this.scopeManager.__nestClassScope(node)?

Copy link
Author

Choose a reason for hiding this comment

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

No, we must discuss this on a case-by-case basis, as it is different. Before explaining this issue, we need to first understand what the following three function calls do specifically:

  • "this.scopeManager.__nestClassScope(node)": Essentially, it creates a child scope s based on the currently used scope p (the node associated with this scope is node, that is: p.block === node), inherits the symbols of p, and sets the "currently used scope" to s.
  • "this.close(node)": Assuming the "currently used scope" is m (m.block === node), this function call sets the "currently used scope" to the parent scope of m.
  • "this.visit(x)": Traverse the x node based on the "currently used scope".

Now let's answer your question.

The first scenario:
In this piece of code, class A extends B { ... }, if we first call: "this.scopeManager.__nestClassScope(node);", this causes the "currently used scope" to be set to the newly created scope (it is a child scope of the top-scope), and then this.visit(node.superClass), this causes the superClass node, that is, B to be analyzed within this child scope, making it a reference to the child scope instead of the top-scope. This is incorrect. Because under the JS specification, the B above should be a reference to the top-scope.

The second scenario:
when superClass is not an Identifier but a complex node, such as the following code:

var C = 'outside';

var cls = class C extends (
    probeHeritage = function() { return C; },
    setHeritage = function() {  }
  ) {
  method() {
    return C;
  }
};

It is a comma expression wrapped in parentheses, containing two assignment expressions. At this time, the C symbol in the superClass node is no longer the var C in the top-scope, but points to the C in class C (which is the issue we need to resolve). Therefore, the solution is quite obvious: we need to traverse the superClass node when the "currently used scope" is in the class scope, in order to make the resolution of the C symbol correct. Different from the first scenario, when superClass is an Identifier, it needs to be traversed when the "currently used scope" is the top-scope, so "this.visit(node.superClass)" needs to be done before "this.scopeManager.__nestClassScope(node);".

tests/es6-class-name-lex-open-heritage.js Show resolved Hide resolved
@nzakas
Copy link
Member

nzakas commented Dec 11, 2023

@lizhihao132 please sign the CLA.

@lizhihao132
Copy link
Author

@lizhihao132 please sign the CLA.

ok,done

@lizhihao132 lizhihao132 changed the title fix bug: Incorrect scope analysis of class name in extends expression fix: Incorrect scope analysis of class name in extends expression Dec 15, 2023
@lizhihao132
Copy link
Author

Can any body tell me how to do the CLA Authorization?i sign more than one times, but no thing change here but:"Missing CLA Authorization"

@lizhihao132 lizhihao132 closed this by deleting the head repository Dec 15, 2023
@lizhihao132
Copy link
Author

lizhihao132 commented Dec 15, 2023

i close this pull-request, and new one:#114

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

Successfully merging this pull request may close these issues.

4 participants