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

Breaking: upgrade espree and support new class features (refs #14343) #14591

Merged
merged 68 commits into from Aug 5, 2021

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented May 16, 2021

WIP.

Remaining steps:

  • update id-denylist
  • update id-length
  • update id-match
  • check if other rules need updates or not.
  • check rules again after rest.
  • release espree and eslint-scope.
  • update package.json with the new versions of espree and eslint-scope.

Refs #14343
Fixes #14632

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[X] Changes an existing rule
[X] Other, please explain: The espree update contains breaking changes.

What changes did you make? (Give an overview)

Commit Rule Description
150fd19 camelcase This is a large refactoring. Now the rule understands code semantics better by using variables and references of eslint-scope.
d8a912b class-methods-use-this It perceives functions at class field initializers.
e50a84c computed-property-spacing It perceives the computed keys of class fields.
e7e860f func-names Functions at field initializers have inferred names. The rule with "as-needed" option recognizes that.
- id-denylist (not yet)
- id-length (not yet)
- id-match (not yet)
e5cff29 indent It perceives class fields and indent class fields.
553aac0 keyword-spacing It's related in three contextual keywords: get, set, static. Tweaks spacing between those and private identifier. And it additinally checks class fields.
7da3ed4 no-dupe-class-members It perceives public class fields. Duplication of private members is syntax error.
4404054, 9d1700d no-eval It perceives that this.eval() at class field initializers is not eval. The ast-utils change in 7ccd811 and 8009406 is related.
29275bd no-extra-semi It perceives empty class elements preceded by class fields.
7ccd811, 8009406 no-invalid-this It perceives that this in class field initializers is valid.
160013d no-multi-assign It perceives assignments at class field initializers.
06c676c no-self-assign It perceives private member accesses.
9a494a2 no-undef-init It perceives unnecessary undefined at class field initializers.
4098d55 no-underscore-dangle It perceives underscore in private identifiers.
40492d3 no-unreachable It perceives that class fields are unreachable in rare cases. See also this comment.
ef87576 no-useless-computed-key It perceives the computed keys of class fields.
b320bbd operator-linebreak It perceives = of class field initializers.
6fffdc6 prefer-exponentiation-operator It perceives private identifier and doesn't confused with that. This upgrades eslint-utils.
803b06a quotes It perceives class fields.
7b89143 semi-spacing It perceives ; of class fields.
b50db18 semi-style It perceives ; of class fields.
560e0cf semi It perceives ; of class fields. Some cases are affected by beforeStatementContinuationChars option.
04e1ace space-infix-ops It perceives = of class field initializers.

And this PR adds some tests to a ton of rules.

Is there anything you'd like reviewers to focus on?

  • Please check if there are more rules that need to be fixed for class fields.
  • More tests?
  • Are there wrong fixes by misunderstandings of the spec?
  • There are rules that generate new errors for class fields. Is that good?
  • A large refactoring exists for camelcase, id-denylist, id-length, and id-match. Is this good or not?

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label May 16, 2021
@mysticatea mysticatea changed the title Breaking: upgrade espree and supports ES2022 class features (fixes # Breaking: upgrade espree and supports ES2022 class features (fixes #14343) May 16, 2021
@eslint-github-bot
Copy link

eslint-github-bot bot commented May 16, 2021

Hi @mysticatea!, thanks for the Pull Request

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

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@mysticatea mysticatea changed the title Breaking: upgrade espree and supports ES2022 class features (fixes #14343) Breaking: upgrade espree and supports new class features (fixes #14343) May 16, 2021
@mysticatea mysticatea mentioned this pull request May 16, 2021
8 tasks
@mysticatea mysticatea changed the title Breaking: upgrade espree and supports new class features (fixes #14343) Breaking: upgrade espree and support new class features (fixes #14343) May 16, 2021
@protoEvangelion
Copy link

protoEvangelion commented Jun 4, 2021

Keep up the good work @mysticatea very excited for this 🙌

@mdjermanovic
Copy link
Member

mdjermanovic commented Jun 8, 2021

@mysticatea do we also plan to update code path analysis, or shall we leave it as is?

For example:

a = 1;

class A {
  foo = a++;
}

b = a;

a++ is in the same code path (and same code segment) as a = 1 and b = a, but it isn't really executed between them.

Non-static PropertyDefinition (or at least PropertyDefinition#value) is logically a separate code path?

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible rule Relates to ESLint's core rules upgrade This change is related to a dependency upgrade and removed triage An ESLint team member will look at this issue soon labels Jun 11, 2021
@mdjermanovic
Copy link
Member

mdjermanovic commented Jun 22, 2021

Rules that use scope1.variableScope === scope2.variableScope logic:

/* eslint no-use-before-define: ["error", { "variables": false }] */

class A {
    foo = x; // false positive?
}

const x = 1;
/* eslint require-atomic-updates: error */

async () => {
    let i;

    foo(class { x = i++ });

    i = i + await bar(); // false negative?
}

Less sure about this one, since formally it isn't a function so maybe we could add an option later:

/* eslint no-loop-func: error */

for (var i = 0; i < 10; i++) {
    foo[i] = class {
        x = i; // false negative?
    }
}

@mdjermanovic
Copy link
Member

mdjermanovic commented Jun 22, 2021

semi false positive:

/* eslint semi: ["error", "never"] */

class A {
    a = b; // syntax error without `;`
    *c(){}
}

@mdjermanovic
Copy link
Member

mdjermanovic commented Jun 22, 2021

dot-notation false positive:

/* eslint dot-notation: ["error", { allowKeywords: false }] */

class C {
    #in;
    foo() {
        this.#in; // autofixed to `this["in"]`
    }
}

"PropertyDefinition > ArrowFunctionExpression.value": enterFunction,
"PropertyDefinition > ArrowFunctionExpression.value:exit": exitFunction,
Copy link
Member

@mdjermanovic mdjermanovic Jun 22, 2021

Choose a reason for hiding this comment

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

Should we put this behind an option, since this isn't a class method by definition?

VariableDeclarator(node) {
const name = sourceCode.getText(node.id),
init = node.init && node.init.name,
"VariableDeclarator, PropertyDefinition"(node) {
Copy link
Member

@mdjermanovic mdjermanovic Jun 22, 2021

Choose a reason for hiding this comment

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

I think PropertyDefinition should be behind an option (or a separate rule?), the docs for this rule says that it checks variable declarations.

@@ -97,6 +97,7 @@ module.exports = {

return {
Property: check,
PropertyDefinition: enforceForClassMembers ? check : lodash.noop,
Copy link
Member

@mdjermanovic mdjermanovic Jun 22, 2021

Choose a reason for hiding this comment

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

We should add something like || node.type === "PropertyDefinition" on line 64:

/* eslint no-useless-computed-key: ["error", { enforceForClassMembers: true }] */

class C {
    ["constructor"]; // false positive
    static ["prototype"]; // false positive
    ["__proto__"]; // false negative
}

(also, lodash has been removed in the meantime)

Copy link
Member

@mdjermanovic mdjermanovic Jul 9, 2021

Choose a reason for hiding this comment

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

static ["constructor"] is also false positive (produces parsing error instead of a runtime error in a maybe-unused code).

For some reason, "constructor" is allowed as the name of a static method, but not as the name of a static field.

ClassElement : static MethodDefinition

  • It is a Syntax Error if PropName of MethodDefinition is "prototype".

ClassElement : static FieldDefinition ;

  • It is a Syntax Error if PropName of FieldDefinition is "prototype" or "constructor".

@mdjermanovic
Copy link
Member

mdjermanovic commented Jun 22, 2021

In class-methods-use-this, "exceptMethods": ["a"] allows both a and #a:

/* eslint class-methods-use-this: ["error", { exceptMethods: ["a"] }] */

class C {
    a() {} // ok
    #a() {} // ok
}

Does it make sense, or should #a() be allowed only by "#a"?

@mdjermanovic
Copy link
Member

mdjermanovic commented Jun 22, 2021

no-extra-parens:

/* eslint no-extra-parens: ["error"] */

class C {
  [(a)]; // false negative
  b = (c + d); // false negative
}

@mdjermanovic
Copy link
Member

mdjermanovic commented Jun 23, 2021

no-dupe-class-members reports instance field and prototype method with the same name as duplicates, but that's a bit different from all other combinations where one definition basically overwrites the other. In this case, both are applied, and someone could write something like this:

class C {
    a = this.a.bind(this);
    a() {
        // uses `this`
    }
}

Thoughts?

@mysticatea
Copy link
Member Author

mysticatea commented Jun 29, 2021

I'm sorry for my inactive while a time. I'm rebooting...

@mysticatea
Copy link
Member Author

mysticatea commented Jun 29, 2021

@mysticatea do we also plan to update code path analysis, or shall we leave it as is?

For example:

a = 1;

class A {
  foo = a++;
}

b = a;

a++ is in the same code path (and same code segment) as a = 1 and b = a, but it isn't really executed between them.

Non-static PropertyDefinition (or at least PropertyDefinition#value) is logically a separate code path?

Oh. I had forgotten about code path analysis.
Yes, by the spec, class field initializers are implicit functions. The initializers define implicit functions and it's called from class instance instantiation process (by the way, the implicit functions define arguments local variable, but the use of arguments in class field initializers is a syntax error). Therefore, it should separate code paths and create scope.

@mdjermanovic mdjermanovic added the new syntax This issue is related to new syntax that has reached stage 4 label Jun 30, 2021
@mdjermanovic
Copy link
Member

mdjermanovic commented Jul 28, 2021

A remaining task is to update code path analysis to create separate code paths for class field initializers (#14591 (comment) and #14591 (comment)). This is a change in the core that affects rules (built-in and custom), so it might be good to implement it before beta release?

Aside from that, I left comments for about 15-20 rules. Some are obvious bugs, some need discussion. I can open issues after we merge this PR, so that they could be worked on in parallel.

@nzakas
Copy link
Member

nzakas commented Jul 29, 2021

I think code path analysis may not make it. @mysticatea is the only one who really understands it. I can take a swing at it but unsure how far I can actually get.

Yes, let’s deal with other rule changes separately. There is always a long tail of rule changes to support new syntax, and that’s part of what the beta period is for.

@nzakas nzakas changed the title Breaking: upgrade espree and support new class features (fixes #14343) Breaking: upgrade espree and support new class features (refs #14343) Jul 29, 2021
@nzakas nzakas linked an issue Jul 29, 2021 that may be closed by this pull request
7 tasks
nzakas
nzakas approved these changes Aug 5, 2021
@nzakas nzakas merged commit b953a4e into master Aug 5, 2021
12 checks passed
@nzakas nzakas deleted the class-fields branch Aug 5, 2021
nzakas added a commit that referenced this pull request Aug 18, 2021
mdjermanovic pushed a commit that referenced this pull request Sep 5, 2021
* Fix: Update semi for class-fields (refs #14591)

* Fixed several bugs

* Ensure beforeStatementContinuationChars does not apply to class fields

* Fix semi rule bugs for class fields

* Fix edge cases
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 2, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible new syntax This issue is related to new syntax that has reached stage 4 rule Relates to ESLint's core rules upgrade This change is related to a dependency upgrade
Projects
None yet
4 participants