Better template literals support in checker

[IllusionMH]

Fixes #31548
Fixes #30962

You can check details for each propose changes in issue. This PR contain implementation for next items from there:

• 1. Support template literals in enum declaration
• 2. Support template literals in enum properties access
• 3. Support template literals in switch statements with typeof condition
• 4. Support template literals in const initializers inside an ambient declarations
• 5. Support template literals in control flow checks
• 6. Unify symbols for computed properties
• 7. Unify isExpressionNode checks
  - 8. Optional suggestion: Support template literals in let a: import(`test`) and import b = require(`test`) Separate issue will be created

At this point last item is still in progress and I would like to know if this change should be implemented at all, because it will also affect types as well as services/findAllReferences.ts and services/importTracker.ts. Should I implement this one in this PR?

Two pints are still under questions for me:

1. Is it OK to use utility function instead of direct comparison node.kind ==== SyntaxKind.X?
2. Is it OK to update existing diagnostic messages (each has only one usage) or should I create new diagnostics?

[IllusionMH]

Any chance to get initial review comments (messages, utility functions) and preference regarding import/require?

[IllusionMH]

Is there anything I should do before this PR can be reviewed?

If it's only 7 of 8 progress - this is because I still waiting for reply about template literals in import types and import...require. This change will involve not only update to checker and utils, but also for various services (findAllReferences, importTracker, rename to name few)

[andrewbranch]

All these computed property name symbol baseline changes are a bit odd... do you know what caused this?

[IllusionMH]

These are result of commit 30ff964

Intent was to bring template literals into line with numbers and regular strings in computed property names.
They produce separate symbols for names https://github.com/microsoft/TypeScript/pull/32064/files/caef2dc945cbb42cbfb021d9737f07033916180d#diff-739db372f229c7fa65d1a1da3101251aR36-R42

This one and last commit were under question for me, because I haven't found what exact behavior they will enable except unifying produced symbols and types with string literals in similar condition.

[andrewbranch]

Huh, interesting. I think that’s probably fine, then.

[IllusionMH]

I have no strong opinion about these commits and will revert if anyone considers its necessary.

These were found as comparisons of StringLiteral and NoSubstitutionTemplateLiteral usages. I would expect that last 2 commits will help in services (not included in this PR) but looks like there are a lot of differences that filter out template literals before symbols for them will be checked (which will need separate investigation).

[andrewbranch]

Optional suggestion: Support template literals in let a: import(`test`) and import b = require(`test`)... Should I implement this one in this PR?

I’d vote no. A separate PR would make it a bit easier, if you want to do it.

Is it OK to use utility function instead of direct comparison node.kind ==== SyntaxKind.X?

Yep.

Is it OK to update existing diagnostic messages (each has only one usage) or should I create new diagnostics?

Updating the diagnostic messages is fine in general, but I’m personally not sure it’s necessary. I casually think of NoSubstitutionTemplateLiterals as “string literals,” and saying that something accepts a “template literal” when really it only accepts a template literal without substitutions is slightly misleading. I think my preference would be to leave the diagnostics as-is.

[IllusionMH]

Thanks for the feedback. I will revert changes to diagnostics and create separate to discuss whether templates should be supported in imports.

[IllusionMH]

I reverted changes to the diagnostic messages.

Separate issue for let a: import(`test`) and import b = require(`test`) is still on my to-do list because I wan't to see what services should be updated as well (e.g. rename.ts)

[IllusionMH]

Is there anything else required from my side?

[andrewbranch]

@IllusionMH not at the moment—since this is essentially a feature and we already released the 3.6 beta, we’re planning to include this in 3.7. I wanted to answer your open questions right away, but I’ll be reviewing the rest of it probably after we release 3.6-rc. Thanks for your patience! 🌟

[IllusionMH]

Oh, I see. Issue was marked as a bug, so I hoped it would make it into 3.6 but I see your point. Will wait then.

[andrewbranch]

@typescript-bot test this

[typescript-bot]

Heya @andrewbranch, I've started to run the extended test suite on this PR at 49a4352. You can monitor the build here. It should now contribute to this PR's status checks.

[andrewbranch]

Cool, all the tests pass!

@IllusionMH I think the icing on the cake to ship with this would be to trigger string literal completions on a backtick character where appropriate:

It’s a bit hard to tell here, but when completions appear inside backticks, it’s because I pressed ⌃Space on my keyboard. So it shows that the string literal completions are available, they’re just not being triggered.

What do you think?

[IllusionMH]

I can take a look at this behavior in scope of this PR if it's preferable way, however I doubt that it will be icing.

AFAIK there are at least two cases that are related to services but don't work in that same was as strings regular strings:

1. template literals in imports (not in this PR, but support in checker is in local branch)
2. Renames of computed properties
   

May be there are more, but I hadn't had a chance to do same search through services. Maybe it make sense to perform separate search for differences and create separate issue for discussion and then PR.

What approach is preferable in this case?

[andrewbranch]

@IllusionMH I think my preference would be that any particular “feature” ships with support in the core compiler and in the services at the same time. So, it’s ok to do import types in a later PR (I also noticed that indexed access types don’t yet work with template strings: T[`key`]), but the features you’ve added to the language here should be fully supported in the services.

Good find with the property renaming. That one seems important. Another thing to check is find-all-references.

The reasons I think it’s good to ship language features with services support:

