Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve escape sequence handling in private names #50856

Merged
merged 7 commits into from Sep 20, 2022
Merged

Improve escape sequence handling in private names #50856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
77d7109
Add tests for identifiers and private identifiers with escape sequences.
DanielRosenwasser Sep 20, 2022
3f8b934
Accepted baselines.
DanielRosenwasser Sep 20, 2022
69195a9
Store the tokenValue instead of tokenText on PrivateIdentifiers, sinc…
DanielRosenwasser Sep 20, 2022
fec0977
Accepted baselines.
DanielRosenwasser Sep 20, 2022
6fccba5
Check for leading escape sequences in PrivateIdentifiers.
DanielRosenwasser Sep 20, 2022
83b3ea3
Accepted baselines.
DanielRosenwasser Sep 20, 2022
82d0fc9
Fix lints.
DanielRosenwasser Sep 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler/parser.ts
View file
Open in desktop
Expand Up @@ -2281,7 +2281,7 @@ namespace ts {

function parsePrivateIdentifier(): PrivateIdentifier {
const pos = getNodePos();
const node = factory.createPrivateIdentifier(internPrivateIdentifier(scanner.getTokenText()));
const node = factory.createPrivateIdentifier(internPrivateIdentifier(scanner.getTokenValue()));
amcasey marked this conversation as resolved.
Show resolved Hide resolved
nextToken();
return finishNode(node, pos);
}
Expand Down
32 changes: 29 additions & 3 deletions src/compiler/scanner.ts
View file
Open in desktop
Expand Up @@ -2052,12 +2052,38 @@ namespace ts {
return token = SyntaxKind.Unknown;
}

if (isIdentifierStart(codePointAt(text, pos + 1), languageVersion)) {
const charAfterHash = codePointAt(text, pos + 1);
if (charAfterHash === CharacterCodes.backslash) {
pos++;
scanIdentifier(codePointAt(text, pos), languageVersion);
const extendedCookedChar = peekExtendedUnicodeEscape();
if (extendedCookedChar >= 0 && isIdentifierStart(extendedCookedChar, languageVersion)) {
pos += 3;
tokenFlags |= TokenFlags.ExtendedUnicodeEscape;
tokenValue = "#" + scanExtendedUnicodeEscape() + scanIdentifierParts();
return token = SyntaxKind.PrivateIdentifier;
}

const cookedChar = peekUnicodeEscape();
if (cookedChar >= 0 && isIdentifierStart(cookedChar, languageVersion)) {
pos += 6;
tokenFlags |= TokenFlags.UnicodeEscape;
tokenValue = "#" + String.fromCharCode(cookedChar) + scanIdentifierParts();
return token = SyntaxKind.PrivateIdentifier;
}
pos--;
}

if (isIdentifierStart(charAfterHash, languageVersion)) {
pos++;
// We're relying on scanIdentifier's behavior and adjusting the token kind after the fact.
// Notably absent from this block is the fact that calling a function named "scanIdentifier",
// but identifiers don't include '#', and that function doesn't deal with it at all.
// This works because 'scanIdentifier' tries to reuse source characters and builds up substrings;
// however, it starts at the 'tokenPos' which includes the '#', and will "accidentally" prepend the '#' for us.
scanIdentifier(charAfterHash, languageVersion);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielRosenwasser Last question ⬆️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
else {
tokenValue = String.fromCharCode(codePointAt(text, pos));
tokenValue = "#";
error(Diagnostics.Invalid_character, pos++, charSize(ch));
}
return token = SyntaxKind.PrivateIdentifier;
Expand Down
215 changes: 215 additions & 0 deletions tests/baselines/reference/privateNamesEscapeSequences01(target=es2015).js
View file
Open in desktop
@@ -0,0 +1,215 @@
//// [tests/cases/conformance/classes/members/privateNames/privateNamesEscapeSequences01.ts] ////

//// [IdentifierNameWithEscape1.ts]
export class IdentifierNameWithEscape1 {
\u0078: number;

constructor() {
this.\u0078 = 0;
}

doThing() {
this.x = 42;
}
}

//// [IdentifierNameWithEscape2.ts]
export class IdentifierNameWithEscape2 {
x\u0078: number;

constructor() {
this.x\u0078 = 0;
}

doThing() {
this.xx = 42;
}
}

//// [IdentifierNameWithExtendedEscape1.ts]
export class IdentifierNameWithExtendedEscape1 {
\u{78}: number;

constructor() {
this.\u{78} = 0;
}

doThing() {
this.x = 42;
}
}

//// [IdentifierNameWithExtendedEscape2.ts]
export class IdentifierNameWithExtendedEscape2 {
x\u{78}: number;

constructor() {
this.x\u{78} = 0;
}

doThing() {
this.xx = 42;
}
}

//// [PrivateIdentifierNameWithEscape1.ts]
export class PrivateIdentifierWithEscape1 {
#\u0078: number;

constructor() {
this.#\u0078 = 0;
}

doThing() {
this.#x = 42;
}
}

//// [PrivateIdentifierNameWithEscape2.ts]
export class PrivateIdentifierWithEscape2 {
#x\u0078: number;

constructor() {
this.#x\u0078 = 0;
}

doThing() {
this.#xx = 42;
}
}

//// [PrivateIdentifierNameWithExtendedEscape1.ts]
export class PrivateIdentifierWithExtendedEscape1 {
#\u{78}: number;

constructor() {
this.#\u{78} = 0;
}

doThing() {
this.#x = 42;
}
}

//// [PrivateIdentifierNameWithExtendedEscape2.ts]
export class PrivateIdentifierWithExtendedEscape2 {
#x\u{78}: number;

constructor() {
this.#x\u{78} = 0;
}

doThing() {
this.#xx = 42;
}
}


//// [IdentifierNameWithEscape1.js]
export class IdentifierNameWithEscape1 {
constructor() {
this.\u0078 = 0;
}
doThing() {
this.x = 42;
}
}
//// [IdentifierNameWithEscape2.js]
export class IdentifierNameWithEscape2 {
constructor() {
this.x\u0078 = 0;
}
doThing() {
this.xx = 42;
}
}
//// [IdentifierNameWithExtendedEscape1.js]
export class IdentifierNameWithExtendedEscape1 {
constructor() {
this.\u{78} = 0;
}
doThing() {
this.x = 42;
}
}
//// [IdentifierNameWithExtendedEscape2.js]
export class IdentifierNameWithExtendedEscape2 {
constructor() {
this.x\u{78} = 0;
}
doThing() {
this.xx = 42;
}
}
//// [PrivateIdentifierNameWithEscape1.js]
var __classPrivateFieldSet = (this && this.__classPrivateFieldSet) || function (receiver, state, value, kind, f) {
if (kind === "m") throw new TypeError("Private method is not writable");
if (kind === "a" && !f) throw new TypeError("Private accessor was defined without a setter");
if (typeof state === "function" ? receiver !== state || !f : !state.has(receiver)) throw new TypeError("Cannot write private member to an object whose class did not declare it");
return (kind === "a" ? f.call(receiver, value) : f ? f.value = value : state.set(receiver, value)), value;
};
var _PrivateIdentifierWithEscape1_x;
export class PrivateIdentifierWithEscape1 {
constructor() {
_PrivateIdentifierWithEscape1_x.set(this, void 0);
__classPrivateFieldSet(this, _PrivateIdentifierWithEscape1_x, 0, "f");
}
doThing() {
__classPrivateFieldSet(this, _PrivateIdentifierWithEscape1_x, 42, "f");
}
}
_PrivateIdentifierWithEscape1_x = new WeakMap();
//// [PrivateIdentifierNameWithEscape2.js]
var __classPrivateFieldSet = (this && this.__classPrivateFieldSet) || function (receiver, state, value, kind, f) {
if (kind === "m") throw new TypeError("Private method is not writable");
if (kind === "a" && !f) throw new TypeError("Private accessor was defined without a setter");
if (typeof state === "function" ? receiver !== state || !f : !state.has(receiver)) throw new TypeError("Cannot write private member to an object whose class did not declare it");
return (kind === "a" ? f.call(receiver, value) : f ? f.value = value : state.set(receiver, value)), value;
};
var _PrivateIdentifierWithEscape2_xx;
export class PrivateIdentifierWithEscape2 {
constructor() {
_PrivateIdentifierWithEscape2_xx.set(this, void 0);
__classPrivateFieldSet(this, _PrivateIdentifierWithEscape2_xx, 0, "f");
}
doThing() {
__classPrivateFieldSet(this, _PrivateIdentifierWithEscape2_xx, 42, "f");
}
}
_PrivateIdentifierWithEscape2_xx = new WeakMap();
//// [PrivateIdentifierNameWithExtendedEscape1.js]
var __classPrivateFieldSet = (this && this.__classPrivateFieldSet) || function (receiver, state, value, kind, f) {
if (kind === "m") throw new TypeError("Private method is not writable");
if (kind === "a" && !f) throw new TypeError("Private accessor was defined without a setter");
if (typeof state === "function" ? receiver !== state || !f : !state.has(receiver)) throw new TypeError("Cannot write private member to an object whose class did not declare it");
return (kind === "a" ? f.call(receiver, value) : f ? f.value = value : state.set(receiver, value)), value;
};
var _PrivateIdentifierWithExtendedEscape1_x;
export class PrivateIdentifierWithExtendedEscape1 {
constructor() {
_PrivateIdentifierWithExtendedEscape1_x.set(this, void 0);
__classPrivateFieldSet(this, _PrivateIdentifierWithExtendedEscape1_x, 0, "f");
}
doThing() {
__classPrivateFieldSet(this, _PrivateIdentifierWithExtendedEscape1_x, 42, "f");
}
}
_PrivateIdentifierWithExtendedEscape1_x = new WeakMap();
//// [PrivateIdentifierNameWithExtendedEscape2.js]
var __classPrivateFieldSet = (this && this.__classPrivateFieldSet) || function (receiver, state, value, kind, f) {
if (kind === "m") throw new TypeError("Private method is not writable");
if (kind === "a" && !f) throw new TypeError("Private accessor was defined without a setter");
if (typeof state === "function" ? receiver !== state || !f : !state.has(receiver)) throw new TypeError("Cannot write private member to an object whose class did not declare it");
return (kind === "a" ? f.call(receiver, value) : f ? f.value = value : state.set(receiver, value)), value;
};
var _PrivateIdentifierWithExtendedEscape2_xx;
export class PrivateIdentifierWithExtendedEscape2 {
constructor() {
_PrivateIdentifierWithExtendedEscape2_xx.set(this, void 0);
__classPrivateFieldSet(this, _PrivateIdentifierWithExtendedEscape2_xx, 0, "f");
}
doThing() {
__classPrivateFieldSet(this, _PrivateIdentifierWithExtendedEscape2_xx, 42, "f");
}
}
_PrivateIdentifierWithExtendedEscape2_xx = new WeakMap();