Forbid unused property renaming in destructuring binding in function types (#41044)

* Forbid renaming a propertyin function type parameters

* add tests

* Remove renaming from declaration output

* accept baseline

* accept baseline

* renew tests (not very right now)

* get correct result

* update diagnostic text

* accept baseline

* add declaration emit test

* fix declaration emit

* fix formatting

* revert unnecessary change

* accept baseline

* extend tests

* Revert "revert unnecessary change"

This reverts commit 17a29ff.

* accept baseline

* Rename and refactor potentialAlways... stuff

* add non-identifier names

* extend check to non-identifier original property names

* update diagnostic message

* add related span

* accept baseline

* add symbol-keyed test case

* oops?

* workaround for unstable test

* fix suggested name

* add comment about non-identifier property names

* simplify isReferenced check

* accept baseline

* move it one step further

Author: uhyo

src/compiler/checker.ts

@@ -1015,6 +1015,7 @@ namespace ts {
         const potentialNewTargetCollisions: Node[] = [];
         const potentialWeakMapSetCollisions: Node[] = [];
         const potentialReflectCollisions: Node[] = [];
+        const potentialUnusedRenamedBindingElementsInTypes: BindingElement[] = [];
         const awaitedTypeStack: number[] = [];
 
         const diagnostics = createDiagnosticCollection();
@@ -5945,19 +5946,29 @@ namespace ts {
                 return parameterNode;
 
                 function cloneBindingName(node: BindingName): BindingName {
-                    return elideInitializerAndSetEmitFlags(node) as BindingName;
-                    function elideInitializerAndSetEmitFlags(node: Node): Node {
+                    return elideInitializerAndPropertyRenamingAndSetEmitFlags(node) as BindingName;
+                    function elideInitializerAndPropertyRenamingAndSetEmitFlags(node: Node): Node {
                         if (context.tracker.trackSymbol && isComputedPropertyName(node) && isLateBindableName(node)) {
                             trackComputedName(node.expression, context.enclosingDeclaration, context);
                         }
-                        let visited = visitEachChild(node, elideInitializerAndSetEmitFlags, nullTransformationContext, /*nodesVisitor*/ undefined, elideInitializerAndSetEmitFlags)!;
+                        let visited = visitEachChild(node, elideInitializerAndPropertyRenamingAndSetEmitFlags, nullTransformationContext, /*nodesVisitor*/ undefined, elideInitializerAndPropertyRenamingAndSetEmitFlags)!;
                         if (isBindingElement(visited)) {
-                            visited = factory.updateBindingElement(
-                                visited,
-                                visited.dotDotDotToken,
-                                visited.propertyName,
-                                visited.name,
-                                /*initializer*/ undefined);
+                            if (visited.propertyName && isIdentifier(visited.propertyName) && isIdentifier(visited.name)) {
+                                visited = factory.updateBindingElement(
+                                    visited,
+                                    visited.dotDotDotToken,
+                                    /* propertyName*/ undefined,
+                                    visited.propertyName,
+                                    /*initializer*/ undefined);
+                            }
+                            else {
+                                visited = factory.updateBindingElement(
+                                    visited,
+                                    visited.dotDotDotToken,
+                                    visited.propertyName,
+                                    visited.name,
+                                    /*initializer*/ undefined);
+                            }
                         }
                         if (!nodeIsSynthesized(visited)) {
                             visited = factory.cloneNode(visited);
@@ -37432,6 +37443,24 @@ namespace ts {
             });
         }
 
+        function checkPotentialUncheckedRenamedBindingElementsInTypes() {
+            for (const node of potentialUnusedRenamedBindingElementsInTypes) {
+                if (!getSymbolOfNode(node)?.isReferenced) {
+                    const wrappingDeclaration = walkUpBindingElementsAndPatterns(node);
+                    Debug.assert(isParameterDeclaration(wrappingDeclaration), "Only parameter declaration should be checked here");
+                    const diagnostic = createDiagnosticForNode(node.name, Diagnostics._0_is_an_unused_renaming_of_1_Did_you_intend_to_use_it_as_a_type_annotation, declarationNameToString(node.name), declarationNameToString(node.propertyName));
+                    if (!wrappingDeclaration.type) {
+                        // entire parameter does not have type annotation, suggest adding an annotation
+                        addRelatedInfo(
+                            diagnostic,
+                            createFileDiagnostic(getSourceFileOfNode(wrappingDeclaration), wrappingDeclaration.end, 1, Diagnostics.We_can_only_write_a_type_for_0_by_adding_a_type_for_the_entire_parameter_here, declarationNameToString(node.propertyName))
+                        );
+                    }
+                    diagnostics.add(diagnostic);
+                }
+            }
+        }
+
         function bindingNameText(name: BindingName): string {
             switch (name.kind) {
                 case SyntaxKind.Identifier:
@@ -37773,6 +37802,19 @@ namespace ts {
             }
 
             if (isBindingElement(node)) {
+                if (
+                  node.propertyName &&
+                  isIdentifier(node.name) &&
+                  isParameterDeclaration(node) &&
+                  nodeIsMissing((getContainingFunction(node) as FunctionLikeDeclaration).body)) {
+                    // type F = ({a: string}) => void;
+                    //               ^^^^^^
+                    // variable renaming in function type notation is confusing,
+                    // so we forbid it even if noUnusedLocals is not enabled
+                    potentialUnusedRenamedBindingElementsInTypes.push(node);
+                    return;
+                }
+
                 if (isObjectBindingPattern(node.parent) && node.dotDotDotToken && languageVersion < ScriptTarget.ES2018) {
                     checkExternalEmitHelpers(node, ExternalEmitHelpers.Rest);
                 }
@@ -41617,6 +41659,7 @@ namespace ts {
                 clear(potentialNewTargetCollisions);
                 clear(potentialWeakMapSetCollisions);
                 clear(potentialReflectCollisions);
+                clear(potentialUnusedRenamedBindingElementsInTypes);
 
                 forEach(node.statements, checkSourceElement);
                 checkSourceElement(node.endOfFileToken);
@@ -41636,6 +41679,9 @@ namespace ts {
                             }
                         });
                     }
+                    if (!node.isDeclarationFile) {
+                        checkPotentialUncheckedRenamedBindingElementsInTypes();
+                    }
                 });
 
                 if (compilerOptions.importsNotUsedAsValues === ImportsNotUsedAsValues.Error &&

src/compiler/diagnosticMessages.json

@@ -3487,6 +3487,14 @@
         "category": "Error",
         "code": 2841
     },
+    "'{0}' is an unused renaming of '{1}'. Did you intend to use it as a type annotation?": {
+        "category": "Error",
+        "code": 2842
+    }, 
+    "We can only write a type for '{0}' by adding a type for the entire parameter here.": {
+        "category": "Error",
+        "code": 2843
+    },
 
     "Import declaration '{0}' is using private name '{1}'.": {
         "category": "Error",

src/compiler/transformers/declarations.ts

@@ -453,7 +453,7 @@ namespace ts {
             return ret;
         }
 
-        function filterBindingPatternInitializers(name: BindingName) {
+        function filterBindingPatternInitializersAndRenamings(name: BindingName) {
             if (name.kind === SyntaxKind.Identifier) {
                 return name;
             }
@@ -471,7 +471,23 @@ namespace ts {
                 if (elem.kind === SyntaxKind.OmittedExpression) {
                     return elem;
                 }
-                return factory.updateBindingElement(elem, elem.dotDotDotToken, elem.propertyName, filterBindingPatternInitializers(elem.name), shouldPrintWithInitializer(elem) ? elem.initializer : undefined);
+                if (elem.propertyName && isIdentifier(elem.propertyName) && isIdentifier(elem.name) && !elem.symbol.isReferenced) {
+                   // Unnecessary property renaming is forbidden in types, so remove renaming
+                    return factory.updateBindingElement(
+                        elem,
+                        elem.dotDotDotToken,
+                        /* propertyName */ undefined,
+                        elem.propertyName,
+                        shouldPrintWithInitializer(elem) ? elem.initializer : undefined
+                    );
+                }
+                return factory.updateBindingElement(
+                    elem,
+                    elem.dotDotDotToken,
+                    elem.propertyName,
+                    filterBindingPatternInitializersAndRenamings(elem.name),
+                    shouldPrintWithInitializer(elem) ? elem.initializer : undefined
+                );
             }
         }
 
@@ -485,7 +501,7 @@ namespace ts {
                 p,
                 maskModifiers(p, modifierMask),
                 p.dotDotDotToken,
-                filterBindingPatternInitializers(p.name),
+                filterBindingPatternInitializersAndRenamings(p.name),
                 resolver.isOptionalParameter(p) ? (p.questionToken || factory.createToken(SyntaxKind.QuestionToken)) : undefined,
                 ensureType(p, type || p.type, /*ignorePrivate*/ true), // Ignore private param props, since this type is going straight back into a param
                 ensureNoInitializer(p)

tests/baselines/reference/contextuallyTypedBindingInitializerNegative.types

@@ -5,7 +5,7 @@ interface Show {
 >x : number
 }
 function f({ show: showRename = v => v }: Show) {}
->f : ({ show: showRename }: Show) => void
+>f : ({ show }: Show) => void
 >show : any
 >showRename : (x: number) => string
 >v => v : (v: number) => number
@@ -32,7 +32,7 @@ interface Nested {
 >nested : Show
 }
 function ff({ nested: nestedRename = { show: v => v } }: Nested) {}
->ff : ({ nested: nestedRename }: Nested) => void
+>ff : ({ nested }: Nested) => void
 >nested : any
 >nestedRename : Show
 >{ show: v => v } : { show: (v: number) => number; }

tests/baselines/reference/declarationEmitBindingPatterns.js

@@ -18,7 +18,7 @@ function f(_a, _b, _c) {
 
 
 //// [declarationEmitBindingPatterns.d.ts]
-declare const k: ({ x: z }: {
+declare const k: ({ x }: {
     x?: string;
 }) => void;
 declare var a: any;

tests/baselines/reference/declarationEmitBindingPatterns.types

@@ -1,7 +1,7 @@
 === tests/cases/compiler/declarationEmitBindingPatterns.ts ===
 const k = ({x: z = 'y'}) => { }
->k : ({ x: z }: { x?: string; }) => void
->({x: z = 'y'}) => { } : ({ x: z }: { x?: string; }) => void
+>k : ({ x }: { x?: string; }) => void
+>({x: z = 'y'}) => { } : ({ x }: { x?: string; }) => void
 >x : any
 >z : string
 >'y' : "y"

tests/baselines/reference/declarationsAndAssignments.types

@@ -417,7 +417,7 @@ function f13() {
 }
 
 function f14([a = 1, [b = "hello", { x, y: c = false }]]) {
->f14 : ([a, [b, { x, y: c }]]: [number, [string, { x: any; y?: boolean; }]]) => void
+>f14 : ([a, [b, { x, y }]]: [number, [string, { x: any; y?: boolean; }]]) => void
 >a : number
 >1 : 1
 >b : string
@@ -438,7 +438,7 @@ function f14([a = 1, [b = "hello", { x, y: c = false }]]) {
 }
 f14([2, ["abc", { x: 0, y: true }]]);
 >f14([2, ["abc", { x: 0, y: true }]]) : void
->f14 : ([a, [b, { x, y: c }]]: [number, [string, { x: any; y?: boolean; }]]) => void
+>f14 : ([a, [b, { x, y }]]: [number, [string, { x: any; y?: boolean; }]]) => void
 >[2, ["abc", { x: 0, y: true }]] : [number, [string, { x: number; y: true; }]]
 >2 : 2
 >["abc", { x: 0, y: true }] : [string, { x: number; y: true; }]
@@ -451,7 +451,7 @@ f14([2, ["abc", { x: 0, y: true }]]);
 
 f14([2, ["abc", { x: 0 }]]);
 >f14([2, ["abc", { x: 0 }]]) : void
->f14 : ([a, [b, { x, y: c }]]: [number, [string, { x: any; y?: boolean; }]]) => void
+>f14 : ([a, [b, { x, y }]]: [number, [string, { x: any; y?: boolean; }]]) => void
 >[2, ["abc", { x: 0 }]] : [number, [string, { x: number; }]]
 >2 : 2
 >["abc", { x: 0 }] : [string, { x: number; }]
@@ -462,7 +462,7 @@ f14([2, ["abc", { x: 0 }]]);
 
 f14([2, ["abc", { y: false }]]);  // Error, no x
 >f14([2, ["abc", { y: false }]]) : void
->f14 : ([a, [b, { x, y: c }]]: [number, [string, { x: any; y?: boolean; }]]) => void
+>f14 : ([a, [b, { x, y }]]: [number, [string, { x: any; y?: boolean; }]]) => void
 >[2, ["abc", { y: false }]] : [number, [string, { y: false; }]]
 >2 : 2
 >["abc", { y: false }] : [string, { y: false; }]

tests/baselines/reference/destructuringInFunctionType.errors.txt

@@ -0,0 +1,32 @@
+tests/cases/conformance/es6/destructuring/destructuringInFunctionType.ts(12,18): error TS2842: 'b' is an unused renaming of 'a'. Did you intend to use it as a type annotation?
+tests/cases/conformance/es6/destructuring/destructuringInFunctionType.ts(12,28): error TS2842: 'a' is an unused renaming of 'b'. Did you intend to use it as a type annotation?
+
+
+==== tests/cases/conformance/es6/destructuring/destructuringInFunctionType.ts (2 errors) ====
+    interface a { a }
+    interface b { b }
+    interface c { c }
+    
+    type T1 = ([a, b, c]);
+    type F1 = ([a, b, c]) => void;
+    
+    type T2 = ({ a });
+    type F2 = ({ a }) => void;
+    
+    type T3 = ([{ a: b }, { b: a }]);
+    type F3 = ([{ a: b }, { b: a }]) => void;
+                     ~
+!!! error TS2842: 'b' is an unused renaming of 'a'. Did you intend to use it as a type annotation?
+!!! related TS2843 tests/cases/conformance/es6/destructuring/destructuringInFunctionType.ts:12:32: We can only write a type for 'a' by adding a type for the entire parameter here.
+                               ~
+!!! error TS2842: 'a' is an unused renaming of 'b'. Did you intend to use it as a type annotation?
+!!! related TS2843 tests/cases/conformance/es6/destructuring/destructuringInFunctionType.ts:12:32: We can only write a type for 'b' by adding a type for the entire parameter here.
+    
+    type T4 = ([{ a: [b, c] }]);
+    type F4 = ([{ a: [b, c] }]) => void;
+    
+    type C1 = new ([{ a: [b, c] }]) => void;
+    
+    var v1 = ([a, b, c]) => "hello";
+    var v2: ([a, b, c]) => string;
+    

tests/baselines/reference/destructuringInFunctionType.js

@@ -52,7 +52,7 @@ declare type T3 = ([{
 }, {
     b: a;
 }]);
-declare type F3 = ([{ a: b }, { b: a }]: [{
+declare type F3 = ([{ a }, { b }]: [{
     a: any;
 }, {
     b: any;

tests/baselines/reference/destructuringInFunctionType.types

@@ -31,7 +31,7 @@ type T3 = ([{ a: b }, { b: a }]);
 >b : a
 
 type F3 = ([{ a: b }, { b: a }]) => void;
->F3 : ([{ a: b }, { b: a }]: [{ a: any; }, { b: any; }]) => void
+>F3 : ([{ a }, { b }]: [{ a: any; }, { b: any; }]) => void
 >a : any
 >b : any
 >b : any