Disallow instantiation expressions on the right side of instanceof #53323

jakebailey · 2023-03-17T19:18:09Z

In reviewing #49863, we noticed that we don't emit any sort of error when you use an instantiation expression on the right side of an instanceof check. E.g. in the below code, <number> does nothing; you'll still get any.

declare class Box<T> {
    value: T;
}

declare const maybeBox: unknown;

if (maybeBox instanceof Box<number>) {
    maybeBox;                     // Still Box<any>!
    const value = maybeBox.value; // Still any!
}

Playground Link

This PR disallows this syntactically.

We could try and figure out how to do:

const BoxOfNumber = Box<number>;
if (maybeBox instanceof BoxOfNumber) {
    // ...
}

And that fix would look different.

jakebailey · 2023-03-17T19:18:36Z

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

typescript-bot · 2023-03-17T19:18:39Z

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

typescript-bot · 2023-03-17T19:18:39Z

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 3478a4c. You can monitor the build here.

typescript-bot · 2023-03-17T19:18:39Z

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

Update: The results are in!

typescript-bot · 2023-03-17T19:18:39Z

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 3478a4c. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-03-17T19:18:39Z

Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 3478a4c. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-03-17T19:18:39Z

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 3478a4c. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-03-17T19:20:19Z

Hey @jakebailey, 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/149831/artifacts?artifactName=tgz&fileId=40D5A8B8FC621449725D3AD2D13A3D623F46F5FB7CFE443F9B419D60A259FEBB02&fileName=/typescript-5.1.0-insiders.20230317.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@5.1.0-pr-53323-7".;

typescript-bot · 2023-03-17T19:31:16Z

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

Everything looks good!

src/compiler/checker.ts

jakebailey · 2023-03-17T19:45:24Z

tests/baselines/reference/instanceofOnInstantiationExpression.errors.txt

+                          ~~~~~~~~~~~
+!!! error TS2848: The right-hand side of an 'instanceof' expression must not be an instantiation expression.
+
+    Box<number> instanceof Object; // OK


I am checking only the right side here, but, should these even be legal? They seem useless in another way.

Is the instantiation expression pointless? Yes. Is it harmful? No. Is it misleading? Probably not really on the LHS, since the only reason I can think of why you'd want one on the LHS is to "satisfy" the RHS, but the RHS forbids them now, soooo...

Yeah. I don't really care either way. I guess this PR is fine as-is.

typescript-bot · 2023-03-17T20:19:22Z

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

Here they are:

Comparison Report - main..53323

