add refactoring: string concatenation to template literals

[bigaru]

Fixes #18267

@ajafff have a look

[gabritto]

Seems good except for some minor things.
Another thing I've noticed is that this refactor has the same problem as other ones regarding comments that disappear. For example, in the code above both "comment b" occurrences go missing after refactoring:

const a = 2;
const comment1 = `test ${ /*comment b*/ a /*comment e*/ }!`;
const comment2 = "test " + /* comment b*/ a /*comment e*/ + "!";

becomes:

const a = 2;
const comment1 = "test " + a /*comment e*/ + "!";
const comment2 = `test ${a /*comment e*/}!`;

I've had the same problem in this refactor so I had to explicitly handle the copying of comments from old nodes to new ones. I'm writing about that problem and solution and will link to it here when I finish.
Update: wrote about comments here: https://gist.github.com/gabritto/fa489e48e09ce6bc089955abba78d91a#trivia

[gabritto]

could you explain why this is necessary?

[bigaru]

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

[gabritto]

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

[bigaru]

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)

[bigaru]

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

[gabritto]

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

[bigaru]

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

[gabritto]

i tested this locally and this is what happens:

so it does seem like there's an extra backslash on the resulting template literal.

[bigaru]

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?

[bigaru]

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

[RyanCavanaugh]

My inclination here is that concat -> template is a good refactor, but it's not clear to me why you'd want the reverse direction.

@DanielRosenwasser thoughts?

[DanielRosenwasser]

@typescript-bot pack this

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 35a3a5f. You can monitor the build here. It should now contribute to this PR's status checks.

[DanielRosenwasser]

Okay now @typescript-bot pack this

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 88795e2. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/58560/artifacts?artifactName=tgz&fileId=E4ED7ECEC258847C2940852925C4BCCCCA6AC9A99B44880962350EAC6E997F6602&fileName=/typescript-3.8.0-insiders.20191223.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[DanielRosenwasser]

Hey @bigaru, I spoke with Ryan and at this time we think convert to template expression is clearly useful from users. On the other hand, we didn't have consensus that users wanted to go the other way. I've removed those bits and pushed up a commit to remove that functionality. In the future, we'll be able to revert ad0f006 and ff4fa1f if we change our minds.

[rauschma]

Nit-pick: At the moment, this refactoring seems to be called “Convert to template string”. The more correct name would be “Convert to template literal”.