Duplicate identifier in function declaration due to stripped property renames

[72636c]

Bug Report

🔎 Search Terms

TS2300

🕗 Version & Regression Information

• This changed between versions 4.7.4 and 4.8.2
• Similar to Isssue generating declaration files with object containing keywords as property #50483 and likely introduced by Forbid unused property renaming in destructuring binding in function types #41044
• Persists in 4.8.3, nightly, and 🤖 Pick PR #50537 (Retain name and propertyName in dec...) into release-4.8 #50538 (https://github.com/72636c/typescript-4.8-declaration-poc)

⏯ Playground Link

https://www.typescriptlang.org/play?#code/MYewdgzgLgBAZmGBeGAKA3jADgJxFgLhgEMAabPQmAIxgF8jNd8iwBXAW2oFMd6BKZAD4SMANQ0A3EA

💻 Code

const fn = ({ prop: a, prop: b }: { prop: number }) => a + b;

Or alternatively:

const fn = ({ prop: a }: { prop: number }, { prop: b }: { prop: number }) => a + b;

🙁 Actual behavior

.D.TS on TypeScript 4.8.2:

// error TS2300: Duplicate identifier 'prop'.
declare const fn: ({ prop, prop }: {
    prop: number;
}) => number;

https://www.typescriptlang.org/play?ts=4.8.2#code/MYewdgzgLgBAZmGBeGAKA3jADgJxFgLhgEMAabPQmAIxgF8jNd8iwBXAW2oFMd6BKZAD4SMANQ0A3EA

🙂 Expected behavior

.D.TS on TypeScript 4.7.4:

declare const fn: ({ prop: a, prop: b }: {
    prop: number;
}) => number;

https://www.typescriptlang.org/play?ts=4.7.4#code/MYewdgzgLgBAZmGBeGAKA3jADgJxFgLhgEMAabPQmAIxgF8jNd8iwBXAW2oFMd6BKZAD4SMANQ0A3EA

[jakebailey]

#50537 fixed this by leaving the renames in if the name was a keyword, but now if the same property is destructured into two different names, the new names still have to be preserved again, too, to avoid an error.

At this point, I'm wondering why we are even emitting these into d.ts files at all... Wouldn't it make more sense to instead emit {}: { prop: number } (or in the other issue, {}: GetLocalesOptions<T>), or at least a made-up parameter name, as these names are an implementation detail of the function?

@weswigham @andrewbranch

[weswigham]

At this point, I'm wondering why we are even emitting these into d.ts files at all...

Extra documentation is about the only reason. Autocomplete for callers is going to come from the type.

[andrewbranch]

I would propose preserving duplicates in 4.8.4 and then I’d be fine with emitting {} in 4.9+, personally.

[jakebailey]

I've been looking into this; unfortunately it seems like the way that #41044 and #50537 operate make it prohibitively difficult to catch the the second case in the post:

const fn = ({ prop: a }: { prop: number }, { prop: b }: { prop: number }) => a + b;

The code that was modified is part of symbolToParameterDeclaration; that is called independently on individual parameters, so you can't really figure out when you're duplicating something, other than if you have a duplicate within the parameter itself (the first example in the post), unless you start sifting around peer parameters or something. Also, doing the elision there would remove these from from things like hover, which feels undesirable.

I think that we should just do a partial revert of #41044 (for both main and 4.8), removing the elision in cloneBindingName. That would restore the pre-4.8 behavior. Then for 4.9, I think that these can be dropped during d.ts emit in the declaration transform, the real place where we don't want these.

[jakebailey]

I have a fix that does drop the binding patterns entirely, except it won't work because this is legal:

function fn({ x }: { x: string }, y: typeof x): typeof x {
    return x + y;
}

If we drop the binding pattern indiscriminately, we can't refer to x in from typeof in other parameters and the return type. I could go so far as to preserve elements that are used, maybe, but I'm sort of thinking that we should leave the status quo.

Also, it appears as though we error when we have a optional binding element parameter and the binding element is empty:

declare function fn1({}?: { x: string }): void; // error

declare function fn2({ x }?: { x: string }): void; // OK?

declare function fn3([]?: [ x: string ]): void; // error

declare function fn4([ x ]?: [ x: string ]): void; // OK?

Playground Link

This trips up at least one test case; this seems like a bug to me and I'll report it elsewhere.

[jakebailey]

I've merged #50779 for now, which does fix this issue, but per the above I don't know if it's a good idea to do the empty binding element transformation.