Repository navigation
[experiment] Skip incompatible discriminants when inferring - #64695
Mateusz Burzyński (Andarist) wants to merge 12 commits into
Conversation
|
TypeScript Bot (@typescript-bot) test it |
There was a problem hiding this comment.
🔵 Needs a closer look
The draft changes broad compiler inference semantics despite strong regression coverage and warrants human evaluation against extended user tests.
0 open findings
What changed in this PR
Adds discriminant-aware filtering to union-source inference, addressing #64690 while expanding compiler regression coverage.
Changes:
- Skips source constituents with incompatible fixed discriminants.
- Tests aliases, nesting, tag types, ordering, and optional-property semantics.
- Adds diagnostics, type, and symbol baselines for both optional-property modes.
| File | Description |
|---|---|
tsc/internal/checker/inference.go |
Implements discriminant-aware inference filtering. |
tsc/testdata/tests/cases/compiler/inferenceDiscriminatedUnionSource.ts |
Adds compiler regression cases. |
tsc/testdata/baselines/reference/compiler/inferenceDiscriminatedUnionSource(exactoptionalpropertytypes=true).types |
Records inferred types. |
tsc/testdata/baselines/reference/compiler/inferenceDiscriminatedUnionSource(exactoptionalpropertytypes=true).symbols |
Records symbols. |
tsc/testdata/baselines/reference/compiler/inferenceDiscriminatedUnionSource(exactoptionalpropertytypes=true).errors.txt |
Records diagnostics. |
tsc/testdata/baselines/reference/compiler/inferenceDiscriminatedUnionSource(exactoptionalpropertytypes=false).types |
Records inferred types. |
tsc/testdata/baselines/reference/compiler/inferenceDiscriminatedUnionSource(exactoptionalpropertytypes=false).symbols |
Records symbols. |
tsc/testdata/baselines/reference/compiler/inferenceDiscriminatedUnionSource(exactoptionalpropertytypes=false).errors.txt |
Records diagnostics. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. Everything looks the same! |
|
Jake Bailey (@jakebailey) Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Man, when we tried this in the JS codebase, the type comparison on every possible inference target ended up blowing up any savings we got from skipping deeper inferences on less appropriate members. But the perf suite shows it as good now, so I'm a fan. Maybe the optional prop fastpath and objecty type carve-outs are saving enough low-value comparisons it's at least perf neutral again? I'd be happier if we used |
|
Jake Bailey (@jakebailey) Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
65b5a44 to
7560713
Compare
|
I still have some extra changes that I'd like to push here (maybe ;p) but I'd like to first see how this version performs in the perf tests and in the wild. Could I ask for a full test rerun here? cc Jake Bailey (@jakebailey) |
|
TypeScript Bot (@typescript-bot) test it |
|
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. Everything looks the same! |
|
Jake Bailey (@jakebailey) Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Jake Bailey (@jakebailey) Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
Looks like it's neutral to +8% check time, project dependent. It's not terrible now, compared to the JS codebase (where it cost way way more to do this) - maybe some caching of the discriminable item results could improve the perf back to neutral? Or maybe adding back in the optional prop fast path you had in before? I'm guessing that's where most of the additional cost lies in the new fully-discriminating version, but I don't know for sure. Could also use some most test cases that show off where is actually makes us produce better inference results, though? A cursory check of the tests don't show me anything that I think improve? I'd assume cases like those from #43853 get improved, though. |
Yeah, I have it locally for a week already - just gotta find a moment to breathe and push it out and ask for a fresh rerun.
I think I already have some here or on that local branch - anyway... I'll get to this some time later this week and push everything I have for this.
Oh, nice! I wasn't aware of this PR - I'll take a closer look at this one too (and its tests) |
|
I'd hope we can make the perf better still? I really hope to not make us slower if we can avoid it |
|
TypeScript Bot (@typescript-bot) test it |
|
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. Everything looks the same! |
|
Jake Bailey (@jakebailey) Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Jake Bailey (@jakebailey) Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
…erence-discriminants
|
TypeScript Bot (@typescript-bot) test it |
|
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. Everything looks the same! |
|
Jake Bailey (@jakebailey) Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Jake Bailey (@jakebailey) Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
Jake Bailey (@jakebailey) could u rerun perf tests here? |
|
TypeScript Bot (@typescript-bot) perf test this faster |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fixes #64690
I was kinda always wondering if a logic like this could be used to cut down on inference work when dealing with discriminated unions and the referenced PR pushed me to try it out.
I'd still like to do more work on this and recheck things. That's why I'm opening this as a draft, I'd be curious what extended user tests etc would show for this