Improve excess property checking for intersections #32582
Merged
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
Still a draft, the implementation needs improvement
This makes parameter lists a lot shorter. Seems like a slight improvement, although I can revert if I change my mind.
|
I think that using a mutable isIntersection in checkTypeRelatedTo is easier to read than extending the parameter lists yet again, but I can revert that commit if needed. (Assigned to @weswigham instead of requesting since the requested reviewers list is broken.) |
weswigham
reviewed
Jul 26, 2019
src/compiler/checker.ts
Outdated
| @@ -29587,7 +29602,7 @@ namespace ts { | |||
| } | |||
| else { | |||
| // Only here do we need to check that the initializer is assignable to the enum type. | |||
| checkTypeAssignableTo(checkExpression(initializer), getDeclaredTypeOfSymbol(getSymbolOfNode(member.parent)), initializer, /*headMessage*/ undefined); | |||
| checkTypeAssignableTo(checkExpression(initializer), getDeclaredTypeOfSymbol(getSymbolOfNode(member.parent)), initializer); | |||
There was a problem hiding this comment.
??? Why was the parameter removed here? AFAIK passing /*headMessage*/ undefined disables error reporting, no?
There was a problem hiding this comment.
not passing it also disables error reporting. I ran a cleanup regex and it applied to this usage too. If you want to keep it to help readability, I can put it back.
weswigham
reviewed
Jul 26, 2019
src/compiler/checker.ts
Outdated
| @@ -12849,13 +12849,15 @@ namespace ts { | |||
| } | |||
| } | |||
| else if (target.flags & TypeFlags.Intersection) { | |||
| isIntersectionConstituent = true; // set here to affect the following trio of checks | |||
There was a problem hiding this comment.
So this affected the recursiveTypeRelatedTo call after this else if block (if the contents of the block did not cause a return) before (the third check of the trio referenced in the comment), but now nothing does. Was the change intentional?
There was a problem hiding this comment.
I think so (the intervening vacation makes things a bit fuzzy). I think I decided it could be dispensed with entirely because isIntersectionConstituent is now set in typeRelatedToEachType (line 13152) instead of here. But it may be there is a difference and that our tests just don't catch it.
In any case, after leaving this PR for a while, I kind of like the threaded-parameter approach. I'm going to measure performance on both commits to make sure that there are no weird limits on parameter count in the VM and go back to explicit parameters if there's no perf difference.
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 74113b3. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@sandersn Here they are:Comparison Report - master..32582
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This reverts commit b8dccff.
|
OK, I reverted the use of a mutable variable to track isIntersectionConstituent. Now it's back to being a parameter threaded through various functions. @typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at b8507d2. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@sandersn Here they are:Comparison Report - master..32582
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Perf numbers are about the same both ways. @weswigham can you take a look at the parameter-threaded management of |
weswigham
approved these changes
Aug 6, 2019
weswigham
reviewed
Aug 6, 2019
| @@ -13584,7 +13584,7 @@ namespace ts { | |||
| if (source.flags & (TypeFlags.Object | TypeFlags.Intersection) && target.flags & TypeFlags.Object) { | |||
| // Report structural errors only if we haven't reported any errors yet | |||
| const reportStructuralErrors = reportErrors && errorInfo === saveErrorInfo && !sourceIsPrimitive; | |||
| result = propertiesRelatedTo(source, target, reportStructuralErrors, /*excludedProperties*/ undefined); | |||
| result = propertiesRelatedTo(source, target, reportStructuralErrors, /*excludedProperties*/ undefined, isIntersectionConstituent); | |||
There was a problem hiding this comment.
Yep, seeing this change here, I have a vague memory of thinking that I didn't need to pass it thru here before because we didn't do deep excess property checking - now that we do, we should.
This was referenced Sep 3, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Excess property checking incorrectly applies to half of an intersection when nested deeply enough because
isIntersectionConstituentis not sufficiently threaded through thecheckTypeRelatedTocode. This PR threadsisIntersectionConstituentthrough several more places.I may scope
isIntersectionConstituentto the entirety ofcheckTypeRelatedToand mutate it in a dynamically scoped way. The fix works as-is, but I think it would be simpler to mutate a variable instead of adding parameters.Fixes #30715