Handle parentless nodes in isParameterPropertyDeclaration (#33472)

DanielRosenwasser · web-flow · commit a58b86bcdfda · 2019-09-17T13:56:22.000-07:00
Handle parentless nodes in isParameterPropertyDeclaration
diff --git 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 = <ClassLikeDeclaration>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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1145,7 +1145,7 @@ namespace ts.FindAllReferences.Core {
     }
 
     export function eachSymbolReferenceInFile<T>(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
@@ -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
@@ -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, <ClassLikeDeclaration>container, accessor) :
+        isParameterPropertyDeclaration(declaration, declaration.parent) ? changeTracker.insertNodeAtClassStart(file, <ClassLikeDeclaration>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
@@ -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
@@ -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
@@ -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
@@ -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;
+}());

Original file line number	Diff line number	Diff line change
`@@ -2859,7 +2859,7 @@ namespace ts {`
`2859`	`2859`
`2860`	`2860`	`// If this is a property-parameter, then also declare the property symbol into the`
`2861`	`2861`	`// containing class.`
`2862`		`- if (isParameterPropertyDeclaration(node)) {`
	`2862`	`+ if (isParameterPropertyDeclaration(node, node.parent)) {`
`2863`	`2863`	`const classDeclaration = <ClassLikeDeclaration>node.parent.parent;`
`2864`	`2864`	`declareSymbol(classDeclaration.symbol.members!, classDeclaration.symbol, node, SymbolFlags.Property \| (node.questionToken ? SymbolFlags.Optional : SymbolFlags.None), SymbolFlags.PropertyExcludes);`
`2865`	`2865`	`}`
Original file line number	Diff line number	Diff line change
`@@ -25705,7 +25705,7 @@ namespace ts {`
`25705`	`25705`	`for (const member of node.members) {`
`25706`	`25706`	`if (member.kind === SyntaxKind.Constructor) {`
`25707`	`25707`	`for (const param of (member as ConstructorDeclaration).parameters) {`
`25708`		`- if (isParameterPropertyDeclaration(param) && !isBindingPattern(param.name)) {`
	`25708`	`+ if (isParameterPropertyDeclaration(param, member) && !isBindingPattern(param.name)) {`
`25709`	`25709`	`addName(instanceNames, param.name, param.name.escapedText, DeclarationMeaning.GetOrSetAccessor);`
`25710`	`25710`	`}`
`25711`	`25711`	`}`
`@@ -27469,7 +27469,7 @@ namespace ts {`
`27469`	`27469`	`const parameter = local.valueDeclaration && tryGetRootParameterDeclaration(local.valueDeclaration);`
`27470`	`27470`	`const name = local.valueDeclaration && getNameOfDeclaration(local.valueDeclaration);`
`27471`	`27471`	`if (parameter && name) {`
`27472`		`- if (!isParameterPropertyDeclaration(parameter) && !parameterIsThisKeyword(parameter) && !isIdentifierThatStartsWithUnderscore(name)) {`
	`27472`	`+ if (!isParameterPropertyDeclaration(parameter, parameter.parent) && !parameterIsThisKeyword(parameter) && !isIdentifierThatStartsWithUnderscore(name)) {`
`27473`	`27473`	`addDiagnostic(parameter, UnusedKind.Parameter, createDiagnosticForNode(name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(local)));`
`27474`	`27474`	`}`
`27475`	`27475`	`}`
Original file line number	Diff line number	Diff line change
`@@ -337,7 +337,7 @@ namespace ts {`
`337`	`337`	`if (constructor && constructor.body) {`
`338`	`338`	`let parameterPropertyDeclarationCount = 0;`
`339`	`339`	`for (let i = indexOfFirstStatement; i < constructor.body.statements.length; i++) {`
`340`		`- if (isParameterPropertyDeclaration(getOriginalNode(constructor.body.statements[i]))) {`
	`340`	`+ if (isParameterPropertyDeclaration(getOriginalNode(constructor.body.statements[i]), constructor)) {`
`341`	`341`	`parameterPropertyDeclarationCount++;`
`342`	`342`	`}`
`343`	`343`	`else {`
Original file line number	Diff line number	Diff line change
`@@ -135,7 +135,7 @@ namespace ts {`
`135`	`135`	`return getReturnTypeVisibilityError;`
`136`	`136`	`}`
`137`	`137`	`else if (isParameter(node)) {`
`138`		`- if (isParameterPropertyDeclaration(node) && hasModifier(node.parent, ModifierFlags.Private)) {`
	`138`	`+ if (isParameterPropertyDeclaration(node, node.parent) && hasModifier(node.parent, ModifierFlags.Private)) {`
