-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Use lib conditional types for type facts if possible #22348
Use lib conditional types for type facts if possible #22348
Conversation
This causes generics to be narrowed as one may expect
Constraints and intersections as mentioned in the issue in the OP fix most problems (if ugly), but the addition of real, visible constraints causes us to infer literal types in some places just because CFA has constrained a type to some domain, which also is probably undesirable. Traded one set of issues for another, really. |
fc2a826
to
fc29365
Compare
fc29365
to
bf6029b
Compare
Just for the reference: https://stackoverflow.com/questions/50819907/typescript-tagged-union-in-generic More complex usecase: https://bit.ly/2JLbSkY |
@weswigham any update? |
@@ -2705,7 +2705,7 @@ namespace ts { | |||
} | |||
|
|||
function asToken<TKind extends SyntaxKind>(value: TKind | Token<TKind>): Token<TKind> { | |||
return typeof value === "number" ? createToken(value) : value; | |||
return typeof value === "number" ? createToken(value) : value as any; // TODO: FIXME |
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.
What's this about?
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.
The typeof
narrows to TKind
in the true branch, but the false branch narrows to NENumber<TKind | Token<TKind>>
without simplifying even though it obviously should (since generic conditionals don't get simplified when distributive), IIRC. Would have to look at it again to know for sure, though. Might not even repro after a merge - it's been a bit and we've iterated in conditional types quite a bit.
@@ -3299,7 +3299,7 @@ Actual: ${stringify(fullActual)}`); | |||
return ts.createTextSpanFromRange(ranges[index]); | |||
} | |||
else { | |||
this.raiseError("Supplied span index: " + index + " does not exist in range list of size: " + (ranges ? 0 : ranges.length)); | |||
this.raiseError("Supplied span index: " + index + " does not exist in range list of size: " + (!ranges ? 0 : ranges.length)); |
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.
🤣
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.
Right? Tfw the change finds a real error in the code.
Would like to review this at the design meeting first. cc @DanielRosenwasser |
This has inspired many a conversation over the years, however as-is we know we have no shot of merging this. Conditional types have too many drawbacks for it to be OK to introduce them in control flow (like being very hard to relate correctly). We may reconsider, however, in a post-negated-types world, assuming the performance can be tuned, using aliases based on negated types instead. In any case, this PR is old, crusty, and never going to be updated (it'd be far better to make a new PR in such a post-negated-types world), so it's time to close it. |
This causes generics to be narrowed as one may expect, similarly to nongeneric types.
I've not accepted the updated baselines (so this PR should fail), as there are some changes I do not like caused by #22347.
Fixes #22137.
These types are definitely more precise than the fact system - in fact, they even found a bug in fourslash (without strict null checks on!).