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
Allow (non-assert) type predicates to narrow by discriminant #57358
Conversation
@typescript-bot run DT |
Heya @gabritto, I've started to run the regular perf test suite on this PR at 24cc86a. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at 24cc86a. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at 24cc86a. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at 24cc86a. You can monitor the build here. Update: The results are in! |
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Hey @gabritto, the results of running the DT tests are ready. |
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
if ( | ||
isOrContainsMatchingReference(reference, argument) | ||
|| optionalChainContainsReference(argument, reference) | ||
|| isAccessExpression(argument) && isMatchingReference(reference, argument.expression) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to getDiscriminantPropertyAccess(argument, type)
so the argument only matches if it has an actual discriminable property? AFAIK, hasMatchingArgument
is only used in two places, so specializing it a bit more for this usecase is probably fine. Technically getDiscriminantPropertyAccess
can also match binding patterns and identifiers aliasing discriminable properties, too, eg,
const kind = fruid.kind;
if (isOneOf(kind, ['apple', 'banana'] as const)) {
fruit.kind
fruit
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll end up calling getDiscriminantPropertyAccess
twice, but I'll try doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know what, at this rate, if we need to call getDiscriminantPropertyAccess
to decide whether we should call narrowTypeByTypePredicate
, which will then possibly call getDiscriminantPropertyAccess
(and also call isMatchingReference
anyways), then we might as well just always call narrowTypeByTypePredicate
? I'll try that and see if it affects performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, getting rid of the hasMatchingArgument
inside narrowTypeByCallExpression
has proved disastrous: it causes a ton of errors, which look completely unrelated to narrowing. From debugging the pyright errors listed by the user tests run, I think what happens is that, inside narrowTypeByCallExpression
, if we call getEffectsSignature(callExpression)
, we now go type checking a bunch of things, causing circularities that then cause a ton of errors.
So it looks like we do need to protect the call to getEffectsSignature(...)
with a check of hasMatchingArgument(...)
, to avoid running into unnecessary circularities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weswigham I've switched to using getCandidateDiscriminantPropertyAccess
for the above reasons.
Can you take a look again?
@typescript-bot pack this |
Hey @gabritto, 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 |
@typescript-bot run DT |
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at 511c333. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at 511c333. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at 511c333. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the regular perf test suite on this PR at 511c333. You can monitor the build here. Update: The results are in! |
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Hey @gabritto, the results of running the DT tests are ready. |
@gabritto Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
@typescript-bot pack this |
Hey @gabritto, 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 |
@typescript-bot run DT |
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at ad62d2c. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at ad62d2c. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the regular perf test suite on this PR at ad62d2c. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at ad62d2c. You can monitor the build here. Update: The results are in! |
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Hey @gabritto, the results of running the DT tests are ready. |
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
@typescript-bot perf test this faster |
Heya @gabritto, I've started to run the faster perf test suite on this PR at ad62d2c. You can monitor the build here. Update: The results are in! |
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Fixes #45770.
In the example:
In
narrowTypeByCallExpression
when the reference to be narrowed isfruit
and the call expression isisOneOf(fruit.kind, ['apple', 'banana'] as const)
, we were simply not doing the narrowing because we though none of the arguments of the call matched the reference. Now, we see that argumentfruit.kind
matchesfruit
.Note that cases like this one still won't be narrowed:
because when matching the reference
what
to the arguments ofisA(what.kind1.kind2)
, we only check one level of access expression, so we only check ifwhat
matcheswhat.kind1
, which it doesn't.But this behavior is on par with how narrowing by discriminant property already works, including for the assert version of the example above (see, for instance, how
getCandidateDiscriminantPropertyAccess
is implemented).