Improve contextually typed parameters with initializers #56506
Conversation
typescript-bot
added
the
For Uncommitted Bug
Andarist
changed the title
never parameter type
src/compiler/checker.ts
Outdated
| type = widenTypeInferredFromInitializer(declaration, checkDeclarationInitializer(declaration, CheckMode.Normal)); | ||
| if (type && declaration.initializer) { | ||
| const initializerType = widenTypeInferredFromInitializer(declaration, checkDeclarationInitializer(declaration, CheckMode.Normal)); | ||
| if (!isTypeAssignableTo(initializerType, type) && isTypeAssignableTo(type, initializerType)) { |
There was a problem hiding this comment.
Hmm... what happens in the case of mutually assignable types?
There was a problem hiding this comment.
How many of them do we have? There is, of course, any and I can also think of bivariant methods but I can't call more on the spot.
That said - if the types are mutually assignable then the old behavior is preserved (the contextual parameter type is preferred). Can you think of any scenario in which this would be problematic?
There was a problem hiding this comment.
I guess you’re right. If the types are mutually assignable then it probably doesn’t matter which one is picked and in the absence of a type annotation it makes more sense to pick the contextual type (especially since it prevents inferred anys from sneaking in)
There was a problem hiding this comment.
especially since it prevents inferred anys from sneaking in
That I'm not so sure about :p
const test: (arg: any) => void = (arg = 5) => {
arg
// ^? (parameter) arg: any
};But inferring number would also not be good here either. It would only give the illusion of type-safety within the function while test would still be callable with just anything.
There was a problem hiding this comment.
I specifically meant inferring any from a default when there’s a more sane contextual type available. That’d be really bad IMO.
|
@typescript-bot run DT |
|
Heya @gabritto, I've started to run the regular perf test suite on this PR at aff1cfa. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at aff1cfa. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at aff1cfa. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at aff1cfa. You can monitor the build here. Update: The results are in! |
|
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@gabritto Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @gabritto, the results of running the DT tests are ready. |
|
@gabritto Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
Minimal repro case for the above: type CanvasDirection = "RIGHT" | "LEFT"
interface GraphActions {
setDirection: (direction: CanvasDirection) => void;
}
declare function create<T>(config: T): void
create<GraphActions>({
setDirection: (direction = "RIGHT") => {
direction // should be `CanvasDirection`
},
}) |
|
Looks to me like this fix should only apply when the initialiser type is not a subtype of the contextual type. The bug's example was Actually, |
|
I thought this change would be ok because, assuming the widened initializer type is I think Nathan's solution works, as we'd only trigger the new behavior in cases where currently we issue an error: |
|
Yes, the problem is with widening. I moved the widening call to be within this if block that prefers the initializer's type and it seems to do the trick. I was also considering using |
| let type = tryGetTypeAtPosition(context, i); | ||
| if (type && declaration.initializer) { | ||
| const initializerType = checkDeclarationInitializer(declaration, CheckMode.Normal); | ||
| if (!isTypeAssignableTo(initializerType, type) && isTypeAssignableTo(type, initializerType)) { |
There was a problem hiding this comment.
I think you want to use the widened initializer type in this second condition (isTypeAssignableTo(type, initializerType)), because consider the following example:
declare function test<
TContext,
TMethods extends Record<string, (ctx: TContext, ...args: (1 | 2)[]) => unknown>,
>(context: TContext, methods: TMethods): void;
test(
{
count: 0,
},
{
checkLimit: (ctx, max = 3) => {},
},
);The initializer type is 3, I think, and the contextual type is 1 | 2. We will still error on that case, and we still want to pick type number over type 1 | 2.
|
|
||
| create<GraphActions>({ | ||
| setDirection: (direction = "RIGHT") => { | ||
| direction; // CanvasDirection |
There was a problem hiding this comment.
Can you modify this test so that instead of direction we have something like f(direction) where f takes a CanvasDirection? I think it makes more clear that this would break if we picked parameter type string over CanvasDirection.
There was a problem hiding this comment.
Do u mean smth like this?
type CanvasDirection = "RIGHT" | "LEFT";
interface GraphActions {
setDirection: (setter: (direction: CanvasDirection) => void) => void;
}
export declare function create<T>(config: T): void;
create<GraphActions>({
setDirection: (setter = (direction: string) => {}) => {
setter;
},
});There was a problem hiding this comment.
I meant something like:
type CanvasDirection = "RIGHT" | "LEFT";
interface GraphActions {
setDirection: (direction: CanvasDirection) => void;
}
declare function takesDirection(x: CanvasDirection): void;
export declare function create<T>(config: T): void;
create<GraphActions>({
setDirection: (direction = "RIGHT") => {
takesDirection(direction);
},
});There was a problem hiding this comment.
ah, makes sense - thanks for the clarification
|
@typescript-bot run DT |
|
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at 7a7f6aa. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the regular perf test suite on this PR at 7a7f6aa. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at 7a7f6aa. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at 7a7f6aa. You can monitor the build here. Update: The results are in! |
|
@typescript-bot pack this |
|
Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@gabritto Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
I think that's worth thinking about, but to be honest I couldn't so far think about an example, so I don't know if it matters. |
|
Hey @gabritto, the results of running the DT tests are ready. |
|
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
fixes #56505