fix(eslint-plugin): [no-useless-template-literals] add parentheses consider precedence #8673

developer-bandi · 2024-03-14T16:06:09Z

PR Checklist

Addresses an existing open issue: fixes Bug: [no-unnecessary-template-expression] Auto fix will change meaning of code #8669
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Fix no consider precedence when fixer function is excution using util function.

typescript-eslint · 2024-03-14T16:06:13Z

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.

netlify · 2024-03-14T16:06:27Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`0bd9dcd`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6641036dbd93a60008a8cdf5
😎 Deploy Preview	https://deploy-preview-8673--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 90 (🔴 down 6 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2024-03-14T16:07:25Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 0bd9dcd. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 31 targets

Sent with 💌 from NxCloud.

codecov · 2024-03-14T16:19:46Z

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.35%. Comparing base (01cbca5) to head (f4c697e).
Report is 333 commits behind head on main.

❗ Current head f4c697e differs from pull request most recent head 0bd9dcd

Please upload reports for the commit 0bd9dcd to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8673      +/-   ##
==========================================
+ Coverage   87.21%   87.35%   +0.14%     
==========================================
  Files         251      255       +4     
  Lines       12305    12505     +200     
  Branches     3880     3925      +45     
==========================================
+ Hits        10732    10924     +192     
- Misses       1303     1306       +3     
- Partials      270      275       +5

Flag	Coverage Δ
unittest	`87.35% <77.77%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...t-plugin/src/rules/no-useless-template-literals.ts	`96.00% <77.77%> (-4.00%)`	⬇️

... and 34 files with indirect coverage changes

JoshuaKGoldberg

I like the approach here of using getWrappingFixer! 👏

I also think the code can be refactored a bit to remove the extra layer of indirection & the as. Requesting changes on that please. :)

packages/eslint-plugin/src/rules/no-useless-template-literals.ts

auvred · 2024-04-08T15:54:02Z

packages/eslint-plugin/src/rules/no-useless-template-literals.ts

+  const replaceResult = getWrappingFixer({
+    sourceCode: context.sourceCode,
+    node: node.expressions[0],
+    wrap: (...code: string[]) => {
+      return code.join('');
+    },
+  })(fixer);


[Refactor] Coming from #8673 (comment):

Instead of adding the overhead of the rest of getWrappingFixer(), suggestion: the specific text generation stuff should be extracted out and used here. Like, you could move the text generation from the getWrappingFixer into a separate function, and use that separate helper here.

I think this means that we should extract codegen part from getWrappingFixer implementation to the separate utility function, not extract whole getWrappingFixer call to the separate function. WDYT?

I think I misunderstood the explanation. thank you The code generation part that generates parentheses was separated and made into a function.

kirkwaiblinger · 2024-04-21T05:08:44Z

packages/eslint-plugin/tests/rules/no-useless-template-literals.test.ts

@@ -207,7 +207,7 @@ ruleTester.run('no-useless-template-literals', rule, {

    {
      code: noFormat`\`\${    'a' + 'b'    }\`;`,
-      output: `'a' + 'b';`,
+      output: `('a' + 'b');`,


How come this test case gains parens? Is that just the getWrappingFixer being overly cautions?

I also have this question. Feels like a bug to me?

At first, I thought it would be good for the expression to be wrapped, but as you said, I think it's excessive and looks like a bug.

However, in order to restore the test code, the getWrappingFixer function must be modified. Would it be a good idea to modify this function?

kirkwaiblinger · 2024-04-21T05:23:01Z

packages/eslint-plugin/src/util/getWrappingFixer.ts

