Fixed type predicate inference for discriminated union parameters #57952
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
@typescript-bot test it |
|
Hey @RyanCavanaugh, the results of running the DT tests are ready. Everything looks the same! |
|
@RyanCavanaugh Here are the results of running the user tests comparing Everything looks good! |
|
@RyanCavanaugh Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@RyanCavanaugh Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
const suites = tasksGroup.filter(group => group.type === 'suite')
const tests = tasksGroup.filter(group => group.type === 'test')
const groups = shuffle([suites, tests], sequence.seed)
tasksGroup = groups.flatMap(group => shuffle(group, sequence.seed))Seems like an expected break |
|
Full TS playground for the above: TS playground |
…ference-discriminated-union
|
@danvk look right to you? |
There was a problem hiding this comment.
LGTM
I'd wondered what the right value for declaredType in the false check was but didn't have a test case that showed it making a difference. Now we do! Thanks for tracking this down, @Andarist.
Not relevant to merging this PR, but I am curious why declaredType matters in this case but not most others. Presumably there are other, unreported bugs that this fixed.
| antecedent, | ||
| node: expr, |
There was a problem hiding this comment.
any particular reason for this change?
There was a problem hiding this comment.
I tried to follow the structure used by the ones created in the binder to keep functions receiving this monomorphic. I'm not sure if my change prompted this further experiment but it certainly looked like it to me since it was open soon after I opened this one here ;p
TLDR: this was meant as an optimization for the engine. I didn't measure it at all since I don't have time for it but it's usually better to create structures in the same way (order of keys etc matters for the engine)
There was a problem hiding this comment.
👍 Makes sense. The calls to getFlowTypeOfReference are so expensive compared to everything else in getTypePredicateFromBody that I doubt micro-optimizations matter (see #57465 (comment) and #57660). But I can't imagine they hurt!
There was a problem hiding this comment.
once u apply micro-optimizations at scale there are some good wins to be had :P #57977 (comment)
Without this TS won't attempt to discriminate the union since unions are not involved, see the condition here. The fix relies on the fact that the assigned type might be different than the declared type and it's still "preserved": type U = { type: "foo" } | { type: "bar" };
const item: U = { type: "foo" };
item;
// ^? const item: { type: "foo"; } |
fixes #57947