Forbid unused property renaming in destructuring binding in function types

[uhyo]

Happy hacktoberfest!! 🎃🥳

Fixes #37454.

The PR implements mainly two things, as suggested in #41044 (comment):

• Emit an error for property renaming in destructing binding in function types if it is unused. This is not under noUnusedLocals nor noUnusedParameters, but declaration files are not checked.
• Changes declaration emit so that property renaming in destructing bunding is removed if it is unused.

Example

type O = { a?: string; b: number; c: number; };
type F1 = (arg: number) => any; // OK
type F2 = ({ a: string }: O) => any; // Error
type F3 = ({ a: string, b, c }: O) => any; // Error
type F4 = ({ a: string }: O) => any; // Error
type F5 = ({ a: string, b, c }: O) => any; // Error
type F6 = ({ a: string }) => typeof string; // OK
type F7 = ({ a: string, b: number }) => typeof number; // Error
type F8 = ({ a, b: number }) => typeof number; // OK
type F9 = ([a, b, c]) => void; // Error

type G1 = (arg: number) => any; // OK
type G2 = ({ a: string }: O) => any; // Error
type G3 = ({ a: string, b, c }: O) => any; // Error
type G4 = ({ a: string }: O) => any; // Error
type G5 = ({ a: string, b, c }: O) => any; // Error
type G6 = ({ a: string }) => typeof string; // OK
type G7 = ({ a: string, b: number }) => typeof number; // Error
type G8 = ({ a, b: number }) => typeof number; // OK
type G9 = ([a, b, c]) => void; // Error

Results

All the errors below are new in this PR.

src/index.ts:3:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

3 type F2 = ({ a: string }: O) => any; // Error
                  ~~~~~~

src/index.ts:4:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

4 type F3 = ({ a: string, b, c }: O) => any; // Error
                  ~~~~~~

src/index.ts:5:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

5 type F4 = ({ a: string }: O) => any; // Error
                  ~~~~~~

src/index.ts:6:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

6 type F5 = ({ a: string, b, c }: O) => any; // Error
                  ~~~~~~

src/index.ts:8:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

8 type F7 = ({ a: string, b: number }) => typeof number; // Error
                  ~~~~~~

src/index.ts:13:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

13 type G2 = ({ a: string }: O) => any; // Error
                   ~~~~~~

src/index.ts:14:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

14 type G3 = ({ a: string, b, c }: O) => any; // Error
                   ~~~~~~

src/index.ts:15:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

15 type G4 = ({ a: string }: O) => any; // Error
                   ~~~~~~

src/index.ts:16:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

16 type G5 = ({ a: string, b, c }: O) => any; // Error
                   ~~~~~~

src/index.ts:18:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

18 type G7 = ({ a: string, b: number }) => typeof number; // Error
                   ~~~~~~


Found 10 errors in the same file, starting at: src/index.ts:3

[sandersn]

@rbuckton this may be affected when you change the way binding patterns are handled.

[andrewbranch]

This PR also modifies declaration emission so that it does not generate invalid function types as to the above rule.

Note that this PR is a breaking change, so can’t be merged into 4.1—it means people consuming existing declarations generated from totally reasonable code (your const k = ({x: z = 'y'}) => { } example) will suddenly get errors. We might even want to consider suppressing this error in declaration files, as it’s pretty annoying to start issuing a bunch of new errors inside people’s node_modules, with obviously no benefit to the owners of those node_modules.

[andrewbranch]

Perhaps a gentler way of doing this is putting it under --noUnusedLocals, since that’s basically what it is—a new name introduced that is highly unlikely to be used (though it technically can be: type F = ({ a: foo }) => typeof foo). --noUnusedLocals checks are downgraded to suggestion diagnostics in declaration files, so it has the advantage of being visible in the editor for someone who’s hand-authoring a declaration file, but will never break a build for declaration consumers.

[uhyo]

Happy hacktoberfest again! 🎃🥳

@andrewbranch cc: @rbuckton
Sorry for spending one year for reflecting your comment. 😓
This PR is wholly renewed and now the check is under noUsedParameters, and declaration emit is untouched.

I would like to request your re-check. Thank you.

[andrewbranch]

Wow, this PR really came back from the dead! 🧟

[andrewbranch]

Still technically a breaking change, so let’s look in the 4.6 timeline. Thanks!

[sandersn]

@andrewbranch your last comment makes it sound like this is ready to be merged, but early in a release cycle. Is that true? Am I misreading it?

[andrewbranch]

I definitely think we should get this into 4.8 because having forgotten what it was about and returning to the test, I totally fell for the trick that this tries to prevent, thinking string in ({ a: string }) => any is a type annotation 😆

I think it may be worth bringing a couple things to a design meeting:

• Should this error be triggered by noUnusedLocals, noUnusedParameters, or neither? I know previously I had suggested it might make sense for it to be behind a flag, and I think you could bikeshed whether a rebinding of a destructured parameter is a parameter or a local. But looking at it now, there is truly no reason to write this syntax, so I think it may be worth considering always making it an error. On the other hand, both noUnused options have pretty wide adoption, so maybe it’s fine as it is.
• Especially if we decide to always make it an error, do we want a specialized message for this? When I looked at the error baseline for the first time in a year, I was briefly confused. It’s normal for names declared in types to be “never read,” and the message really sounds cryptic if you were under the impression that : string was a type annotation. I think a better error message would indicate what the syntax is: “Rebinding a destructured parameter is not allowed in a function type” or something (needs some work).
• I’m still concerned about new errors popping up in declaration files that we generated from existing code. I’m second-guessing my previous feedback that we shouldn’t change declaration emit to avoid this pattern, and I also want to think about whether we can/should suppress this check in declaration files.

[andrewbranch]

Looking at the error span on this, it feels a bit odd for it to be the whole binding pattern rather than just the element or just string. It would be good to add a test like

type F2 = ({ a: string, b }) => any;

which I think will really indicate whether the error span is appropriate.

[andrewbranch]

Decision from design meeting:

1. Let's bring back the commit that changed the declaration emit
2. For back compat reasons, we cannot issue the error on .d.ts files
3. For implementation files, we actually want to issue an error regardless of noUnusedParameter/noUnusedLocals. We should craft a new error message, not the “declared but never read” message.
4. However, we want to silence the error if the rebound name is used in a type query somewhere else in the type:

   type F1 = ({ x: renamedX }) => void;             // Error
   type F2 = ({ x: renamedX }) => typeof renamedX;  // No error

[andrewbranch]

@uhyo are you interested in making these changes, almost 2 years later? 😆

[uhyo]

Sure! I'll work on this within a few days.

[andrewbranch]

Almost there 👍

[andrewbranch]

I believe the interpolation here is intended to be We can only write a type for 'a'...

[andrewbranch]

Can you add a comment explaining that string has to be retained in declaration emit even though it’s an error, because eliding it would leave invalid syntax (since the property names aren’t identifiers)?

[uhyo]

fixed!

[andrewbranch]

This is incorrectly labeled “Error”

[uhyo]

oops 🤕

[andrewbranch]

Huh, isReferenced is marked with Type in the case of typeof renamedThing? I would have thought that counts as a value usage. The tests look right so I assume this works, I’m just surprised. Do we need to check against Type at all or can we just say if (!(getSymbolOfNode(node)?.isReferenced)?

[uhyo]

Apparently just checking truthiness of isReferenced is enough. Thank you 🙂

[andrewbranch]

Thank you! 🌟

[Mister-Hope]

I think #50483 is probably cause by this PR, could anyone check it?