fix(eslint-plugin): [no-unnecessary-template-expression] add missing parentheses in autofix #8673
Conversation
|
Thanks for the PR, @developer-bandi! 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. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 1d536b6. 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 2 targetsSent with 💌 from NxCloud. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8673 +/- ##
==========================================
- Coverage 88.02% 88.01% -0.01%
==========================================
Files 406 406
Lines 13871 13881 +10
Branches 4053 4056 +3
==========================================
+ Hits 12210 12218 +8
- Misses 1352 1354 +2
Partials 309 309
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
| const replaceResult = getWrappingFixer({ | ||
| sourceCode: context.sourceCode, | ||
| node: node.expressions[0], | ||
| wrap: (...code: string[]) => { | ||
| return code.join(''); | ||
| }, | ||
| })(fixer); |
There was a problem hiding this comment.
[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 thegetWrappingFixerinto 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?
There was a problem hiding this comment.
I think I misunderstood the explanation. thank you The code generation part that generates parentheses was separated and made into a function.
| @@ -207,7 +207,7 @@ ruleTester.run('no-useless-template-literals', rule, { | |||
|
|
|||
| { | |||
| code: noFormat`\`\${ 'a' + 'b' }\`;`, | |||
| output: `'a' + 'b';`, | |||
| output: `('a' + 'b');`, | |||
There was a problem hiding this comment.
How come this test case gains parens? Is that just the getWrappingFixer being overly cautions?
There was a problem hiding this comment.
I also have this question. Feels like a bug to me?
There was a problem hiding this comment.
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?
| export function getWrappingFixer( | ||
| params: WrappingFixerParams, | ||
| ): TSESLint.ReportFixFunction { | ||
| export function getWrappingCode(params: WrappingFixerParams): string { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
this might well be related actually: https://github.com/typescript-eslint/typescript-eslint/pull/8673/files/5a9f63085e1a3b08ae1c63e29deb34ca93b5165d#r1573616588
There was a problem hiding this comment.
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.
| export function getWrappingFixer( | ||
| params: WrappingFixerParams, | ||
| ): TSESLint.ReportFixFunction { | ||
| export function getWrappingCode(params: WrappingFixerParams): string { |
There was a problem hiding this comment.
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.
|
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 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! |
There was a problem hiding this comment.
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.
| const parent = node.parent!; | ||
| function isWeakPrecedenceParent( | ||
| node: TSESTree.Node, | ||
| parent = node.parent, |
There was a problem hiding this comment.
Let's keep the parent deduced by the node... so,
function isWeakPrecedenceParent(node: TSESTree.Node): boolean {
const parent = node.parent;
if (!parent) {
return false;
}| @@ -75,6 +75,23 @@ export function getWrappingFixer( | |||
| }; | |||
| } | |||
|
|
|||
| export function getWrappingCode(params: { | |||
There was a problem hiding this comment.
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})`;
}There was a problem hiding this comment.
It looks much more intuitive and good. We will reflect it appropriately
| ]), | ||
| ]; | ||
| fix(fixer): TSESLint.RuleFix | null { | ||
| const wrappingCode = getMovedNodeCode({ |
There was a problem hiding this comment.
(nit) the name wrappingCode is slightly stale with the rename, but I don't have many good ideas for a better name 🙃. Any ideas? codeToReplaceTemplate? Up to you if you want to change this or leave it 🤷♂️
There was a problem hiding this comment.
Since the function name is getMovedNodeCode, how about changing the variable name to movedNodeCode? If you don't think it's appropriate, it might be a good idea to keep the current name.
|
👋 @developer-bandi just checking in, is this something you still have time and energy for? |
|
@JoshuaKGoldberg I haven't been able to proceed with the work recently due to lack of time. �I plan to start working on it this weekend. |
…eslint into fix/no-useless-template-literals
|
copy work to [no-unnecessary-template-expression] and delete no-useless-template-literals to resolve conflict |
| return code; | ||
| } | ||
|
|
||
| if (!isWeakPrecedenceParent(destinationNode)) { |
There was a problem hiding this comment.
[Praise] Great reuse of the existing functions!
90655d1
into
typescript-eslint:main
PR Checklist
Overview
Fix no consider precedence when fixer function is excution using util function.