Skip to content

Remove originalKeywordKind from Identifiers #51497

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.

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ import {
SpreadElement,
Statement,
StringLiteral,
stringToToken,
SuperExpression,
SwitchStatement,
Symbol,
Expand Down Expand Up @@ -2419,14 +2420,19 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void {
!(node.flags & NodeFlags.JSDoc) &&
!isIdentifierName(node)) {

const originalKeywordKind = stringToToken(node.escapedText as string);
if (originalKeywordKind === undefined) {
return;
}

// strict mode identifiers
if (inStrictMode &&
node.originalKeywordKind! >= SyntaxKind.FirstFutureReservedWord &&
node.originalKeywordKind! <= SyntaxKind.LastFutureReservedWord) {
originalKeywordKind >= SyntaxKind.FirstFutureReservedWord &&
originalKeywordKind <= SyntaxKind.LastFutureReservedWord) {
file.bindDiagnostics.push(createDiagnosticForNode(node,
getStrictModeIdentifierMessage(node), declarationNameToString(node)));
}
else if (node.originalKeywordKind === SyntaxKind.AwaitKeyword) {
else if (originalKeywordKind === SyntaxKind.AwaitKeyword) {
if (isExternalModule(file) && isInTopLevelContext(node)) {
file.bindDiagnostics.push(createDiagnosticForNode(node,
Diagnostics.Identifier_expected_0_is_a_reserved_word_at_the_top_level_of_a_module,
Expand All @@ -2438,7 +2444,7 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void {
declarationNameToString(node)));
}
}
else if (node.originalKeywordKind === SyntaxKind.YieldKeyword && node.flags & NodeFlags.YieldContext) {
else if (originalKeywordKind === SyntaxKind.YieldKeyword && node.flags & NodeFlags.YieldContext) {
file.bindDiagnostics.push(createDiagnosticForNode(node,
Diagnostics.Identifier_expected_0_is_a_reserved_word_that_cannot_be_used_here,
declarationNameToString(node)));
Expand Down Expand Up @@ -2739,14 +2745,14 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void {
bindBlockScopedDeclaration(parentNode as Declaration, SymbolFlags.TypeAlias, SymbolFlags.TypeAliasExcludes);
break;
}
checkContextualIdentifier(node as Identifier);
// falls through
case SyntaxKind.ThisKeyword:
// TODO: Why use `isExpression` here? both Identifier and ThisKeyword are expressions.
if (currentFlow && (isExpression(node) || parent.kind === SyntaxKind.ShorthandPropertyAssignment)) {
(node as Identifier | ThisExpression).flowNode = currentFlow;
}
// TODO: a `ThisExpression` is not an Identifier, this cast is unsound
return checkContextualIdentifier(node as Identifier);
break;
case SyntaxKind.QualifiedName:
if (currentFlow && isPartOfTypeQuery(node)) {
(node as QualifiedName).flowNode = currentFlow;
Expand Down
7 changes: 5 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -923,6 +923,7 @@ import {
StringLiteralLike,
StringLiteralType,
StringMappingType,
stringToToken,
stripQuotes,
StructuredType,
SubstitutionType,
Expand Down Expand Up @@ -23117,11 +23118,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
break;
case SyntaxKind.Parameter:
const param = declaration as ParameterDeclaration;
let keywordKind: SyntaxKind | undefined;
if (isIdentifier(param.name) &&
(isCallSignatureDeclaration(param.parent) || isMethodSignature(param.parent) || isFunctionTypeNode(param.parent)) &&
param.parent.parameters.indexOf(param) > -1 &&
(resolveName(param, param.name.escapedText, SymbolFlags.Type, undefined, param.name.escapedText, /*isUse*/ true) ||
param.name.originalKeywordKind && isTypeNodeKind(param.name.originalKeywordKind))) {
(keywordKind = stringToToken(param.name.escapedText as string)) && isTypeNodeKind(keywordKind))
) {
const newName = "arg" + param.parent.parameters.indexOf(param);
const typeName = declarationNameToString(param.name) + (param.dotDotDotToken ? "[]" : "");
errorOrSuggestion(noImplicitAny, declaration, Diagnostics.Parameter_has_a_name_but_no_type_Did_you_mean_0_Colon_1, newName, typeName);
Expand Down Expand Up @@ -46687,7 +46690,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

function checkGrammarNameInLetOrConstDeclarations(name: Identifier | BindingPattern): boolean {
if (name.kind === SyntaxKind.Identifier) {
if (name.originalKeywordKind === SyntaxKind.LetKeyword) {
if (name.escapedText === "let") {
return grammarErrorOnNode(name, Diagnostics.let_is_not_allowed_to_be_used_as_a_name_in_let_or_const_declarations);
}
}
Expand Down
33 changes: 16 additions & 17 deletions src/compiler/factory/nodeFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,6 @@ import {
startsWith,
Statement,
StringLiteral,
stringToToken,
SuperExpression,
SwitchStatement,
SyntaxKind,
Expand Down Expand Up @@ -1150,16 +1149,15 @@ export function createNodeFactory(flags: NodeFactoryFlags, baseFactory: BaseNode
// Identifiers
//

function createBaseIdentifier(escapedText: __String, originalKeywordKind: SyntaxKind | undefined) {
function createBaseIdentifier(escapedText: __String) {
const node = baseFactory.createBaseIdentifierNode(SyntaxKind.Identifier) as Mutable<Identifier>;
node.originalKeywordKind = originalKeywordKind;
node.escapedText = escapedText;
node.autoGenerateFlags = GeneratedIdentifierFlags.None;
return node;
}

function createBaseGeneratedIdentifier(text: string, autoGenerateFlags: GeneratedIdentifierFlags, prefix: string | GeneratedNamePart | undefined, suffix: string | undefined) {
const node = createBaseIdentifier(escapeLeadingUnderscores(text), /*originalKeywordKind*/ undefined) as Mutable<GeneratedIdentifier>;
const node = createBaseIdentifier(escapeLeadingUnderscores(text)) as Mutable<GeneratedIdentifier>;
node.autoGenerateFlags = autoGenerateFlags;
node.autoGenerateId = nextAutoGenerateId;
node.autoGeneratePrefix = prefix;
Expand All @@ -1168,16 +1166,17 @@ export function createNodeFactory(flags: NodeFactoryFlags, baseFactory: BaseNode
return node;
}

// @api
function createIdentifier(text: string, typeArguments?: readonly (TypeNode | TypeParameterDeclaration)[], originalKeywordKind?: SyntaxKind, hasExtendedUnicodeEscape?: boolean): Identifier {
if (originalKeywordKind === undefined && text) {
originalKeywordKind = stringToToken(text);
}
if (originalKeywordKind === SyntaxKind.Identifier) {
originalKeywordKind = undefined;
}

const node = createBaseIdentifier(escapeLeadingUnderscores(text), originalKeywordKind);
/**
*
* @param text
* @param typeArguments
* @param originalKeywordKind ONLY provided by the parser.
* @param hasExtendedUnicodeEscape ONLY provided by the parser.
* @returns
*/
// @api
function createIdentifier(text: string, typeArguments?: readonly (TypeNode | TypeParameterDeclaration)[], originalKeywordKind?: SyntaxKind, hasExtendedUnicodeEscape?: boolean): Identifier {
const node = createBaseIdentifier(escapeLeadingUnderscores(text));
node.typeArguments = asNodeArray(typeArguments);
node.hasExtendedUnicodeEscape = hasExtendedUnicodeEscape;
node.jsDoc = undefined; // initialized by parser (JsDocContainer)
Expand All @@ -1186,7 +1185,7 @@ export function createNodeFactory(flags: NodeFactoryFlags, baseFactory: BaseNode
node.symbol = undefined!; // initialized by checker

// NOTE: we do not include transform flags of typeArguments in an identifier as they do not contribute to transformations
if (node.originalKeywordKind === SyntaxKind.AwaitKeyword) {
if (originalKeywordKind === SyntaxKind.AwaitKeyword) {
node.transformFlags |= TransformFlags.ContainsPossibleTopLevelAwait;
}
if (node.hasExtendedUnicodeEscape) {
Expand Down Expand Up @@ -6372,7 +6371,7 @@ export function createNodeFactory(flags: NodeFactoryFlags, baseFactory: BaseNode
}

function cloneGeneratedIdentifier(node: GeneratedIdentifier): GeneratedIdentifier {
const clone = createBaseIdentifier(node.escapedText, /*originalKeywordKind*/ undefined) as Mutable<GeneratedIdentifier>;
const clone = createBaseIdentifier(node.escapedText) as Mutable<GeneratedIdentifier>;
clone.flags |= node.flags & ~NodeFlags.Synthesized;
clone.autoGenerateFlags = node.autoGenerateFlags;
clone.autoGenerateId = node.autoGenerateId;
Expand All @@ -6384,7 +6383,7 @@ export function createNodeFactory(flags: NodeFactoryFlags, baseFactory: BaseNode
}

function cloneIdentifier(node: Identifier): Identifier {
const clone = createBaseIdentifier(node.escapedText, node.originalKeywordKind);
const clone = createBaseIdentifier(node.escapedText);
clone.flags |= node.flags & ~NodeFlags.Synthesized;
clone.typeArguments = node.typeArguments;
clone.hasExtendedUnicodeEscape = node.hasExtendedUnicodeEscape;
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3194,7 +3194,7 @@ namespace Parser {
// into an actual .ConstructorDeclaration.
const methodDeclaration = node as MethodDeclaration;
const nameIsConstructor = methodDeclaration.name.kind === SyntaxKind.Identifier &&
methodDeclaration.name.originalKeywordKind === SyntaxKind.ConstructorKeyword;
methodDeclaration.name.escapedText === "constructor";

return !nameIsConstructor;
}
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/transformers/es2015.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ import {
isHoistedFunction,
isHoistedVariableStatement,
isIdentifier,
isIdentifierANonContextualKeyword,
isIfStatement,
isInternalName,
isIterationStatement,
Expand All @@ -122,6 +121,7 @@ import {
isSpreadElement,
isStatement,
isStatic,
isStringANonContextualKeyword,
isSuperProperty,
isSwitchStatement,
isTryStatement,
Expand Down Expand Up @@ -1074,7 +1074,7 @@ export function transformES2015(context: TransformationContext): (x: SourceFile
function transformClassBody(node: ClassExpression | ClassDeclaration, extendsClauseElement: ExpressionWithTypeArguments | undefined): Block {
const statements: Statement[] = [];
const name = factory.getInternalName(node);
const constructorLikeName = isIdentifierANonContextualKeyword(name) ? factory.getGeneratedNameForNode(name) : name;
const constructorLikeName = isStringANonContextualKeyword(idText(name)) ? factory.getGeneratedNameForNode(name) : name;
startLexicalEnvironment();
addExtendsHelperIfNeeded(statements, node, extendsClauseElement);
addConstructor(statements, node, constructorLikeName, extendsClauseElement);
Expand Down
4 changes: 1 addition & 3 deletions src/compiler/transformers/es5.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
Expression,
getOriginalNodeId,
Identifier,
idText,
isIdentifier,
isPrivateIdentifier,
isPropertyAccessExpression,
Expand All @@ -15,7 +14,6 @@ import {
JsxOpeningElement,
JsxSelfClosingElement,
Node,
nodeIsSynthesized,
PropertyAccessExpression,
PropertyAssignment,
setTextRange,
Expand Down Expand Up @@ -139,7 +137,7 @@ export function transformES5(context: TransformationContext): (x: SourceFile | B
* @param name An Identifier
*/
function trySubstituteReservedName(name: Identifier) {
const token = name.originalKeywordKind || (nodeIsSynthesized(name) ? stringToToken(idText(name)) : undefined);
const token = stringToToken(name.escapedText as string);
if (token !== undefined && token >= SyntaxKind.FirstReservedWord && token <= SyntaxKind.LastReservedWord) {
return setTextRange(factory.createStringLiteralFromNode(name), name);
}
Expand Down
4 changes: 3 additions & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1677,7 +1677,6 @@ export interface Identifier extends PrimaryExpression, Declaration, JSDocContain
* Text of identifier, but if the identifier begins with two underscores, this will begin with three.
*/
readonly escapedText: __String;
readonly originalKeywordKind?: SyntaxKind; // Original syntaxKind which get set so that we can report an error later
Copy link
Contributor

Choose a reason for hiding this comment

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

My only concern with this is that it is a public API breaking change. Do we expose any other way to get this information? stringToToken is currently marked /** @internal */.

If we find that there are consumers in the field that need this information, we could potentially expose it via a getter in the src/deprecatedCompat project with a @deprecated comment and the caveat that repeated access would be slower since it would be recalculated each time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a best practice to add deprecated accessors to prototypes right now?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no prototype to add it to, since everything inherits from Node. You would have to add it to the instance. There's an addNodeFactoryPatcher function that lets you hook into the creation of a NodeFactory to patch in a deprecation, but it doesn't affect factories that already exist (i.e., parseNodeFactory in parser.ts, the internal factory inside of the Parser namespace in parser.ts, or factory in nodeFactory.ts).

The current patching mechanism was primarily to handle patching the factory API itself, and only the one that is reachable during transformations. As a result, the current patching mechanism doesn't touch the factory used internally by the parser, which is the one you'd want most if you want to patch node instances returned by the API.

Copy link
Member Author

@DanielRosenwasser DanielRosenwasser Dec 14, 2022

Choose a reason for hiding this comment

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

It does seem like a win in terms of memory usage. If we really want to avoid the break, #51498 gives us both.

We should still deprecate it either way.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't notice the memory improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I misspoke. we could potentially add it to the prototype of the Identifier constructor we return from objectAllocator, although this is overridden in services so you'd need to do that in two places.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, honestly, maybe it won't be that slow and sticking it on Identifier is enough. (I was hoping caching it somehow would help but that might be slower in general...)

Copy link
Contributor

Choose a reason for hiding this comment

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

We'd have to test whether defining an accessor on Identifier actually improves memory usage. If it's a wash I'd leave originalSyntaxKind alone, or just deprecate it and remove it later.

/** @internal */ readonly autoGenerateFlags?: GeneratedIdentifierFlags; // Specifies whether to auto-generate the text for an identifier.
/** @internal */ readonly autoGenerateId?: number; // Ensures unique generated identifiers get unique names, but clones get the same name.
/** @internal */ readonly autoGeneratePrefix?: string | GeneratedNamePart;
Expand All @@ -1686,6 +1685,9 @@ export interface Identifier extends PrimaryExpression, Declaration, JSDocContain
isInJSDocNamespace?: boolean; // if the node is a member in a JSDoc namespace
/** @internal */ typeArguments?: NodeArray<TypeNode | TypeParameterDeclaration>; // Only defined on synthesized nodes. Though not syntactically valid, used in emitting diagnostics, quickinfo, and signature help.
/** @internal */ jsdocDotPos?: number; // Identifier occurs in JSDoc-style generic: Id.<T>
/**
* @deprecated `originalKeywordKind` will be removed in the future.
*/ readonly originalKeywordKind?: SyntaxKind;
/**@internal*/ hasExtendedUnicodeEscape?: boolean;
}

Expand Down
13 changes: 7 additions & 6 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4340,11 +4340,6 @@ export function isStringAKeyword(name: string) {
return token !== undefined && isKeyword(token);
}

/** @internal */
export function isIdentifierANonContextualKeyword({ originalKeywordKind }: Identifier): boolean {
return !!originalKeywordKind && !isContextualKeyword(originalKeywordKind);
}

/** @internal */
export function isTrivia(token: SyntaxKind): token is TriviaSyntaxKind {
return SyntaxKind.FirstTriviaToken <= token && token <= SyntaxKind.LastTriviaToken;
Expand Down Expand Up @@ -5709,7 +5704,7 @@ export function isThisInTypeQuery(node: Node): boolean {

/** @internal */
export function identifierIsThisKeyword(id: Identifier): boolean {
return id.originalKeywordKind === SyntaxKind.ThisKeyword;
return id.escapedText === "this";
}

/** @internal */
Expand Down Expand Up @@ -7322,6 +7317,12 @@ function Identifier(this: Mutable<Node>, kind: SyntaxKind, pos: number, end: num
(this as Identifier).flowNode = undefined;
}

Object.defineProperty(Identifier.prototype, "originalKeywordKind" satisfies keyof Identifier, {
get(this: Identifier) {
return stringToToken(this.escapedText as string);
}
});

function SourceMapSource(this: SourceMapSource, fileName: string, text: string, skipTrivia?: (pos: number) => number) {
this.fileName = fileName;
this.text = text;
Expand Down
7 changes: 1 addition & 6 deletions src/harness/harnessUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,15 +206,10 @@ export function sourceFileToJSON(file: ts.Node): string {
break;

case "hasExtendedUnicodeEscape":
if ((n as any).hasExtendedUnicodeEscape) {
if ((n as ts.Identifier | ts.LiteralLikeNode).hasExtendedUnicodeEscape) {
o.hasExtendedUnicodeEscape = true;
}
break;

case "originalKeywordKind":
o[propertyName] = getKindName((n as any)[propertyName]);
break;

case "flags":
// Clear the flags that are produced by aggregating child values. That is ephemeral
// data we don't care about in the dump. We only care what the parser set directly
Expand Down
7 changes: 3 additions & 4 deletions src/services/codefixes/convertToEsModule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ import {
isExportsOrModuleExportsOrAlias,
isFunctionExpression,
isIdentifier,
isNonContextualKeyword,
isObjectLiteralExpression,
isPropertyAccessExpression,
isRequireCall,
isStringANonContextualKeyword,
isVariableStatement,
makeImport,
map,
Expand Down Expand Up @@ -161,9 +161,8 @@ type ExportRenames = ReadonlyMap<string, string>;
function collectExportRenames(sourceFile: SourceFile, checker: TypeChecker, identifiers: Identifiers): ExportRenames {
const res = new Map<string, string>();
forEachExportReference(sourceFile, node => {
const { text, originalKeywordKind } = node.name;
if (!res.has(text) && (originalKeywordKind !== undefined && isNonContextualKeyword(originalKeywordKind)
|| checker.resolveName(text, node, SymbolFlags.Value, /*excludeGlobals*/ true))) {
const text = node.name.text;
if (!res.has(text) && (isStringANonContextualKeyword(text) || checker.resolveName(text, node, SymbolFlags.Value, /*excludeGlobals*/ true))) {
// Unconditionally add an underscore in case `text` is a keyword.
res.set(text, makeUniqueName(`_${text}`, identifiers));
}
Expand Down
13 changes: 8 additions & 5 deletions src/services/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ import {
isStatement,
isStatic,
isString,
isStringAKeyword,
isStringANonContextualKeyword,
isStringLiteralLike,
isStringLiteralOrTemplate,
Expand Down Expand Up @@ -1690,8 +1691,11 @@ function isModifierLike(node: Node): ModifierSyntaxKind | undefined {
if (isModifier(node)) {
return node.kind;
}
if (isIdentifier(node) && node.originalKeywordKind && isModifierKind(node.originalKeywordKind)) {
return node.originalKeywordKind;
if (isIdentifier(node)) {
const keywordKind = stringToToken(node.text);
if (keywordKind && isModifierKind(keywordKind)) {
return keywordKind;
}
}
return undefined;
}
Expand Down Expand Up @@ -4720,7 +4724,7 @@ function isFunctionLikeBodyKeyword(kind: SyntaxKind) {
}

function keywordForNode(node: Node): SyntaxKind {
return isIdentifier(node) ? node.originalKeywordKind || SyntaxKind.Unknown : node.kind;
return isIdentifier(node) ? stringToToken(node.text) ?? SyntaxKind.Unknown : node.kind;
}

function getContextualKeywords(
Expand Down Expand Up @@ -4824,8 +4828,7 @@ function tryGetObjectTypeDeclarationCompletionContainer(sourceFile: SourceFile,
}
break;
case SyntaxKind.Identifier: {
const originalKeywordKind = (location as Identifier).originalKeywordKind;
if (originalKeywordKind && isKeyword(originalKeywordKind)) {
if (isStringAKeyword((location as Identifier).text)) {
return undefined;
}
// class c { public prop = c| }
Expand Down
4 changes: 2 additions & 2 deletions src/services/findAllReferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1940,8 +1940,8 @@ export namespace Core {

// For `export { foo as bar }`, rename `foo`, but not `bar`.
if (!isForRenameWithPrefixAndSuffixText(state.options) || alwaysGetReferences) {
const isDefaultExport = referenceLocation.originalKeywordKind === SyntaxKind.DefaultKeyword
|| exportSpecifier.name.originalKeywordKind === SyntaxKind.DefaultKeyword;
const isDefaultExport = referenceLocation.text === "default"
|| exportSpecifier.name.text === "default";
const exportKind = isDefaultExport ? ExportKind.Default : ExportKind.Named;
const exportSymbol = Debug.checkDefined(exportSpecifier.symbol);
const exportInfo = getExportInfo(exportSymbol, exportKind, state.checker);
Expand Down
Loading