-
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
Add unmeasurable variance kind for marking types whose variance result is unreliable #30416
Add unmeasurable variance kind for marking types whose variance result is unreliable #30416
Conversation
This also fixes #29698. |
src/compiler/checker.ts
Outdated
@@ -13014,7 +13025,9 @@ namespace ts { | |||
getCombinedMappedTypeOptionality(source) <= getCombinedMappedTypeOptionality(target)); | |||
if (modifiersRelated) { | |||
let result: Ternary; | |||
if (result = isRelatedTo(getConstraintTypeFromMappedType(target), getConstraintTypeFromMappedType(source), reportErrors)) { |
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.
Is it possible to only report as unmeasurable when the mapped types have modifiers?
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.
Isn't the Record
example exactly the case of when there isn't a modifier?
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.
This was the kind of (hack) I had in mind: https://pastebin.com/n431Puhu. I have no idea if this is even feasible, or correct. I thought I'd at least play around with something rather than solely firing off random questions.
@typescript-bot pack this |
Heya @weswigham, I've started to run the tarball bundle task on this PR at 33b6e42. You can monitor the build here. It should now contribute to this PR's status checks. |
[Pasting my reply from #30973] Fixes lodash — 485k → 85k types. (vs 83k on 3.3) Reactstrap 8.0.1 does better than this fix does relative to 8.0.0 on master. In fact, 8.0.1 no longer benefits from this fix. On master, it creates 107k types, and on this branch it creates 108k types (80k on 3.3). |
@typescript-bot run dt & @typescript-bot user test this |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at bbfb25f. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the community code test suite on this PR at bbfb25f. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot test this 😃 |
Heya @weswigham, I've started to run the extended test suite on this PR at bbfb25f. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot run dt so I can get a full error log on each shard thanks |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at bbfb25f. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot run dt now that I've patched |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 15f8c10. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot run dt once more - I've amended |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at dc40484. You can monitor the build here. It should now contribute to this PR's status checks. |
5e624e8
to
4e96d98
Compare
@typescript-bot run dt once more - per an offline request by @ahejlsberg I've walked back a bit of the change (and added a test to show what's still broken) to make what's here a bit easier to swallow so we can iterate on it. |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at b646f28. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot run dt again |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 18e5656. You can monitor the build here. It should now contribute to this PR's status checks. |
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.
Assuming we're good with the two DT changes, I think we should get this merged.
Yeah, the one is from current |
6e0ee3a
to
a7fc378
Compare
@typescript-bot run dt to see if my fix for the circular structural check error breaks anything. We don't have to ship it, since the error is kinda justifiable based on the comparisons we need to perform, but I feel like the |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at a7fc378. You can monitor the build here. It should now contribute to this PR's status checks. |
Oho, clean DT run. Noice. |
@ahejlsberg we can talk about 6a9e337 seperately if you like, but I've merged it with this PR (and merged this PR) for now, since it fixed the remaining regression on DT so this PR is squeaky clean. |
Those types being mapped types which are generic on their keys - in these cases our variance results are inaccurate, but not consistently on the positive or negative side. This is simply because the rules for relating object types are more permissive then mapped types when it comes to values, but are less permissive when it come to modifiers and keys. Simply disabling variance probing on mapped types isn't sufficient - that just causes us to kick the can down the line to a type containing the generic mapped type, which then exhibits the same issue.
Fixes #29991