Skip to content

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

Merged
merged 17 commits into from
Sep 19, 2022
Merged

Improve checking of in operator #50666

merged 17 commits into from
Sep 19, 2022

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Sep 7, 2022

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.

@typescript-bot typescript-bot added Author: Team For Backlog Bug PRs that fix a backlog bug For Milestone Bug PRs that fix a bug with a specific milestone labels Sep 7, 2022
# Conflicts:
#	src/compiler/checker.ts
@ahejlsberg
Copy link
Member Author

@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
Copy link
Collaborator

typescript-bot commented Sep 7, 2022

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
Copy link
Collaborator

typescript-bot commented Sep 7, 2022

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
Copy link
Collaborator

typescript-bot commented Sep 7, 2022

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
Copy link
Collaborator

typescript-bot commented Sep 7, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 7, 2022

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 ahejlsberg added the Breaking Change Would introduce errors in existing code label Sep 7, 2022
@typescript-bot
Copy link
Collaborator

@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
Copy link
Collaborator

@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 Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × 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
Copy link
Collaborator

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
Copy link
Collaborator

@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-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-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-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-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-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-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/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/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-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.

JedWatson/react-select

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

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

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'.

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'.

@marekdedic
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link
Member Author

ahejlsberg commented Sep 7, 2022

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
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 7, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 7, 2022

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".;

@bennycode
Copy link
Contributor

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 🎬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Breaking Change Would introduce errors in existing code For Backlog Bug PRs that fix a backlog bug For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

in operator on T & object narrows to never Suggestion: treat in operator as type guard which asserts property existence