-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Improve optionality detection in mapped type indexed access substitutions #57946
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
Conversation
src/compiler/checker.ts
Outdated
@@ -14115,7 +14115,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
} | |||
|
|||
function getModifiersTypeOptionality(type: Type): number { | |||
return type.flags & TypeFlags.Intersection ? Math.max(...map((type as IntersectionType).types, getModifiersTypeOptionality)) : | |||
return type.flags & TypeFlags.Intersection ? Math.min(...map((type as IntersectionType).types, getModifiersTypeOptionality)) : |
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.
Only claim that intersections add optionality when all constituents do so. Previously, optionality in any constituent would cause us to claim optionality for the entire intersection, but that is too aggressive.
src/compiler/checker.ts
Outdated
return addOptionality(instantiatedTemplateType, /*isProperty*/ true, getCombinedMappedTypeOptionality(objectType) > 0); | ||
const isOptional = getMappedTypeOptionality(objectType) > 0 || (isGenericType(objectType) ? | ||
getModifiersTypeOptionality(getModifiersTypeFromMappedType(objectType)) > 0 : | ||
couldAccessOptionalProperty(objectType, index)); |
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.
When processing a substitution for { [P in K]: E}[X]
, when { [P in K]: E }
is non-generic, examine the actual properties selected by X
to see if some are optional.
@typescript-bot test top200 |
@ahejlsberg Here are the results of running the user tests comparing Everything looks good! |
Hey @ahejlsberg, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: vue3-carousel-3d
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@ahejlsberg Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
@typescript-bot test it |
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
@ahejlsberg Here are the results of running the user tests comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@ahejlsberg Here are the results of running the top 400 repos comparing Everything looks good! |
Tests and performance are clean. This is ready for a review. |
be careful merging, this will almost guaranteed break main like the last two PRs. |
Fixes #57860.