Improve escape sequence handling in private names

[Daniel Rosenwasser (DanielRosenwasser)]

This PR makes two changes:

1. It fixes how the parser acquires the name of a PrivateIdentifier from the scanner, preferring the "cooked" tokenValue after processing escape sequences rather than just reading the plain source (the "token text"). Previously, two private names with the same effective/cooked names would not resolve to each other unless their source text was verbatim identical.
2. It fixes the scanner, which previously didn't expect a that a hash/pound (#) could be followed by a backlash (\).

There is still a slight problem around the fact that under ES3 and ES5 (our defaults), we don't recognize extended escape sequences at all. Ideally we would improve this by gracefully parsing and issuing an error that we can't emit them or something. But what that means is that I'm not yet certain as to whether the language service issue discovered within test262 (under formatjs) in #50835 is actually fixed by this. That might need to be a follow-up.

Fixes #50851.

[Daniel Rosenwasser (DanielRosenwasser)]

TypeScript Bot (@typescript-bot) pack this

[TypeScript Bot (typescript-bot)]

Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the tarball bundle task on this PR at 82d0fc9. You can monitor the build here.

[Andrew Casey (amcasey)]

There is still a slight problem around the fact that under ES3 and ES5 (our defaults), we don't recognize extended escape sequences at all. Ideally we would improve this by gracefully parsing and issuing an error that we can't emit them or something.

I'm not sure I follow. Those defaults are about our output, aren't they? We always parse according to the latest spec supported by that version of TS? Also, why would we be unable to emit? Wouldn't we just emit the actual unicode character instead of the escape sequence?

But what that means is that I'm not yet certain as to whether the language service issue discovered within test262 (under formatjs) in #50835 is actually fixed by this. That might need to be a follow-up.

Fortunately, you can replay the repro locally with your locally-build compiler. 😄

[Daniel Rosenwasser (DanielRosenwasser)]

Fortunately, you can replay the repro locally with your locally-build compiler.

I work on a codespace and I'm not certain how easy it is to wget the replay. That's why I asked the bot to pack it.

Those defaults are about our output, aren't they?

Of course you're right, and this is a ridiculous problem.

We always parse according to the latest spec supported by that version of TS?

Unfortunately this is not the case - check out the differences in the new es5/es2015 .errors.txt files.

Also

TypeScript/src/compiler/scanner.ts

Line 1489 in 812ebcf

if (languageVersion >= ScriptTarget.ES2015 && codePointAt(text, pos + 1) === CharacterCodes.u && codePointAt(text, pos + 2) === CharacterCodes.openBrace) {

Wouldn't we just emit the actual unicode character instead of the escape sequence?

In theory we should - but we don't because I believe we update the list of valid identifiers depending on the language version.

[TypeScript Bot (typescript-bot)]

Hey Daniel Rosenwasser (@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/134739/artifacts?artifactName=tgz&fileId=9DE877BE6F6BE931B73F6E1F9B8BAA32C1E15947AA029F49AF7D99F972183DB702&fileName=/typescript-4.9.0-insiders.20220920.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.9.0-pr-50856-4".;

[Andrew Casey (amcasey)]

This change looks consistent with unicode escape handling elsewhere in the scanner, but I'm not sure I understand why scanIdentifier doesn't handle them. Is it illegal to start an identifier with an escape sequence? (Maybe that would introduce an ambiguity, but none jumps to mind.)

[Andrew Casey (amcasey)]

Am I reading this correctly? It looks like any identifier can start with a unicode escape. If so, why wouldn't the fix be in scanIdentifier?

[Andrew Casey (amcasey)]

Daniel Rosenwasser (@DanielRosenwasser) Last question ⬆️

[Daniel Rosenwasser (DanielRosenwasser)]

scanIdentifier could handle them if we just checked if the first character was either an identifier start or a \, but right now the logic is to not advance if we can't get at least one complete identifier start.

This was meant to be consistent with identifiers. Right now the handling for any of the following incomplete escape sequences...

\
\u
\u0
\u00
\u000
\u{}

is to not munch up these characters, and identify the \ as an unknown token, followed by whatever.

As an extension, what the current code does with private fields is to make each of

#\
#\u
#\u0
#\u00
#\u000
#\u{}

an incomplete private field with the name #, followed by an unknown token \ followed by whatever.

Arguably, private fields could diverge here for some better errors.

[Andrew Casey (amcasey)]

I work on a codespace and I'm not certain how easy it is to wget the replay. That's why I asked the bot to pack it.

I'm pretty sure npm i works, even if wget doesn't.

We always parse according to the latest spec supported by that version of TS?

Unfortunately this is not the case - check out the differences in the new es5/es2015 .errors.txt files.

TIL

Wouldn't we just emit the actual unicode character instead of the escape sequence?

In theory we should - but we don't because I believe we update the list of valid identifiers depending on the language version.

Still not following. In each version, either the character is allowed in identifiers or it's not - it doesn't actually matter whether it was originally authored as an escape?

[Daniel Rosenwasser (DanielRosenwasser)]

In each version, either the character is allowed in identifiers or it's not - it doesn't actually matter whether it was originally authored as an escape?

Exactly - we already track whether something is an escape, and the emitter can handle that differently. I think whether the source character translates to a valid Unicode identifier in the target version is separate from whether we can parse out an extended escape.

I would rather do that in a separate PR though.

[Andrew Casey (amcasey)]

Thanks!