Improve checking of in operator

[ahejlsberg]

This PR improves checking of the in operator:

• In the expression key in obj, we now require the type of key to be assignable to string | number | symbol. Previously we allowed key to be of an unconstrained type parameter type.
• In the expression key in obj, we now require the type of obj to be assignable to object. Previously we allowed obj to be of an unconstrained generic type (which might be a primitive type, causing an exception to be thrown at runtime).
• In control flow analysis, an expression key in obj, where key is of a string literal, numeric literal, or unique symbol type, narrows obj as follows:
  • If key names a property that exists in some constituent of the type of obj, the type of obj is narrowed based on the presence or absence of that property. Previously we only did this for string literal keys.
  • If key names a property that doesn't exist in any constituent of the type of obj, the type of obj is narrowed by intersecting with Record<typeof key, unknown>. Previously we did nothing in this case.

Some examples:

const sym = Symbol();

function f1(x: unknown) {
    if (x && typeof x === "object" && "a" in x && 1 in x && sym in x) {
        x;  // Record<"a", unknown> & Record<1, unknown> & Record<typeof sym, unknown>
        x.a;  // Ok
        x[1];  // Ok
        x[sym];  // Ok
    }
}

function f2(x: { a: string } | { b: string }) {
    if ("a" in x) {
        x;  // { a: string }
    }
    else if ("b" in x) {
        x;  // { b: string }
    }
    else {
        x;  // never
    }
}

I'm marking this PR as fixing #21732 even though it doesn't address the case of key in obj where key is of some generic type. In general, when key is of some non-finite type (like string), the only effect we can meaningfully reflect following a key in obj check is that obj[key] should be a valid expression (of type unknown), provided key and obj are both known not to have been modified since the check. This might be possible to do in the true branch of an if 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.

[ahejlsberg]

@typescript-bot test this
@typescript-bot user test this inline
@typescript-bot run dt
@typescript-bot perf test faster
@typescript-bot test top100

[typescript-bot]

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!

[typescript-bot]

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at d178ea4. You can monitor the build here.

[typescript-bot]

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!

[typescript-bot]

Heya @ahejlsberg, I've started to run the extended test suite on this PR at d178ea4. You can monitor the build here.

[typescript-bot]

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!

[typescript-bot]

@ahejlsberg Here are the results of running the user test suite comparing main and refs/pull/50666/merge:

Something interesting changed - please have a look.

Details

lodash

/mnt/ts_downloads/lodash/tsconfig.json

• [NEW] error TS2322: Type 'string | any[]' is not assignable to type 'string | number | symbol'.
  • /mnt/ts_downloads/lodash/node_modules/lodash/_baseHasIn.js(10,28)
• [MISSING] error TS2360: The left-hand side of an 'in' expression must be a private identifier or of type 'any', 'string', 'number', or 'symbol'.
  • /mnt/ts_downloads/lodash/node_modules/lodash/_baseHasIn.js(10,28)

uglify-js

/mnt/ts_downloads/uglify-js/tsconfig.json

• [MISSING] error TS2339: Property 'proto_value' does not exist on type 'never'.
  • /mnt/ts_downloads/uglify-js/node_modules/uglify-js/lib/utils.js(196,54)
  • /mnt/ts_downloads/uglify-js/node_modules/uglify-js/lib/utils.js(202,43)
  • /mnt/ts_downloads/uglify-js/node_modules/uglify-js/lib/utils.js(211,52)

[typescript-bot]

@ahejlsberg
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..50666

