Handle parentless nodes in isParameterPropertyDeclaration

mprobst · mprobst · commit e8be5e85bce0 · 2019-09-17T17:11:16.000+02:00
Fixes microsoft#33295. This follows a similar pattern as in microsoft#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
diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts
@@ -2843,7 +2843,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
@@ -25659,7 +25659,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);
                         }
                     }
@@ -27420,7 +27420,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
@@ -892,7 +892,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) {
@@ -1904,7 +1904,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 {
@@ -4959,8 +4959,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
@@ -1147,7 +1147,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;
@@ -1888,7 +1888,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
@@ -161,7 +161,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)
+        isParameterPropertyDeclaration(declaration, declaration.parent)
             ? changeTracker.insertNodeAtClassStart(file, <ClassLikeDeclaration>container, accessor)
             : isPropertyAssignment(declaration)
                 ? changeTracker.insertNodeAfterComma(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
@@ -3264,7 +3264,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
@@ -3264,7 +3264,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
`@@ -2843,7 +2843,7 @@ namespace ts {`
`2843`	`2843`
`2844`	`2844`	`// If this is a property-parameter, then also declare the property symbol into the`
`2845`	`2845`	`// containing class.`
`2846`		`- if (isParameterPropertyDeclaration(node)) {`
	`2846`	`+ if (isParameterPropertyDeclaration(node, node.parent)) {`
`2847`	`2847`	`const classDeclaration = <ClassLikeDeclaration>node.parent.parent;`
`2848`	`2848`	`declareSymbol(classDeclaration.symbol.members!, classDeclaration.symbol, node, SymbolFlags.Property \| (node.questionToken ? SymbolFlags.Optional : SymbolFlags.None), SymbolFlags.PropertyExcludes);`
`2849`	`2849`	`}`
Original file line number	Diff line number	Diff line change
`@@ -25659,7 +25659,7 @@ namespace ts {`
`25659`	`25659`	`for (const member of node.members) {`
`25660`	`25660`	`if (member.kind === SyntaxKind.Constructor) {`
`25661`	`25661`	`for (const param of (member as ConstructorDeclaration).parameters) {`
`25662`		`- if (isParameterPropertyDeclaration(param) && !isBindingPattern(param.name)) {`
	`25662`	`+ if (isParameterPropertyDeclaration(param, member) && !isBindingPattern(param.name)) {`
`25663`	`25663`	`addName(instanceNames, param.name, param.name.escapedText, DeclarationMeaning.GetOrSetAccessor);`
`25664`	`25664`	`}`
`25665`	`25665`	`}`
`@@ -27420,7 +27420,7 @@ namespace ts {`
`27420`	`27420`	`const parameter = local.valueDeclaration && tryGetRootParameterDeclaration(local.valueDeclaration);`
`27421`	`27421`	`const name = local.valueDeclaration && getNameOfDeclaration(local.valueDeclaration);`
`27422`	`27422`	`if (parameter && name) {`
`27423`		`- if (!isParameterPropertyDeclaration(parameter) && !parameterIsThisKeyword(parameter) && !isIdentifierThatStartsWithUnderscore(name)) {`
	`27423`	`+ if (!isParameterPropertyDeclaration(parameter, parameter.parent) && !parameterIsThisKeyword(parameter) && !isIdentifierThatStartsWithUnderscore(name)) {`
`27424`	`27424`	`addDiagnostic(parameter, UnusedKind.Parameter, createDiagnosticForNode(name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(local)));`
`27425`	`27425`	`}`
`27426`	`27426`	`}`
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 {`
`@@ -4959,8 +4959,8 @@ namespace ts {`
`4959`	`4959`	`}`
`4960`	`4960`
`4961`	`4961`	`export type ParameterPropertyDeclaration = ParameterDeclaration & { parent: ConstructorDeclaration, name: Identifier };`
`4962`		`- export function isParameterPropertyDeclaration(node: Node): node is ParameterPropertyDeclaration {`
`4963`		`- return hasModifier(node, ModifierFlags.ParameterPropertyModifier) && node.parent.kind === SyntaxKind.Constructor;`
	`4962`	`+ export function isParameterPropertyDeclaration(node: Node, parent: Node): node is ParameterPropertyDeclaration {`
	`4963`	`+ return hasModifier(node, ModifierFlags.ParameterPropertyModifier) && parent.kind === SyntaxKind.Constructor;`
`4964`	`4964`	`}`
`4965`	`4965`
`4966`	`4966`	`export function isEmptyBindingPattern(node: BindingName): node is BindingPattern {`
Original file line number	Diff line number	Diff line change
`@@ -161,7 +161,7 @@ namespace ts.NavigationBar {`
`161`	`161`
`162`	`162`	`// Parameter properties are children of the class, not the constructor.`
`163`	`163`	`for (const param of ctr.parameters) {`
`164`		`- if (isParameterPropertyDeclaration(param)) {`
	`164`	`+ if (isParameterPropertyDeclaration(param, ctr)) {`
`165`	`165`	`addLeafNode(param);`
`166`	`166`	`}`
`167`	`167`	`}`
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)`
	`217`	`+ isParameterPropertyDeclaration(declaration, declaration.parent)`
`218`	`218`	`? changeTracker.insertNodeAtClassStart(file, <ClassLikeDeclaration>container, accessor)`
`219`	`219`	`: isPropertyAssignment(declaration)`
`220`	`220`	`? changeTracker.insertNodeAfterComma(file, declaration, accessor)`