From 6bb7e5c086ecbe40dfda84c75b5b5b46a8a389c2 Mon Sep 17 00:00:00 2001 From: Martin Probst Date: Mon, 9 Sep 2019 14:08:00 +0200 Subject: [PATCH] Handle parentless nodes in isParameterPropertyDeclaration Fixes #33295. This follows a similar pattern as in #20314 by requiring an explicit `parent` parameter. Where possible, it uses the appopriate variable at the call sites. In several locations there is no context available though (e.g. inspecting `valueDeclarations`) and we access `.parent` as the code previously did. From a cursory inspection this seems correct, these callpaths originate in phases where there must be a `parent` (i.e. in checker, binder, etc). Change-Id: I28e4726777b57237bec776e4001e9e69ac591b11 --- src/compiler/binder.ts | 2 +- src/compiler/checker.ts | 4 +-- src/compiler/transformers/classFields.ts | 2 +- .../transformers/declarations/diagnostics.ts | 2 +- src/compiler/transformers/ts.ts | 4 +-- src/compiler/utilities.ts | 6 ++-- src/services/findAllReferences.ts | 4 +-- src/services/navigationBar.ts | 2 +- .../generateGetAccessorAndSetAccessor.ts | 4 +-- src/testRunner/unittests/transform.ts | 30 ++++++++++++++++++- .../reference/api/tsserverlibrary.d.ts | 2 +- tests/baselines/reference/api/typescript.d.ts | 2 +- ...rmsCorrectly.transformParameterProperty.js | 18 +++++++++++ 13 files changed, 64 insertions(+), 18 deletions(-) create mode 100644 tests/baselines/reference/transformApi/transformsCorrectly.transformParameterProperty.js diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index 3bc03cdbab9..705ec3b75f4 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -2859,7 +2859,7 @@ namespace ts { // If this is a property-parameter, then also declare the property symbol into the // containing class. - if (isParameterPropertyDeclaration(node)) { + if (isParameterPropertyDeclaration(node, node.parent)) { const classDeclaration = node.parent.parent; declareSymbol(classDeclaration.symbol.members!, classDeclaration.symbol, node, SymbolFlags.Property | (node.questionToken ? SymbolFlags.Optional : SymbolFlags.None), SymbolFlags.PropertyExcludes); } diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index f8c88178885..2fa1d8acf4a 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -25705,7 +25705,7 @@ namespace ts { for (const member of node.members) { if (member.kind === SyntaxKind.Constructor) { for (const param of (member as ConstructorDeclaration).parameters) { - if (isParameterPropertyDeclaration(param) && !isBindingPattern(param.name)) { + if (isParameterPropertyDeclaration(param, member) && !isBindingPattern(param.name)) { addName(instanceNames, param.name, param.name.escapedText, DeclarationMeaning.GetOrSetAccessor); } } @@ -27469,7 +27469,7 @@ namespace ts { const parameter = local.valueDeclaration && tryGetRootParameterDeclaration(local.valueDeclaration); const name = local.valueDeclaration && getNameOfDeclaration(local.valueDeclaration); if (parameter && name) { - if (!isParameterPropertyDeclaration(parameter) && !parameterIsThisKeyword(parameter) && !isIdentifierThatStartsWithUnderscore(name)) { + if (!isParameterPropertyDeclaration(parameter, parameter.parent) && !parameterIsThisKeyword(parameter) && !isIdentifierThatStartsWithUnderscore(name)) { addDiagnostic(parameter, UnusedKind.Parameter, createDiagnosticForNode(name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(local))); } } diff --git a/src/compiler/transformers/classFields.ts b/src/compiler/transformers/classFields.ts index 4efe4ad7b38..37473d9bb20 100644 --- a/src/compiler/transformers/classFields.ts +++ b/src/compiler/transformers/classFields.ts @@ -337,7 +337,7 @@ namespace ts { if (constructor && constructor.body) { let parameterPropertyDeclarationCount = 0; for (let i = indexOfFirstStatement; i < constructor.body.statements.length; i++) { - if (isParameterPropertyDeclaration(getOriginalNode(constructor.body.statements[i]))) { + if (isParameterPropertyDeclaration(getOriginalNode(constructor.body.statements[i]), constructor)) { parameterPropertyDeclarationCount++; } else { diff --git a/src/compiler/transformers/declarations/diagnostics.ts b/src/compiler/transformers/declarations/diagnostics.ts index 61d4dd30a67..91c960ea942 100644 --- a/src/compiler/transformers/declarations/diagnostics.ts +++ b/src/compiler/transformers/declarations/diagnostics.ts @@ -135,7 +135,7 @@ namespace ts { return getReturnTypeVisibilityError; } else if (isParameter(node)) { - if (isParameterPropertyDeclaration(node) && hasModifier(node.parent, ModifierFlags.Private)) { + if (isParameterPropertyDeclaration(node, node.parent) && hasModifier(node.parent, ModifierFlags.Private)) { return getVariableDeclarationTypeVisibilityError; } return getParameterDeclarationTypeVisibilityError; diff --git a/src/compiler/transformers/ts.ts b/src/compiler/transformers/ts.ts index 4cbc8e565df..06b3fb53ec1 100644 --- a/src/compiler/transformers/ts.ts +++ b/src/compiler/transformers/ts.ts @@ -895,7 +895,7 @@ namespace ts { const members: ClassElement[] = []; const constructor = getFirstConstructorWithBody(node); const parametersWithPropertyAssignments = constructor && - filter(constructor.parameters, isParameterPropertyDeclaration); + filter(constructor.parameters, p => isParameterPropertyDeclaration(p, constructor)); if (parametersWithPropertyAssignments) { for (const parameter of parametersWithPropertyAssignments) { @@ -1907,7 +1907,7 @@ namespace ts { function transformConstructorBody(body: Block, constructor: ConstructorDeclaration) { const parametersWithPropertyAssignments = constructor && - filter(constructor.parameters, isParameterPropertyDeclaration); + filter(constructor.parameters, p => isParameterPropertyDeclaration(p, constructor)); if (!some(parametersWithPropertyAssignments)) { return visitFunctionBody(body, visitor, context); } diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index cb71df1fa93..1645b9339ae 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -1009,7 +1009,7 @@ namespace ts { } export function isDeclarationReadonly(declaration: Declaration): boolean { - return !!(getCombinedModifierFlags(declaration) & ModifierFlags.Readonly && !isParameterPropertyDeclaration(declaration)); + return !!(getCombinedModifierFlags(declaration) & ModifierFlags.Readonly && !isParameterPropertyDeclaration(declaration, declaration.parent)); } export function isVarConst(node: VariableDeclaration | VariableDeclarationList): boolean { @@ -4983,8 +4983,8 @@ namespace ts { } export type ParameterPropertyDeclaration = ParameterDeclaration & { parent: ConstructorDeclaration, name: Identifier }; - export function isParameterPropertyDeclaration(node: Node): node is ParameterPropertyDeclaration { - return hasModifier(node, ModifierFlags.ParameterPropertyModifier) && node.parent.kind === SyntaxKind.Constructor; + export function isParameterPropertyDeclaration(node: Node, parent: Node): node is ParameterPropertyDeclaration { + return hasModifier(node, ModifierFlags.ParameterPropertyModifier) && parent.kind === SyntaxKind.Constructor; } export function isEmptyBindingPattern(node: BindingName): node is BindingPattern { diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index 5d667ee55a4..f98fb2f75b8 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -1145,7 +1145,7 @@ namespace ts.FindAllReferences.Core { } export function eachSymbolReferenceInFile(definition: Identifier, checker: TypeChecker, sourceFile: SourceFile, cb: (token: Identifier) => T): T | undefined { - const symbol = isParameterPropertyDeclaration(definition.parent) + const symbol = isParameterPropertyDeclaration(definition.parent, definition.parent.parent) ? first(checker.getSymbolsOfParameterPropertyDeclaration(definition.parent, definition.text)) : checker.getSymbolAtLocation(definition); if (!symbol) return undefined; @@ -1886,7 +1886,7 @@ namespace ts.FindAllReferences.Core { const res = fromRoot(symbol); if (res) return res; - if (symbol.valueDeclaration && isParameterPropertyDeclaration(symbol.valueDeclaration)) { + if (symbol.valueDeclaration && isParameterPropertyDeclaration(symbol.valueDeclaration, symbol.valueDeclaration.parent)) { // For a parameter property, now try on the other symbol (property if this was a parameter, parameter if this was a property). const paramProps = checker.getSymbolsOfParameterPropertyDeclaration(cast(symbol.valueDeclaration, isParameter), symbol.name); Debug.assert(paramProps.length === 2 && !!(paramProps[0].flags & SymbolFlags.FunctionScopedVariable) && !!(paramProps[1].flags & SymbolFlags.Property)); // is [parameter, property] diff --git a/src/services/navigationBar.ts b/src/services/navigationBar.ts index 7a91fcdcad2..dd3bdcffd32 100644 --- a/src/services/navigationBar.ts +++ b/src/services/navigationBar.ts @@ -197,7 +197,7 @@ namespace ts.NavigationBar { // Parameter properties are children of the class, not the constructor. for (const param of ctr.parameters) { - if (isParameterPropertyDeclaration(param)) { + if (isParameterPropertyDeclaration(param, ctr)) { addLeafNode(param); } } diff --git a/src/services/refactors/generateGetAccessorAndSetAccessor.ts b/src/services/refactors/generateGetAccessorAndSetAccessor.ts index 7e4ca1a6645..a1bd32b37fc 100644 --- a/src/services/refactors/generateGetAccessorAndSetAccessor.ts +++ b/src/services/refactors/generateGetAccessorAndSetAccessor.ts @@ -92,7 +92,7 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor { } function isAcceptedDeclaration(node: Node): node is AcceptedDeclaration { - return isParameterPropertyDeclaration(node) || isPropertyDeclaration(node) || isPropertyAssignment(node); + return isParameterPropertyDeclaration(node, node.parent) || isPropertyDeclaration(node) || isPropertyAssignment(node); } function createPropertyName (name: string, originalName: AcceptedNameType) { @@ -214,7 +214,7 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor { } function insertAccessor(changeTracker: textChanges.ChangeTracker, file: SourceFile, accessor: AccessorDeclaration, declaration: AcceptedDeclaration, container: ContainerDeclaration) { - isParameterPropertyDeclaration(declaration) ? changeTracker.insertNodeAtClassStart(file, container, accessor) : + isParameterPropertyDeclaration(declaration, declaration.parent) ? changeTracker.insertNodeAtClassStart(file, container, accessor) : isPropertyAssignment(declaration) ? changeTracker.insertNodeAfterComma(file, declaration, accessor) : changeTracker.insertNodeAfter(file, declaration, accessor); } diff --git a/src/testRunner/unittests/transform.ts b/src/testRunner/unittests/transform.ts index 5203e69e813..ffd5ba7609f 100644 --- a/src/testRunner/unittests/transform.ts +++ b/src/testRunner/unittests/transform.ts @@ -300,6 +300,34 @@ namespace ts { }); }); + // https://github.com/microsoft/TypeScript/issues/33295 + testBaseline("transformParameterProperty", () => { + return transpileModule("", { + transformers: { + before: [transformAddParameterProperty], + }, + compilerOptions: { + target: ScriptTarget.ES5, + newLine: NewLineKind.CarriageReturnLineFeed, + } + }).outputText; + + function transformAddParameterProperty(_context: TransformationContext) { + return (sourceFile: SourceFile): SourceFile => { + return visitNode(sourceFile); + }; + function visitNode(sf: SourceFile) { + // produce `class Foo { constructor(@Dec private x) {} }`; + // The decorator is required to trigger ts.ts transformations. + const classDecl = createClassDeclaration([], [], "Foo", /*typeParameters*/ undefined, /*heritageClauses*/ undefined, [ + createConstructor(/*decorators*/ undefined, /*modifiers*/ undefined, [ + createParameter(/*decorators*/ [createDecorator(createIdentifier("Dec"))], /*modifiers*/ [createModifier(SyntaxKind.PrivateKeyword)], /*dotDotDotToken*/ undefined, "x")], createBlock([])) + ]); + return updateSourceFileNode(sf, [classDecl]); + } + } + }); + function baselineDeclarationTransform(text: string, opts: TranspileOptions) { const fs = vfs.createFromFileSystem(Harness.IO, /*caseSensitive*/ true, { documents: [new documents.TextDocument("/.src/index.ts", text)] }); const host = new fakes.CompilerHost(fs, opts.compilerOptions); @@ -389,7 +417,7 @@ class Clazz { } `, { transformers: { - before: [addSyntheticComment(n => isPropertyDeclaration(n) || isParameterPropertyDeclaration(n) || isClassDeclaration(n) || isConstructorDeclaration(n))], + before: [addSyntheticComment(n => isPropertyDeclaration(n) || isParameterPropertyDeclaration(n, n.parent) || isClassDeclaration(n) || isConstructorDeclaration(n))], }, compilerOptions: { target: ScriptTarget.ES2015, diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index 41ea5756204..58d513b2e0a 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -3274,7 +3274,7 @@ declare namespace ts { parent: ConstructorDeclaration; name: Identifier; }; - function isParameterPropertyDeclaration(node: Node): node is ParameterPropertyDeclaration; + function isParameterPropertyDeclaration(node: Node, parent: Node): node is ParameterPropertyDeclaration; function isEmptyBindingPattern(node: BindingName): node is BindingPattern; function isEmptyBindingElement(node: BindingElement): boolean; function walkUpBindingElementsAndPatterns(binding: BindingElement): VariableDeclaration | ParameterDeclaration; diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts index 8402150f2a0..bc71eaffa37 100644 --- a/tests/baselines/reference/api/typescript.d.ts +++ b/tests/baselines/reference/api/typescript.d.ts @@ -3274,7 +3274,7 @@ declare namespace ts { parent: ConstructorDeclaration; name: Identifier; }; - function isParameterPropertyDeclaration(node: Node): node is ParameterPropertyDeclaration; + function isParameterPropertyDeclaration(node: Node, parent: Node): node is ParameterPropertyDeclaration; function isEmptyBindingPattern(node: BindingName): node is BindingPattern; function isEmptyBindingElement(node: BindingElement): boolean; function walkUpBindingElementsAndPatterns(binding: BindingElement): VariableDeclaration | ParameterDeclaration; diff --git a/tests/baselines/reference/transformApi/transformsCorrectly.transformParameterProperty.js b/tests/baselines/reference/transformApi/transformsCorrectly.transformParameterProperty.js new file mode 100644 index 00000000000..d121694872f --- /dev/null +++ b/tests/baselines/reference/transformApi/transformsCorrectly.transformParameterProperty.js @@ -0,0 +1,18 @@ +var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) { + var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d; + if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc); + else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r; + return c > 3 && r && Object.defineProperty(target, key, r), r; +}; +var __param = (this && this.__param) || function (paramIndex, decorator) { + return function (target, key) { decorator(target, key, paramIndex); } +}; +var Foo = /** @class */ (function () { + function Foo(x) { + this.x = x; + } + Foo = __decorate([ + __param(0, Dec) + ], Foo); + return Foo; +}());