Normalize effective constraint intersection before checking if source is a part of it #49956
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
…traint of its intersection
typescript-bot
added
Author: Team
For Milestone Bug
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
jakebailey
commented
Jul 20, 2022
jakebailey
changed the title
|
@typescript-bot test this |
This comment was marked as outdated.
This comment was marked as outdated.
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 73c6219. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 73c6219. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 73c6219. You can monitor the build here. Update: The results are in! |
This comment was marked as outdated.
This comment was marked as outdated.
|
@jakebailey |
This comment was marked as outdated.
This comment was marked as outdated.
|
@typescript-bot perf test this faster |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 73c6219. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:Comparison Report - main..49956
System
Hosts
Scenarios
Developer Information: |
| @@ -12118,7 +12118,7 @@ namespace ts { | |||
| } | |||
| } | |||
| } | |||
| return getIntersectionType(constraints); | |||
| return getNormalizedType(getIntersectionType(constraints), /*writing*/ false); // The source types were normalized; ensure the result is normalized too. | |||
There was a problem hiding this comment.
the writing parameter has me a little suspicious here.
Side note, the comment makes this line a bit too long.
Suggested change
| return getNormalizedType(getIntersectionType(constraints), /*writing*/ false); // The source types were normalized; ensure the result is normalized too. | |
| // The source types were normalized; ensure the result is normalized too. | |
| return getNormalizedType(getIntersectionType(constraints), /*writing*/ false); |
There was a problem hiding this comment.
This function is only called in one place on the source types during relation checking; this mirrors how the same function calls getNormalizedType on the source itself earlier. I had the following code instead at the one call site:
// earlier...
const source = getNormalizedType(originalSource, /*writing*/ false);
// later...
let constraint = getEffectiveConstraintOfIntersection(source.flags & TypeFlags.Intersection ? (source as IntersectionType).types: [source], !!(target.flags & TypeFlags.Union));
if (constraint) {
constraint = getNormalizedType(constraint, /*writing*/ false);
}
if (constraint && everyType(constraint, c => c !== source)) { // Skip comparison if expansion contains the source itself
// ...
}But opted to stick it into getEffectiveConstraintOfIntersection directly because there is only one call site because it was less complicated. I'm happy to do it more explicitly, of course.
(I think I was worried that someone would add a new call to this function and also forget to normalize it if needed, but maybe that's just not a concern, or nobody will ever add a new call?)
There was a problem hiding this comment.
The other option is maybe to move all of those argument expressions into getEffectiveConstraintOfIntersection like
function getEffectiveConstraintOfIntersection(source: Type, target: Type) {
const types = source.flags & TypeFlags.Intersection ? (source as IntersectionType).types: [source];
const targetIsUnion = !!(target.flags & TypeFlags.Union);
// ...
}And then move it into the relation closure to make it obvious that this should only be used in this one place and not for another purpose?
There was a problem hiding this comment.
FWIW the above is sort of what I'd prefer (accept source and target). Better yet, move getEffectiveConstraintOfIntersection down into relation to make it obvious what you are/aren't supposed to use it for, but the diff won't be followable.
There was a problem hiding this comment.
I've moved the comment per Daniel's comment.
DanielRosenwasser
approved these changes
Jul 25, 2022
|
@typescript-bot pack this |
|
The bot's been acting up recently and doesn't seem to respond half the time. Most of those are run above, if you want to see the results. I'm happy to shift the code around if you want, of course. |
|
Oh, no, the perf test absolutely did not run earlier, right. |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 73c6219. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 73c6219. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here they are:
CompilerComparison Report - main..49956
System
Hosts
Scenarios
TSServerComparison Report - main..49956
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Perf looks the same to my eye. Any opinions on where I do the normalization? Hoping to get this one in sooner rather than later just because it's causing so many problems in things like zod (and how bad it is to be allowing assignments of type variables bound by aliases to any union type). |
sandersn
approved these changes
Jul 27, 2022
There was a problem hiding this comment.
Looks good to me, but @weswigham or @ahejlsberg should take a look too.
ahejlsberg
approved these changes
Jul 27, 2022
|
Alright, I'll leave this as-is, then, with Daniel's comment move suggestion. |
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.
Fixes #49937
During relation checking, we end up with a source type of
TableClass<any> & T. Because it's an intersection, we try relating the result ofgetEffectiveConstraintOfIntersection(source)againsttargetto see if that succeeds.getEffectiveConstraintOfIntersectionproduces an intersection of types and their contraints (the source type in this example is one such intersection). However, #49119 changedgetConstraintOfTypesuch thatgetConstraintOfType(T)isTable, notTableClass<any>, which I believe is correct.But the caller of
getEffectiveConstraintOfIntersectionchecks if the result contains the source type by exact equality. Since the source by this point has been normalized, equality will fail as the constraint is not yet normalized (which in this case would replaceTablewithTableClass<any>.So, make sure that constraint is normalized.