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.

Sign up for GitHub

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

Jump to bottom

Update: support class fields (refs eslint/eslint#14343) #69

Merged
merged 12 commits into from
Jul 2, 2021
Merged

Update: support class fields (refs eslint/eslint#14343) #69

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ad618bc
Update: support class fields (refs eslint/eslint#14343)
mysticatea Apr 28, 2021
7d0944b
upgrade eslint-visitor-keys
mysticatea May 7, 2021
2e10322
update espree
mysticatea Jun 29, 2021
ea72415
make function-scope for class field initializers
mysticatea Jun 30, 2021
1f1e549
Update package.json
nzakas Jun 30, 2021
ddf9af0
Update package.json
nzakas Jun 30, 2021
9239ebd
Update tests/class-fields.js
nzakas Jun 30, 2021
ddabe0d
Update tests/class-fields.js
mysticatea Jun 30, 2021
a002ea2
Remove trailing whitespace to fix lint failure
btmills Jul 1, 2021
e73b479
introduce class-field-initializer scope
mysticatea Jul 1, 2021
936ade5
fix typo
mysticatea Jul 1, 2021
c8ae315
fix variableScope
mysticatea Jul 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions lib/referencer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,12 @@ class Referencer extends esrecurse.Visitor {
this.currentScope().__referencing(node);
}

// eslint-disable-next-line class-methods-use-this
PrivateIdentifier() {

// Do nothing.
}

UpdateExpression(node) {
if (PatternVisitor.isPattern(node.argument)) {
this.currentScope().__referencing(node.argument, Reference.RW, null);
Expand All @@ -453,6 +459,17 @@ class Referencer extends esrecurse.Visitor {
this.visitProperty(node);
}

PropertyDefinition(node) {
if (node.computed) {
this.visit(node.key);
}
if (node.value) {
this.scopeManager.__nestFunctionScope(node, /* strict= */ true);
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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)

this.visit(node.value);
this.close(node);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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";
}

Copy link
Member

Choose a reason for hiding this comment

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

@mdjermanovic what would you suggest instead? Let’s make sure to keep comments actionable so we can move things along quickly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. The implicit function is the class field initializer, but not whole the class field. Probably the function scope is only the initializer node.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think scope.block = PropertyDefinition.value is 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;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.


MethodDefinition(node) {
this.visitProperty(node);
}
Expand Down
8 changes: 6 additions & 2 deletions lib/scope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,8 +647,12 @@ class FunctionScope extends Scope {
super(scopeManager, "function", upperScope, block, isMethodDefinition);

// section 9.2.13, FunctionDeclarationInstantiation.
// NOTE Arrow functions never have an arguments objects.
if (this.block.type !== Syntax.ArrowFunctionExpression) {
// NOTE Arrow functions and class field initializers never have an arguments objects.
if (
this.block.type === Syntax.FunctionDeclaration ||
this.block.type === Syntax.FunctionExpression ||
this.block.type === Syntax.Program // for Node.js scope
) {
this.__defineArguments();
}
}
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@
"eslint-config-eslint": "^5.0.1",
"eslint-plugin-node": "^9.1.0",
"eslint-release": "^1.0.0",
"eslint-visitor-keys": "^1.2.0",
"espree": "^7.1.0",
"eslint-visitor-keys": "^3.0.0",
"espree": "^8.0.0",
"istanbul": "^0.4.5",
"mocha": "^6.1.4",
"npm-license": "^0.3.3",
Expand Down
290 changes: 290 additions & 0 deletions tests/class-fields.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,290 @@
/**
* @fileoverview Tests for class fields syntax.
* @author Toru Nagashima
*/

"use strict";
nzakas marked this conversation as resolved.
Show resolved Hide resolved

const assert = require("assert");
const espree = require("espree");
const { KEYS } = require("eslint-visitor-keys");
const { analyze } = require("../lib/index");

describe("Class fields", () => {
describe("class C { f = g }", () => {
let scopes;

beforeEach(() => {
const ast = espree.parse("class C { f = g }", { ecmaVersion: 13 });
const manager = analyze(ast, { ecmaVersion: 13, childVisitorKeys: KEYS });

scopes = manager.globalScope.childScopes;
});

it("should create a class scope.", () => {
assert.strictEqual(scopes.length, 1);
assert.strictEqual(scopes[0].type, "class");
});

it("The class scope has no references.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.references.length, 0);
});

it("The class scope has only the variable 'C'; it doesn't have the field name 'f'.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.variables.length, 1);
assert.strictEqual(classScope.variables[0].name, "C");
});

it("The class scope has a function scope.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.childScopes.length, 1);
assert.strictEqual(classScope.childScopes[0].type, "function");
});

it("The function scope has only the reference 'g'.", () => {
const classScope = scopes[0];
const fieldInitializerScope = classScope.childScopes[0];

assert.strictEqual(fieldInitializerScope.references.length, 1);
assert.strictEqual(fieldInitializerScope.references[0].identifier.name, "g");
});

it("The function scope has no variables.", () => {
const classScope = scopes[0];
const fieldInitializerScope = classScope.childScopes[0];

assert.strictEqual(fieldInitializerScope.variables.length, 0);
});
});

describe("class C { f }", () => {
let scopes;

beforeEach(() => {
const ast = espree.parse("class C { f }", { ecmaVersion: 13 });
const manager = analyze(ast, { ecmaVersion: 13, childVisitorKeys: KEYS });

scopes = manager.globalScope.childScopes;
});

it("should create a class scope.", () => {
assert.strictEqual(scopes.length, 1);
assert.strictEqual(scopes[0].type, "class");
});

it("The class scope has no references.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.references.length, 0);
});

it("The class scope has no child scopes; fields that don't have initializers don't create any function scopes.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.childScopes.length, 0);
});

it("The class scope has only the variable 'C'; it doesn't have the field name 'f'.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.variables.length, 1);
assert.strictEqual(classScope.variables[0].name, "C");
});
});

describe("class C { #f = g }", () => {
let scopes;

beforeEach(() => {
const ast = espree.parse("class C { #f = g }", { ecmaVersion: 13 });
const manager = analyze(ast, { ecmaVersion: 13, childVisitorKeys: KEYS });

scopes = manager.globalScope.childScopes;
});

it("should create a class scope.", () => {
assert.strictEqual(scopes.length, 1);
assert.strictEqual(scopes[0].type, "class");
});

it("The class scope has no references.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.references.length, 0);
});

it("The class scope has only the variable 'C'; it doesn't have the field name '#f'.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.variables.length, 1);
assert.strictEqual(classScope.variables[0].name, "C");
});

it("The class scope has a function scope.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.childScopes.length, 1);
assert.strictEqual(classScope.childScopes[0].type, "function");
});

it("The function scope has only the reference 'g'.", () => {
const classScope = scopes[0];
const fieldInitializerScope = classScope.childScopes[0];

assert.strictEqual(fieldInitializerScope.references.length, 1);
assert.strictEqual(fieldInitializerScope.references[0].identifier.name, "g");
});

it("The function scope has no variables.", () => {
const classScope = scopes[0];
const fieldInitializerScope = classScope.childScopes[0];

assert.strictEqual(fieldInitializerScope.variables.length, 0);
});
});

