Fixed accidental propagation of caching-related objectFlags
#52546
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Andarist
commented
Feb 1, 2023
| @@ -16285,7 +16285,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| let type = unionTypes.get(id); | |||
| if (!type) { | |||
| type = createType(TypeFlags.Union) as UnionType; | |||
| type.objectFlags = objectFlags | getPropagatingFlagsOfTypes(types, /*excludeKinds*/ TypeFlags.Nullable); | |||
| type.objectFlags = (objectFlags | getPropagatingFlagsOfTypes(types, /*excludeKinds*/ TypeFlags.Nullable)) & ~(ObjectFlags.IsUnknownLikeUnionComputed | ObjectFlags.IsUnknownLikeUnion); | |||
There was a problem hiding this comment.
This is a naive~ fix for the issue. I think that it would be best to remove those object flags and use a different mechanism to cache this information. This would prevent accidental propagation like this as this information should be unique to any given type.
I'm just not sure what's the preferred place to keep this sort of information. Let me know what is your preferred solution for this kind of caching and I will adjust the PR.
cc @ahejlsberg
There was a problem hiding this comment.
Thanks for tracking this down. The call to getUnionTypeFromSortedList that causes the issue comes from filterType. I'd suggest changing that call to
return getUnionTypeFromSortedList(filtered, (type as UnionType).objectFlags & (ObjectFlags.PrimitiveUnion | ObjectFlags.ContainsIntersections), /*aliasSymbol*/ undefined, /*aliasTypeArguments*/ undefined, newOrigin);i.e. filter the flags there and leave getUnionTypeFromSortedList alone. It might make sense to also change the parameter name from objectFlags to precomputedObjectFlags just to make its use clear. BTW, strictly speaking it isn't correct to reuse the old ObjectFlags.ContainsIntersections in filterType since the filtering operation may have removed all intersections. However, that flag is just an optimization hint, so no harm comes from propagating it.
There was a problem hiding this comment.
Regarding where to put the flags, I think they're fine where they are. We really don't want to introduce another property for this.
There was a problem hiding this comment.
You mention PrimitiveUnion and ContainsIntersections in the suggested change and in the following comment. I'm a little bit lost on how this relates to the reused IsUnknownLikeUnionComputed and IsUnknownLikeUnion that I have identified to be the root cause here.
There was a problem hiding this comment.
Those two flags are the only ones we want propagated into the new union type. All other flags should be zero. Including the ones you identified as the root cause.
There was a problem hiding this comment.
Ah, I see - that makes sense. I pushed out the requested change.
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 3e34273. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 3e34273. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at 3e34273. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 3e34273. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 3e34273. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at 3e34273. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 3e34273. You can monitor the build here. |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
1 similar comment
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey Here they are:
CompilerComparison Report - main..52546
System
Hosts
Scenarios
TSServerComparison Report - main..52546
System
Hosts
Scenarios
StartupComparison Report - main..52546
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
jakebailey
approved these changes
Feb 1, 2023
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsnocodb/nocodb
|
ahejlsberg
approved these changes
Feb 1, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #52475
cc @jakebailey @RyanCavanaugh