Properly handle `typeof this.xxx` in `isTypeParameterPossiblyReferenced` #54208

ahejlsberg · 2023-05-10T15:10:48Z

This PR fixes a defect in the logic introduced by #50070. Specifically, that logic wasn't properly handling type queries of the form typeof this.xxx.

Fixes #54167.

gabritto · 2023-05-10T18:21:49Z

src/compiler/checker.ts

-                        return some(firstIdentifierSymbol.declarations, idDecl => isNodeDescendantOf(idDecl, tpScope)) ||
-                            some((node as TypeQueryNode).typeArguments, containsReference);
+                    if (!isThisIdentifier(firstIdentifier)) { // Don't attempt to analyze typeof this.xxx
+                        const firstIdentifierSymbol = getResolvedSymbol(firstIdentifier);


Just out of curiosity, where/why do things go wrong if firstIdentifier is this? Is it in this call to getResolvedSymbol?

Yes, it goes wrong because this is never actually a symbol you can look up. In expressions, this isn't even an identifier, it's encoded as a ThisExpression AST node. However, a type query stores the dotted name as an EntityName, and in those this is just an identifier.

ahejlsberg · 2023-05-19T20:20:13Z

@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 · 2023-05-19T20:20:16Z

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

Update: The results are in!

typescript-bot · 2023-05-19T20:20:16Z

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

typescript-bot · 2023-05-19T20:20:16Z

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

Update: The results are in!

typescript-bot · 2023-05-19T20:20:16Z

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

Update: The results are in!

typescript-bot · 2023-05-19T20:20:16Z

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

Update: The results are in!

typescript-bot · 2023-05-19T20:44:17Z

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

There were infrastructure failures potentially unrelated to your change:

1 instance of "Unknown failure"
1 instance of "Package install failed"

Otherwise...

Everything looks good!

typescript-bot · 2023-05-19T21:27:40Z

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

Here they are:

Comparison Report - main..54208

_Metric	_main	₅₄₂₀₈	_Delta	_Best	_Worst	_p-value
_{Angular - node (v16.17.1, x64)}
_{Memory used}	_{365,135k (± 0.01%)}	_{365,097k (± 0.02%)}	_~	_365,017k	_365,220k	_{p=0.230 n=6}
_{Parse Time}	_{3.56s (± 0.42%)}	_{3.55s (± 0.41%)}	_~	_3.53s	_3.57s	_{p=0.564 n=6}
_{Bind Time}	_{1.18s (± 0.35%)}	_{1.18s (± 0.54%)}	_~	_1.17s	_1.19s	_{p=0.673 n=6}
_{Check Time}	_{9.56s (± 0.38%)}	_{9.61s (± 0.54%)}	_~	_9.53s	_9.68s	_{p=0.092 n=6}
_{Emit Time}	_{7.90s (± 0.42%)}	_{7.94s (± 0.73%)}	_~	_7.88s	_8.05s	_{p=0.145 n=6}
_{Total Time}	_{22.19s (± 0.21%)}	_{22.28s (± 0.29%)}	_{+0.09s (+ 0.40%)}	_22.19s	_22.39s	_{p=0.036 n=6}
_{Compiler-Unions - node (v16.17.1, x64)}
_{Memory used}	_{192,939k (± 0.03%)}	_{192,901k (± 0.02%)}	_~	_192,868k	_192,948k	_{p=0.298 n=6}
_{Parse Time}	_{1.59s (± 0.94%)}	_{1.59s (± 1.66%)}	_~	_1.56s	_1.63s	_{p=0.513 n=6}
_{Bind Time}	_{0.82s (± 0.63%)}	_{0.83s (± 0.99%)}	_~	_0.82s	_0.84s	_{p=0.523 n=6}
_{Check Time}	_{10.17s (± 0.83%)}	_{10.19s (± 0.72%)}	_~	_10.13s	_10.29s	_{p=0.810 n=6}
_{Emit Time}	_{3.00s (± 1.22%)}	_{2.98s (± 0.81%)}	_~	_2.94s	_3.01s	_{p=0.373 n=6}
_{Total Time}	_{15.59s (± 0.45%)}	_{15.59s (± 0.52%)}	_~	_15.51s	_15.70s	_{p=0.810 n=6}
_{Monaco - node (v16.17.1, x64)}
_{Memory used}	_{345,876k (± 0.00%)}	_{345,877k (± 0.00%)}	_~	_345,860k	_345,895k	_{p=1.000 n=6}
_{Parse Time}	_{2.72s (± 0.30%)}	_{2.73s (± 0.38%)}	_~	_2.71s	_2.74s	_{p=0.865 n=6}
_{Bind Time}	_{1.08s (± 0.38%)}	_{1.09s (± 0.47%)}	_~	_1.08s	_1.09s	_{p=0.112 n=6}
_{Check Time}	_{7.82s (± 0.32%)}	_{7.81s (± 0.47%)}	_~	_7.77s	_7.86s	_{p=0.809 n=6}
_{Emit Time}	_{4.43s (± 0.53%)}	_{4.46s (± 0.62%)}	_~	_4.44s	_4.51s	_{p=0.191 n=6}
_{Total Time}	_{16.06s (± 0.21%)}	_{16.08s (± 0.32%)}	_~	_16.04s	_16.18s	_{p=0.936 n=6}
_{TFS - node (v16.17.1, x64)}
_{Memory used}	_{299,962k (± 0.00%)}	_{299,954k (± 0.01%)}	_~	_299,929k	_299,988k	_{p=0.630 n=6}
_{Parse Time}	_{2.16s (± 0.49%)}	_{2.16s (± 0.38%)}	_~	_2.15s	_2.17s	_{p=0.865 n=6}
_{Bind Time}	_{1.24s (± 0.61%)}	_{1.24s (± 1.22%)}	_~	_1.22s	_1.25s	_{p=1.000 n=6}
_{Check Time}	_{7.29s (± 0.18%)}	_{7.28s (± 0.50%)}	_~	_7.22s	_7.33s	_{p=0.460 n=6}
_{Emit Time}	_{4.33s (± 0.74%)}	_{4.34s (± 1.26%)}	_~	_4.27s	_4.42s	_{p=0.936 n=6}
_{Total Time}	_{15.01s (± 0.28%)}	_{15.02s (± 0.50%)}	_~	_14.94s	_15.11s	_{p=1.000 n=6}
_{material-ui - node (v16.17.1, x64)}
_{Memory used}	_{480,938k (± 0.01%)}	_{480,936k (± 0.00%)}	_~	_480,910k	_480,966k	_{p=0.936 n=6}
_{Parse Time}	_{3.25s (± 0.19%)}	_{3.25s (± 0.63%)}	_~	_3.24s	_3.29s	_{p=0.498 n=6}
_{Bind Time}	_{0.94s (± 0.43%)}	_{0.94s (± 0.00%)}	_~	_0.94s	_0.94s	_{p=0.405 n=6}
_{Check Time}	_{17.86s (± 0.62%)}	_{17.92s (± 1.47%)}	_~	_17.68s	_18.37s	_{p=0.810 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{22.05s (± 0.53%)}	_{22.12s (± 1.20%)}	_~	_21.86s	_22.56s	_{p=0.872 n=6}
_{xstate - node (v16.17.1, x64)}
_{Memory used}	_{560,498k (± 0.02%)}	_{560,495k (± 0.02%)}	_~	_560,391k	_560,685k	_{p=0.810 n=6}
_{Parse Time}	_{4.01s (± 0.53%)}	_{3.99s (± 0.25%)}	_~	_3.98s	_4.00s	_{p=0.164 n=6}
_{Bind Time}	_{1.76s (± 0.23%)}	_{1.76s (± 0.43%)}	_~	_1.75s	_1.77s	_{p=0.389 n=6}
_{Check Time}	_{3.06s (± 1.04%)}	_{3.08s (± 0.86%)}	_~	_3.05s	_3.11s	_{p=0.295 n=6}
_{Emit Time}	_{0.09s (± 0.00%)}	_{0.09s (± 4.62%)}	_~	_0.08s	_0.09s	_{p=0.405 n=6}
_{Total Time}	_{8.92s (± 0.46%)}	_{8.93s (± 0.36%)}	_~	_8.90s	_8.98s	_{p=1.000 n=6}

System

_{Machine Name}	_ts-ci-ubuntu
_Platform	_{linux 5.4.0-148-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-05-19T22:05:53Z

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

Everything looks good!

typescript-bot · 2023-05-19T22:18:56Z

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

DanielRosenwasser · 2023-05-26T17:29:35Z

@typescript-bot cherry-pick this to release-5.1

typescript-bot · 2023-05-26T17:29:38Z

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-5.1 on this PR at e67193d. You can monitor the build here.

typescript-bot · 2023-05-26T17:36:29Z

Hey @DanielRosenwasser, I've opened #54413 for you.

Component commits: d3529bd Properly handle 'typeof this.xxx' in isTypeParameterPossiblyReferenced fda0a8f Add regression test e67193d Merge branch 'main' into fix54167

…e-5.1 (#54413) Co-authored-by: Anders Hejlsberg <andersh@microsoft.com>

ahejlsberg added 2 commits May 10, 2023 07:52

Properly handle 'typeof this.xxx' in isTypeParameterPossiblyReferenced

d3529bd

Add regression test

fda0a8f

typescript-bot assigned ahejlsberg May 10, 2023

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels May 10, 2023

ahejlsberg requested review from RyanCavanaugh and gabritto May 10, 2023 15:11

gabritto reviewed May 10, 2023

View reviewed changes

gabritto approved these changes May 10, 2023

View reviewed changes

sandersn added this to Not started in PR Backlog May 19, 2023

Merge branch 'main' into fix54167

e67193d

sandersn moved this from Not started to Needs merge in PR Backlog May 20, 2023

ahejlsberg merged commit 6f7704e into main May 20, 2023
20 checks passed

PR Backlog automation moved this from Needs merge to Done May 20, 2023

ahejlsberg deleted the fix54167 branch May 20, 2023 13:37

typescript-bot mentioned this pull request May 26, 2023

🤖 Pick PR #54208 (Properly handle typeof this.xxx i...) into release-5.1 #54413

Merged

DanielRosenwasser pushed a commit that referenced this pull request May 29, 2023

🤖 Pick PR #54208 (Properly handle typeof this.xxx i...) into releas…

898edb5

…e-5.1 (#54413) Co-authored-by: Anders Hejlsberg <andersh@microsoft.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly handle `typeof this.xxx` in `isTypeParameterPossiblyReferenced` #54208

Properly handle `typeof this.xxx` in `isTypeParameterPossiblyReferenced` #54208

ahejlsberg commented May 10, 2023

gabritto May 10, 2023

ahejlsberg May 10, 2023

ahejlsberg commented May 19, 2023

typescript-bot commented May 19, 2023 •

edited

typescript-bot commented May 19, 2023 •

edited

typescript-bot commented May 19, 2023 •

edited

typescript-bot commented May 19, 2023 •

edited

typescript-bot commented May 19, 2023 •

edited

typescript-bot commented May 19, 2023

typescript-bot commented May 19, 2023

typescript-bot commented May 19, 2023

typescript-bot commented May 19, 2023

DanielRosenwasser commented May 26, 2023

typescript-bot commented May 26, 2023 •

edited

typescript-bot commented May 26, 2023

Properly handle typeof this.xxx in isTypeParameterPossiblyReferenced #54208

Properly handle typeof this.xxx in isTypeParameterPossiblyReferenced #54208

Conversation

ahejlsberg commented May 10, 2023

gabritto May 10, 2023

Choose a reason for hiding this comment

ahejlsberg May 10, 2023

Choose a reason for hiding this comment

ahejlsberg commented May 19, 2023

typescript-bot commented May 19, 2023 • edited

typescript-bot commented May 19, 2023 • edited

typescript-bot commented May 19, 2023 • edited

typescript-bot commented May 19, 2023 • edited

typescript-bot commented May 19, 2023 • edited

typescript-bot commented May 19, 2023

typescript-bot commented May 19, 2023

typescript-bot commented May 19, 2023

typescript-bot commented May 19, 2023

DanielRosenwasser commented May 26, 2023

typescript-bot commented May 26, 2023 • edited

typescript-bot commented May 26, 2023

Properly handle `typeof this.xxx` in `isTypeParameterPossiblyReferenced` #54208

Properly handle `typeof this.xxx` in `isTypeParameterPossiblyReferenced` #54208

typescript-bot commented May 19, 2023 •

edited

typescript-bot commented May 19, 2023 •

edited

typescript-bot commented May 19, 2023 •

edited

typescript-bot commented May 19, 2023 •

edited

typescript-bot commented May 19, 2023 •

edited

typescript-bot commented May 26, 2023 •

edited