`139`	`139`	`return getVariableDeclarationTypeVisibilityError;`
`140`	`140`	`}`
`141`	`141`	`return getParameterDeclarationTypeVisibilityError;`
Original file line number	Diff line number	Diff line change
`@@ -1009,7 +1009,7 @@ namespace ts {`
`1009`	`1009`	`}`
`1010`	`1010`
`1011`	`1011`	`export function isDeclarationReadonly(declaration: Declaration): boolean {`
`1012`		`- return !!(getCombinedModifierFlags(declaration) & ModifierFlags.Readonly && !isParameterPropertyDeclaration(declaration));`
	`1012`	`+ return !!(getCombinedModifierFlags(declaration) & ModifierFlags.Readonly && !isParameterPropertyDeclaration(declaration, declaration.parent));`
`1013`	`1013`	`}`
`1014`	`1014`
`1015`	`1015`	`export function isVarConst(node: VariableDeclaration \| VariableDeclarationList): boolean {`
`@@ -4983,8 +4983,8 @@ namespace ts {`
`4983`	`4983`	`}`
`4984`	`4984`
`4985`	`4985`	`export type ParameterPropertyDeclaration = ParameterDeclaration & { parent: ConstructorDeclaration, name: Identifier };`
`4986`		`- export function isParameterPropertyDeclaration(node: Node): node is ParameterPropertyDeclaration {`
`4987`		`- return hasModifier(node, ModifierFlags.ParameterPropertyModifier) && node.parent.kind === SyntaxKind.Constructor;`
	`4986`	`+ export function isParameterPropertyDeclaration(node: Node, parent: Node): node is ParameterPropertyDeclaration {`
	`4987`	`+ return hasModifier(node, ModifierFlags.ParameterPropertyModifier) && parent.kind === SyntaxKind.Constructor;`
`4988`	`4988`	`}`
`4989`	`4989`
`4990`	`4990`	`export function isEmptyBindingPattern(node: BindingName): node is BindingPattern {`
Original file line number	Diff line number	Diff line change
`@@ -197,7 +197,7 @@ namespace ts.NavigationBar {`
`197`	`197`
`198`	`198`	`// Parameter properties are children of the class, not the constructor.`
`199`	`199`	`for (const param of ctr.parameters) {`
`200`		`- if (isParameterPropertyDeclaration(param)) {`
	`200`	`+ if (isParameterPropertyDeclaration(param, ctr)) {`
`201`	`201`	`addLeafNode(param);`
`202`	`202`	`}`
`203`	`203`	`}`
Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,7 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {`
`92`	`92`	`}`
`93`	`93`
`94`	`94`	`function isAcceptedDeclaration(node: Node): node is AcceptedDeclaration {`
`95`		`- return isParameterPropertyDeclaration(node) \|\| isPropertyDeclaration(node) \|\| isPropertyAssignment(node);`
	`95`	`+ return isParameterPropertyDeclaration(node, node.parent) \|\| isPropertyDeclaration(node) \|\| isPropertyAssignment(node);`
`96`	`96`	`}`
`97`	`97`
`98`	`98`	`function createPropertyName (name: string, originalName: AcceptedNameType) {`
`@@ -214,7 +214,7 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {`
`214`	`214`	`}`
`215`	`215`
`216`	`216`	`function insertAccessor(changeTracker: textChanges.ChangeTracker, file: SourceFile, accessor: AccessorDeclaration, declaration: AcceptedDeclaration, container: ContainerDeclaration) {`
`217`		`- isParameterPropertyDeclaration(declaration) ? changeTracker.insertNodeAtClassStart(file, <ClassLikeDeclaration>container, accessor) :`
	`217`	`+ isParameterPropertyDeclaration(declaration, declaration.parent) ? changeTracker.insertNodeAtClassStart(file, <ClassLikeDeclaration>container, accessor) :`
`218`	`218`	`isPropertyAssignment(declaration) ? changeTracker.insertNodeAfterComma(file, declaration, accessor) :`
`219`	`219`	`changeTracker.insertNodeAfter(file, declaration, accessor);`
`220`	`220`	`}`