describe("class C { [fname] }", () => {
let scopes;

beforeEach(() => {
const ast = espree.parse("class C { [fname] }", { ecmaVersion: 13 });
const manager = analyze(ast, { ecmaVersion: 13, childVisitorKeys: KEYS });

scopes = manager.globalScope.childScopes;
});

it("should create a class scope.", () => {
assert.strictEqual(scopes.length, 1);
assert.strictEqual(scopes[0].type, "class");
});

it("The class scope has only the reference 'fname'.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.references.length, 1);
assert.strictEqual(classScope.references[0].identifier.name, "fname");
});

it("The class scope has no child scopes; fields that don't have initializers don't create any function scopes.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.childScopes.length, 0);
});
});

describe("class C { [fname] = value }", () => {
let scopes;

beforeEach(() => {
const ast = espree.parse("class C { [fname] = value }", { ecmaVersion: 13 });
const manager = analyze(ast, { ecmaVersion: 13, childVisitorKeys: KEYS });

scopes = manager.globalScope.childScopes;
});

it("should create a class scope.", () => {
assert.strictEqual(scopes.length, 1);
assert.strictEqual(scopes[0].type, "class");
});

it("The class scope has only the reference 'fname'; it doesn't have the reference 'value'.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.references.length, 1);
assert.strictEqual(classScope.references[0].identifier.name, "fname");
});

it("The class scope has a function scope.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.childScopes.length, 1);
assert.strictEqual(classScope.childScopes[0].type, "function");
});

it("The function scope has the reference 'value'.", () => {
const classScope = scopes[0];
const fieldInitializerScope = classScope.childScopes[0];

assert.strictEqual(fieldInitializerScope.references.length, 1);
assert.strictEqual(fieldInitializerScope.references[0].identifier.name, "value");
});

it("The function scope has no variables.", () => {
const classScope = scopes[0];
const fieldInitializerScope = classScope.childScopes[0];

assert.strictEqual(fieldInitializerScope.variables.length, 0);
});
});
mysticatea marked this conversation as resolved.
Show resolved Hide resolved

btmills marked this conversation as resolved.
Show resolved Hide resolved
describe("class C { #f = g; e = this.#f }", () => {
let scopes;

beforeEach(() => {
const ast = espree.parse("class C { #f = g; e = this.#f }", { ecmaVersion: 13 });
const manager = analyze(ast, { ecmaVersion: 13, childVistorKeys: KEYS });

scopes = manager.globalScope.childScopes;
});

it("should create a class scope.", () => {
assert.strictEqual(scopes.length, 1);
assert.strictEqual(scopes[0].type, "class");
});

it("The class scope has no references.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.references.length, 0);
});

it("The class scope has only the variable 'C'; it doesn't have the field names '#f' or 'e'.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.variables.length, 1);
assert.strictEqual(classScope.variables[0].name, "C");
});

it("The class scope has two function scopes.", () => {
const classScope = scopes[0];

assert.strictEqual(classScope.childScopes.length, 2);
assert.strictEqual(classScope.childScopes[0].type, "function");
assert.strictEqual(classScope.childScopes[1].type, "function");
});

it("The first function scope has only the reference 'g'.", () => {
const classScope = scopes[0];
const fieldInitializerScope = classScope.childScopes[0];

assert.strictEqual(fieldInitializerScope.references.length, 1);
assert.strictEqual(fieldInitializerScope.references[0].identifier.name, "g");
});

it("The first function scope has no variables.", () => {
const classScope = scopes[0];
const fieldInitializerScope = classScope.childScopes[0];

assert.strictEqual(fieldInitializerScope.variables.length, 0);
});

it("The second function scope has no references.", () => {
const classScope = scopes[0];
const fieldInitializerScope = classScope.childScopes[1];

assert.strictEqual(fieldInitializerScope.references.length, 0);
});

it("The second function scope has no variables.", () => {
const classScope = scopes[0];
const fieldInitializerScope = classScope.childScopes[1];

assert.strictEqual(fieldInitializerScope.variables.length, 0);
});
});
});