From 43d0794ba355efb8fe257ef5fffc1db186c37cd0 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Thu, 21 Jun 2018 10:01:39 -0700 Subject: [PATCH] Fix crash when binding jsdoc-style inner namepaths (#25106) * getDeclarationIdentifier handles undefined name getNameOfDeclaration actually doesn't handle all declarations, only those that produce names that could be reasonably used as an identifier. Until now, getDeclarationIdentifier assumed that getNameOfDeclaration always returned a name. This caused crashes whenever we tried to get the name of something like a Constructor. * Add test and baselines * getNameOfDeclaration can return undefined This requires all callers to handle it, which turns out now to be too disruptive. * Fix lint --- src/compiler/checker.ts | 23 +++++++-------- src/compiler/factory.ts | 8 +++--- src/compiler/utilities.ts | 23 ++++++++------- src/services/codefixes/inferFromUsage.ts | 5 ++-- src/services/completions.ts | 4 +-- src/services/findAllReferences.ts | 5 ++-- src/services/formatting/formatting.ts | 5 +++- src/services/navigateTo.ts | 2 +- .../reference/api/tsserverlibrary.d.ts | 8 +++--- tests/baselines/reference/api/typescript.d.ts | 4 +-- .../typedefInnerNamepaths.errors.txt | 28 +++++++++++++++++++ .../reference/typedefInnerNamepaths.symbols | 14 ++++++++++ .../reference/typedefInnerNamepaths.types | 14 ++++++++++ .../jsdoc/typedefInnerNamepaths.ts | 13 +++++++++ 14 files changed, 113 insertions(+), 43 deletions(-) create mode 100644 tests/baselines/reference/typedefInnerNamepaths.errors.txt create mode 100644 tests/baselines/reference/typedefInnerNamepaths.symbols create mode 100644 tests/baselines/reference/typedefInnerNamepaths.types create mode 100644 tests/cases/conformance/jsdoc/typedefInnerNamepaths.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 648439f06b7..03742128935 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -22807,11 +22807,7 @@ namespace ts { addDiagnostic(UnusedKind.Local, createDiagnosticForNode(node, message, name)); } - function parameterNameStartsWithUnderscore(parameterName: DeclarationName) { - return parameterName && isIdentifierThatStartsWithUnderScore(parameterName); - } - - function isIdentifierThatStartsWithUnderScore(node: Node) { + function isIdentifierThatStartsWithUnderscore(node: Node) { return isIdentifier(node) && idText(node).charCodeAt(0) === CharacterCodes._; } @@ -22859,7 +22855,7 @@ namespace ts { const typeParameters = getEffectiveTypeParameterDeclarations(node); if (!(node.flags & NodeFlags.Ambient) && last(getSymbolOfNode(node).declarations) === node) { for (const typeParameter of typeParameters) { - if (!(getMergedSymbol(typeParameter.symbol).isReferenced! & SymbolFlags.TypeParameter) && !isIdentifierThatStartsWithUnderScore(typeParameter.name)) { + if (!(getMergedSymbol(typeParameter.symbol).isReferenced! & SymbolFlags.TypeParameter) && !isIdentifierThatStartsWithUnderscore(typeParameter.name)) { addDiagnostic(UnusedKind.Parameter, createDiagnosticForNode(typeParameter.name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(typeParameter.symbol))); } } @@ -22897,7 +22893,7 @@ namespace ts { for (const declaration of local.declarations) { if (isAmbientModule(declaration) || - (isVariableDeclaration(declaration) && isForInOrOfStatement(declaration.parent.parent) || isImportedDeclaration(declaration)) && isIdentifierThatStartsWithUnderScore(declaration.name!)) { + (isVariableDeclaration(declaration) && isForInOrOfStatement(declaration.parent.parent) || isImportedDeclaration(declaration)) && isIdentifierThatStartsWithUnderscore(declaration.name!)) { continue; } @@ -22916,9 +22912,9 @@ namespace ts { } else { const parameter = local.valueDeclaration && tryGetRootParameterDeclaration(local.valueDeclaration); - if (parameter) { - const name = getNameOfDeclaration(local.valueDeclaration); - if (!isParameterPropertyDeclaration(parameter) && !parameterIsThisKeyword(parameter) && !parameterNameStartsWithUnderscore(name)) { + const name = getNameOfDeclaration(local.valueDeclaration); + if (parameter && name) { + if (!isParameterPropertyDeclaration(parameter) && !parameterIsThisKeyword(parameter) && !isIdentifierThatStartsWithUnderscore(name)) { addDiagnostic(UnusedKind.Parameter, createDiagnosticForNode(name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(local))); } } @@ -24213,18 +24209,19 @@ namespace ts { } const propDeclaration = prop.valueDeclaration; + const name = getNameOfDeclaration(propDeclaration); // index is numeric and property name is not valid numeric literal - if (indexKind === IndexKind.Number && !(propDeclaration ? isNumericName(getNameOfDeclaration(propDeclaration)) : isNumericLiteralName(prop.escapedName))) { + if (indexKind === IndexKind.Number && !(name ? isNumericName(name) : isNumericLiteralName(prop.escapedName))) { return; } // perform property check if property or indexer is declared in 'type' // this allows us to rule out cases when both property and indexer are inherited from the base class let errorNode: Node | undefined; - if (propDeclaration && + if (propDeclaration && name && (propDeclaration.kind === SyntaxKind.BinaryExpression || - getNameOfDeclaration(propDeclaration).kind === SyntaxKind.ComputedPropertyName || + name.kind === SyntaxKind.ComputedPropertyName || prop.parent === containingType.symbol)) { errorNode = propDeclaration; } diff --git a/src/compiler/factory.ts b/src/compiler/factory.ts index 91ff766678a..7b8a50ef603 100644 --- a/src/compiler/factory.ts +++ b/src/compiler/factory.ts @@ -194,10 +194,10 @@ namespace ts { } /** Create a unique name generated for a node. */ - export function getGeneratedNameForNode(node: Node): Identifier; - /* @internal */ export function getGeneratedNameForNode(node: Node, flags: GeneratedIdentifierFlags): Identifier; // tslint:disable-line unified-signatures - export function getGeneratedNameForNode(node: Node, flags?: GeneratedIdentifierFlags): Identifier { - const name = createIdentifier(isIdentifier(node) ? idText(node) : ""); + export function getGeneratedNameForNode(node: Node | undefined): Identifier; + /* @internal */ export function getGeneratedNameForNode(node: Node | undefined, flags: GeneratedIdentifierFlags): Identifier; // tslint:disable-line unified-signatures + export function getGeneratedNameForNode(node: Node | undefined, flags?: GeneratedIdentifierFlags): Identifier { + const name = createIdentifier(node && isIdentifier(node) ? idText(node) : ""); name.autoGenerateFlags = GeneratedIdentifierFlags.Node | flags!; name.autoGenerateId = nextAutoGenerateId; name.original = node; diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 7c2a13abbf7..93415815e74 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -745,8 +745,8 @@ namespace ts { // Return display name of an identifier // Computed property names will just be emitted as "[]", where is the source // text of the expression in the computed property. - export function declarationNameToString(name: DeclarationName | QualifiedName) { - return getFullWidth(name) === 0 ? "(Missing)" : getTextOfNode(name); + export function declarationNameToString(name: DeclarationName | QualifiedName | undefined) { + return !name || getFullWidth(name) === 0 ? "(Missing)" : getTextOfNode(name); } export function getNameFromIndexInfo(info: IndexInfo): string | undefined { @@ -4858,7 +4858,7 @@ namespace ts { function getDeclarationIdentifier(node: Declaration | Expression): Identifier | undefined { const name = getNameOfDeclaration(node); - return isIdentifier(name) ? name : undefined; + return name && isIdentifier(name) ? name : undefined; } export function getNameOfJSDocTypedef(declaration: JSDocTypedefTag): Identifier | undefined { @@ -4870,16 +4870,15 @@ namespace ts { return !!(node as NamedDeclaration).name; // A 'name' property should always be a DeclarationName. } - // TODO: GH#18217 This is often used as if it returns a defined result - export function getNameOfDeclaration(declaration: Declaration | Expression): DeclarationName { + export function getNameOfDeclaration(declaration: Declaration | Expression): DeclarationName | undefined { if (!declaration) { - return undefined!; + return undefined; } switch (declaration.kind) { case SyntaxKind.ClassExpression: case SyntaxKind.FunctionExpression: if (!(declaration as ClassExpression | FunctionExpression).name) { - return getAssignedName(declaration)!; + return getAssignedName(declaration); } break; case SyntaxKind.Identifier: @@ -4901,19 +4900,19 @@ namespace ts { case SpecialPropertyAssignmentKind.PrototypeProperty: return (expr.left as PropertyAccessExpression).name; default: - return undefined!; + return undefined; } } case SyntaxKind.JSDocCallbackTag: - return (declaration as JSDocCallbackTag).name!; + return (declaration as JSDocCallbackTag).name; case SyntaxKind.JSDocTypedefTag: - return getNameOfJSDocTypedef(declaration as JSDocTypedefTag)!; + return getNameOfJSDocTypedef(declaration as JSDocTypedefTag); case SyntaxKind.ExportAssignment: { const { expression } = declaration as ExportAssignment; - return isIdentifier(expression) ? expression : undefined!; + return isIdentifier(expression) ? expression : undefined; } } - return (declaration as NamedDeclaration).name!; + return (declaration as NamedDeclaration).name; } function getAssignedName(node: Node): DeclarationName | undefined { diff --git a/src/services/codefixes/inferFromUsage.ts b/src/services/codefixes/inferFromUsage.ts index c58c001b403..3e26e4cdd32 100644 --- a/src/services/codefixes/inferFromUsage.ts +++ b/src/services/codefixes/inferFromUsage.ts @@ -33,8 +33,9 @@ namespace ts.codefix { const token = getTokenAtPosition(sourceFile, start, /*includeJsDocComment*/ false); let declaration!: Declaration | undefined; const changes = textChanges.ChangeTracker.with(context, changes => { declaration = doChange(changes, sourceFile, token, errorCode, program, cancellationToken, /*markSeenseen*/ returnTrue); }); - return changes.length === 0 ? undefined - : [createCodeFixAction(fixId, changes, [getDiagnostic(errorCode, token), getNameOfDeclaration(declaration!).getText(sourceFile)], fixId, Diagnostics.Infer_all_types_from_usage)]; + const name = getNameOfDeclaration(declaration!); + return !name || changes.length === 0 ? undefined + : [createCodeFixAction(fixId, changes, [getDiagnostic(errorCode, token), name.getText(sourceFile)], fixId, Diagnostics.Infer_all_types_from_usage)]; }, fixIds: [fixId], getAllCodeActions(context) { diff --git a/src/services/completions.ts b/src/services/completions.ts index e31ac4e81f3..21d704661b2 100644 --- a/src/services/completions.ts +++ b/src/services/completions.ts @@ -1122,7 +1122,7 @@ namespace ts.Completions { // If this is e.g. [Symbol.iterator], add a completion for `Symbol`. const symbolSymbol = firstDefined(symbol.declarations, decl => { const name = getNameOfDeclaration(decl); - const leftName = name.kind === SyntaxKind.ComputedPropertyName ? getLeftMostName(name.expression) : undefined; + const leftName = name && name.kind === SyntaxKind.ComputedPropertyName ? getLeftMostName(name.expression) : undefined; return leftName && typeChecker.getSymbolAtLocation(leftName); }); if (symbolSymbol) { @@ -1966,7 +1966,7 @@ namespace ts.Completions { // NOTE: if one only performs this step when m.name is an identifier, // things like '__proto__' are not filtered out. const name = getNameOfDeclaration(m); - existingName = isPropertyNameLiteral(name) ? getEscapedTextOfIdentifierOrLiteral(name) : undefined; + existingName = name && isPropertyNameLiteral(name) ? getEscapedTextOfIdentifierOrLiteral(name) : undefined; } existingMemberNames.set(existingName!, true); // TODO: GH#18217 diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index 46dd83851c1..0fbcd5cd6e9 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -981,6 +981,7 @@ namespace ts.FindAllReferences.Core { function getReferenceForShorthandProperty({ flags, valueDeclaration }: Symbol, search: Search, state: State): void { const shorthandValueSymbol = state.checker.getShorthandAssignmentValueSymbol(valueDeclaration)!; + const name = getNameOfDeclaration(valueDeclaration); /* * Because in short-hand property assignment, an identifier which stored as name of the short-hand property assignment * has two meanings: property name and property value. Therefore when we do findAllReference at the position where @@ -988,8 +989,8 @@ namespace ts.FindAllReferences.Core { * the position in short-hand property assignment excluding property accessing. However, if we do findAllReference at the * position of property accessing, the referenceEntry of such position will be handled in the first case. */ - if (!(flags & SymbolFlags.Transient) && search.includes(shorthandValueSymbol)) { - addReference(getNameOfDeclaration(valueDeclaration), shorthandValueSymbol, state); + if (!(flags & SymbolFlags.Transient) && name && search.includes(shorthandValueSymbol)) { + addReference(name, shorthandValueSymbol, state); } } diff --git a/src/services/formatting/formatting.ts b/src/services/formatting/formatting.ts index eeec4b15ea7..f37fb1bc5dd 100644 --- a/src/services/formatting/formatting.ts +++ b/src/services/formatting/formatting.ts @@ -512,7 +512,10 @@ namespace ts.formatting { // falls through case SyntaxKind.PropertyDeclaration: case SyntaxKind.Parameter: - return getNameOfDeclaration(node).kind; + const name = getNameOfDeclaration(node); + if (name) { + return name.kind; + } } } diff --git a/src/services/navigateTo.ts b/src/services/navigateTo.ts index 723d7f59b3a..dc03c736f3c 100644 --- a/src/services/navigateTo.ts +++ b/src/services/navigateTo.ts @@ -113,7 +113,7 @@ namespace ts.NavigateTo { // First, if we started with a computed property name, then add all but the last // portion into the container array. const name = getNameOfDeclaration(declaration); - if (name.kind === SyntaxKind.ComputedPropertyName && !tryAddComputedPropertyName(name.expression, containers, /*includeLastPortion*/ false)) { + if (name && name.kind === SyntaxKind.ComputedPropertyName && !tryAddComputedPropertyName(name.expression, containers, /*includeLastPortion*/ false)) { return undefined; } diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index 9ff2d42228d..a98241125a5 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -6106,7 +6106,7 @@ declare namespace ts { function isLateVisibilityPaintedStatement(node: Node): node is LateVisibilityPaintedStatement; function isAnyImportOrReExport(node: Node): node is AnyImportOrReExport; function getEnclosingBlockScopeContainer(node: Node): Node; - function declarationNameToString(name: DeclarationName | QualifiedName): string; + function declarationNameToString(name: DeclarationName | QualifiedName | undefined): string; function getNameFromIndexInfo(info: IndexInfo): string | undefined; function getTextOfPropertyName(name: PropertyName): __String; function entityNameToString(name: EntityNameOrEntityNameExpression): string; @@ -6685,7 +6685,7 @@ declare namespace ts { function isNamedDeclaration(node: Node): node is NamedDeclaration & { name: DeclarationName; }; - function getNameOfDeclaration(declaration: Declaration | Expression): DeclarationName; + function getNameOfDeclaration(declaration: Declaration | Expression): DeclarationName | undefined; /** * Gets the JSDoc parameter tags for the node if present. * @@ -7702,8 +7702,8 @@ declare namespace ts { /** Create a unique name based on the supplied text. This does not consider names injected by the transformer. */ function createFileLevelUniqueName(text: string): Identifier; /** Create a unique name generated for a node. */ - function getGeneratedNameForNode(node: Node): Identifier; - function getGeneratedNameForNode(node: Node, flags: GeneratedIdentifierFlags): Identifier; + function getGeneratedNameForNode(node: Node | undefined): Identifier; + function getGeneratedNameForNode(node: Node | undefined, flags: GeneratedIdentifierFlags): Identifier; function createToken(token: TKind): Token; function createSuper(): SuperExpression; function createThis(): ThisExpression & Token; diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts index 77bb50c3065..9995fec649c 100644 --- a/tests/baselines/reference/api/typescript.d.ts +++ b/tests/baselines/reference/api/typescript.d.ts @@ -3188,7 +3188,7 @@ declare namespace ts { */ function unescapeIdentifier(id: string): string; function getNameOfJSDocTypedef(declaration: JSDocTypedefTag): Identifier | undefined; - function getNameOfDeclaration(declaration: Declaration | Expression): DeclarationName; + function getNameOfDeclaration(declaration: Declaration | Expression): DeclarationName | undefined; /** * Gets the JSDoc parameter tags for the node if present. * @@ -3623,7 +3623,7 @@ declare namespace ts { /** Create a unique name based on the supplied text. This does not consider names injected by the transformer. */ function createFileLevelUniqueName(text: string): Identifier; /** Create a unique name generated for a node. */ - function getGeneratedNameForNode(node: Node): Identifier; + function getGeneratedNameForNode(node: Node | undefined): Identifier; function createToken(token: TKind): Token; function createSuper(): SuperExpression; function createThis(): ThisExpression & Token; diff --git a/tests/baselines/reference/typedefInnerNamepaths.errors.txt b/tests/baselines/reference/typedefInnerNamepaths.errors.txt new file mode 100644 index 00000000000..179c36b0edd --- /dev/null +++ b/tests/baselines/reference/typedefInnerNamepaths.errors.txt @@ -0,0 +1,28 @@ +tests/cases/conformance/jsdoc/bug25104.js(1,7): error TS2300: Duplicate identifier 'C'. +tests/cases/conformance/jsdoc/bug25104.js(3,19): error TS1005: '}' expected. +tests/cases/conformance/jsdoc/bug25104.js(4,26): error TS2300: Duplicate identifier 'C'. +tests/cases/conformance/jsdoc/bug25104.js(6,18): error TS8024: JSDoc '@param' tag has name '', but there is no parameter with that name. +tests/cases/conformance/jsdoc/bug25104.js(6,18): error TS1005: '}' expected. + + +==== tests/cases/conformance/jsdoc/bug25104.js (5 errors) ==== + class C { + ~ +!!! error TS2300: Duplicate identifier 'C'. + /** + * @typedef {C~A} C~B + ~ +!!! error TS1005: '}' expected. + * @typedef {object} C~A + ~ +!!! error TS2300: Duplicate identifier 'C'. + */ + /** @param {C~A} o */ + +!!! error TS8024: JSDoc '@param' tag has name '', but there is no parameter with that name. + ~ +!!! error TS1005: '}' expected. + constructor(o) { + } + } + \ No newline at end of file diff --git a/tests/baselines/reference/typedefInnerNamepaths.symbols b/tests/baselines/reference/typedefInnerNamepaths.symbols new file mode 100644 index 00000000000..2ddcf419734 --- /dev/null +++ b/tests/baselines/reference/typedefInnerNamepaths.symbols @@ -0,0 +1,14 @@ +=== tests/cases/conformance/jsdoc/bug25104.js === +class C { +>C : Symbol(C, Decl(bug25104.js, 0, 0)) + + /** + * @typedef {C~A} C~B + * @typedef {object} C~A + */ + /** @param {C~A} o */ + constructor(o) { +>o : Symbol(o, Decl(bug25104.js, 6, 16)) + } +} + diff --git a/tests/baselines/reference/typedefInnerNamepaths.types b/tests/baselines/reference/typedefInnerNamepaths.types new file mode 100644 index 00000000000..16e8ab556a7 --- /dev/null +++ b/tests/baselines/reference/typedefInnerNamepaths.types @@ -0,0 +1,14 @@ +=== tests/cases/conformance/jsdoc/bug25104.js === +class C { +>C : C + + /** + * @typedef {C~A} C~B + * @typedef {object} C~A + */ + /** @param {C~A} o */ + constructor(o) { +>o : any + } +} + diff --git a/tests/cases/conformance/jsdoc/typedefInnerNamepaths.ts b/tests/cases/conformance/jsdoc/typedefInnerNamepaths.ts new file mode 100644 index 00000000000..ee71909c2ed --- /dev/null +++ b/tests/cases/conformance/jsdoc/typedefInnerNamepaths.ts @@ -0,0 +1,13 @@ +// @noEmit: true +// @allowJs: true +// @checkJs: true +// @Filename: bug25104.js +class C { + /** + * @typedef {C~A} C~B + * @typedef {object} C~A + */ + /** @param {C~A} o */ + constructor(o) { + } +}