Skip to content
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

Merged
merged 13 commits into from May 3, 2019

Conversation

weswigham
Copy link
Member

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

@jack-williams
Copy link
Collaborator

This also fixes #29698.

@@ -13014,7 +13025,9 @@ namespace ts {
getCombinedMappedTypeOptionality(source) <= getCombinedMappedTypeOptionality(target));
if (modifiersRelated) {
let result: Ternary;
if (result = isRelatedTo(getConstraintTypeFromMappedType(target), getConstraintTypeFromMappedType(source), reportErrors)) {
Copy link
Collaborator

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?

Copy link
Member Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, adding the modifier check means that this will not fix #29698. But the two issues aren't exactly the same so I was wondering if there exists two distinct fixes. #29698 only really breaks because when using string as a key type.

Copy link
Collaborator

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.

@weswigham
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 19, 2019

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.

@sandersn
Copy link
Member

sandersn commented Apr 19, 2019

[Pasting my reply from #30973]

Fixes lodash — 485k → 85k types. (vs 83k on 3.3)
Fixes reactstrap 8.0.0, basically — 1538k → 591k types (vs 493k on 3.3)
So it fixes #30973

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).

@weswigham
Copy link
Member Author

weswigham commented Apr 24, 2019

@typescript-bot run dt & @typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 24, 2019

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
Copy link
Collaborator

typescript-bot commented Apr 24, 2019

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.

@weswigham
Copy link
Member Author

@typescript-bot test this 😃

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 24, 2019

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.

@weswigham
Copy link
Member Author

@typescript-bot run dt so I can get a full error log on each shard thanks

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 24, 2019

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.

@weswigham
Copy link
Member Author

@typescript-bot run dt now that I've patched Readonly<any> to be any instead of the absolutely-not-as-permissive-as-desired {readonly [index: string]: any}.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 25, 2019

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.

@weswigham
Copy link
Member Author

@typescript-bot run dt once more - I've amended Unmeasurable variances to still allow a non-structural out for identical types and comparisons with any. While the second isn't quite correct, in the cases where it's not, the structural errors are essentially useless in figuring out what nonlinear relationship made your any not be related enough to whatever else (spoiler: usually mapped type keys, since keyof any is just string | number | symbol which is just a string indexer, which isn't nearly as strong a type as the mapped type is being related in higher order as). Since going to structural checks there seems to break some people in DT, imma just defer that to when we fix the higher-order mapped type relationship to actually capture the nuance of index signature behavior.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 25, 2019

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.

@weswigham weswigham force-pushed the add-unmeasurable-variance-kind branch from 5e624e8 to 4e96d98 Compare April 29, 2019 20:38
@weswigham
Copy link
Member Author

@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.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 29, 2019

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.

@weswigham
Copy link
Member Author

@typescript-bot run dt again

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 29, 2019

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.

Copy link
Member

@ahejlsberg ahejlsberg left a 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.

@weswigham
Copy link
Member Author

weswigham commented May 3, 2019

Assuming we're good with the two DT changes, I think we should get this merged.

Yeah, the one is from current master and the other is a new circularity caused by some newly structural checking. (We lookup the type of a call expression, looking at the signature's return, the signature has a conditional on this in its return, which causes us to then compare that member's signature with the conditional extends member of the same name, which requires relating the same signature)

@weswigham weswigham force-pushed the add-unmeasurable-variance-kind branch from 6e0ee3a to a7fc378 Compare May 3, 2019 20:48
@weswigham
Copy link
Member Author

@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 method(): this extends SpecificSubtype ? A : B pattern is probably common enough in the wild to want it to not break, since it already worked provided the SpecificSubtype was generic (despite it only occurring once in DT).

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 3, 2019

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.

@weswigham
Copy link
Member Author

Oho, clean DT run. Noice.

@weswigham weswigham merged commit b365e65 into microsoft:master May 3, 2019
@weswigham weswigham deleted the add-unmeasurable-variance-kind branch May 3, 2019 21:42
@weswigham
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Required<T> optional properties type validation will not work
6 participants