feat: Fix suggestion for "no-template-curly-in-string"

[ajuanjojjj]

Prerequisites checklist

• I have read the contributing guidelines.

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[x] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Added a suggestion to rule "no-template-curly-in-string", that will trim the 1st and last characters from an erroring snippets, and replace them with backticks to convert the string to a template.

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

I haven't made new tests for this functionality; I haven't used anything similar in the past and I don't have time today to look into it, so I'd rather make the PR as a draft today and have someone point me in the right direction. My tests were successful, but I understand that's not good enough.

[linux-foundation-easycla]

• ❌ The commit (f49dde9) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

[eslint-github-bot]

Hi @ajuanjojjj!, thanks for the Pull Request

The first commit message 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

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

[linux-foundation-easycla]

• ❌ The commit (f49dde9) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

[eslint-github-bot]

Hi @ajuanjojjj!, thanks for the Pull Request

The first commit message 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

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

[nzakas]

Without seeing the tests, it’s hard to know if this change makes sense. If you’d like to provide a few examples of the suggestions being made, we can let you know if it’s something we’d consider.

[ajuanjojjj]

I tested it with most of the warnings I had on my code, as well as all the tests from the rule, with no unexpected results.

//This
test = 'Hello, ${name}';
test = "Hello, ${name}";
test = '${greeting}, ${name}';
test = 'Hello, ${index + 1}';
test = 'Hello, ${name + " foo"}';
test = 'Hello, ${name || "foo"}';
test = 'Hello, ${{foo: "bar"}.foo}';

//Was changed into this
test = `Hello, ${name}`;
test = `Hello, ${name}`;
test = `${greeting}, ${name}`;
test = `Hello, ${index + 1}`;
test = `Hello, ${name + " foo"}`;
test = `Hello, ${name || "foo"}`;
test = `Hello, ${{foo: "bar"}.foo}`;

However, I just realised I wasn't taking into account escaping the backticks inside the string, nor unexcaping the old quotes. I'm working on a commit that fixes both these issues, hopefully I'm able to complete it today (Stuck on regex :/).

I also added some more tests for the normal rule, in order to handle the suggestions on all the quotes situations i could come up with, although they check the suggestions, just the rule (Yet!)

//This
test =  "\"the\" 'brown' `fox` ${jumped} ${\"over\"} ${'the'} ${`lazy`} dog ";
test = '\'Hello\', ${\'name\'}';
test = '"Hello", ${"name"}';
test = '`Hello`, ${`name`}';
test = "'Hello', ${'name'}";
test = "\"Hello\", ${\"name\"}";
test = "`Hello`, ${`name`}";

//Was changed into this
test =  `"the" 'brown' \`fox\` ${jumped} ${"over"} ${'the'} ${`lazy`} dog `;
test = `'Hello', ${'name'}`;
test = `"Hello", ${"name"}`;
test = `\`Hello\`, ${`name`}`;
test = `'Hello', ${'name'}`;
test = `"Hello", ${"name"}`;
test = `\`Hello\`, ${`name`}`;

[mdjermanovic]

This could produce syntactically invalid code, depending on the content of the original string literal. In particular, the content inside ${ ... } substrings, as they now become JS expressions.

For example:

"${*}"

After the suggestion is applied:

`${*}` // Parsing error: Unexpected token *

