Fix: Check division operator in astUtils.canTokensBeAdjacent #12879

mdjermanovic · 2020-02-06T06:01:29Z

Prerequisites checklist

I have read the contributing guidelines.
The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

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

[X] Bug fix

Tell us about your environment

ESLint Version: 7.0.0-alpha.0
Node Version: v12.14.0
npm Version: v6.13.4

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

Configuration

module.exports = {
    parserOptions: {
        ecmaVersion: 2015
    },
};

What did you do? Please include the actual source code causing the issue.

/* eslint no-extra-parens: ["error"] */
/* eslint operator-assignment: ["error", "never"] */

a/(/**/b);

a/(//
  b);

a/(/regex/);

a/=/**/b;

Online Demo Link (v6.8.0 actual)

What did you expect to happen?

4 errors and auto-fix that would not comment out some code.

What actually happened? Please include the actual, raw output from ESLint.

4 errors and auto-fix to:

/* eslint no-extra-parens: ["error"] */
/* eslint operator-assignment: ["error", "never"] */

a//**/b;

a///
  b;

a//regex/;

a= a//**/b;

In all 4 lines / becomes start of a line comment.

What changes did you make? (Give an overview)

Fixed astUtils.canTokensBeAdjacent to account for the / operator on the left side and block comment, line comment or regex on the right side.
Fixed no-extra-parens and operator-assignment rules to get and pass possible comment tokens as well.
Since there are now rules that can pass comment tokens to astUtils.canTokensBeAdjacent, had to also add the logic for comments (I believe they can be adjacent with everything except for / on the left side).
Fixed part of the code in astUtils.canTokensBeAdjacent that deals with string input (using espree.tokenize) to handle comments as well. I don't think this will affect the rules that are using this feature at the moment, but it should be a correct change and it also allowed testing this function with comments (all tests actually pass strings, maybe we should add some tests with tokens at some point since it's the most common case).
Also upgraded espree in order to pass espree.latestEcmaVersion instead of a hard-coded version to espree.tokenize(), and added try-catch around espree.tokenize(). This should fix partially or entirely astUtils.canTokensBeAdjacent hardcoded espree.tokenize #12291 (edited to prevent auto-closing astUtils.canTokensBeAdjacent hardcoded espree.tokenize #12291),

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

Is this the correct way to get comments from espree.tokenize?

This can also happen in operator-linebreak in some edge cases. That rule has its own code to handle adjacent tokens, I'll see if it is possible to switch to astUtils.canTokensBeAdjacent (or fix the same issue there if it isn't) in a follow-up PR.

Marked as accepted since it's an obvious autofix bug.

kaicataldo · 2020-02-06T18:27:10Z

lib/rules/utils/ast-utils.js

        let leftToken;

        if (typeof leftValue === "string") {
-            const leftTokens = espree.tokenize(leftValue, { ecmaVersion: 2015 });
+            const tokens = espree.tokenize(leftValue, espreeOptions);


Do you mind adding a test for when a Shebang comment token is given as the left value? I don't think this could ever be a real world use case (since another token can't be on the same line as the shebang), but I do think we should make sure that this won't crash the process when Espree tries to tokenize this.

For reference, we mutate the text input to make the shebang a regular JS line comment before we parse in lib/linter/linter.js and then transform the token post-parse to a Shebang comment token.

You're right, that would crash! Maybe we could already add try-catch as suggested in #12291 (comment) and also close #12291 with this PR?

canTokensBeAdjacent also didn't assume it could get a code (as a string argument) without any tokens, so it doesn't support a comment-only code. This isn't changed with this PR, it would still crash, so it isn't possible to call it with e.g.,:

astUtils.canTokensBeAdjacent(someToken, "/* comment */");

Then, it might also make sense to support empty code as well, and maybe even to account for spaces at the start/end of the code (e.g., canTokensBeAdjacent("a ", "b") would return true).

Maybe we could do that as well in this PR?

That sounds good to me! I guess another option (or something we could do additionally) is to short circuit it and add an early return of false if the left value token is of type Shebang. That would save the process from having to run the tokenizing at all.

Oh, interesting! Since this is already an improvement over the existing code, I would be fine with splitting these bug fixes over multiple PRs to keep them manageable. What do you think?

That sounds good to me! I guess another option (or something we could do additionally) is to short circuit it and add an early return of false if the left value token is of type Shebang. That would save the process from having to run the tokenizing at all.

Maybe I didn't understand, but I think it isn't possible to check if the token is Shebang before tokenizing, because we don't have the token yet?

Oh, I see, I missed that it could be a string or a token. Yes, if the left value is a string that won't work. I think the approach you suggested sounds good 👍

kaicataldo · 2020-02-07T02:08:24Z

lib/rules/utils/ast-utils.js