Metric	main	50666	Delta	Best	Worst
Angular - node (v14.15.1, x64)
Memory used	338,731k (± 0.00%)	338,745k (± 0.01%)	+14k (+ 0.00%)	338,713k	338,787k
Parse Time	2.07s (± 0.95%)	2.09s (± 0.86%)	+0.01s (+ 0.63%)	2.05s	2.12s
Bind Time	0.80s (± 0.62%)	0.79s (± 0.28%)	-0.01s (- 0.63%)	0.79s	0.80s
Check Time	5.84s (± 0.37%)	5.85s (± 0.41%)	+0.01s (+ 0.24%)	5.81s	5.92s
Emit Time	6.16s (± 0.57%)	6.16s (± 0.65%)	+0.00s (+ 0.03%)	6.10s	6.27s
Total Time	14.86s (± 0.28%)	14.89s (± 0.32%)	+0.03s (+ 0.20%)	14.79s	15.01s
Compiler-Unions - node (v14.15.1, x64)
Memory used	192,656k (± 0.01%)	192,653k (± 0.01%)	-3k (- 0.00%)	192,601k	192,730k
Parse Time	0.85s (± 0.35%)	0.85s (± 0.94%)	0.00s ( 0.00%)	0.84s	0.88s
Bind Time	0.49s (± 1.03%)	0.49s (± 0.61%)	+0.00s (+ 0.62%)	0.48s	0.49s
Check Time	6.72s (± 0.56%)	6.74s (± 0.43%)	+0.02s (+ 0.28%)	6.69s	6.84s
Emit Time	2.41s (± 0.96%)	2.43s (± 1.26%)	+0.01s (+ 0.46%)	2.39s	2.53s
Total Time	10.47s (± 0.53%)	10.51s (± 0.33%)	+0.03s (+ 0.33%)	10.45s	10.60s
Monaco - node (v14.15.1, x64)
Memory used	326,520k (± 0.01%)	326,528k (± 0.00%)	+9k (+ 0.00%)	326,498k	326,560k
Parse Time	1.57s (± 0.94%)	1.57s (± 0.53%)	+0.00s (+ 0.13%)	1.56s	1.60s
Bind Time	0.72s (± 0.50%)	0.73s (± 0.85%)	+0.00s (+ 0.69%)	0.72s	0.74s
Check Time	5.73s (± 0.51%)	5.70s (± 0.53%)	-0.03s (- 0.58%)	5.60s	5.75s
Emit Time	3.32s (± 0.59%)	3.31s (± 0.51%)	-0.00s (- 0.09%)	3.28s	3.35s
Total Time	11.35s (± 0.40%)	11.32s (± 0.43%)	-0.03s (- 0.25%)	11.18s	11.40s
TFS - node (v14.15.1, x64)
Memory used	289,654k (± 0.01%)	289,634k (± 0.01%)	-20k (- 0.01%)	289,556k	289,703k
Parse Time	1.31s (± 0.90%)	1.31s (± 0.44%)	-0.00s (- 0.15%)	1.30s	1.32s
Bind Time	0.79s (± 1.93%)	0.80s (± 0.62%)	+0.00s (+ 0.13%)	0.79s	0.81s
Check Time	5.35s (± 0.36%)	5.37s (± 0.60%)	+0.01s (+ 0.26%)	5.31s	5.43s
Emit Time	3.56s (± 0.63%)	3.56s (± 0.48%)	-0.00s (- 0.11%)	3.52s	3.59s
Total Time	11.02s (± 0.34%)	11.03s (± 0.42%)	+0.01s (+ 0.07%)	10.95s	11.12s
material-ui - node (v14.15.1, x64)
Memory used	436,279k (± 0.06%)	436,325k (± 0.06%)	+45k (+ 0.01%)	435,289k	436,478k
Parse Time	1.87s (± 0.52%)	1.87s (± 0.67%)	-0.00s (- 0.21%)	1.84s	1.89s
Bind Time	0.58s (± 1.15%)	0.58s (± 0.69%)	+0.00s (+ 0.17%)	0.57s	0.59s
Check Time	12.91s (± 0.52%)	12.90s (± 0.59%)	-0.01s (- 0.07%)	12.79s	13.10s
Emit Time	0.00s (± 0.00%)	0.00s (± 0.00%)	0.00s ( NaN%)	0.00s	0.00s
Total Time	15.36s (± 0.45%)	15.35s (± 0.52%)	-0.01s (- 0.05%)	15.21s	15.58s
xstate - node (v14.15.1, x64)
Memory used	546,732k (± 0.00%)	546,752k (± 0.00%)	+20k (+ 0.00%)	546,729k	546,773k
Parse Time	2.60s (± 0.27%)	2.60s (± 0.32%)	-0.00s (- 0.00%)	2.58s	2.62s
Bind Time	0.97s (± 1.09%)	0.97s (± 0.84%)	-0.01s (- 0.51%)	0.95s	0.99s
Check Time	1.52s (± 0.66%)	1.53s (± 0.75%)	+0.01s (+ 0.66%)	1.51s	1.56s
Emit Time	0.07s (± 3.14%)	0.07s (± 4.13%)	+0.00s (+ 1.41%)	0.07s	0.08s
Total Time	5.17s (± 0.36%)	5.17s (± 0.30%)	+0.00s (+ 0.04%)	5.15s	5.21s