_Metric	_main	₅₃₃₂₃	_Delta	_Best	_Worst	_p-value
_{Angular - node (v16.17.1, x64)}
_{Memory used}	_{360,905k (± 0.01%)}	_{360,935k (± 0.01%)}	_~	_360,886k	_360,966k	_{p=0.108 n=6}
_{Parse Time}	_{3.53s (± 0.85%)}	_{3.51s (± 0.69%)}	_~	_3.48s	_3.54s	_{p=0.517 n=6}
_{Bind Time}	_{1.19s (± 0.75%)}	_{1.19s (± 0.69%)}	_~	_1.18s	_1.20s	_{p=0.550 n=6}
_{Check Time}	_{9.46s (± 0.73%)}	_{9.49s (± 0.34%)}	_~	_9.45s	_9.54s	_{p=0.630 n=6}
_{Emit Time}	_{7.95s (± 1.06%)}	_{7.90s (± 0.53%)}	_~	_7.86s	_7.97s	_{p=0.296 n=6}
_{Total Time}	_{22.13s (± 0.76%)}	_{22.09s (± 0.24%)}	_~	_21.99s	_22.14s	_{p=0.873 n=6}
_{Compiler-Unions - node (v16.17.1, x64)}
_{Memory used}	_{193,116k (± 0.69%)}	_{193,130k (± 0.70%)}	_~	_192,437k	_195,882k	_{p=0.810 n=6}
_{Parse Time}	_{1.58s (± 1.37%)}	_{1.57s (± 1.36%)}	_~	_1.55s	_1.60s	_{p=0.219 n=6}
_{Bind Time}	_{0.82s (± 0.50%)}	_{0.82s (± 0.50%)}	_~	_0.82s	_0.83s	_{p=1.000 n=6}
_{Check Time}	_{10.13s (± 0.53%)}	_{10.06s (± 0.39%)}	_~	_10.01s	_10.10s	_{p=0.108 n=6}
_{Emit Time}	_{2.99s (± 0.40%)}	_{3.00s (± 0.68%)}	_~	_2.98s	_3.02s	_{p=0.738 n=6}
_{Total Time}	_{15.53s (± 0.45%)}	_{15.45s (± 0.39%)}	_~	_15.37s	_15.53s	_{p=0.065 n=6}
_{Monaco - node (v16.17.1, x64)}
_{Memory used}	_{345,438k (± 0.01%)}	_{345,444k (± 0.01%)}	_~	_345,414k	_345,462k	_{p=0.378 n=6}
_{Parse Time}	_{2.73s (± 0.64%)}	_{2.73s (± 0.36%)}	_~	_2.72s	_2.74s	_{p=0.677 n=6}
_{Bind Time}	_{1.09s (± 0.47%)}	_{1.09s (± 0.69%)}	_~	_1.08s	_1.10s	_{p=0.784 n=6}
_{Check Time}	_{7.68s (± 0.55%)}	_{7.68s (± 0.37%)}	_~	_7.63s	_7.71s	_{p=1.000 n=6}
_{Emit Time}	_{4.46s (± 0.49%)}	_{4.44s (± 0.68%)}	_~	_4.41s	_4.49s	_{p=0.326 n=6}
_{Total Time}	_{15.95s (± 0.48%)}	_{15.95s (± 0.32%)}	_~	_15.89s	_16.01s	_{p=1.000 n=6}
_{TFS - node (v16.17.1, x64)}
_{Memory used}	_{299,800k (± 0.01%)}	_{299,795k (± 0.01%)}	_~	_299,764k	_299,835k	_{p=0.810 n=6}
_{Parse Time}	_{2.17s (± 0.35%)}	_{2.17s (± 0.71%)}	_~	_2.15s	_2.19s	_{p=0.934 n=6}
_{Bind Time}	_{1.24s (± 1.11%)}	_{1.24s (± 1.21%)}	_~	_1.22s	_1.26s	_{p=0.315 n=6}
_{Check Time}	_{7.15s (± 0.37%)}	_{7.14s (± 0.40%)}	_~	_7.10s	_7.18s	_{p=0.686 n=6}
_{Emit Time}	_{4.34s (± 0.91%)}	_{4.33s (± 0.90%)}	_~	_4.29s	_4.40s	_{p=0.374 n=6}
_{Total Time}	_{14.90s (± 0.16%)}	_{14.88s (± 0.45%)}	_~	_14.78s	_14.96s	_{p=0.630 n=6}
_{material-ui - node (v16.17.1, x64)}
_{Memory used}	_{477,442k (± 0.01%)}	_{477,455k (± 0.02%)}	_~	_477,353k	_477,582k	_{p=1.000 n=6}
_{Parse Time}	_{3.22s (± 0.80%)}	_{3.22s (± 0.48%)}	_~	_3.19s	_3.23s	_{p=0.808 n=6}
_{Bind Time}	_{0.96s (± 1.43%)}	_{0.95s (± 0.88%)}	_~	_0.95s	_0.97s	_{p=0.931 n=6}
_{Check Time}	_{17.97s (± 0.33%)}	_{18.01s (± 0.50%)}	_~	_17.92s	_18.13s	_{p=0.628 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{22.15s (± 0.17%)}	_{22.19s (± 0.41%)}	_~	_22.11s	_22.32s	_{p=0.809 n=6}
_{xstate - node (v16.17.1, x64)}
_{Memory used}	_{550,757k (± 0.02%)}	_{550,794k (± 0.03%)}	_~	_550,647k	_551,028k	_{p=0.521 n=6}
_{Parse Time}	_{3.94s (± 0.26%)}	_{3.94s (± 0.30%)}	_~	_3.92s	_3.95s	_{p=0.806 n=6}
_{Bind Time}	_{1.80s (± 0.97%)}	_{1.81s (± 0.46%)}	_~	_1.79s	_1.81s	_{p=0.402 n=6}
_{Check Time}	_{3.04s (± 0.80%)}	_{3.04s (± 0.54%)}	_~	_3.02s	_3.06s	_{p=1.000 n=6}
_{Emit Time}	_{0.09s (± 0.00%)}	_{0.09s (± 0.00%)}	_~	_0.09s	_0.09s	_{p=1.000 n=6}
_{Total Time}	_{8.87s (± 0.35%)}	_{8.88s (± 0.20%)}	_~	_8.86s	_8.90s	_{p=0.685 n=6}

System

_{Machine Name}	_ts-ci-ubuntu
_Platform	_{linux 5.4.0-135-generic}
_Architecture	_x64
_{Available Memory}	_{16 GB}
_{Available Memory}	_{15 GB}
_CPUs	_{4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz}

Hosts

node (v16.17.1, x64)

Scenarios

Angular - node (v16.17.1, x64)
Compiler-Unions - node (v16.17.1, x64)
Monaco - node (v16.17.1, x64)
TFS - node (v16.17.1, x64)
material-ui - node (v16.17.1, x64)
xstate - node (v16.17.1, x64)

_Benchmark	_Name	_Iterations
_Current	₅₃₃₂₃	₆
_Baseline	_main	₆

Developer Information:

Download Benchmark

typescript-bot · 2023-03-17T21:25:17Z

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

Everything looks good!

typescript-bot · 2023-03-17T21:29:40Z

Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

jakebailey added 2 commits March 17, 2023 12:12

Add test

c5d76d6

Disallow instantiation expressions on the right side of instanceof

3478a4c

typescript-bot assigned jakebailey Mar 17, 2023

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 17, 2023

weswigham approved these changes Mar 17, 2023

View reviewed changes

Fix bad rebase

337071c

jakebailey commented Mar 17, 2023

View reviewed changes

src/compiler/checker.ts Show resolved Hide resolved

jakebailey commented Mar 17, 2023

View reviewed changes

jakebailey merged commit b7b0b52 into microsoft:main Mar 17, 2023

jakebailey deleted the instanceof-instantiation-expression branch March 17, 2023 22:32

JamieMagee mentioned this pull request Jun 13, 2023

chore(deps): update dependency typescript to v5.1.3 renovatebot/renovate#22641

Merged

1 task

dsherret mentioned this pull request Jul 10, 2023

TS2848 error (instanceof) on v1.35.0 denoland/deno#19780

Closed

Disallow instantiation expressions on the right side of instanceof #53323

Disallow instantiation expressions on the right side of instanceof #53323

Uh oh!

Conversation

jakebailey commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakebailey commented Mar 17, 2023

Uh oh!

typescript-bot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Mar 17, 2023

Uh oh!

Uh oh!

jakebailey Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

weswigham Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

jakebailey Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Mar 17, 2023

Uh oh!

typescript-bot commented Mar 17, 2023

Uh oh!

typescript-bot commented Mar 17, 2023

Uh oh!

Uh oh!

jakebailey commented Mar 17, 2023 •

edited

Loading

typescript-bot commented Mar 17, 2023 •

edited

Loading

typescript-bot commented Mar 17, 2023 •

edited

Loading

typescript-bot commented Mar 17, 2023 •

edited

Loading

typescript-bot commented Mar 17, 2023 •

edited

Loading

typescript-bot commented Mar 17, 2023 •

edited

Loading

typescript-bot commented Mar 17, 2023 •

edited

Loading

typescript-bot commented Mar 17, 2023 •

edited

Loading