biomejs / biome

A toolchain for web projects, aimed to provide functionalities to maintain them. Biome offers formatter and linter, usable via CLI and LSP.
https://biomejs.dev
Apache License 2.0
14.11k stars 431 forks source link

📎 Implement `nursery/noUselessEscape` - `eslint/no-useless-escape` #1090

Open Conaclos opened 9 months ago

Conaclos commented 9 months ago

Description

Implement no-useless-escape.

Want to contribute? Lets you know you are interested! We will assign you to the issue to prevent several people to work on the same issue. Don't worry, we can unassign you later if you are no longer interested in the issue! Read our contributing guide and analyzer contributing guide.

Notes

The implementer should assess if there is some value of splitting the rule into two rules: noUselessEscapeInString and noUselessEscapeInRegex.

eryue0220 commented 8 months ago

Could I have a try for this?

ematipico commented 8 months ago

All yours @eryue0220

eryue0220 commented 7 months ago

Hi, @ematipico @Conaclos

I have an extra question when I implemented this rule:

Why biome didn't provide a syntax kind of JsTaggedTemplateExpression ?

ematipico commented 7 months ago

@eryue0220

Sorry, I completely forgot to answer. What do you mean exactly? We don't have such node in our grammar, I think.

Can you share a snippet?

eryue0220 commented 7 months ago

@ematipico

It's ok, I'm just curious that Why biome doesn't have this kind for the string template.

var foo = myFunc`\\.`

For example, while eslint parse the code, some of the result looks like:

{
  "//": "...",
  "init": {
     "type": "TaggedTemplateExpression",
     "start": "",
     "end": "",
    "quasi": {
       "type": "TemplateLiteral"
      "//": "..."
     },
  },
}

And in Biome, just have template expression, but no TaggedTemplateExpression.

ematipico commented 7 months ago

Probably because there's no need to have a different node for a template expression with a tag. The tag becomes an optional child of the template expression.

Conaclos commented 1 month ago

Most of the cases covered by the ESLint rule are already handled by our formatter. For instance, "\a" is formatted to "a".

It remains only cases for regexes and untagged template literals.

The formatter should not remove useless escapes in regexes because a user can retrieve the source of a regex (myRegex.source). Thus, I think we could create a dedicated rule noUslessEscapeInRegex.

Regarding untagged template literals, Prettier preserves useless escapes in regexes. I didn't find any opened or closed issue on the Prettier repository about that. I think it could be safe to remove useless escape on untagged template literals when the file is formatted. Unfortunately this could create a divergence with Prettier. I suggest we open an issue on the Prettier repository and see what they say. Issue opened.

Conaclos commented 1 month ago

Prettier will not unescape useless escapes in untagged template literals. Actually they plan to no longer remove useless escapes in any strings, leaving the responsibility to linters.

The question is: should we align with Prettier if they eventually do that? Or diverge from Prettier there ?

chansuke commented 3 weeks ago

I read the argument in Prettier, and my personal opinion aligns with theirs. It seems reasonable that Prettier dropped the removal of backslashes to maintain consistency between template literals and string literals.

Conaclos commented 3 weeks ago

I think I'm leaning towards the Prettier decision too. It looks good to keep strings untouched. We have first to check if they also make the change for useless escape in JSX strings. We should have a consistent approach towards all strings.

Furthermore, if I remember correctly, Prettier replaces some escape sequences with the encoded character. Biome doesn't do that. I think it's also a good thing to keep strings untouched.

If we follow the lead of Prettier, we could provide a rule named noUselessEscapeInString. I think it is good to separate the logic of no-useless-escape into two rules noUselessEscapeInString and noUselessEscapeInRegex.

ematipico commented 3 weeks ago

If we decide to go that way, that will be a breaking change, so we will have to release it for v2

Conaclos commented 3 weeks ago

If we decide to go that way, that will be a breaking change, so we will have to release it for v2

We cite the goal of following Prettier? We already make such change to match Prettier?

Anyway, I think we should wait that the change be released by Prettier before making the change.