Uh oh!
There was an error while loading. Please reload this page.
Improve excess property checking for intersections - #32582
Merged
Nathan Shively-Sanders (sandersn) merged 5 commits intoAug 6, 2019
Merged
Conversation
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.
MemberAuthor
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 Wesley Wigham (@weswigham) instead of requesting since the requested reviewers list is broken.) |
| 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?
MemberAuthor
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.
| } | ||
| } | ||
| 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?
MemberAuthor
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.
Nathan Shively-Sanders (sandersn)
commented
Aug 6, 2019
MemberAuthor
TypeScript Bot (@typescript-bot) perf test this |
Contributor
Heya Nathan Shively-Sanders (@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! |
TypeScript Bot (typescript-bot)
commented
Aug 6, 2019
Contributor
Nathan Shively-Sanders (@sandersn) Here they are:Comparison Report - master..32582
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This reverts commit b8dccff.
Nathan Shively-Sanders (sandersn)
commented
Aug 6, 2019
MemberAuthor
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 (@typescript-bot) perf test this |
Contributor
Heya Nathan Shively-Sanders (@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! |
TypeScript Bot (typescript-bot)
commented
Aug 6, 2019
Contributor
Nathan Shively-Sanders (@sandersn) Here they are:Comparison Report - master..32582
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Nathan Shively-Sanders (sandersn)
commented
Aug 6, 2019
MemberAuthor
Perf numbers are about the same both ways. Wesley Wigham (@weswigham) can you take a look at the parameter-threaded management of |
Wesley Wigham (weswigham)
approved these changes
Aug 6, 2019
Wesley Wigham (weswigham)
left a comment
Member
There was a problem hiding this comment.
I'm still concerned that isIntersectionConstituent needs to somehow be included in the relation cache ID - but I had that concern before this PR (and nobody's yet submitted a bug making that concern real), so I suppose nothing has changed.
| // 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.
Nathan Shively-Sanders (sandersn)
deleted the
excess-property-check-nested-array-intersection
branch
August 6, 2019 22:03
This was referenced Sep 3, 2019
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 freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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