Apply isLiteralContextualType recursively for unions and intersections. #19684
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
Fixes microsoft#19632. The issue was isLiteralContextualType was only checking for type parameters that extended a literal base type at the top level. It was already recursively checking the constituents of unions and intersections but it did so using maybeTypeOfKind which only compared type flags matching literal or index. Also adds a test case that failed before this change and succeeds now.
|
|
mhegazy
reviewed
Nov 3, 2017
| @@ -18458,7 +18458,17 @@ namespace ts { | |||
| } | |||
| contextualType = constraint; | |||
| } | |||
| return maybeTypeOfKind(contextualType, (TypeFlags.Literal | TypeFlags.Index)); | |||
There was a problem hiding this comment.
return forEachType(contextualType, t => t.flags & (TypeFlags.Literal | TypeFlags.Index));There was a problem hiding this comment.
@mhegazy That wouldn't accomplish the same thing as my change. maybeTypeOfKind actually already did something very similar to what you propose, except that it looked for UnionOrIntersectionType for the recursion while forEachType only looks for UnionType.
The problem is the behavior from #19632 was caused by not extracting the any generic constraint types during the recursion into the unions and intersections.
I did go ahead and refactor the code I wrote to use the core forEach function, and also added a few more test cases.
The test cases cover more scenarios that failed before this change and work now, plus added a couple of tests where the code is still expected to have compile errors because the generics do not extend string or number just to verify that my change did not affect that behavior.
|
Closing because the problem this was addressing was fixed by something else at some point. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 #19632. The issue was isLiteralContextualType was only checking
for type parameters that extended a literal base type at the top level.
It was already recursively checking the constituents of unions and
intersections but it did so using maybeTypeOfKind which only compared
type flags matching literal or index.
Also adds a test case that failed before this change and succeeds now.
I realize this wasn't an issue marked as "help wanted" so I understand the PR might not be accepted. I was just curious why the issue I was having was happening and figured it'd be a good way to learn more about how Typescript worked internally, and once I figured out why it was happening it looked easy to fix so I figured I'd try.