fix(eslint-plugin): [no-unnecessary-template-expression] do not render escaped strings in autofixes #8688
Conversation
…ed strings in autofixes
|
Thanks for the PR, @auvred! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v8 #8688 +/- ##
==========================================
- Coverage 87.91% 87.91% -0.01%
==========================================
Files 397 397
Lines 13558 13577 +19
Branches 3939 3947 +8
==========================================
+ Hits 11920 11936 +16
- Misses 1324 1327 +3
Partials 314 314
Flags with carried forward coverage won't be shown. Click here to find out more.
|
packages/eslint-plugin/src/rules/no-useless-template-literals.ts
Outdated
Show resolved
Hide resolved
|
good stuff here! In addition to the comment, one thing though that I want to flag is that in the issue it was also intended that `${'a'}`;
// should autofix to
`a`;
// rather than
'a';though it's possible that that flew under the radar on the issue itself as well when it was marked accepting PRs. I also note that #8669 somewhat conflicts with that change (in that it would be made redundant). Could be worth flagging there, and/or dropping the quotes -> backticks change from #8677 entirely and mentioning that explicitly on the issue. Not strongly opinionated to which decision is made but just think we should document it. |
There was a problem hiding this comment.
In addition to the comment, one thing though that I want to flag is that in the issue it was also intended that
`${'a'}`; // should autofix to `a`; // rather than 'a';
@kirkwaiblinger, Ohh, I missed that in the original issue!
I'm 50/50 on this!
though it's possible that that flew under the radar on the issue itself as well when it was marked accepting PRs.
Maybe..
Fixing to string literal vs template literal sounds a bit stylistic to me, I think it would be great to hear more opinions on this!
There was a problem hiding this comment.
Yeah, agreed. I will say, at the time, I didn't realize it was just removing the wrapping template syntax, as opposed to overriding whatever the user had typed with one particular quote style. So my motivation for that change has softened a lot as well. It still feels a little surprising to me, but it does have a very justifiable internal logic as-is.
So unless someone comes in and has a strong opinion, I'd say, just leave it as-is :)
There was a problem hiding this comment.
Yeah +1, I think ideally it wouldn't change the syntax, but that's a small implementation detail and not worth doing a bunch of work for. If folks care they should lint for it separately.
| if (isLiteral(expression)) { | ||
| const escapedValue = | ||
| typeof expression.value === 'string' | ||
| ? expression.raw.slice(1, -1).replace(/([`$])/g, '\\$1') |
There was a problem hiding this comment.
(nit, optional) - why are the escaping patterns different for each branch? Maybe worth a comment?
There was a problem hiding this comment.
Yeah, it does seem a bit confusing
Does e07e14607+ae589b436 clear it up?
There was a problem hiding this comment.
oof, i think i get it, but i still struggle tbh 😆 It's just really tricky to me to figure out which things are in escaped-land and which things are non-escaped in the code comment there (it's even hard for me to type out the special characters in this comment 😆 ). Is the idea
raw string ⇒
- Removing enclosing quotation characters
- just add escapes to characters that need escape in template string, but didn't as a string literal (` and $)
cooked string ⇒
- escape everything that needs escapes (`, $, and \)
I really think a few English words would be helpful in the code comments (to go along with the examples), since my brain is not big enough to make meaning of all the special characters at a glance 😄
packages/eslint-plugin/tests/rules/no-useless-template-literals.test.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/no-useless-template-literals.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/no-useless-template-literals.ts
Outdated
Show resolved
Hide resolved
|
Thanks for the reviews! I tried to cover all the possible edge cases (although I've likely missed something). Currently this PR is blocked by #8883, because some test cases, like this one below, require multiple fixer passes: ` ${'$'}${''}{} `;So I'm going to convert this PR do a draft until #8554 issue is resolved. |
|
#8554 resolved! I will revisit and mark this PR as ready for review in a little while! Marking it as |
There was a problem hiding this comment.
I wonder - do you think it'd be worth having a 2nd pass test here which does like
cases.invalid.forEach(
testCase =>
expect(eval(testCase.code)).toEqual(eval(testCase.output))
)to really validate that we don't regress the value transform?
probably not worth the effort but definitely an interesting thought...
| code: '`${0b1010} ${0b1111}`;', | ||
| output: '`10 15`;', |
There was a problem hiding this comment.
part of me wonders if we should not handle these weird number representation cases and just leave them as is.
eg I could see more value in writing code with a bunch of variations on the binary string to ensure completeness vs having the converted int val.
for example
`${0b00} ${0b01} ${0b10} ${0b11}` encodes information better (i.e. that all binary cases are represented) rather than
`0 1 2 3`Obviously this is a simplified case but one could imagine some complex binary sequence represented (eg power of two or something).
Same reason that it's not uncommon for people to define flag enums as
enum Flag {
A = 1 << 0,
B = 1 << 1,
C = 1 << 2,
D = 1 << 3,
E = 1 << 4,
}
// rather than
enum Flag {
A = 1,
B = 2,
C = 4,
D = 8,
E = 16,
}
probably just put a pin in it and leave it as a potential future exercise
There was a problem hiding this comment.
Interesting point, yea, I can imagine someone would want their binary/hex numbers to remain untouched. But this PR already has a lot of changes beyond the original issue. So I'd say we should file a follow-up issue or wait for someone to report it.
probably just put a pin in it and leave it as a potential future exercise
👍 +1 here
Wow, I was waiting for someone to mention this! That's exactly what I did locally, and it helped catch a lot of errors, but I didn't see this being common practice in typescript-eslint, so I decided not to commit it! I'll be sure to add those tests! |
There was a problem hiding this comment.
What a complicated problem this turned out to be!
I've spent some amount time in the playground testing out confusing test cases...
- They all passed
- When I looked closer, you already had an equivalent for each of them in your added test cases. Great job on test thoroughness!
My only complaint is that the big fixableExpressions.forEach() callback has gotten huge (~100 lines - in fairness, many of which are comments) and really complicated (order-dependent control flow, context.report() side effects, local functions, and several different behaviors based on node type).
Now, to be sure, a fair amount of that complexity may be irreducible, since... this is just a complex problem. So, I'm ok with merging as is (especially in light of your excellent test coverage 🙂). But if you think that there is room for some simplification, I would certainly welcome it as a reader. It does look like at a minimum the regex evenNumOfBackslashesRegExp and local function endsWithUnescapedDollarSign could be taken out of the callback, since don't use any variables or state from the closure.
So, leaving it to your discretion if you'd like to click the green merge button or make any further tweaks.
Most importantly, though, once again great job in tackling this really complex bug!
|
Oh, FYI - please, before merging, double check whether this will cause heinous merge conflicts with #8673 or not. If so, maybe wait for that to go through, since it's targeted to main rather than v8. |
|
Actually - there is one oddity stemming from the fact that the fixes are mutually dependent, though, I don't think I'd consider it a bug. If you're using the editor, you can apply fixes separately, rather than all at once... So, given `${'$'}${'{'}`
~~~ ~~~if we fix the first error only in the editor, we'll get `\$${'{'}`
~~~even though `$${'{'}`
~~~would have been fine. Note that `${'$'}${''}`
~~~ ~~fixes the first error to `$${''}`
~~I think that's an acceptable quirk that will most likely go completely unnoticed by users. Just noting it here to leave a bradcrumb unless some really obscure bug somehow arises in relation to this. |
Yes, I can imagine how difficult this is to read for someone who hasn't struggled with this code for a decent amount of time, but I don't see any way to simplify this code :(
Fortunately, #8673 changes a part of the rule that wasn't touched in this PR, so I think there should be no problem with the merge! |
PR Checklist
Overview
Addresses many issues with escaping of
`and${. To better understand what's going on here, I'd suggest reading the comments in the code.NOTE: I changed the base branch for this PR to
v8since this PR uses multipass fixes - #8883