Update: support class fields (refs eslint/eslint#14343) #69
Conversation
|
Switching to a draft because this is WIP |
|
|
|
@mysticatea Hi. Any date when this will be updated and merged? Or we have to wait until a stable |
|
@sanex3339 please stop asking for updates on everything. Updates are always posted on the issues/PRs themselves when there is something to share. We do not have set timelines for releasing features. As everyone is working in their spare time, features get done as people are available to work on them. This PR cannot move forward until the final Espree v8.0.0 is released, as already indicated. The beta release does not unblock this PR. |
|
@nzakas thank you for the clarification, anyway I don't see any problem to ask (especially if I did it once in a different repository before) |
|
@sanex3339 The reason not to ask is because I am asking you not to. We get a lot of notifications every day, and as I already said, we always post when there are updates and there are no schedules to adhere to, so you are just creating more notifications for us to deal with. Please be patient and subscribe to the issues for updates. Thanks. |
|
Espree v8.0.0 has been released and |
|
@eslint/eslint-tsc if you could give this another review to make sure I didn't miss anything, I'd appreciate it. I'd like to merge this before #71. |
lib/referencer.js
Outdated
| this.visit(node.key); | ||
| } | ||
| if (node.value) { | ||
| this.scopeManager.__nestFunctionScope(node, /* strict= */ true); |
There was a problem hiding this comment.
how come you are nesting a function scope specifically for a property value?
or rather - why do you nest a scope at all for properties?
They can't declare variables (unless ofc the value is a function), right?
There was a problem hiding this comment.
In the spec, class field initializers are implicit functions. Those implicit functions are called from the class instantiation process. This scope represents that straightly.
The expressions in the computed properties are evaluated one-time at defining the class. On the other hand, the expressions in the class field initializers are evaluated at each instantiating class instance. I think it's odd if those are mixed in the same scope.
They can't declare variables (unless ofc the value is a function), right?
The access to the arguments variable in the class field initializers is a syntax error, so we can't access the local variable of the implicit functions. But I'm not sure how the direct eval() behaves in the implicit functions.
There was a problem hiding this comment.
Interesting!
Relevant part of the spec (I think):
In particular 3.d.
https://tc39.es/proposal-class-fields/#runtime-semantics-class-field-definition-evaluation
3. If Initializer_opt is present,
a. Let lex be the Lexical Environment of the running execution context.
b. Let formalParameterList be an instance of the production FormalParameters:[empty] .
c. Let privateScope be the PrivateEnvironment of the running execution context.
d. Let initializer be FunctionCreate(Method, formalParameterList, Initializer, lex, true, privateScope).
e. Perform MakeMethod(initializer, homeObject).
f. Let isAnonymousFunctionDefinition be IsAnonymousFunctionDefinition(Initializer).
If I'm understanding it correctly that means that it's essentially saying that at runtime it essentially defines a field as this:
class Foo {
prop = 1;
}Is more or less the same as
class Foo {
constructor() {
this.prop = (() => 1)();
}
}With the one caveat that if the intialiser contains the identifier arguments, then it's a syntax error (as defined in section 2.17.2)
Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>
lib/referencer.js
Outdated
| PropertyDefinition(node) { | ||
| if (node.computed) { | ||
| this.visit(node.key); | ||
| } | ||
| if (node.value) { | ||
| this.scopeManager.__nestFunctionScope(node, /* strict= */ true); | ||
| this.visit(node.value); | ||
| this.close(node); | ||
| } | ||
| } |
There was a problem hiding this comment.
This correctly handles scope-reference relations, but it looks unusual that a node that creates a scope can contain parts that belong to its upper scope. For example, it's unexpected for context.getScope() and it will return a wrong scope when called from within a computed key:
class A {
[x] = x;
}"Identifier[name='x']"() {
context.getScope().type // "function" "function";
}There was a problem hiding this comment.
@mdjermanovic what would you suggest instead? Let’s make sure to keep comments actionable so we can move things along quickly.
There was a problem hiding this comment.
Indeed. The implicit function is the class field initializer, but not whole the class field. Probably the function scope is only the initializer node.
There was a problem hiding this comment.
It's difficult to predict the impact on the existing logic in rules.
scope.block = PropertyDefinition is surprising because of the tree hierarchy.
scope.block = PropertyDefinition.value is surprising because any expression can create a scope.
If the AST spec had a function node between property and its value, that would be probably the least surprising.
I think scope.block = PropertyDefinition.value is logically correct choice.
There was a problem hiding this comment.
I think
scope.block = PropertyDefinition.valueis logically correct choice.
Also, if we decide on this approach, would it be useful to have a different scope.type (e.g., "class-field-initializer") to make a distinction between the initializer scope and the scope that the value itself creates in some cases?
For example, if we use "function", then the ArrowFunctionExpression in the following example has two "function" scopes:
class A {
x = () => y;
}There was a problem hiding this comment.
It's difficult to predict the impact on the existing logic in rules.
I agree.
I have introduced the class-field-initializer scope.
Though we cannot put variable declarations into the scope, the scope's variableScope is the scope itself, as the same as function scopes. Because some rules use variableScope's equality to detect being in the same function, it will be better.
Because context.getScope(node) for function nodes returns the innermost scope for the function, context.getScope(node) for function nodes never returns the new class-field-initializer scope.
There was a problem hiding this comment.
Though we cannot put variable declarations into the scope, the scope's
variableScopeis the scope itself, as the same as function scopes. Because some rules usevariableScope's equality to detect being in the same function, it will be better.
I agree, the documentation says it's about var variables, but the practical use is to find the nearest enclosing function scope. This would match the expectations of rules in eslint/eslint#14591 (comment).
There was a problem hiding this comment.
Except for static fields, which seem to be a special case as they are initialized right after the class definition. From the perspective of eslint-scope I think it makes sense to treat static fields the same as instance fields, but rules will have to handle them specifically, if needed.
|
@mysticatea it looks like the last change introduced a lint error |
| if (value) { | ||
| this.scopeManager.__nestClassFieldInitializerScope(value); | ||
| this.visit(value); | ||
| this.close(value); |
There was a problem hiding this comment.
Just a non-blocking observation: the class field initializer scope might have been already closed at this point, triggered by closing a child function/class scope on the same node, but then this will simply do nothing as the current scope is already set to the upper class scope.
WIP.
This PR adds the class field support.
Remaining steps:
eslint-visitor-keys.espree.eslint-visitor-keysandespreein package.json.