-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Accurate constraintType for indexedAccessType #53059
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accurate constraintType for indexedAccessType #53059
Conversation
277732e
to
0f6bc92
Compare
src/compiler/checker.ts
Outdated
const indexType = type.indexType; | ||
let indexConstraint = getSimplifiedTypeOrConstraint(indexType); | ||
if (indexConstraint && indexConstraint.flags & TypeFlags.Index) { | ||
const constraint = getBaseConstraintOfType((indexConstraint as IndexType).type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, by using getBaseConstraintOfType
here like this, you're shortcutting a lot of steps in structuredTypeRelatedToWorker
. That's what the baseline changes here show. Specifically, presume you have a T[keyof U]
being related to a A
where A extends T[keyof T]
and U extends T
- this will cause us to skip past the T[keyof T]
constraint for T[keyof U]
straight to T[string | number | symbol]
(assuming T
is unconstrained), thereby missing that T[keyof U]
is an allowed subset of A
because T[keyof T]
is A
's constraint (under our current rules - read how we have lax indexed access constraint relationships elsewhere). This is, unfortunately, almost definitely a visible breaking change as-is. Rather than skipping so many steps here, explicitly adjusting structuredTypeRelatedToWorker
to do some extra constraint exploration for IndexedAccess
types within the sourceFlags & TypeFlags.TypeVariable
section would be preferable and probably less likely to be breaky if it's just an alternate non-error-reporting success path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Process in sourceFlags & TypeFlags.TypeVariable
section, so that existing error.txt is not affected !
…ags.TypeVariable section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided tests come back clean, I think this looks pretty good
@typescript-bot test this |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 845f5ab. You can monitor the build here. Update: The results are in! |
Heya @weswigham, I've started to run the extended test suite on this PR at 845f5ab. You can monitor the build here. |
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at 845f5ab. You can monitor the build here. Update: The results are in! |
Heya @weswigham, I've started to run the perf test suite on this PR at 845f5ab. You can monitor the build here. Update: The results are in! |
Hey @weswigham, the results of running the DT tests are ready. |
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
@weswigham Here they are:
CompilerComparison Report - main..53059
System
Hosts
Scenarios
TSServerComparison Report - main..53059
System
Hosts
Scenarios
StartupComparison Report - main..53059
System
Hosts
Scenarios
Developer Information: |
Fixes #52399