System

Machine Name	ts-ci-ubuntu
Platform	linux 4.4.0-210-generic
Architecture	x64
Available Memory	16 GB
Available Memory	15 GB
CPUs	4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz

Hosts

• node (v14.15.1, x64)

Scenarios

• Angular - node (v14.15.1, x64)
• Compiler-Unions - node (v14.15.1, x64)
• Monaco - node (v14.15.1, x64)
• TFS - node (v14.15.1, x64)
• material-ui - node (v14.15.1, x64)
• xstate - node (v14.15.1, x64)

Benchmark	Name	Iterations
Current	50666	10
Baseline	main	10

Developer Information:

Download Benchmark

[typescript-bot]

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.

[typescript-bot]

@ahejlsberg Here are the results of running the top-repos suite comparing main and refs/pull/50666/merge:

Something interesting changed - please have a look.

Details

Eugeny/tabby

9 of 29 projects failed to build with the old tsc and were ignored

tabby-community-color-schemes/tsconfig.json

• error TS2352: Conversion of type 'MouseEvent & Record<"wheelDeltaY", unknown>' to type 'WheelEvent' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  • tabby-terminal/src/api/baseTerminalTab.component.ts#L672

tabby-electron/tsconfig.json

• error TS2352: Conversion of type 'MouseEvent & Record<"wheelDeltaY", unknown>' to type 'WheelEvent' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  • tabby-terminal/src/api/baseTerminalTab.component.ts#L672

tabby-linkifier/tsconfig.json

• error TS2352: Conversion of type 'MouseEvent & Record<"wheelDeltaY", unknown>' to type 'WheelEvent' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  • tabby-terminal/src/api/baseTerminalTab.component.ts#L672

tabby-local/tsconfig.json

• error TS2352: Conversion of type 'MouseEvent & Record<"wheelDeltaY", unknown>' to type 'WheelEvent' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  • tabby-terminal/src/api/baseTerminalTab.component.ts#L672

tabby-serial/tsconfig.json

• error TS2352: Conversion of type 'MouseEvent & Record<"wheelDeltaY", unknown>' to type 'WheelEvent' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  • tabby-terminal/src/api/baseTerminalTab.component.ts#L672

tabby-ssh/tsconfig.json

• error TS2352: Conversion of type 'MouseEvent & Record<"wheelDeltaY", unknown>' to type 'WheelEvent' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  • tabby-terminal/src/api/baseTerminalTab.component.ts#L672

tabby-telnet/tsconfig.json

• error TS2352: Conversion of type 'MouseEvent & Record<"wheelDeltaY", unknown>' to type 'WheelEvent' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  • tabby-terminal/src/api/baseTerminalTab.component.ts#L672

tabby-terminal/tsconfig.json

• error TS2352: Conversion of type 'MouseEvent & Record<"wheelDeltaY", unknown>' to type 'WheelEvent' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  • tabby-terminal/src/api/baseTerminalTab.component.ts#L672

tabby-terminal/tsconfig.typings.json

• error TS2352: Conversion of type 'MouseEvent & Record<"wheelDeltaY", unknown>' to type 'WheelEvent' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  • tabby-terminal/src/api/baseTerminalTab.component.ts#L672

tabby-web-demo/tsconfig.json

• error TS2352: Conversion of type 'MouseEvent & Record<"wheelDeltaY", unknown>' to type 'WheelEvent' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  • tabby-terminal/src/api/baseTerminalTab.component.ts#L672