-export function getWrappingFixer(
-  params: WrappingFixerParams,
-): TSESLint.ReportFixFunction {
+export function getWrappingCode(params: WrappingFixerParams): string {


One thing worth noting is that this uses the parent node to infer whether parens are needed (see if (isWeakPrecedenceParent(node))). So I'm wondering if that could give mistaken results for a scenario where we're reparenting code?

I understand that you are using the above node to infer whether parentheses are needed, but I don't understand this part (So I'm wondering if that could give mistaken results for a scenario where we're reparenting code?). Can you tell me in more detail what the scenario is for reparenting the code?

Sorry for taking a bit to get back to you - took me a while to figure out what I meant 😆 . Basically, getWrappingFixer() allows you to transform a node whose parent did not change.

// input <stuff before> nodeToBeModified <stuff after> // output <stuff before> modifiedNode <stuff after>

But, now that getWrappingCode has been extracted, it can be used instead like

// input <stuff before> nodeToBeModified <stuff after> // getWrappingCode output modifiedNode // used in fixer as <DIFFERENT stuff before> modifiedNode <DIFFERENT stuff after>

In fact, that's the whole reason that you've extracted it in this PR.

// input condition ? `${'left' || 'right'}`.trim() : 'lol'; // getWrappingCodeOutput ('left' || 'right') // used in fixer as condition ? ('left' || 'right').trim() : 'lol';

So, getWrappingCode needs to deduce whether to wrap the expression in parentheses based on the NEW parent, not the existing one.

Used within the getWrappingFixer those were the one and the same, so the line if (isWeakPrecedenceParent(node)) was justified, but now that getWrappingCode may be used indepdently, we'd need something like if (isWeakPrecedenceNewParent(node, newParent)), in order for the function to give expected results in general.

I'd have to have a longer think to come up with test cases where this discrepancy would manifest, but hopefully this gets you started? If it's still unclear let me know and I can think up some test cases. The bottom line is that we need to be aware of whether parens are needed for the new parent, not the existing parent, in order for getWrappingCode to be safe for general use outside of getWrappingFixer.

this might well be related actually: https://github.com/typescript-eslint/typescript-eslint/pull/8673/files/5a9f63085e1a3b08ae1c63e29deb34ca93b5165d#r1573616588

I apologize for not clear question. But the answers were the best!

If my understanding is correct, the parent that the isWeakPrecedenceNewParent function checks must be the new parent. In fact, the previous code was checking the parent of the expression, so it was always checking the templete literal node as the parent, so it was not getting normal results.

However, if i slightly modify the code to satisfy the above conditions, i will get the following results depending on the conditions of innerNode and parents condition.

// before true ? `${'test' || ''}`.trim() : undefined; // after true ? (('test' || '')).trim() : undefined;

In my opinion, it seems better to use parentheses when both the logic of applying parentheses to innerNode and the logic of examining the state of the parent are satisfied. so i revert getWrappingFixer and make new getWrappingCode function

I think we're just getting started, so i appreciate your feedback.

kirkwaiblinger · 2024-05-08T18:26:46Z

packages/eslint-plugin/src/util/getWrappingFixer.ts

-export function getWrappingFixer(
-  params: WrappingFixerParams,
-): TSESLint.ReportFixFunction {
+export function getWrappingCode(params: WrappingFixerParams): string {


Sorry for taking a bit to get back to you - took me a while to figure out what I meant 😆 . Basically, getWrappingFixer() allows you to transform a node whose parent did not change.

// input <stuff before> nodeToBeModified <stuff after> // output <stuff before> modifiedNode <stuff after>

But, now that getWrappingCode has been extracted, it can be used instead like

// input <stuff before> nodeToBeModified <stuff after> // getWrappingCode output modifiedNode // used in fixer as <DIFFERENT stuff before> modifiedNode <DIFFERENT stuff after>

In fact, that's the whole reason that you've extracted it in this PR.

// input condition ? `${'left' || 'right'}`.trim() : 'lol'; // getWrappingCodeOutput ('left' || 'right') // used in fixer as condition ? ('left' || 'right').trim() : 'lol';

So, getWrappingCode needs to deduce whether to wrap the expression in parentheses based on the NEW parent, not the existing one.

Used within the getWrappingFixer those were the one and the same, so the line if (isWeakPrecedenceParent(node)) was justified, but now that getWrappingCode may be used indepdently, we'd need something like if (isWeakPrecedenceNewParent(node, newParent)), in order for the function to give expected results in general.

I'd have to have a longer think to come up with test cases where this discrepancy would manifest, but hopefully this gets you started? If it's still unclear let me know and I can think up some test cases. The bottom line is that we need to be aware of whether parens are needed for the new parent, not the existing parent, in order for getWrappingCode to be safe for general use outside of getWrappingFixer.

kirkwaiblinger · 2024-06-03T03:55:06Z

FYI @developer-bandi - This will have a semantic merge conflict with #8821, which has now been merged. When this is ready to go, you'll want to duplicate the fix to the no-unnecessary-template-expression copy of the rule

Also, I'm sorry for taking so long to do another review pass. I'm setting an intention for myself to get another pass in this week. Please bug me if I don't get to it within that time!

kirkwaiblinger

I spent a long time playing around with this on my machine.... this is a difficult area of the code. Requesting a few changes for clarity, but I think the essential logic is in place! 🙂

Also, would like to tag in another member of @typescript-eslint/triage-team to help make sure we're on the right track.

kirkwaiblinger · 2024-06-21T04:46:05Z

packages/eslint-plugin/src/util/getWrappingFixer.ts

-  const parent = node.parent!;
+function isWeakPrecedenceParent(
+  node: TSESTree.Node,
+  parent = node.parent,


Let's keep the parent deduced by the node... so,

function isWeakPrecedenceParent(node: TSESTree.Node): boolean { const parent = node.parent; if (!parent) { return false; }

kirkwaiblinger · 2024-06-21T04:57:27Z

packages/eslint-plugin/src/util/getWrappingFixer.ts

@@ -75,6 +75,23 @@ export function getWrappingFixer(
  };
 }

+export function getWrappingCode(params: {


What do you think of reframing this function to look something like this?

/** * Useful jsdoc here */ export function getMovedNodeCode(params: { sourceCode: Readonly<TSESLint.SourceCode>; nodeToMove: TSESTree.Node; destinationNode: TSESTree.Node; }): string { const { sourceCode, nodeToMove: existingNode, destinationNode } = params; const code = sourceCode.getText(existingNode); if (isStrongPrecedenceNode(existingNode)) { // Moved node never needs parens return code; } if (!isWeakPrecedenceParent(destinationNode)) { // Destination would never needs parens, regardless what node moves there return code; } // Parens may be necessary return `(${code})`; }

fix: precedence concider with parentheses

e2f2e7f

JoshuaKGoldberg requested changes Mar 25, 2024

View reviewed changes

packages/eslint-plugin/src/rules/no-useless-template-literals.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Mar 25, 2024

refactor: seperate replaceNodeText fn

f4c697e

developer-bandi requested a review from JoshuaKGoldberg March 30, 2024 02:36

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 30, 2024

bradzacher added the bug Something isn't working label Apr 4, 2024

auvred requested changes Apr 8, 2024

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Apr 8, 2024

refactor: make getWrappingCode fn

5a9f630

developer-bandi requested a review from auvred April 10, 2024 07:00

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 10, 2024

This was referenced Apr 14, 2024

feat(eslint-plugin): [no-useless-template-literals] rename to no-useless-template-expression (deprecate no-useless-template-literals) #8821

Merged

Repo: Ideas for avoiding drift between a renamed rule and the original #8935

Open

kirkwaiblinger reviewed Apr 21, 2024

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Apr 23, 2024

developer-bandi requested a review from kirkwaiblinger April 28, 2024 14:00

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 28, 2024

kirkwaiblinger requested changes May 8, 2024

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label May 8, 2024

developer-bandi added 2 commits May 13, 2024 00:39

fix: make new fn

67f19bd

fix: revert getWrappingFixer and make getWrappingCode

92db97f

developer-bandi requested a review from kirkwaiblinger May 12, 2024 15:41

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label May 12, 2024

fix: lint error fix

28ca1d0

developer-bandi added 3 commits May 13, 2024 02:39

fix: lint error fix

8290e5a

fix: lint error fix

90e8748

fix: lint error fix

0bd9dcd

kirkwaiblinger mentioned this pull request Jun 3, 2024

feat(eslint-plugin): remove no-useless-template-literals #9207

Merged

3 tasks

abrahamguo mentioned this pull request Jun 3, 2024

feat: use eslint 9 internally #9119

Open

3 tasks

kirkwaiblinger requested changes Jun 21, 2024

View reviewed changes

kirkwaiblinger requested a review from a team June 21, 2024 05:06

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label Jun 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [no-useless-template-literals] add parentheses consider precedence #8673

fix(eslint-plugin): [no-useless-template-literals] add parentheses consider precedence #8673

developer-bandi commented Mar 14, 2024

typescript-eslint bot commented Mar 14, 2024

netlify bot commented Mar 14, 2024 •

edited

nx-cloud bot commented Mar 14, 2024 •

edited

codecov bot commented Mar 14, 2024 •

edited

JoshuaKGoldberg left a comment

auvred Apr 8, 2024

developer-bandi Apr 10, 2024

kirkwaiblinger Apr 21, 2024

JoshuaKGoldberg Apr 23, 2024

developer-bandi Apr 28, 2024

kirkwaiblinger Apr 21, 2024

developer-bandi Apr 28, 2024

kirkwaiblinger May 8, 2024

kirkwaiblinger May 8, 2024

developer-bandi May 12, 2024

kirkwaiblinger May 8, 2024

kirkwaiblinger commented Jun 3, 2024 •

edited

kirkwaiblinger left a comment

kirkwaiblinger Jun 21, 2024

kirkwaiblinger Jun 21, 2024

fix(eslint-plugin): [no-useless-template-literals] add parentheses consider precedence #8673

Are you sure you want to change the base?

fix(eslint-plugin): [no-useless-template-literals] add parentheses consider precedence #8673

Conversation

developer-bandi commented Mar 14, 2024

PR Checklist

Overview

typescript-eslint bot commented Mar 14, 2024

netlify bot commented Mar 14, 2024 • edited

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Mar 14, 2024 • edited

☁️ Nx Cloud Report

codecov bot commented Mar 14, 2024 • edited

Codecov Report

JoshuaKGoldberg left a 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

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

Choose a reason for hiding this comment

kirkwaiblinger commented Jun 3, 2024 • edited

kirkwaiblinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Mar 14, 2024 •

edited

nx-cloud bot commented Mar 14, 2024 •

edited

codecov bot commented Mar 14, 2024 •

edited

kirkwaiblinger commented Jun 3, 2024 •

edited