Trust variance checking during identity checking#30903
Closed
weswigham wants to merge 3 commits intomicrosoft:masterfrom
Closed
Trust variance checking during identity checking#30903weswigham wants to merge 3 commits intomicrosoft:masterfrom
weswigham wants to merge 3 commits intomicrosoft:masterfrom
Conversation
Member
|
@ahejlsberg thoughts on which should go into 3.4.4 (or both?) |
73ab894 to
0923a7c
Compare
Member
|
With #30877 the typesafe-joi project compiles successfully: With #30877 plus the bail out that always trusts variances with identity relations it gets slightly better: However, using this PR (i.e. So, my take is that #30877 should go in, possibly with the bail out that always trusts variance checks. I don't think we want this PR. |
ahejlsberg
requested changes
Apr 13, 2019
Member
ahejlsberg
left a comment
There was a problem hiding this comment.
I don't think this PR is ready for prime time as per my comments above.
Member
Author
|
#30416 essentially improves the same core area as this, so I'm going to close this. |
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
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.
This reverts the substantive change in #30877 in favor of an alternative fix that simply makes identity checking more efficient in many cases by always using the "variance" result, if present (I mentioned this in the comments in that PR),
We always used to do this until 3.4 where we realized variance checking was often inaccurate, and made it a fast path for the
truecase, but still fell back to structural checking - that change is ultimately what broketypesafe-joi. By retaining that behavior for identity checking, we retain our old performance characteristics there.