Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(eslint-plugin): [no-unnecessary-template-expression] add missing parentheses in autofix #8673

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
25 changes: 10 additions & 15 deletions packages/eslint-plugin/src/rules/no-useless-template-literals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
getConstrainedTypeAtLocation,
getParserServices,
getStaticStringValue,
getWrappingCode,
isTypeFlagSet,
isUndefinedIdentifier,
} from '../util';
Expand Down Expand Up @@ -92,21 +93,15 @@ export default createRule<[], MessageId>({
context.report({
node: node.expressions[0],
messageId: 'noUselessTemplateLiteral',
fix(fixer): TSESLint.RuleFix[] {
const [prevQuasi, nextQuasi] = node.quasis;

// Remove the quasis and backticks.
return [
fixer.removeRange([
prevQuasi.range[1] - 3,
node.expressions[0].range[0],
]),

fixer.removeRange([
node.expressions[0].range[1],
nextQuasi.range[0] + 2,
]),
];
fix(fixer): TSESLint.RuleFix | null {
const wrappingCode = getWrappingCode({
sourceCode: context.sourceCode,
replaceNode: node.expressions[0],
originNode: node,
parent: node.parent,
});

return fixer.replaceText(node, wrappingCode);
},
});

Expand Down
27 changes: 24 additions & 3 deletions packages/eslint-plugin/src/util/getWrappingFixer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,23 @@ export function getWrappingFixer(
};
}

export function getWrappingCode(params: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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})`;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks much more intuitive and good. We will reflect it appropriately

sourceCode: Readonly<TSESLint.SourceCode>;
replaceNode: TSESTree.Node;
originNode: TSESTree.Node;
parent: TSESTree.Node;
}): string {
const { sourceCode, replaceNode, originNode, parent } = params;
const code = sourceCode.getText(replaceNode);
const isNodeNeedParen = !isStrongPrecedenceNode(replaceNode);
const isParentNeedParam = isWeakPrecedenceParent(originNode, parent);

if (isNodeNeedParen && isParentNeedParam) {
return `(${code})`;
}
return code;
}

/**
* Check if a node will always have the same precedence if it's parent changes.
*/
Expand All @@ -97,9 +114,13 @@ export function isStrongPrecedenceNode(innerNode: TSESTree.Node): boolean {
/**
* Check if a node's parent could have different precedence if the node changes.
*/
function isWeakPrecedenceParent(node: TSESTree.Node): boolean {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const parent = node.parent!;
function isWeakPrecedenceParent(
node: TSESTree.Node,
parent = node.parent,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

): boolean {
if (!parent) {
return false;
}

if (
parent.type === AST_NODE_TYPES.UpdateExpression ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -637,5 +637,14 @@ declare const nested: string, interpolation: string;
},
],
},
{
code: "true ? `${'test' || ''}`.trim() : undefined;",
output: "true ? ('test' || '').trim() : undefined;",
errors: [
{
messageId: 'noUselessTemplateLiteral',
},
],
},
],
});
Loading