@@ -1357,17 +1357,43 @@ module.exports = {
     * next to each other, behavior is undefined (although it should return `true` in most cases).
     */
    canTokensBeAdjacent(leftValue, rightValue) {
+        const espreeOptions = { ecmaVersion: 2020, comment: true, range: true };


Non-blocker for this PR, but it could be nice to add the ability to specify “latest” somehow, so that this never gets out of date.

That would be great! But, I'm not sure if there is a way to do that at the moment? Does espree support something like "latest" or provides a list of available versions?

v/_ \ I don't think espree has such a thing.

It doesn’t, no! Definitely not a blocker for this - just an idea I had and wanted to write down for posterity 😄

A list of all versions could be also useful for the online demo, it's hard-coded there and has to be updated manually.

PR/proposal here: eslint/espree#430

kaicataldo · 2020-02-07T02:09:46Z

lib/rules/utils/ast-utils.js

        let leftToken;

        if (typeof leftValue === "string") {
-            const leftTokens = espree.tokenize(leftValue, { ecmaVersion: 2015 });
+            const tokens = espree.tokenize(leftValue, espreeOptions);


Oh, interesting! Since this is already an improvement over the existing code, I would be fine with splitting these bug fixes over multiple PRs to keep them manageable. What do you think?

kaicataldo · 2020-02-07T19:42:46Z

Sorry, meant to submit those two comments yesterday.

mdjermanovic · 2020-02-08T04:00:50Z

Since this is already an improvement over the existing code, I would be fine with splitting these bug fixes over multiple PRs to keep them manageable. What do you think?

Agreed! Just added try-catch and explicit check for Shebang (test with the "token" returns false because of the condition, test with the string return false on tokenizing).

This PR is now ready for review since it fixes some issues and (I believe) doesn't add any new ones.

nzakas · 2020-02-28T01:17:57Z

What are next steps here? Are we waiting for the Espree change?

kaicataldo · 2020-02-28T01:36:28Z

I can run an Espree release tomorrow as well. It would be good to get the updated API in before we run the next major release with eslint/espree#429.

mdjermanovic · 2020-02-28T02:01:35Z

If it would be easier for reviewing, I can add a note in #12291 and update canTokensBeAdjacent with the new Espree interface in another PR when the new Espree version gets published.

kaicataldo · 2020-03-02T17:08:27Z

espree@6.2.0 has been released. latestEcmaVersion is now available for this PR!

mdjermanovic · 2020-03-03T14:17:54Z

espree@6.2.0 has been released. latestEcmaVersion is now available for this PR!

Done!

…12879) * Fix: Check division operator in astUtils.canTokensBeAdjacent * Add try-catch and Shebang check * Upgrade espree, use latestEcmaVersion

Fix: Check division operator in astUtils.canTokensBeAdjacent

193c5da

mdjermanovic added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion autofix This change is related to ESLint's autofixing capabilities labels Feb 6, 2020

kaicataldo reviewed Feb 6, 2020

View reviewed changes

mdjermanovic changed the title ~~Fix: Check division operator in astUtils.canTokensBeAdjacent~~ [WIP] Fix: Check division operator in astUtils.canTokensBeAdjacent Feb 7, 2020

mdjermanovic added the do not merge This pull request should not be merged yet label Feb 7, 2020

kaicataldo reviewed Feb 7, 2020

View reviewed changes

Add try-catch and Shebang check

3b498bc

mdjermanovic removed the do not merge This pull request should not be merged yet label Feb 8, 2020

mdjermanovic changed the title ~~[WIP] Fix: Check division operator in astUtils.canTokensBeAdjacent~~ Fix: Check division operator in astUtils.canTokensBeAdjacent Feb 8, 2020

kaicataldo mentioned this pull request Feb 12, 2020

Update: add latestEcmaVersion & supportedEcmaVersions eslint/espree#430

Merged

Upgrade espree, use latestEcmaVersion

1f851e0

mdjermanovic added the upgrade This change is related to a dependency upgrade label Mar 3, 2020

nzakas approved these changes Mar 5, 2020

View reviewed changes

nzakas merged commit 6cef0d5 into master Mar 9, 2020

nzakas deleted the adjacent-division branch March 9, 2020 23:43

mdjermanovic mentioned this pull request Apr 18, 2020

astUtils.canTokensBeAdjacent hardcoded espree.tokenize #12291

Closed

eslint-deprecated bot locked and limited conversation to collaborators Sep 7, 2020

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Check division operator in astUtils.canTokensBeAdjacent #12879

Fix: Check division operator in astUtils.canTokensBeAdjacent #12879

mdjermanovic commented Feb 6, 2020 •

edited

kaicataldo Feb 6, 2020 •

edited

mdjermanovic Feb 6, 2020

mdjermanovic Feb 7, 2020

kaicataldo Feb 7, 2020 •

edited

kaicataldo Feb 7, 2020

mdjermanovic Feb 7, 2020

kaicataldo Feb 7, 2020 •

edited

kaicataldo Feb 7, 2020

mdjermanovic Feb 7, 2020

aladdin-add Feb 7, 2020

kaicataldo Feb 12, 2020

mdjermanovic Feb 12, 2020

kaicataldo Feb 12, 2020 •

edited

kaicataldo Feb 7, 2020

kaicataldo commented Feb 7, 2020

mdjermanovic commented Feb 8, 2020

nzakas commented Feb 28, 2020

kaicataldo commented Feb 28, 2020

mdjermanovic commented Feb 28, 2020

kaicataldo commented Mar 2, 2020

mdjermanovic commented Mar 3, 2020

Fix: Check division operator in astUtils.canTokensBeAdjacent #12879

Fix: Check division operator in astUtils.canTokensBeAdjacent #12879

Conversation

mdjermanovic commented Feb 6, 2020 • edited

Prerequisites checklist

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

What changes did you make? (Give an overview)

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

kaicataldo Feb 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Feb 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Feb 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Feb 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo commented Feb 7, 2020

mdjermanovic commented Feb 8, 2020

nzakas commented Feb 28, 2020

kaicataldo commented Feb 28, 2020

mdjermanovic commented Feb 28, 2020

kaicataldo commented Mar 2, 2020

mdjermanovic commented Mar 3, 2020

mdjermanovic commented Feb 6, 2020 •

edited

kaicataldo Feb 6, 2020 •

edited

kaicataldo Feb 7, 2020 •

edited

kaicataldo Feb 7, 2020 •

edited

kaicataldo Feb 12, 2020 •

edited