Prefer a likely literal over anonymous type in --noImplicitAny codefixes #36015
Conversation
Before trying to make an anonymous type for a type's usage, we'll first check if there is exactly one builtin primitive the usage is assignable to, and use it if so. Right now that's only `number` and `string` because `boolean` has no distinguishable members. A couple of implementation details: * `tryInsertTypeAnnotation` needed to know to insert a type _after_ a node's `exclamationToken` if it exists * This code area was written before `??` 😉
There was a problem hiding this comment.
I think you only need to fix the handling of void to get your fix to work.
The title says "prefer literal over anonymous type", and that might still be necessary, at least for more complex examples, but I think the right way to make that happen is by tweaking priorities inside combineTypes.
| @@ -1016,7 +1021,11 @@ namespace ts.codefix { | |||
| return !sigs.length || !checker.isTypeAssignableTo(source, getFunctionFromCalls(propUsage.calls)); | |||
| } | |||
| else { | |||
| return !checker.isTypeAssignableTo(source, combineFromUsage(propUsage)); | |||
| const combinedPropUsage = combineFromUsage(propUsage); | |||
| if (combinedPropUsage.flags & TypeFlags.Void) { | |||
There was a problem hiding this comment.
I think the correct fix for Too Much Void is to make combineTypes convert it to any when appropriate. Maybe combineFromUsage if it needs context, but from the example it seems like only a small amount of context might be needed: whether the property is in a call expression (void ok) or not (void not ok).
|
Thanks so much for the helpful PR review @sandersn! e33cc59 goes along with your suggestions at addressing the inferred type in existing logic, I think. |
…mUsagePropertyAccessJS.ts
| verify.rangeAfterCodeFix("a: { b: { c: void; }; }, m: { n: () => number; }, x: { y: { z: number[]; }; }", /*includeWhiteSpace*/ undefined, /*errorCode*/ undefined, /*index*/0); | ||
| verify.rangeAfterCodeFix("a: { b: { c: any; }; }, m: { n: () => number; }, x: { y: { z: number[]; }; }", /*includeWhiteSpace*/ undefined, /*errorCode*/ undefined, /*index*/0); |
There was a problem hiding this comment.
This looks like an unintended positive bug fix, right? Other places (e.g. codeFixInferFromCallInAssignment) infer any for variable types that would otherwise be describable as unknown.
There was a problem hiding this comment.
I'm not sure what you mean by "unintended positive". void was wrong and any is right as far as I can tell.
I think, in most positions, inferring any vs unknown is a matter of preference -- how strict do you want the result to be? Right now I assume that people want pretty loose results, since I expect people will use this after converting from .js to .ts, and before trying to get rid of any explicit any.
We could discuss adding another fix to the codefix, one that defaults to unknown when possible. I think it would be a decent idea, but hard to explain in the space of a codefix title.
There was a problem hiding this comment.
Oh yes I think we're on the same page - I meant that the change from void to any is positive, and I didn't initially intend it as part of this PR's changes. 👍
| verify.rangeAfterCodeFix("a: { b: { c: void; }; }, m: { n: () => number; }, x: { y: { z: number[]; }; }", /*includeWhiteSpace*/ undefined, /*errorCode*/ undefined, /*index*/0); | ||
| verify.rangeAfterCodeFix("a: { b: { c: any; }; }, m: { n: () => number; }, x: { y: { z: number[]; }; }", /*includeWhiteSpace*/ undefined, /*errorCode*/ undefined, /*index*/0); |
There was a problem hiding this comment.
I'm not sure what you mean by "unintended positive". void was wrong and any is right as far as I can tell.
I think, in most positions, inferring any vs unknown is a matter of preference -- how strict do you want the result to be? Right now I assume that people want pretty loose results, since I expect people will use this after converting from .js to .ts, and before trying to get rid of any explicit any.
We could discuss adding another fix to the codefix, one that defaults to unknown when possible. I think it would be a decent idea, but hard to explain in the space of a codefix title.
| @@ -668,6 +668,10 @@ namespace ts.codefix { | |||
| } | |||
| } | |||
|
|
|||
| function inferTypeFromExpressionStatement(node: Expression, usage: Usage): void { | |||
| addCandidateType(usage, isPropertyAccessExpression(node) ? checker.getAnyType() : checker.getVoidType()); | |||
There was a problem hiding this comment.
I think it would be safer to only infer void from call expressions and give any for everything else.
|
Thanks for your patience on this one @JoshuaKGoldberg. |
Before trying to make an anonymous type for a type's usage, we'll first check if there is exactly one builtin primitive the usage is assignable to, and use it if so. Right now that's only
numberandstringbecausebooleanhas no distinguishable members.Notes on implementation details:
TypeFlags.Voidcheck feels... wrong? But I'm not familiar enough with the checker to know a better way to allow uses of typevoid.tryInsertTypeAnnotationneeded to know to insert a type after a node'sexclamationTokenif it exists.??😉Fixes #29321