We recently had a discussion about suggestions (#15279 and 2021-12-16 meeting), and one of the conclusions was that suggested fixes shouldn't produce syntax errors. This fix adds new, unknown code parts that were originally just the contents of a string. Probably the only way to check if the produced code is syntactically valid is to parse the whole file again, but I'm not sure if we want to do that.

@eslint/eslint-tsc thoughts about this?

[ajuanjojjj]

That's the main reason behind making it a "Suggestion" as opposed to a fix. I'm not completly sure of how they work via the command line, I assumed they each had their own behaviour, but if this is not the case, I can easily see why this would be a bad idea; automatic fixes shouldn't break stuff

[btmills]

To avoid re-parsing the whole file, would it be valid to check the syntax by re-parsing the string by itself as a bare ExpressionStatement? How context-dependent is the template curly expression grammar?

[mdjermanovic]

In TemplateLiterals, template curly expression can be any Expression, so parsing template literal's code as a program and checking if the parsing has successfully produced Program with a body consisting of a single ExpressionStatement would mostly work well.

There are some cases where this check could mistakenly allow a fix that does produce syntax errors due to the surrounding context, but those are probably all edge cases. For example:

class C {
    x = "${arguments}";
}

There are also some edge cases where this check could mistakenly prevent a valid fix, but that's certainly acceptable.

class C {
    foo() {
        "${super.foo()}";
    }
}

[ajuanjojjj]

Hopefully someone can explain the issue with the lookbehind regex; adding the u tag to satisfy require-unicode-regexp results in parse error :|

[nzakas]

Where are we on this pull request? What are the next steps?

[ajuanjojjj]

There's a "high severity security vulnerability" on an unescaped regex, the need to decide on the fact that the fix can does produce syntax errors on some (probably edge) cases, and to implement the #15574 (comment) tests here for the fix, since as of now they're just checking for rule trigger

[nzakas]

Hmm, I’m not sure what that error is about. @eslint/eslint-tsc any ideas?

[nzakas]

@eslint/eslint-tsc pinging again

[mdjermanovic]

This suggestion turns strings into code. In a recent TSC meeting, we agreed that suggestions should not produce parsing errors. Unless we want to change that policy, this suggestion would have to run a parser to check if the produced code is syntactically valid. This would be the first core rule that runs parsing. The main question is do we want to do that. Then, would the suggestion use the configured parser or always just espree, and is it necessary to parse the whole file (#15574 (comment)).

[mdjermanovic]

I'm not sure what exactly CodeQL expects here, but this doesn't check if the backtick was already escaped.

const textNoQuotes = "\\`";
const escapeBackticks = textNoQuotes.replace(/`/gui, "\\`");

console.log(textNoQuotes); // \`
console.log(escapeBackticks); // \\`

For example, when the suggestion fixes this string:

/* eslint no-template-curly-in-string: "error" */

var foo = "\`${bar}";

it will produce a template literal with escaped backslash, but the backtick isn't escaped:

/* eslint no-template-curly-in-string: "error" */

var foo = `\\`${bar}`; // Parsing error, unexpected token $

[btmills]

Hi @ajuanjojjj! Thank you for your work on this already to improve ESLint. Unfortunately, the parsing-related complexity required to make sure these suggestions would be valid outweighs the benefit, so we’ve decided not to add suggestions for this rule. While this particular feature isn’t viable, I hope you contribute to ESLint again in the future!

[ajuanjojjj]

Hi @btmills;

Although I understand and obviously respect the decision, I (As just an user with not that much experience in the code behind eslint) humbly disagree, at least on the fact that the complexity outweights the benefit.

I'm sure thought has been put in this general stance, taking into account pros and cons to it, but nevertheless, I'd still like to express my point of view. It's my belief that it severely limits what eslint suggestions and fixes can do as long as this roadblock is still in place. Its not just about this rule in particular, but lots of other rules whose fixes could introduce problems.

I do understand that the main scope for the project is warning and asking users to manually fix stuff, specially with multiple devs working on a project. However, IMO eslint becomes a way more powerful tool when used alongside an IDE that allows you to quickly fix the issues with your code, or someone else's; probably quicker for most of the devs! (Although I know some mechanography gods will disagree).

Hopefuly this is reconsidered in the future; maybe when there's more interest in adding suggestions as a whole (Assuming it even happens, maybe I'll die alone on this hill lol), or when there's more free dev time to work on the requirements to make it possible. I'll still be eager to help with what I can, and needless to say, if I find another thing to contribute in, I won't hesitate to do so, at least if it's not as overly complex as this one ended up being!