1. People actually use the nightly and it’s usually pretty stable—if someone finds a bug in it and we say “oh yeah, we knew about that, just didn’t write support for the services yet,” it makes the nightly seem less usable—stuff we merge into master we should generally be able to call “done.”
2. Maybe adding support in the services will reveal a place that actually needs to be updated in the core compiler—for instance, find all references relies heavily on the work of the binder. Once we can be sure that find-all-references works for these new cases, I’ll feel even more confident that the changes made in the binder are correct and complete.

Thanks as always for your excellent work 🙌

[IllusionMH]

OK, I'll take a look at T[`key`] case and then will go through services.
I'll also check if it will be possible to bring imports part to this PR. Initially I was worried about expanding scope to services but if they should be here - makes sense to finish imports too.

---

UPD. Checked case T[`key`] and it is #29331

type KeyType<T> = T[`key`];
//                  ^^^^^ ExpressionStatement                      
declare module `*.tpl` {}
//      ^^^^^^^^^^^^^^ ExpressionStatement
let a: Promise<`test`>;
//             ^^^^^^^^ ExpressionStatement

In these cases template literals treated as not in LiteralType, but as ExpressionStatement resulting in unexpected(IMHO wrong AST, because in works for import type).

let b: Promise<'test'>;
//             ^^^^^^ LiteralType
let i: import(`module`);
//            ^^^^^^^^ LiteralType

Frankly speaking at this point I don't have ideas how to fix #29331. But since it affects only TS specific cases (types position, should work fine in valid JS positions) I would prefer to exclude that fix from this PR as mentioned in original issue for this PR #31548. Probably in the end I will avoid imports part for consistency (import(`module`) is parsed correctly, but reported as error ATM, so basically template literals don't allowed in any TS specific parts).

However services affects valid JS parts so I will work on fixes for them.

[IllusionMH]

@andrewbranch I've created microsoft/vscode#80234 to implement triggers for completions on backticks (or any required changes related to services in future)

[andrewbranch]

@IllusionMH thanks for investigating all these threads! I think your updated plan sounds good.

However services affects valid JS parts so I will work on fixes for them.

Ping me when you’re ready for a review of whatever service changes you make 👍

[andrewbranch]

👋 @IllusionMH is there anything I can do to help here? Just wanted to give you a heads up that we’ll be releaseing 3.7 beta on October 1, so we won’t be able to merge new features for 3.7 after that. Of course you’re not obligated to anything, just didn’t want it to come as a surprise. I might be able to help get this over the finish line if you’re short on time.

[IllusionMH]

Still working on this one, but have less time than before.
Thanks for the ping, I saw iteration plan and expected this as deadline for merge, however expected a bit more time for 3 month iteration, but looks like this one will be 2 month.

Good news - still working on rename service - now it renames computed properties and indexed access with template literals, but also removes surrounding ` 😆

Bad news - there are more cases in services where StringLiterals are checked and NoSubstitutionTemplateLiterals are not (even more that in current PR), and it's harder to see where there should be both checks.

[IllusionMH]

Looks like change to accessKind worked fine (in my initial opinion) for cases when element access used with string or template literal, however when Identifier was passed instead of default Read it received Write flag.
Since this behavior is not directly related to this PR I've reverted changes to accessKind.

I can see 2 options:

1. Add check and return Read for Identifier (as before) but will use previous logic otherwise. This will make a.prop and a['prop'] both have "isWriteAccess": true (should they be equivalents or no?)
2. Leave it as is since there are 2 PRs that change this(or pretty close) behavior.

Any suggestions in this situation?

[andrewbranch]

Let’s leave those as-is. I think this PR should make NoSubstitutionTemplateLiterals work the same as StringLiterals currently work.

[IllusionMH]

Couple of questions:

1. Is there anything else expected from me in this PR?
2. Is there a chance that this one will make it to 3.7?

UPD. I'rechecked original scope + services folder to find any noticeable diffrences, but there is nothing that I was able to see in editor -most things are related to places where template literals are no allowed (import/export declarations, field names, type parameters etc.).

Only thing that looks off - src/services/preProcess.ts that doesn't consume const a = import(`./foo`) or const a = require(`./foo`), however they work correctly in editor and with tsc even without this PR. I would prefer to not include these changes in PR, but can add if needed.

[andrewbranch]

Just curious, what was this one needed for?

[IllusionMH]

Template literals and string literals with extended Unicode escapes will be first processed with es2015 transformer which will replace them with Synthesized string literals that don't have reference to parent element or to original node.

Under the hood getTextOfNode(node.argumentExpression) will call getSourceFileOfNode on Synthesized string literal which will return undefined (because node.parent is undefined) and then getSourceTextOfNodeFromSourceFile that will try to access sourceFile.text and exception will be thrown.

On the other hand ElementAccessExpression even if will be replaced with Synthesized node, it will have link to original node. Therefore originalNode.argumentExpression will have proper parent references up to source file and text will be retrieved correctly.

[andrewbranch]

@typescript-bot user test this
@typescript-bot test this

[typescript-bot]

Heya @andrewbranch, I've started to run the extended test suite on this PR at cc8a514. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Heya @andrewbranch, I've started to run the parallelized community code test suite on this PR at cc8a514. You can monitor the build here. It should now contribute to this PR's status checks.

[andrewbranch]

I’d like to get another set of eyes on this if possible, so I’m going to leave it open until Friday to try to give the rest of the team some time to look at it. But assuming the extended tests don‘t turn up anything horrifying, I think this is great 👍

[typescript-bot]

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

[andrewbranch]

^ Changes/failures look unrelated

[sandersn]

Since I'd like to get this into the beta, I'll make the simple change I requested and then merge after we discuss at the design meeting.

[IllusionMH]

Thank you for suggestions, reviews, and patience!