microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.94k stars 12.47k forks source link

Isolated declarations quick fix defaults to including renamed properties #59547

Open MichaelMitchell-at opened 3 months ago

MichaelMitchell-at commented 3 months ago

πŸ”Ž Search Terms

isolated declarations quick fix rename ts2842 ts9007

πŸ•— Version & Regression Information

⏯ Playground Link

https://www.typescriptlang.org/play/?isolatedDeclarations=true&ts=5.6.0-dev.20240807#code/KYDwDg9gTgLgBAMwK4DsDGMCWEWIBQCUcA3gFBwVxTAxJS5x57ECGAXHAEYC+HrHABm4EOANwiYAJnAC8APhLCAhAG5S3IA

πŸ’» Code

export function f() {
    return  (({a: b}: {a: 0}): void => {})!;
}

πŸ™ Actual behavior

Quick fix introduces a type error:

export function f(): ({ a: b }: { a: 0; }) => void {
//                         ~
//                         'b' is an unused renaming of 'a'. Did you intend to use it as a type annotation? (2842)
    return  (({a: b}: {a: 0}): void => {})!;
}

πŸ™‚ Expected behavior

export function f(): ({a}: { a: 0; }) => void {
    return  (({a: b}: {a: 0}): void => {})!;
}

Additional information about the issue

While the quick fix is consistent with the emitted declaration, it ideally wouldn't lead to another type error which can be avoided. There are cases where it may make sense to preserve the renaming, most obviously

export function f(): ({ a: b }: { a: 0; }) => typeof b {
    return (({a: b}: {a: 0}): typeof b => b)!;
}

but also perhaps

export function f() {
    return  (({a: b}: {a: 0}, a: {}): void => {})!;
}

though, maybe surprisingly, this would currently this still result in ts(2842)

export function f(): ({ a: b }: { a: 0; }, a: {}) => void {
//                         ~
//                         'b' is an unused renaming of 'a'. Did you intend to use it as a type annotation? (2842)
    return  (({a: b}: {a: 0}, a: {}): void => {})!;
}

Perhaps this could be relaxed. While b is unused in the direct sense, it technically has value in that it avoids the naming collision between the as.

jakebailey commented 3 months ago

IIRC we've tried to fix this particular thing multiple times now (at least for declaration emit), and have ended up just reverting all of the code that tries to elide these as the effort was just far too error-fraught.

See also:

Maybe we can do better in the quick fix? But, then the quick fix would produce a result that differs from declaration emit via inference. It just seems like exactly the same problem space.