Fix template string refactoring and nodeFactory bug by elibarzilay · Pull Request #44313 · microsoft/TypeScript

Eli Barzilay (elibarzilay) · 2021-05-28T04:46:59Z

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#40625

Eli Barzilay (elibarzilay) · 2021-06-03T04:46:39Z

Question

This is using JSON.stringify to do the escaping, which is generally fine but one possible improvement is handling NUL characters. As is, they're converted to \u0000. It might be better to make these convert to \0 instead unless it's followed by a digit. But this is rare and I'm not sure if it's worth the extra code.

Ryan Cavanaugh (RyanCavanaugh) · 2021-06-09T22:15:20Z

ping Ron Buckton (@rbuckton)

Review was incorrect

Ron Buckton (rbuckton) · 2021-07-07T00:11:33Z

 // @api
 function createTemplateLiteralLikeNode(kind: TemplateLiteralToken["kind"], text: string, rawText: string | undefined, templateFlags: TokenFlags | undefined) {
 const node = createBaseToken<TemplateLiteralLikeNode>(kind);
+ rawText ??= JSON.stringify(text).slice(1,-1).replace(/\\.|\$/g, s =>


I don't think we should try to synthesize rawText here. Generally when rawText is undefined we intend to pull the "raw" version of the template literal from the source file during emit. rawText was added to allow API consumers to pass their own "raw" text, and in those cases it goes through createTemplateLiteralLikeNodeChecked which verifies that the "cooked" text matches the supplied "raw" text.
If someone were to pass the string "\t " (byte sequence 5c 74 09) for text, this would generate a "raw" text of "\t\t". There's no way to be 100% sure what the user intended, so its better to be explicit.
I think the issue is actually in the implementation of the refactor, which should be reading the raw source text of the string literal rather than node.text.

Its also important that rawText remains undefined for our tagged template transform to accurately get the actual raw text from the source file for non-user-created nodes:
TypeScript/src/compiler/transformers/taggedTemplate.ts
Lines 78 to 84 in eb3645f
functiongetRawLiteral(node: TemplateLiteralLikeNode,currentSourceFile: SourceFile){
// Find original source text, since we need to emit the raw strings of the tagged template.
// The raw strings contain the (escaped) strings of what the user wrote.
// Examples: `\n` is converted to "\\n", a template string with a newline to "\n".
lettext=node.rawText;
if(text===undefined){
text=getSourceTextOfNodeFromSourceFile(currentSourceFile,node);

Ron Buckton (rbuckton) · 2021-07-15T21:15:16Z


 let token = rawTextScanner.scan();
- if (token === SyntaxKind.CloseBracketToken) {
+ if (token === SyntaxKind.CloseBraceToken) {


Obvious typo I wish I'd caught sooner 😔. Good catch.

1. `getRawLiteral()`: barf if `currentSourceFile` is missing, since if it is, then the following `getSourceTextOfNodeFromSourceFile` will return a bogus `""`. 2. Some `||` -> `??`. 3. `backtickQuoteEscapedCharsRegExp`: escape the usual control characters except for a simple LF. This code does get used to generate backtick strings when `rawText` is not given, and not escaping things like TAB characters can get mangled by editor settings. Worse, not escaping a CRLF and putting it verbatim in sthe string source will interpret it as LF, so add a special case for escaping these as `\r\n`. Related to microsoft#44313 and microsoft#40625.

Ron Buckton (rbuckton)

A few minor nits but this looks good.

1. `getRawLiteral()`: barf if `currentSourceFile` is missing, since if it is, then the following `getSourceTextOfNodeFromSourceFile` will return a bogus `""`. 2. One `||` -> `??` change. 3. `backtickQuoteEscapedCharsRegExp`: escape the usual control characters except for a simple LF. This code does get used to generate backtick strings when `rawText` is not given, and not escaping things like TAB characters can get mangled by editor settings. Worse, not escaping a CRLF and putting it verbatim in sthe string source will interpret it as LF, so add a special case for escaping these as `\r\n`. Related to microsoft#44313 and microsoft#40625.

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. Fixesmicrosoft#40625

1. `getRawLiteral()`: barf if `currentSourceFile` is missing, since if it is, then the following `getSourceTextOfNodeFromSourceFile` will return a bogus `""`. 2. One `||` -> `??` change. 3. `backtickQuoteEscapedCharsRegExp`: escape the usual control characters except for a simple LF. This code does get used to generate backtick strings when `rawText` is not given, and not escaping things like TAB characters can get mangled by editor settings. Worse, not escaping a CRLF and putting it verbatim in sthe string source will interpret it as LF, so add a special case for escaping these as `\r\n`. Added test. Related to microsoft#44313 and microsoft#40625.

1. `getRawLiteral()`: barf if `currentSourceFile` is missing, since if it is, then the following `getSourceTextOfNodeFromSourceFile` will return a bogus `""`. 2. One `||` -> `??` change. 3. `backtickQuoteEscapedCharsRegExp`: escape the usual control characters except for a simple LF. This code does get used to generate backtick strings when `rawText` is not given, and not escaping things like TAB characters can get mangled by editor settings. Worse, not escaping a CRLF and putting it verbatim in sthe string source will interpret it as LF, so add a special case for escaping these as `\r\n`. Added test. Related to #44313 and #40625.

TypeScript Bot (typescript-bot) assigned Eli Barzilay (elibarzilay)May 28, 2021

TypeScript Bot (typescript-bot) added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels May 28, 2021

Eli Barzilay (elibarzilay)force-pushed the 40625 branch 2 times, most recently from e2da2bb to d069dcfCompare June 3, 2021 04:41

Nathan Shively-Sanders (sandersn) requested review from Ron Buckton (rbuckton) and Wesley Wigham (weswigham)June 8, 2021 22:13

Andrew Branch (andrewbranch) previously requested changes Jun 30, 2021
View reviewed changes

Comment threadtests/cases/fourslash/refactorConvertStringOrTemplateLiteral_escapeSequences.ts Outdated

Ron Buckton (rbuckton) suggested changes Jul 7, 2021
View reviewed changes

Eli Barzilay (elibarzilay)force-pushed the 40625 branch 2 times, most recently from 30d1f7c to ee7b39dCompare July 15, 2021 00:16

Eli Barzilay (elibarzilay) changed the title ~~Generate escaped string in createTemplateHead*~~Fix template string refactoring and nodeFactory bugJul 15, 2021

Ron Buckton (rbuckton) reviewed Jul 15, 2021
View reviewed changes

Eli Barzilay (elibarzilay) mentioned this pull request Jul 19, 2021
Some tweaks for backtick strings #45099
Merged

Ron Buckton (rbuckton) approved these changes Jul 28, 2021
View reviewed changes

Eli Barzilay (elibarzilay)force-pushed the 40625 branch 2 times, most recently from 93acbe8 to 8e01e23Compare July 28, 2021 21:06

Eli Barzilay (elibarzilay)force-pushed the 40625 branch from 8e01e23 to c8fd1fdCompare July 29, 2021 04:19

Eli Barzilay (elibarzilay) merged commit 7e8bba6 into microsoft:mainJul 29, 2021

Eli Barzilay (elibarzilay) deleted the 40625 branch July 29, 2021 08:23

Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025

Navigation Menu

Uh oh!

Fix template string refactoring and nodeFactory bug - #44313

Fix template string refactoring and nodeFactory bug#44313
Eli Barzilay (elibarzilay) merged 1 commit into
microsoft:mainfrom
elibarzilay:40625

Eli Barzilay (elibarzilay) commented May 28, 2021•
edited
Loading

Uh oh!

Eli Barzilay (elibarzilay) commented Jun 3, 2021

Uh oh!

Ryan Cavanaugh (RyanCavanaugh) commented Jun 9, 2021

Uh oh!

Uh oh!

Ron Buckton (rbuckton)Jul 7, 2021

Uh oh!

Ron Buckton (rbuckton)Jul 7, 2021

Uh oh!

Ron Buckton (rbuckton)Jul 15, 2021

Uh oh!

Ron Buckton (rbuckton) left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	functiongetRawLiteral(node: TemplateLiteralLikeNode,currentSourceFile: SourceFile){
	// Find original source text, since we need to emit the raw strings of the tagged template.
	// The raw strings contain the (escaped) strings of what the user wrote.
	// Examples: `\n` is converted to "\\n", a template string with a newline to "\n".
	lettext=node.rawText;
	if(text===undefined){
	text=getSourceTextOfNodeFromSourceFile(currentSourceFile,node);

Uh oh!

Conversation

Eli Barzilay (elibarzilay) commented May 28, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eli Barzilay (elibarzilay) commented Jun 3, 2021

Question

Uh oh!

Ryan Cavanaugh (RyanCavanaugh) commented Jun 9, 2021

Uh oh!

Uh oh!

Ron Buckton (rbuckton)Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

Ron Buckton (rbuckton)Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

Ron Buckton (rbuckton)Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

Ron Buckton (rbuckton) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Eli Barzilay (elibarzilay) commented May 28, 2021•
edited
Loading