feat(53736): Switch statement over a union of literals is not exhaustive when checking against enum members #53751
Conversation
This file contains 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
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 5c3104f. You can monitor the build here. Update: The results are in! |
|
@sandersn Here they are:
CompilerComparison Report - main..53751
System
Hosts
Scenarios
TSServerComparison Report - main..53751
System
Hosts
Scenarios
StartupComparison Report - main..53751
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jakebailey
reviewed
Jul 18, 2023
| @@ -26021,7 +26021,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
|
|
|||
| function eachTypeContainedIn(source: Type, types: Type[]) { | |||
| return source.flags & TypeFlags.Union ? !forEach((source as UnionType).types, t => !contains(types, t)) : contains(types, source); | |||
| const equalityComparer = (sourceType: Type, targetType: Type) => | |||
| sourceType === targetType || !!(sourceType.flags & TypeFlags.Literal && targetType.flags & TypeFlags.Literal && (sourceType as LiteralType).value === (targetType as LiteralType).value); | |||
There was a problem hiding this comment.
I'm not a total expert, but my gut tells that this isn't the right place to do this, given this is presumably a hot function that previously did something very simple (check if one type is in another by reference only), but now also encodes some niche literal equality behavior.
I feel like I would need to debug and see who's calling this to understand why this is the function to change and not something else...
There was a problem hiding this comment.
Ah, this isn't the function I was thinking of; it's named generically but is only used once in switch case checking. Maybe it hasn't always been that way?
Doing some sleuthing, and this was introduced in #9163 and all references but one were removed in #9407 (280b27e), then never touched again. So, it has been this way for a very long time, it seems.
Not sure what that means for this, but I still somewhat feel like this isn't quite the right place. Maybe the caller's mapType would be better off mapping things differently? Unsure.
@ahejlsberg probably knows better or maybe @weswigham
There was a problem hiding this comment.
I'd maybe rename eachTypeContainedIn to reflect its' specialized equality comparison (or pass the specialized equality comparison in at the callsite), just for clarity of purpose. But it definitely makes sense for switch narrowing to check the runtime representation of the types for equality and not consider the nominal branding for its narrowing, so.... yeah.
There was a problem hiding this comment.
The thing I wonder is that the linked PRs showed code that used this function and then switched away to another method of checking things, which makes me feel like there's some other more-robust-and-consistent way that this would be done if it were reimplemented today, e.g. without this simple exact equality check.
There was a problem hiding this comment.
Should the LiteralType be handled differently for the enums? I'm just trying to figure out in which direction to investigate this issue...
There was a problem hiding this comment.
Handled where?
I think the thing I was hoping was that this function could be dropped entirely in place of some relation + getUnionType(switchTypes) calls in computeExhaustiveSwitchStatement, but I'm having trouble getting it to work by flailing around.
Mainly, I'm just trying to figure out if there's existing code which can already do this narrowing e.g. used by plain ol if statements or something.
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 #53736