-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Improve checking of in
operator
#50666
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
Conversation
# Conflicts: # src/compiler/checker.ts
@typescript-bot test this |
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at d178ea4. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at d178ea4. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at d178ea4. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at d178ea4. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at d178ea4. You can monitor the build here. Update: The results are in! |
@ahejlsberg Here are the results of running the user test suite comparing Something interesting changed - please have a look. Details
|
@ahejlsberg Here they are:Comparison Report - main..50666
System
Hosts
Scenarios
Developer Information: |
Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
@ahejlsberg Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
Hi, |
@marekdedic Sorry, I didn't originally notice the existing PR. Yes, I'd say this one is an improvement in that it tightens the checking of the individual operands (e.g. disallowing unconstrained generic types), it removes dubious exceptions for |
The user code test suite has two changes: One is a better error message, the other is an error that goes away because we now reflect an The RWC test suite has a few new errors due to No changes in Definitely Typed tests. The top 100 tests are catching new errors in Performance is unchanged. |
One question I'm looking for feedback on is this: function foo1(x: {}) {
if ("a" in x) {
x; // Record<"a", unknown>
}
} The tightened operand validation rules requires the right hand operand to be of a type that is assignable to The issue above is particularly unfortunate in function foo(x: unknown) {
if (x && "a" in x) {
x; // Record<"a", unknown>
}
} The truthy check narrows I'm leaning towards closing this hole, though it may cause additional breaks. EDIT: We now check that the type of the right operand isn't |
@typescript-bot pack this |
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 5d31ba8. You can monitor the build here. |
Hey @andrewbranch, 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 |
For me the problem got fixed when updating to TypeScript 4.9. I also made a video about this improvement: https://www.youtube.com/watch?v=oPkthSFxAHc 🎬 |
This PR improves checking of the
in
operator:key in obj
, we now require the type ofkey
to be assignable tostring | number | symbol
. Previously we allowedkey
to be of an unconstrained type parameter type.key in obj
, we now require the type ofobj
to be assignable toobject
. Previously we allowedobj
to be of an unconstrained generic type (which might be a primitive type, causing an exception to be thrown at runtime).key in obj
, wherekey
is of a string literal, numeric literal, or unique symbol type, narrowsobj
as follows:key
names a property that exists in some constituent of the type ofobj
, the type ofobj
is narrowed based on the presence or absence of that property. Previously we only did this for string literal keys.key
names a property that doesn't exist in any constituent of the type ofobj
, the type ofobj
is narrowed by intersecting withRecord<typeof key, unknown>
. Previously we did nothing in this case.Some examples:
I'm marking this PR as fixing #21732 even though it doesn't address the case of
key in obj
wherekey
is of some generic type. In general, whenkey
is of some non-finite type (likestring
), the only effect we can meaningfully reflect following akey in obj
check is thatobj[key]
should be a valid expression (of typeunknown
), providedkey
andobj
are both known not to have been modified since the check. This might be possible to do in the true branch of anif
statement or a ternary operator, but it isn't possible in all control flow scenarios.This PR is a breaking change because of the more accurate checking of the
in
operands and the more consistent narrowing of the right hand operand in control flow analysis.Fixes #21732.
Fixes #50639.