JedWatson/react-select

3 of 5 projects failed to build with the old tsc and were ignored

packages/react-select/src/tests/tsconfig.json

• error TS2322: Type 'Option | Group' is not assignable to type 'object'.
  • packages/react-select/src/Select.tsx#L382

react-bootstrap/react-bootstrap

test/tsconfig.json

• error TS2345: Argument of type '(Element & Record<"setState", unknown>) | (ComponentClass<{}, any> & Record<"setState", unknown>)' is not assignable to parameter of type 'ReactInstance | null | undefined'.
  • src/safeFindDOMNode.ts#L7

tsconfig.json

• error TS2345: Argument of type '(Element & Record<"setState", unknown>) | (ComponentClass<{}, any> & Record<"setState", unknown>)' is not assignable to parameter of type 'ReactInstance | null | undefined'.
  • src/safeFindDOMNode.ts#L7

[marekdedic]

Hi,
I see that you opened this PR even though #46403 is waiting to be merged - is this an improvement over that PR? In that case I'd be glad to close my PR in favor of a better solution...

[ahejlsberg]

@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 this types and differences between object types and equivalent intersections types, and it ensures better caching of equivalent types by using instantiations of Record<K, T> instead of creating anonymous types on the fly. Still, I appreciate the work and time you put into your PR. I'll see if I can perhaps reuse some of your tests.

[ahejlsberg]

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 in check by intersecting with Record<K, T>.

The RWC test suite has a few new errors due to in being applied to unconstrained type parameters (a good change). There is also one suspicious error in RxJS where we now correctly narrow to never because the types indicate that the false branch will never happen. The code tests an XMLHttpRequest for the "response" property, which is declared as a non-optional property and therefore should always be present. Apparently there are corner cases with old IE versions where it isn't.

No changes in Definitely Typed tests.

The top 100 tests are catching new errors in react-select and react-bootstrap (a good change). There are also new errors in several tabby-* packages. They're due to fact that intersecting with Record<K, T> creates a type that is sufficiently different for an as type assertion to become an error. It's unfortunate, but acceptable I think.

Performance is unchanged.

[ahejlsberg]

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 object. Unfortunately, {} is (unsoundly) assignable to object, even when the {} doesn't originate in a fresh object literal. A non-fresh {} can be any type except null or undefined. This includes primitive types, which, as right hand operands, cause the in operator to throw an exception. We have previously rejected fixing the {} assignable to object issue due to breaking change concerns, but we could conceivably error for a right hand {} type with the in operator.

The issue above is particularly unfortunate in --strictNullChecks mode because the the following doesn't error:

function foo(x: unknown) {
    if (x && "a" in x) {
        x;  // Record<"a", unknown>
    }
}

The truthy check narrows x from unknown to {} and we then permit the in check when x could actually be a primitive type that raises an exception. The check needs to be written x && typeof x === "object" && "a" in x to actually be safe.

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 {} or a generic type constrained to {}.

[andrewbranch]

@typescript-bot pack this

[typescript-bot]

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 5d31ba8. You can monitor the build here.

[typescript-bot]

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/133713/artifacts?artifactName=tgz&fileId=54CABE28ADD269CF2A8342181BB7D0A007C312FDBC645CDDB0DA60006E12597902&fileName=/typescript-4.9.0-insiders.20220907.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.9.0-pr-50666-16".;

[ahejlsberg]

@bradzacher While I agree that your example is very likely a mistake, it obviously is possible that someone has a Map object with some additional properties that they want to test for. So, technically it is a valid scenario. Again, agree that it's highly likely to be an error, but perhaps better covered by a linter?

[bradzacher]

@ahejlsberg yeah a lint rule makes sense that way users can opt-in easily. Filed typescript-eslint/typescript-eslint#5677

[reverofevil]

Sorry, but what about #43284? :(

[andreialecu]

Does this also fix: #43614 ?

Update: just tested the playground and it seems it does not fix the problem above.

[Andarist]

@andreialecu no, it doesn't: nightly TS playground. I had no idea that you can write an invariant this way in TS 😱

[bennycode]

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 🎬