Skip to content

add refactoring: string concatenation to template literals #30565

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

Merged
merged 46 commits into from
Jan 3, 2020
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
2db0745
add skeleton
bigaru Dec 5, 2018
7620615
add test cases
bigaru Dec 5, 2018
2bb2a82
add test cases
bigaru Dec 5, 2018
6952b1f
add visibility tests
bigaru Dec 5, 2018
03f0f88
add diagnostic messages
bigaru Dec 5, 2018
b84f95d
add working conversion to template literal
bigaru Dec 6, 2018
fc13b2b
add test cases
bigaru Dec 6, 2018
3d2b552
complete toTemplate
bigaru Dec 6, 2018
2b29994
complete toString
bigaru Dec 6, 2018
576271e
catch empty head of template literal
bigaru Dec 6, 2018
3b28488
add toString visibility from expression and from middle part
bigaru Dec 7, 2018
76ce1c6
fix test case
bigaru Dec 7, 2018
6fe4663
combine preceding expressions to one
bigaru Dec 7, 2018
6de23d7
do not offer refactoring for tagged templates
bigaru Dec 7, 2018
882e616
optimize preceding expression
bigaru Dec 7, 2018
7d9e8f4
treat corner cases
bigaru Dec 7, 2018
74e3cd7
remove parentheses also when expression at ending
bigaru Dec 7, 2018
1594468
add possibility to invoke from parentheses
bigaru Dec 7, 2018
cba0ddc
only show toString if expression is not binary
bigaru Dec 7, 2018
6721966
extract creation of templateHead
bigaru Dec 7, 2018
08ed6cf
optimize nodesToTemplate
bigaru Dec 7, 2018
ad0614a
extract getEdits for string concatenation
bigaru Dec 7, 2018
9b9aa35
optimize getEdits string concatenation
bigaru Dec 7, 2018
2b08bd3
change from tuple to object literal
bigaru Dec 8, 2018
16109df
optimize templateLiteral check
bigaru Dec 8, 2018
3ce2168
extract getEdits for template literal
bigaru Dec 9, 2018
cf25c12
add test cases
bigaru Dec 9, 2018
806eb12
add skeleton for handling octal escape
bigaru Dec 9, 2018
6a1df73
complete handling for octal escape
bigaru Dec 9, 2018
d2ab0bd
support single quotes when decoding raw string
bigaru Mar 22, 2019
1bcd8da
clean test cases
bigaru Mar 24, 2019
29fc8c3
support case when variable is re-assigned
bigaru Mar 24, 2019
2a15acb
refactor creation of template expression
bigaru Mar 24, 2019
935cf04
optimize and add more tests for parenthesized case
bigaru Mar 27, 2019
8ef6990
catch case when there is only single expr and optimize arrayToTree
bigaru Mar 27, 2019
834c5df
add support for hex and unicode escapes
bigaru Mar 27, 2019
9fa112e
test also the output from visibility tests
bigaru Mar 27, 2019
17f3861
copy comments from template literal to string
bigaru Mar 27, 2019
dd89a49
copy comments from string to template literal
bigaru Mar 28, 2019
4b95c1f
optimize treeToArray
bigaru Mar 29, 2019
04f96db
use stringLiteral property text instead of decodeRawString
bigaru Jun 30, 2019
3e0d34c
remove explicit escaping for placeholder opening
bigaru Jun 30, 2019
35a3a5f
Merge remote-tracking branch 'origin' into convert-to-template
DanielRosenwasser Dec 21, 2019
88795e2
Fixed lints.
DanielRosenwasser Dec 21, 2019
ad0f006
Remove refactoring from template expression to string concatenation.
DanielRosenwasser Jan 3, 2020
ff4fa1f
Cleaned up refactoring names, descriptions.
DanielRosenwasser Jan 3, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -4930,5 +4930,17 @@
"Convert parameters to destructured object": {
"category": "Message",
"code": 95075
},
"Convert string concatenation or template literal": {
"category": "Message",
"code": 95076
},
"Convert to template literal": {
"category": "Message",
"code": 95077
},
"Convert to string concatenation": {
"category": "Message",
"code": 95078
}
}
190 changes: 190 additions & 0 deletions src/services/refactors/convertStringOrTemplateLiteral.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
/* @internal */
namespace ts.refactor.convertStringOrTemplateLiteral {
const refactorName = "Convert string concatenation or template literal";
const toTemplateLiteralActionName = "Convert to template literal";
const toStringConcatenationActionName = "Convert to string concatenation";

const refactorDescription = getLocaleSpecificMessage(Diagnostics.Convert_string_concatenation_or_template_literal);
const toTemplateLiteralDescription = getLocaleSpecificMessage(Diagnostics.Convert_to_template_literal);
const toStringConcatenationDescription = getLocaleSpecificMessage(Diagnostics.Convert_to_string_concatenation);

registerRefactor(refactorName, { getEditsForAction, getAvailableActions });

function getAvailableActions(context: RefactorContext): ReadonlyArray<ApplicableRefactorInfo> {
const { file, startPosition } = context;
const node = getNodeOrParentOfParentheses(file, startPosition);
const maybeBinary = getParentBinaryExpression(node);
const refactorInfo: ApplicableRefactorInfo = { name: refactorName, description: refactorDescription, actions: [] };

if ((isBinaryExpression(maybeBinary) || isStringLiteral(maybeBinary)) && isStringConcatenationValid(maybeBinary)) {
refactorInfo.actions.push({ name: toTemplateLiteralActionName, description: toTemplateLiteralDescription });
return [refactorInfo];
}

const templateLiteral = findAncestor(node, n => isTemplateLiteral(n));

if (templateLiteral && !isTaggedTemplateExpression(templateLiteral.parent)) {
refactorInfo.actions.push({ name: toStringConcatenationActionName, description: toStringConcatenationDescription });
return [refactorInfo];
}

return emptyArray;
}

function getNodeOrParentOfParentheses(file: SourceFile, startPosition: number) {
const node = getTokenAtPosition(file, startPosition);
if (isParenthesizedExpression(node.parent) && isBinaryExpression(node.parent.parent)) return node.parent.parent;
Copy link
Member

Choose a reason for hiding this comment

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

could you explain why this is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the idea behind is that
even if your cursor is inside a parentheses, you can still invoke this refactoring (as long as it's not a string binary expression)

both examples were invoked inside the parentheses:

// before
const foo = "foobar is " + (42 + 6) + " years old"
const fooStr = "foobar is " + (42 + 6 + "years") + " old"

// after
const foo = `foobar is ${42 + 6} years old`
const fooStr = "foobar is " + (`${42 + 6}years`) + " old"

It is not necessary but nice to have

return node;
}

function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined {
const { file, startPosition } = context;
const node = getNodeOrParentOfParentheses(file, startPosition);

switch (actionName) {
case toTemplateLiteralActionName:
return { edits: getEditsForToTemplateLiteral(context, node) };

case toStringConcatenationActionName:
return { edits: getEditsForToStringConcatenation(context, node) };

default:
return Debug.fail("invalid action");
}
}

function getEditsForToTemplateLiteral(context: RefactorContext, node: Node) {
const maybeBinary = getParentBinaryExpression(node);
const arrayOfNodes = transformTreeToArray(maybeBinary);
const templateLiteral = nodesToTemplate(arrayOfNodes);
return textChanges.ChangeTracker.with(context, t => t.replaceNode(context.file, maybeBinary, templateLiteral));
}

function getEditsForToStringConcatenation(context: RefactorContext, node: Node) {
const templateLiteral = findAncestor(node, n => isTemplateLiteral(n))! as TemplateLiteral;

if (isTemplateExpression(templateLiteral)) {
const { head, templateSpans } = templateLiteral;
const arrayOfNodes = templateSpans.map(templateSpanToExpressions)
.reduce((accumulator, nextArray) => accumulator.concat(nextArray));

if (head.text.length !== 0) arrayOfNodes.unshift(createStringLiteral(head.text));

const binaryExpression = arrayToTree(arrayOfNodes);
return textChanges.ChangeTracker.with(context, t => t.replaceNode(context.file, templateLiteral, binaryExpression));
}
else {
const stringLiteral = createStringLiteral(templateLiteral.text);
return textChanges.ChangeTracker.with(context, t => t.replaceNode(context.file, node, stringLiteral));
}
}

function templateSpanToExpressions(templateSpan: TemplateSpan): Expression[] {
const { expression, literal } = templateSpan;
const text = literal.text;
return text.length === 0 ? [expression] : [expression, createStringLiteral(text)];
}

function isNotEqualsOperator(node: BinaryExpression) {
return node.operatorToken.kind !== SyntaxKind.EqualsToken;
}

function getParentBinaryExpression(expr: Node) {
while (isBinaryExpression(expr.parent) && isNotEqualsOperator(expr.parent)) {
expr = expr.parent;
}
return expr;
}

function arrayToTree(nodes: ReadonlyArray<Expression>, accumulator?: BinaryExpression): BinaryExpression {
if (nodes.length === 0) return accumulator!;

if (!accumulator) {
const left = nodes[0];
const right = nodes[1];

const binary = createBinary(left, SyntaxKind.PlusToken, right);
return arrayToTree(nodes.slice(2), binary);
}

const right = nodes[0];
const binary = createBinary(accumulator, SyntaxKind.PlusToken, right);
return arrayToTree(nodes.slice(1), binary);
}

function isStringConcatenationValid(node: Node): boolean {
const { containsString, areOperatorsValid } = treeToArray(node);
return containsString && areOperatorsValid;
}

function transformTreeToArray(node: Node): ReadonlyArray<Expression> {
return treeToArray(node).nodes;
}

function treeToArray(node: Node): { nodes: ReadonlyArray<Expression>, containsString: boolean, areOperatorsValid: boolean} {
if (isBinaryExpression(node)) {
const { nodes: leftNodes, containsString: leftHasString, areOperatorsValid: leftOperatorValid } = treeToArray(node.left);
const { nodes: rightNodes, containsString: rightHasString, areOperatorsValid: rightOperatorValid } = treeToArray(node.right);

if (!leftHasString && !rightHasString) {
return { nodes: [node], containsString: false, areOperatorsValid: true };
}

const currentOperatorValid = node.operatorToken.kind === SyntaxKind.PlusToken;
const areOperatorsValid = leftOperatorValid && currentOperatorValid && rightOperatorValid;

return { nodes: leftNodes.concat(rightNodes), containsString: true, areOperatorsValid };
}

return { nodes: [node as Expression], containsString: isStringLiteral(node), areOperatorsValid: true };
}

function concatConsecutiveString(index: number, nodes: ReadonlyArray<Expression>): [number, string] {
let text = "";

while (index < nodes.length && isStringLiteral(nodes[index])) {
text = text + decodeRawString(nodes[index].getText());
Copy link
Member

Choose a reason for hiding this comment

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

you could cast nodes[index] to StringLiteral and use nodes[index].text instead of using getText. then decodeRawString becomes unnecessary i think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I first used as StringLiteral.
but last time @ajafff pointed out that Octal escapes are disallowed inside template literal

If I use (nodes[index] as StringLiteral).text then happens that:

// before
const foo = "Unicode \u0023 \u{0023} " + "Hex \x23 " + "Octal \43";

// after
const foo = `Unicode # # Hex # Octal 43`;

As you can see the octal escape is not supported with .text That's why I created a suboptimal function decodeRawString. It seems that the parser has a bug.

What do you think?
Should I switch back to: (nodes[index] as StringLiteral).text ?
and provide a fix for the bug (as a separate PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched back to (nodes[index] as StringLiteral).text

@RyanCavanaugh
Is there a reason why octal escape sequence is not interpreted from scanner (even when strict mode is off)

// before
const foo = "Unicode \u0023 \u{0023} " + "Hex \x23 " + "Octal \43";

// after
const foo = `Unicode # # Hex # Octal 43`;

index++;
}

text = escapeText(text);
return [index, text];
}

function nodesToTemplate(nodes: ReadonlyArray<Expression>) {
const templateSpans: TemplateSpan[] = [];
const [begin, headText] = concatConsecutiveString(0, nodes);
const templateHead = createTemplateHead(headText);

if (begin === nodes.length) return createNoSubstitutionTemplateLiteral(headText);

for (let i = begin; i < nodes.length; i++) {
const expression = isParenthesizedExpression(nodes[i]) ? (nodes[i] as ParenthesizedExpression).expression : nodes[i];
const [newIndex, subsequentText] = concatConsecutiveString(i + 1, nodes);
i = newIndex - 1;

const templatePart = i === nodes.length - 1 ? createTemplateTail(subsequentText) : createTemplateMiddle(subsequentText);
templateSpans.push(createTemplateSpan(expression, templatePart));
}

return createTemplateExpression(templateHead, templateSpans);
}

const octalToUnicode = (_match: string, grp: string) => String.fromCharCode(parseInt(grp, 8));

function decodeRawString(content: string) {
const outerQuotes = /["']((.|\s)*)["']/;
const octalEscape = /\\((?:[1-7][0-7]{0,2}|[0-7]{2,3}))/g;

return content.replace(outerQuotes, (_match, grp) => grp)
.replace(octalEscape, octalToUnicode);

}

function escapeText(content: string) {
return content.replace("`", "\`") // back-tick
.replace("${", "$\\{"); // placeholder alike beginning
Copy link
Member

Choose a reason for hiding this comment

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

i think there's an extra backslash here. maybe it should be .replace("${", "$\{")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately if I pass as $\{, the backslash will not be preserved.
because the parser optimize this \{ to {

Copy link
Member

Choose a reason for hiding this comment

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

i tested this locally and this is what happens:
image
so it does seem like there's an extra backslash on the resulting template literal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that "$\\{" changes the external behavior

to explain my dilemma

// before 
const escape = "with ${dollar}"

// after using .replace("${", "$\{")
const escape = `with ${dollar}`

// after using .replace("${", "\${")
const escape = `with ${dollar}`

// after using .replace("${", "$\\{")
const escape = `with $\\{dollar}`

// after using .replace("${", "\\${")
const escape = `with \\${dollar}`

using $\{ or \${

  • the backslash completely disappears after refactoring
  • and therefore, it leaves the template literal in an invalid state

using $\\{ or \\${

  • there is one backslash too much
  • however, the template literal is still in valid state

Both approaches change the external behavior.
As it seems, neither $ nor { can be escaped with single backslash.
However, the backtick ` can be escaped with just a single backslash.

What do you think?
How should I deal with this matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the external behavior cannot be preserved with one or two backslash, I've decided to remove escaping of opening placeholder. .replace("${", "$\\{")

}

}

1 change: 1 addition & 0 deletions src/services/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
"refactors/moveToNewFile.ts",
"refactors/addOrRemoveBracesToArrowFunction.ts",
"refactors/convertParamsToDestructuredObject.ts",
"refactors/convertStringOrTemplateLiteral.ts",
"services.ts",
"breakpoints.ts",
"transform.ts",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/// <reference path='fourslash.ts' />

//// console.log(`/*x*/f/*y*/oobar is ${ 32 } years old`)

goTo.select("x", "y");
edit.applyRefactor({
refactorName: "Convert string concatenation or template literal",
actionName: "Convert to string concatenation",
actionDescription: "Convert to string concatenation",
newContent:
`console.log("foobar is " + 32 + " years old")`,
});

Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/// <reference path='fourslash.ts' />

//// const age = 22
//// const name = "Eddy"
//// const /*z*/f/*y*/oo = /*x*/`/*w*/M/*v*/r/*u*/ /*t*/$/*s*/{ /*r*/n/*q*/ame } is ${ /*p*/a/*o*/ge + 34 } years old`

goTo.select("z", "y");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to string concatenation");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to template literal");

goTo.select("x", "w");
verify.refactorAvailable("Convert string concatenation or template literal", "Convert to string concatenation");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to template literal");

goTo.select("v", "u");
verify.refactorAvailable("Convert string concatenation or template literal", "Convert to string concatenation");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to template literal");

goTo.select("t", "s");
verify.refactorAvailable("Convert string concatenation or template literal", "Convert to string concatenation");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to template literal");

goTo.select("r", "q");
verify.refactorAvailable("Convert string concatenation or template literal", "Convert to string concatenation");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to template literal");

goTo.select("p", "o");
verify.refactorAvailable("Convert string concatenation or template literal", "Convert to string concatenation");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to template literal");

Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/// <reference path='fourslash.ts' />

//// function tag(literals: TemplateStringsArray, ...placeholders: any[]) { return "tagged" }
//// const alpha = tag/*z*/`/*y*/foobar`
//// const beta = tag/*x*/`/*w*/foobar ${/*v*/4/*u*/2}`

goTo.select("z", "y");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to string concatenation");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to template literal");

goTo.select("x", "w");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to string concatenation");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to template literal");

goTo.select("v", "u");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to string concatenation");
verify.not.refactorAvailable("Convert string concatenation or template literal", "Convert to template literal");

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/// <reference path='fourslash.ts' />

//// const foo = `/*x*/f/*y*/oobar is ${ 42 + 6 } years old`

goTo.select("x", "y");
edit.applyRefactor({
refactorName: "Convert string concatenation or template literal",
actionName: "Convert to string concatenation",
actionDescription: "Convert to string concatenation",
newContent:
`const foo = "foobar is " + (42 + 6) + " years old"`,
});

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path='fourslash.ts' />

//// const age = 22
//// const name = "Eddy"
//// const foo = `/*x*/$/*y*/{ name } is ${ age } years old`

goTo.select("x", "y");
edit.applyRefactor({
refactorName: "Convert string concatenation or template literal",
actionName: "Convert to string concatenation",
actionDescription: "Convert to string concatenation",
newContent:
`const age = 22
const name = "Eddy"
const foo = name + " is " + age + " years old"`,
});

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/// <reference path='fourslash.ts' />

//// const age = 42
//// const foo = `foobar is a ${ age < 18 ? 'child' : /*x*/`/*y*/grown-up ${ age > 40 ? 'who needs probably assistance' : ''}` }`

goTo.select("x", "y");
edit.applyRefactor({
refactorName: "Convert string concatenation or template literal",
actionName: "Convert to string concatenation",
actionDescription: "Convert to string concatenation",
newContent:
`const age = 42
const foo = \`foobar is a \${ age < 18 ? 'child' : "grown-up " + (age > 40 ? 'who needs probably assistance' : '') }\``,
});

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/// <reference path='fourslash.ts' />

//// const age = 42
//// const foo = `foobar is a ${ `/*x*/3/*y*/4` }`

goTo.select("x", "y");
edit.applyRefactor({
refactorName: "Convert string concatenation or template literal",
actionName: "Convert to string concatenation",
actionDescription: "Convert to string concatenation",
newContent:
`const age = 42
const foo = \`foobar is a \${ "34" }\``,
});

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/// <reference path='fourslash.ts' />

//// const age = 42
//// const foo = `foobar is a ${ /*x*/a/*y*/ge < 18 ? 'child' : `grown-up ${ age > 40 ? 'who needs probaply assistance': ''}` }`

goTo.select("x", "y");
edit.applyRefactor({
refactorName: "Convert string concatenation or template literal",
actionName: "Convert to string concatenation",
actionDescription: "Convert to string concatenation",
newContent:
`const age = 42
const foo = "foobar is a " + (age < 18 ? 'child' : \`grown-up \${age > 40 ? 'who needs probaply assistance' : ''}\`)`,
});

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/// <reference path='fourslash.ts' />

//// const age = 42
//// const foo = `/*x*/f/*y*/oobar is ${ age } years old`

goTo.select("x", "y");
edit.applyRefactor({
refactorName: "Convert string concatenation or template literal",
actionName: "Convert to string concatenation",
actionDescription: "Convert to string concatenation",
newContent:
`const age = 42
const foo = "foobar is " + age + " years old"`,
});

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/// <reference path='fourslash.ts' />

//// const foo = `/*x*/f/*y*/oobar is ${ 42 * 6 % 4} years old`

goTo.select("x", "y");
edit.applyRefactor({
refactorName: "Convert string concatenation or template literal",
actionName: "Convert to string concatenation",
actionDescription: "Convert to string concatenation",
newContent:
`const foo = "foobar is " + 42 * 6 % 4 + " years old"`,
});

Loading