Skip to content

Commit 7e8bba6

Browse files
elibarzilayelibarzilay
committed
Fix template string refactoring and nodeFactory bug
Instead of letting `createTemplate*` generate a broken raw string from the cooked one, grab the source code for it. Also, add a missing bit to `\`-quote `$`s. As the comment in the code says, it could just `\`-quote `${` since other `$`s are valid, but I think that it's less confusing to always quote $s (but the change is in the comment if minimalism is preferred). Also, a small-but-confusing bug in `getCookedText()`. Many tests for all of this. Fixes microsoft#40625
1 parent 52ca2dd commit 7e8bba6

File tree

3 files changed

+134
-14
lines changed

3 files changed

+134
-14
lines changed

‎src/compiler/factory/nodeFactory.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6148,7 +6148,7 @@ namespace ts {
61486148
}
61496149

61506150
let token = rawTextScanner.scan();
6151-
if (token === SyntaxKind.CloseBracketToken) {
6151+
if (token === SyntaxKind.CloseBraceToken) {
61526152
token = rawTextScanner.reScanTemplateToken(/*isTaggedTemplate*/ false);
61536153
}
61546154

‎src/services/refactors/convertStringOrTemplateLiteral.ts

Lines changed: 37 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -147,61 +147,85 @@ namespace ts.refactor.convertStringOrTemplateLiteral {
147147
}
148148
};
149149

150-
function concatConsecutiveString(index: number, nodes: readonly Expression[]): [number, string, number[]] {
150+
function escapeStringForTemplate(s: string) {
151+
// Escaping for $s in strings that are to be used in template strings
152+
// Naive implementation: replace \x by itself and otherwise $ by \$.
153+
// But to complicate it a bit, this should work for raw strings too.
154+
// And another bit: escape the $ in the replacement for JS's .replace().
155+
return s.replace(/\\.|\$/g, m => m === "$" ? "\\\$" : m);
156+
// Finally, a less-backslash-happy version can work too, doing only ${ instead of all $s:
157+
// s.replace(/\\.|\${/g, m => m === "${" ? "\\\${" : m);
158+
// but `\$${foo}` is likely more clear than the more-confusing-but-still-working `$${foo}`.
159+
}
160+
161+
function getRawTextOfTemplate(node: TemplateHead | TemplateMiddle | TemplateTail) {
162+
// in these cases the right side is ${
163+
const rightShaving = isTemplateHead(node) || isTemplateMiddle(node) ? -2 : -1;
164+
return getTextOfNode(node).slice(1, rightShaving);
165+
}
166+
167+
function concatConsecutiveString(index: number, nodes: readonly Expression[]): [nextIndex: number, text: string, rawText: string, usedIndexes: number[]] {
151168
const indexes = [];
152-
let text = "";
169+
let text = "", rawText = "";
153170
while (index < nodes.length) {
154171
const node = nodes[index];
155172
if (isStringLiteralLike(node)) { // includes isNoSubstitutionTemplateLiteral(node)
156-
text = text + node.text;
173+
text += escapeStringForTemplate(node.text);
174+
rawText += escapeStringForTemplate(getTextOfNode(node).slice(1, -1));
157175
indexes.push(index);
158176
index++;
159177
}
160178
else if (isTemplateExpression(node)) {
161-
text = text + node.head.text;
179+
text += node.head.text;
180+
rawText += getRawTextOfTemplate(node.head);
162181
break;
163182
}
164183
else {
165184
break;
166185
}
167186
}
168-
return [index, text, indexes];
187+
return [index, text, rawText, indexes];
169188
}
170189

171190
function nodesToTemplate({ nodes, operators }: { nodes: readonly Expression[], operators: Token<BinaryOperator>[] }, file: SourceFile) {
172191
const copyOperatorComments = copyTrailingOperatorComments(operators, file);
173192
const copyCommentFromStringLiterals = copyCommentFromMultiNode(nodes, file, copyOperatorComments);
174-
const [begin, headText, headIndexes] = concatConsecutiveString(0, nodes);
193+
const [begin, headText, rawHeadText, headIndexes] = concatConsecutiveString(0, nodes);
175194

176195
if (begin === nodes.length) {
177-
const noSubstitutionTemplateLiteral = factory.createNoSubstitutionTemplateLiteral(headText);
196+
const noSubstitutionTemplateLiteral = factory.createNoSubstitutionTemplateLiteral(headText, rawHeadText);
178197
copyCommentFromStringLiterals(headIndexes, noSubstitutionTemplateLiteral);
179198
return noSubstitutionTemplateLiteral;
180199
}
181200

182201
const templateSpans: TemplateSpan[] = [];
183-
const templateHead = factory.createTemplateHead(headText);
202+
const templateHead = factory.createTemplateHead(headText, rawHeadText);
184203
copyCommentFromStringLiterals(headIndexes, templateHead);
185204

186205
for (let i = begin; i < nodes.length; i++) {
187206
const currentNode = getExpressionFromParenthesesOrExpression(nodes[i]);
188207
copyOperatorComments(i, currentNode);
189208

190-
const [newIndex, subsequentText, stringIndexes] = concatConsecutiveString(i + 1, nodes);
209+
const [newIndex, subsequentText, rawSubsequentText, stringIndexes] = concatConsecutiveString(i + 1, nodes);
191210
i = newIndex - 1;
192211
const isLast = i === nodes.length - 1;
193212

194213
if (isTemplateExpression(currentNode)) {
195214
const spans = map(currentNode.templateSpans, (span, index) => {
196215
copyExpressionComments(span);
197-
const nextSpan = currentNode.templateSpans[index + 1];
198-
const text = span.literal.text + (nextSpan ? "" : subsequentText);
199-
return factory.createTemplateSpan(span.expression, isLast ? factory.createTemplateTail(text) : factory.createTemplateMiddle(text));
216+
const isLastSpan = index === currentNode.templateSpans.length - 1;
217+
const text = span.literal.text + (isLastSpan ? subsequentText : "");
218+
const rawText = getRawTextOfTemplate(span.literal) + (isLastSpan ? rawSubsequentText : "");
219+
return factory.createTemplateSpan(span.expression, isLast
220+
? factory.createTemplateTail(text, rawText)
221+
: factory.createTemplateMiddle(text, rawText));
200222
});
201223
templateSpans.push(...spans);
202224
}
203225
else {
204-
const templatePart = isLast ? factory.createTemplateTail(subsequentText) : factory.createTemplateMiddle(subsequentText);
226+
const templatePart = isLast
227+
? factory.createTemplateTail(subsequentText, rawSubsequentText)
228+
: factory.createTemplateMiddle(subsequentText, rawSubsequentText);
205229
copyCommentFromStringLiterals(stringIndexes, templatePart);
206230
templateSpans.push(factory.createTemplateSpan(currentNode, templatePart));
207231
}

‎tests/cases/fourslash/refactorConvertStringOrTemplateLiteral_escapeSequences.ts

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @Filename: /a.ts
4+
////let s = /*a1*/"\0\b\f\t\r\n" + text + "\n"/*a2*/;
5+
6+
goTo.select("a1", "a2");
7+
edit.applyRefactor({
8+
refactorName: "Convert to template string",
9+
actionName: "Convert to template string",
10+
actionDescription: ts.Diagnostics.Convert_to_template_string.message,
11+
newContent: 'let s = `\\0\\b\\f\\t\\r\\n${text}\\n`;'
12+
});
13+
14+
// @Filename: /b.ts
15+
////let s = /*b1*/'"' + text + "'"/*b2*/;
16+
17+
goTo.select("b1", "b2");
18+
edit.applyRefactor({
19+
refactorName: "Convert to template string",
20+
actionName: "Convert to template string",
21+
actionDescription: ts.Diagnostics.Convert_to_template_string.message,
22+
// newContent is: let s = `"${text}'`;
23+
newContent: 'let s = `"${text}\'`;'
24+
});
25+
26+
// @Filename: /c.ts
27+
////let s = /*c1*/'$' + text + "\\"/*c2*/;
28+
29+
goTo.select("c1", "c2");
30+
edit.applyRefactor({
31+
refactorName: "Convert to template string",
32+
actionName: "Convert to template string",
33+
actionDescription: ts.Diagnostics.Convert_to_template_string.message,
34+
// newContent is: let s = `\$${text}\\`;
35+
newContent: 'let s = `\\$${text}\\\\`;'
36+
});
37+
38+
// @Filename: /d.ts
39+
////let s = /*d1*/`$` + text + `\\`/*d2*/;
40+
41+
goTo.select("d1", "d2");
42+
edit.applyRefactor({
43+
refactorName: "Convert to template string",
44+
actionName: "Convert to template string",
45+
actionDescription: ts.Diagnostics.Convert_to_template_string.message,
46+
// newContent is: let s = `\$${text}\\`;
47+
newContent: 'let s = `\\$${text}\\\\`;'
48+
});
49+
50+
// @Filename: /e.ts
51+
////let s = /*e1*/'${' + text + "}"/*e2*/;
52+
53+
goTo.select("e1", "e2");
54+
edit.applyRefactor({
55+
refactorName: "Convert to template string",
56+
actionName: "Convert to template string",
57+
actionDescription: ts.Diagnostics.Convert_to_template_string.message,
58+
// newContent is: let s = `\${${text}}`;
59+
newContent: 'let s = `\\${${text}}`;'
60+
});
61+
62+
// @Filename: /f.ts
63+
////let s = /*f1*/`\${` + text + `}`/*f2*/;
64+
65+
goTo.select("f1", "f2");
66+
edit.applyRefactor({
67+
refactorName: "Convert to template string",
68+
actionName: "Convert to template string",
69+
actionDescription: ts.Diagnostics.Convert_to_template_string.message,
70+
// newContent is: let s = `\${${text}}`;
71+
newContent: 'let s = `\\${${text}}`;'
72+
});
73+
74+
// @Filename: /g.ts
75+
////let s = /*g1*/'\\$' + text + "\\"/*g2*/;
76+
77+
goTo.select("g1", "g2");
78+
edit.applyRefactor({
79+
refactorName: "Convert to template string",
80+
actionName: "Convert to template string",
81+
actionDescription: ts.Diagnostics.Convert_to_template_string.message,
82+
// newContent is: let s = `\\\$${text}\\`;
83+
newContent: 'let s = `\\\\\\$${text}\\\\`;'
84+
});
85+
86+
// @Filename: /h.ts
87+
////let s = /*h1*/"\u0041\u0061" + text + "\0\u0000"/*h2*/;
88+
89+
goTo.select("h1", "h2");
90+
edit.applyRefactor({
91+
refactorName: "Convert to template string",
92+
actionName: "Convert to template string",
93+
actionDescription: ts.Diagnostics.Convert_to_template_string.message,
94+
// newContent is: let s = `\u0041\u0061${text}\0\u0000`;
95+
newContent: 'let s = `\\u0041\\u0061${text}\\0\\u0000`;'
96+
});

0 commit comments

Comments
 (0)