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

Is a class's heritage referenced from the wrong scope? #59

Closed
bradzacher opened this issue May 18, 2020 · 7 comments · Fixed by #116
Closed

Is a class's heritage referenced from the wrong scope? #59

bradzacher opened this issue May 18, 2020 · 7 comments · Fixed by #116
Assignees

Comments

@bradzacher
Copy link

bradzacher commented May 18, 2020

Am I misunderstanding the spec here?
I'm new to attempting to read the ECMAScript spec, so I wouldn't be surprised if I'm wrong...

To me it looks like the superclass is referenced from within the class's scope:

https://www.ecma-international.org/ecma-262/8.0/#sec-runtime-semantics-classdefinitionevaluation

  1. Let lex be the LexicalEnvironment of the running execution context.
  2. Let classScope be NewDeclarativeEnvironment(lex).
  3. ...
  4. If className is not undefined, then
    a) ...
  5. If ClassHeritage_opt is not present, then
    a) ...
  6. Else,
    a) Set the running execution context's LexicalEnvironment to classScope.
    b) Let superclass be the result of evaluating ClassHeritage.
    c) ...
  7. ...

When superclass is evaluated, it is specifically within classScope. So this means the reference should be placed within the class scope, right?

However, the scope manager references the superclass before it nests the class scope (i.e. it is referenced from the parent's scope):

visitClass(node) {
if (node.type === Syntax.ClassDeclaration) {
this.currentScope().__define(node.id,
new Definition(
Variable.ClassName,
node.id,
node,
null,
null,
null
));
}
this.visit(node.superClass);
this.scopeManager.__nestClassScope(node);


I tried to dig through the old escope history, but there's no information attached to the commit (estools/escope@c3e23e2)

@nzakas
Copy link
Member

nzakas commented Jun 1, 2020

I think the only answer we can give here is we don't know. We forked escope in order to have a better way to get it updated to fit in with our release cycle, but we still don't know all the dark secrets contained within.

@bradzacher
Copy link
Author

It's not a huge deal ultimately - there's still a reference to it, and I don't think anyone consuming this package is explicitly asserting a reference comes from a specific scope.

I was just working on a ground-up reimplementation which supports typescript (typescript-eslint/typescript-eslint#1939) and noticed this. I have some snapshot tests to validate the structure of the scope. When I added a snapshot test for the heritage, I noticed the placement of the reference and was confused.

I was mainly hoping someone more familiar with the spec could tell me if I'm interpreting the JS spec correctly or not 😅

@mysticatea
Copy link
Member

I'm sorry for my overlooked.

I think @bradzacher is right. The references of the extends clause should be in the class scope. Therefore, in the following rare case:

const A = class A extends A {}

the third A must be resolved to the second A rather than the first A. But, the current eslint-scope resolves it to the first A. It looks like a bug.

@nzakas
Copy link
Member

nzakas commented Apr 30, 2021

@mysticatea is this something you feel like addressing? If not, I can try to do it

@mdjermanovic
Copy link
Member

@nzakas are you maybe working on this? If not, I could take it.

@nzakas
Copy link
Member

nzakas commented Dec 8, 2023

I'm not. Please feel free.

@mdjermanovic
Copy link
Member

I'm working on this.

nzakas pushed a commit that referenced this issue Jan 2, 2024
* fix!: class `extends` is evaluated in the class scope

Fixes #59

* add tests with nested scopes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants