astUtils.canTokensBeAdjacent hardcoded espree.tokenize

[mdjermanovic]

Tell us about your environment

• ESLint Version: 6.4.0
• Node Version: 10.16.0
• npm Version: 6.9.0

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

Please show your full configuration:

Configuration

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

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

/* eslint no-implicit-coercion:error */

let x ="" + 1n;

eslint index.js

What did you expect to happen?

let x =String(1n);

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

SyntaxError: Identifier directly after number at Espree.raise ... canTokensBeAdjacent.

Problem

astUtils.canTokensBeAdjacent can accept a string value, which is useful when fixer adds some new code. But, it's using hardcoded espree.tokenize with ecmaVersion: 2015.

The same problem occurs with the following code when babel-eslint is used:

/* eslint no-implicit-coercion:error */

class Foo {
  #a = 0;
  bar() {
    let x ="" + this.#a;
  }
}

Are you willing to submit a pull request to fix this bug?

Yes, but I'm not sure what would be the best approach. Whether to use the configured parser and configured options, or to somehow avoid this feature e.g., pass a token-like object instead of a string (that would require a change in 4-5 rules though).

[kaicataldo]

Interesting. I definitely think we should change this. Ideally, we'd be able to use a custom configured parser for this, but it also forces custom parsers to implement a tokenize() method. As a starting point, would it make sense to pass parserOptions to the Espree#tokenize() method calls?

Maybe it could check if the configured parser has a tokenize() method and, if it does, call that with the configured parserOptions. If the method doesn't exist, it would call Espree#tokenize() with the configured parserOptions.

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[kaicataldo]

Reopening because I think this is an important discussion to have. @eslint/eslint-team Any other thoughts on this?

[btmills]

Not sure I have any bright ideas. Passing through parserOptions seems better than nothing, and maybe we could detect if the configured parser has a tokenize method and fall back to espree otherwise?

[ilyavolodin]

I think we should always call tokenize method on the configured parser, and mark it as required as part of the parser implementation. We do the same for parse method, so now all custom parsers have to implement 2 methods.

[platinumazure]

I agree that we should eventually require tokenize, but I don't think we can do that for 7.0. I think we should check the parser object and emit a warning in 7.0 if it doesn't have tokenize (and fall back to espree.tokenize internally), and we could throw an error in 8.0.

[ilyavolodin]

That sounds like a good plan to me.

[not-an-aardvark]

I'm not sure it's a good idea to require all parsers to have tokenize just for this niche use case. Could it be exposed as an optional parserServices method instead? (We could also just keep using espree everywhere and fall back to a default behavior gracefully in case espree can't tokenize something.)

[platinumazure]

I'm not sure it's a good idea to require all parsers to have tokenize just for this niche use case. Could it be exposed as an optional parserServices method instead?

I would be okay with this as well.

[ilyavolodin]

We could also just keep using espree everywhere and fall back to a default behavior gracefully in case espree can't tokenize something.

Wouldn't that break every time espree tried to tokenize some new syntax?

[not-an-aardvark]

We could use a try-catch and return a safe default value if espree fails.

astUtils.canTokensBeAdjacent is a function that makes a best-effort attempt to avoid unnecessary spacing in autofixes. It's an extremely minor feature, and it doesn't need to be completely perfect as long as it does fallbacks appropriately (worst-case scenario, some autofixed text will have an unnecessary space in it). That's why I think it's silly to require parsers to supply an entirely new API for this. We should stop it from crashing, but we don't need to do the tokenization perfectly.

[ilyavolodin]

Sounds good to me. I do think we can make it optional for parsers. Since most parsers most likely already have a tokenizer included somewhere in the code, it would be a minor enhancement for them to expose it. But having fallback to espree with try-catch is fine, I think.

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[kaicataldo]

@mdjermanovic Would you like to assign this to yourself to avoid this being auto-closed again?

[mdjermanovic]

Now, after merging #12879, astUtils.canTokensBeAdjacent has:

• espree.latestEcmaVersion instead of a hard-coded version
• try-catch around espree.tokenize()

Based on this comment by @not-an-aardvark and votes on it, it looks like we can close this issue?

[nzakas]

Agreed.