microsoft / TypeScript

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

Isolated declarations quick fix results in redundant type arguments when the RHS is a new expression with explicit type arguments #59768

Open blickly opened 1 month ago

blickly commented 1 month ago

Acknowledgement

Comment

// @declaration: true
// @isolatedDeclarations: true
export const s = new Set<string>();

After generating and applying a quick-fix, the current resulting code is:

export const s: Set<string> = new Set<string>();

An ideal fix would remove the redundant type arguments in the RHS expression when adding the same type arguments to the suggested type annotation, resulting in something like:

export const s: Set<string> = new Set();
iisaduan commented 1 month ago

The fix should consider adding another ID fix option that will both add the annotation and do the RHS removal, instead of changing the current quickfixes to always removing the type parameters. The default behavior of lint rules such as https://typescript-eslint.io/rules/consistent-generic-constructors/ give codefixes that undo the ID annotation, so it would be nice to leave a codefix that could leave ID compatible with that rule

blickly commented 1 month ago

Thanks for pointing that out!

It also seems like if you prefer the non-redundant style, you could set your project to use the type-annotation setting and potentially run eslint --fix before running the isolated declarations quick-fixes to get a less redundant result without needing to change how the isolated declarations quick-fixer works. To me, that indicates that this issue is lower priority than it would have been otherwise.

(I also wonder if the default for typescript-eslint.io/rules/consistent-generic-constructors/ would make more sense to be type-annotation now that isolatedDeclarations exists, but that's probably a separate discussion)

iisaduan commented 1 month ago

(I also wonder if the default for typescript-eslint.io/rules/consistent-generic-constructors/ would make more sense to be type-annotation now that isolatedDeclarations exists, but that's probably a separate discussion)

If the particular declaration doesn't require the type annotation for ID (such as in a codebase without ID on), it seems to make a lot of sense to leave the default on the RHS, as with inference, you often don't need the type annotation at all. But like you said, this is probably a discussion that should/could be had over at their repo

It also seems like if you prefer the non-redundant style,..

😄 in the PR to adopt ID, I removed that rule out of the TS codebase today in https://github.com/microsoft/TypeScript/pull/59635/commits/6296b1489c08d53ae84f7c7a1f60e262a59fb90a, and while filing issues for possible ID improvements that I noticed while making that PR, I found this issue. It would have been nice to have a one stroke codefix to update just the ID related annotations in this way.

blickly commented 1 month ago

Great point. In some cases the RHS style is clearly cleaner, even though there are other cases where it clashes with ID.

I'm still interested in the idea of having the ID quickfix move the type arguments from RHS->LHS rather than duplicate it. I'm a little hesitant to include an additional mode to generate the duplicate annotations, since it's not clear to me that the "duplicated type arguments" style is really something that folks actively support vs. just being an accidental quirk of how the current implementation of the https://typescript-eslint.io/rules/consistent-generic-constructors/ rule interacts with ID. I've filed https://github.com/typescript-eslint/typescript-eslint/issues/10001 over there to try to get more clarity.