Use evaluator for isolatedModules enum restrictions by andrewbranch · Pull Request #57966 · microsoft/TypeScript

Andrew Branch (andrewbranch) · 2024-03-27T18:17:01Z

With #57914, it becomes possible to enable isolatedModules in our own codebase. I tried this, but there were a few inconvenient changes required to enum declarations due to #56736. You can see them here: https://github.com/andrewbranch/TypeScript/compare/bug/37774...andrewbranch:TypeScript:enable-isolatedModules?expand=1

The checks added in #56736 seem to be a bit too restrictive. Specifically, references to same-file constants and other enum members were not recognized. I refactored the evaluator to return extra information that lets us produce the same errors with higher fidelity to the capabilities of the evaluator. It seems that transpilers (tested esbuild and Babel) have built their behavior around what our own evaluator can do; they’re just obviously limited to analyzing a single file (when not bundling). So, this PR removes isolatedModules errors from a lot of code that transpiles perfectly fine.

(cc Jan Kühle (@frigus02))

Jake Bailey (jakebailey) · 2024-03-27T18:17:38Z

The worst PR race; we just merged #57955 which moved this code.

Andrew Branch (andrewbranch) · 2024-03-27T22:56:06Z

TypeScript Bot (@typescript-bot) test it

TypeScript Bot (typescript-bot) · 2024-03-27T22:56:10Z

Starting jobs; this comment will be updated as builds start and complete.

Command	Status	Results
`test top400`	✅ Started	✅ Results
`user test this`	✅ Started	✅ Results
`run dt`	✅ Started	✅ Results
`perf test this faster`	✅ Started	👀 Results

TypeScript Bot (typescript-bot) · 2024-03-27T23:20:36Z

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure Daniel Rosenwasser (@DanielRosenwasser) and Ryan Cavanaugh (@RyanCavanaugh) are aware of the changes, just as a heads up.

Jake Bailey (jakebailey) · 2024-03-27T23:21:32Z

Uh oh, the last merge just broke the PR a lot

Andrew Branch (andrewbranch) · 2024-03-27T23:21:47Z

Yeah I was not on the branch I thought I was on 🤦🏻‍♂️

TypeScript Bot (typescript-bot) · 2024-03-27T23:26:57Z

Hey Andrew Branch (@andrewbranch), the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

TypeScript Bot (typescript-bot) · 2024-03-27T23:35:06Z

Andrew Branch (@andrewbranch) Here are the results of running the user tests comparing main and refs/pull/57966/merge:

Everything looks good!

TypeScript Bot (typescript-bot) · 2024-03-27T23:41:53Z

Andrew Branch (@andrewbranch)
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

_Metric	_baseline	_pr	_Delta	_Best	_Worst	_p-value
_{Angular - node (v18.15.0, x64)}
_{Memory used}	_{295,692k (± 0.01%)}	_{295,678k (± 0.00%)}	_~	_295,663k	_295,686k	_{p=0.936 n=6}
_{Parse Time}	_{2.66s (± 0.24%)}	_{2.66s (± 0.31%)}	_~	_2.65s	_2.67s	_{p=0.432 n=6}
_{Bind Time}	_{0.83s (± 0.62%)}	_{0.83s (± 0.00%)}	_~	_0.83s	_0.83s	_{p=0.174 n=6}
_{Check Time}	_{8.24s (± 0.26%)}	_{8.25s (± 0.13%)}	_~	_8.23s	_8.26s	_{p=1.000 n=6}
_{Emit Time}	_{7.02s (± 0.19%)}	_{7.05s (± 0.39%)}	_~	_7.00s	_7.07s	_{p=0.142 n=6}
_{Total Time}	_{18.76s (± 0.11%)}	_{18.78s (± 0.11%)}	_~	_18.75s	_18.81s	_{p=0.091 n=6}
_{Compiler-Unions - node (v18.15.0, x64)}
_{Memory used}	_{195,095k (± 0.68%)}	_{193,283k (± 0.97%)}	_~	_192,035k	_195,817k	_{p=0.230 n=6}
_{Parse Time}	_{1.65s (± 0.99%)}	_{1.66s (± 1.32%)}	_~	_1.64s	_1.70s	_{p=0.805 n=6}
_{Bind Time}	_{0.88s (± 1.18%)}	_{0.88s (± 1.33%)}	_~	_0.86s	_0.89s	_{p=0.801 n=6}
_{Check Time}	_{11.27s (± 0.47%)}	_{11.22s (± 0.37%)}	_~	_11.18s	_11.28s	_{p=0.107 n=6}
_{Emit Time}	_{3.16s (± 0.47%)}	_{3.13s (± 0.72%)}	_~	_3.11s	_3.17s	_{p=0.089 n=6}
_{Total Time}	_{16.96s (± 0.19%)}	_{16.89s (± 0.29%)}	_{-0.07s (- 0.40%)}	_16.83s	_16.98s	_{p=0.045 n=6}
_{Monaco - node (v18.15.0, x64)}
_{Memory used}	_{347,421k (± 0.01%)}	_{347,517k (± 0.00%)}	_{+96k (+ 0.03%)}	_347,492k	_347,537k	_{p=0.005 n=6}
_{Parse Time}	_{3.68s (± 1.37%)}	_{3.67s (± 0.72%)}	_~	_3.64s	_3.71s	_{p=1.000 n=6}
_{Bind Time}	_{1.38s (± 2.35%)}	_{1.38s (± 0.59%)}	_~	_1.37s	_1.39s	_{p=1.000 n=6}
_{Check Time}	_{10.23s (± 0.48%)}	_{10.24s (± 0.46%)}	_~	_10.16s	_10.29s	_{p=0.630 n=6}
_{Emit Time}	_{6.03s (± 0.49%)}	_{6.02s (± 0.60%)}	_~	_5.98s	_6.06s	_{p=0.627 n=6}
_{Total Time}	_{21.31s (± 0.27%)}	_{21.30s (± 0.25%)}	_~	_21.20s	_21.34s	_{p=0.809 n=6}
_{TFS - node (v18.15.0, x64)}
_{Memory used}	_{302,767k (± 0.01%)}	_{302,797k (± 0.01%)}	_{+30k (+ 0.01%)}	_302,761k	_302,812k	_{p=0.037 n=6}
_{Parse Time}	_{2.42s (± 0.85%)}	_{2.39s (± 0.89%)}	_{-0.04s (- 1.44%)}	_2.36s	_2.42s	_{p=0.034 n=6}
_{Bind Time}	_{1.23s (± 1.08%)}	_{1.19s (± 0.34%)}	_{🟩-0.04s (- 3.26%)}	_1.18s	_1.19s	_{p=0.003 n=6}
_{Check Time}	_{7.51s (± 0.40%)}	_{7.51s (± 0.22%)}	_~	_7.48s	_7.53s	_{p=0.871 n=6}
_{Emit Time}	_{4.26s (± 0.76%)}	_{4.26s (± 0.75%)}	_~	_4.22s	_4.31s	_{p=0.809 n=6}
_{Total Time}	_{15.42s (± 0.25%)}	_{15.35s (± 0.26%)}	_{-0.07s (- 0.48%)}	_15.30s	_15.41s	_{p=0.020 n=6}
_{material-ui - node (v18.15.0, x64)}
_{Memory used}	_{510,066k (± 0.01%)}	_{510,143k (± 0.01%)}	_{+77k (+ 0.02%)}	_510,078k	_510,231k	_{p=0.045 n=6}
_{Parse Time}	_{3.18s (± 0.44%)}	_{3.19s (± 0.32%)}	_~	_3.17s	_3.20s	_{p=0.410 n=6}
_{Bind Time}	_{1.18s (± 1.07%)}	_{1.18s (± 0.44%)}	_~	_1.18s	_1.19s	_{p=0.448 n=6}
_{Check Time}	_{20.56s (± 0.38%)}	_{20.56s (± 0.25%)}	_~	_20.46s	_20.60s	_{p=0.872 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{24.92s (± 0.34%)}	_{24.94s (± 0.20%)}	_~	_24.84s	_24.97s	_{p=0.415 n=6}
_{mui-docs - node (v18.15.0, x64)}
_{Memory used}	_{1,744,450k (± 0.00%)}	_{1,744,496k (± 0.00%)}	_{+46k (+ 0.00%)}	_1,744,448k	_1,744,544k	_{p=0.045 n=6}
_{Parse Time}	_{9.61s (± 0.60%)}	_{9.62s (± 0.73%)}	_~	_9.51s	_9.70s	_{p=0.748 n=6}
_{Bind Time}	_{3.47s (± 1.71%)}	_{3.48s (± 0.60%)}	_~	_3.44s	_3.50s	_{p=0.872 n=6}
_{Check Time}	_{82.18s (± 0.54%)}	_{81.78s (± 0.47%)}	_~	_81.43s	_82.48s	_{p=0.199 n=6}
_{Emit Time}	_{0.19s (± 0.00%)}	_{0.19s (± 4.22%)}	_~	_0.19s	_0.21s	_{p=0.405 n=6}
_{Total Time}	_{95.45s (± 0.49%)}	_{95.06s (± 0.43%)}	_~	_94.62s	_95.76s	_{p=0.199 n=6}
_{self-build-src - node (v18.15.0, x64)}
_{Memory used}	_{2,396,011k (± 0.04%)}	_{2,397,757k (± 0.03%)}	_{+1,746k (+ 0.07%)}	_2,396,700k	_2,398,381k	_{p=0.013 n=6}
_{Parse Time}	_{5.96s (± 0.72%)}	_{6.04s (± 0.79%)}	_{+0.07s (+ 1.26%)}	_5.99s	_6.10s	_{p=0.005 n=6}
_{Bind Time}	_{2.25s (± 0.83%)}	_{2.27s (± 0.87%)}	_~	_2.24s	_2.30s	_{p=0.568 n=6}
_{Check Time}	_{39.82s (± 0.42%)}	_{39.74s (± 0.26%)}	_~	_39.61s	_39.85s	_{p=0.521 n=6}
_{Emit Time}	_{3.15s (± 1.41%)}	_{3.17s (± 3.66%)}	_~	_3.07s	_3.39s	_{p=1.000 n=6}
_{Total Time}	_{51.22s (± 0.35%)}	_{51.24s (± 0.44%)}	_~	_51.00s	_51.64s	_{p=0.936 n=6}
_{self-compiler - node (v18.15.0, x64)}
_{Memory used}	_{415,832k (± 0.01%)}	_{416,010k (± 0.01%)}	_{+178k (+ 0.04%)}	_415,940k	_416,053k	_{p=0.005 n=6}
_{Parse Time}	_{3.34s (± 1.01%)}	_{3.33s (± 0.48%)}	_~	_3.31s	_3.35s	_{p=0.568 n=6}
_{Bind Time}	_{1.30s (± 0.94%)}	_{1.29s (± 0.69%)}	_~	_1.28s	_1.30s	_{p=0.445 n=6}
_{Check Time}	_{18.21s (± 0.25%)}	_{18.24s (± 0.32%)}	_~	_18.17s	_18.33s	_{p=0.463 n=6}
_{Emit Time}	_{1.36s (± 0.72%)}	_{1.36s (± 0.89%)}	_~	_1.34s	_1.37s	_{p=0.498 n=6}
_{Total Time}	_{24.21s (± 0.21%)}	_{24.21s (± 0.27%)}	_~	_24.11s	_24.31s	_{p=0.627 n=6}
_{vscode - node (v18.15.0, x64)}
_{Memory used}	_{2,896,320k (± 0.00%)}	_{2,896,637k (± 0.00%)}	_{+317k (+ 0.01%)}	_2,896,566k	_2,896,778k	_{p=0.005 n=6}
_{Parse Time}	_{15.95s (± 0.29%)}	_{15.96s (± 0.28%)}	_~	_15.91s	_16.04s	_{p=0.687 n=6}
_{Bind Time}	_{5.06s (± 0.59%)}	_{5.05s (± 0.69%)}	_~	_5.01s	_5.10s	_{p=0.808 n=6}
_{Check Time}	_{87.48s (± 0.43%)}	_{87.59s (± 0.49%)}	_~	_87.19s	_88.42s	_{p=0.936 n=6}
_{Emit Time}	_{23.77s (± 0.54%)}	_{23.69s (± 0.80%)}	_~	_23.43s	_23.90s	_{p=0.470 n=6}
_{Total Time}	_{132.26s (± 0.32%)}	_{132.30s (± 0.30%)}	_~	_131.83s	_133.03s	_{p=0.810 n=6}
_{webpack - node (v18.15.0, x64)}
_{Memory used}	_{408,950k (± 0.03%)}	_{408,855k (± 0.02%)}	_~	_408,752k	_409,039k	_{p=0.173 n=6}
_{Parse Time}	_{3.91s (± 0.65%)}	_{3.89s (± 0.73%)}	_~	_3.85s	_3.93s	_{p=0.197 n=6}
_{Bind Time}	_{1.69s (± 0.31%)}	_{1.68s (± 0.72%)}	_~	_1.66s	_1.69s	_{p=0.142 n=6}
_{Check Time}	_{16.81s (± 0.10%)}	_{16.81s (± 0.25%)}	_~	_16.74s	_16.85s	_{p=0.372 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{22.40s (± 0.16%)}	_{22.38s (± 0.13%)}	_~	_22.34s	_22.43s	_{p=0.293 n=6}
_{xstate - node (v18.15.0, x64)}
_{Memory used}	_{513,385k (± 0.00%)}	_{513,386k (± 0.01%)}	_~	_513,338k	_513,424k	_{p=0.936 n=6}
_{Parse Time}	_{3.28s (± 0.36%)}	_{3.27s (± 0.66%)}	_~	_3.23s	_3.29s	_{p=0.864 n=6}
_{Bind Time}	_{1.57s (± 0.52%)}	_{1.57s (± 0.52%)}	_~	_1.56s	_1.58s	_{p=1.000 n=6}
_{Check Time}	_{2.91s (± 0.29%)}	_{2.91s (± 0.52%)}	_~	_2.90s	_2.94s	_{p=0.503 n=6}
_{Emit Time}	_{0.07s (± 7.03%)}	_{0.07s (±11.12%)}	_~	_0.07s	_0.09s	_{p=0.752 n=6}
_{Total Time}	_{7.84s (± 0.24%)}	_{7.83s (± 0.22%)}	_~	_7.81s	_7.86s	_{p=0.469 n=6}

System info unknown

Hosts

node (v18.15.0, x64)

Scenarios

Angular - node (v18.15.0, x64)
Compiler-Unions - node (v18.15.0, x64)
Monaco - node (v18.15.0, x64)
TFS - node (v18.15.0, x64)
material-ui - node (v18.15.0, x64)
mui-docs - node (v18.15.0, x64)
self-build-src - node (v18.15.0, x64)
self-compiler - node (v18.15.0, x64)
vscode - node (v18.15.0, x64)
webpack - node (v18.15.0, x64)
xstate - node (v18.15.0, x64)

_Benchmark	_Name	_Iterations
_Current	_pr	₆
_Baseline	_baseline	₆

Developer Information:

Download Benchmarks

Jake Bailey (jakebailey) · 2024-03-28T00:04:09Z

@@ -5626,6 +5626,7 @@ export interface EmitResolver {
 isEntityNameVisible(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node): SymbolVisibilityResult;
 // Returns the constant value this property access resolves to, or 'undefined' for a non-constant
 getConstantValue(node: EnumMember | PropertyAccessExpression | ElementAccessExpression): string | number | undefined;


Does this still need to have EnumMember?

Probably not, but the same signature is public API on TypeChecker so maybe it’s best not to change it?

Actually, declarations.ts still calls getConstantValue but only on an enum value, which makes me think that the resolver wouldn't need that func at all anymore. But, ts.ts's tryGetConstEnumValue still calls it for property access / element accesses :(

TypeScript Bot (typescript-bot) · 2024-03-28T00:31:30Z

Andrew Branch (@andrewbranch) Here are the results of running the top 400 repos comparing main and refs/pull/57966/merge:

Everything looks good!

Jake Bailey (jakebailey) · 2024-04-02T17:24:14Z

 equateValues,
 escapeLeadingUnderscores,
 escapeString,
+ type EvaluatorResult,


This is a nit but we haven't really used type imports anywhere else quite yet (besides that protocol.ts blunder); I don't think we've set the settings in settings.json yet that would allow sorting to be inline either... Maybe we just avoid type imports until we can switch them all?

~~Fine with me, but this is what auto-imports did automatically 😬~~ My bad, that’s my own setting; I guess I was testing it a while back. It works!

Jake Bailey (jakebailey)

This seems good to me, though I'm still hoping that we can reduce the EmitResolver API surface since there are so few calls to the old API left...

Maybe Wesley Wigham (@weswigham)Titian Cernicova-Dragomir (@dragomirtitian) have some feedback on that front just from needing to implement that resolver in a minimal fashion, but given this code all exists and works via the existing evaluator that was just pulled out, I doubt this is really a big deal.

Titian Cernicova-Dragomir (dragomirtitian) · 2024-04-03T13:17:00Z

Maybe Wesley Wigham (@weswigham)Titian Cernicova-Dragomir (@dragomirtitian) have some feedback on that front just from needing to implement that resolver in a minimal fashion.

This looks ok to me from the point of view of creating a minimalist resolver.

Use evaluator for isolatedModules enum restrictions
b90b2d8

TypeScript Bot (typescript-bot) assigned Andrew Branch (andrewbranch)Mar 27, 2024

TypeScript Bot (typescript-bot) added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 27, 2024

Andrew Branch (andrewbranch) added 4 commits March 27, 2024 15:14

Merge branch 'main' into bug/isolated-modules-enum-restrictions
79b5ff1

Roll back outer expression skipping
392976d

Merge branch 'main' into bug/isolated-modules-enum-restrictions
ed0ea55

Update test
2035fc6

Andrew Branch (andrewbranch)force-pushed the bug/isolated-modules-enum-restrictions branch from 2d4e834 to 2035fc6Compare March 27, 2024 23:21

Andrew Branch (andrewbranch) requested a review from Jake Bailey (jakebailey)March 27, 2024 23:49

Jake Bailey (jakebailey) reviewed Mar 28, 2024
View reviewed changes

Andrew Branch (andrewbranch) requested review from Jake Bailey (jakebailey) and Nathan Shively-Sanders (sandersn)April 2, 2024 15:50

Jake Bailey (jakebailey) reviewed Apr 2, 2024
View reviewed changes

Remove type-only ipmorts
4f69ba7

Jake Bailey (jakebailey) approved these changes Apr 2, 2024
View reviewed changes

Andrew Branch (andrewbranch) merged commit 9f8a231 into microsoft:mainApr 4, 2024

Andrew Branch (andrewbranch) deleted the bug/isolated-modules-enum-restrictions branch April 4, 2024 16:56

Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 16, 2025

Uh oh!

Conversation

Andrew Branch (andrewbranch) commented Mar 27, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jake Bailey (jakebailey) commented Mar 27, 2024

Uh oh!

Andrew Branch (andrewbranch) commented Mar 27, 2024

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 27, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 27, 2024

Uh oh!

Jake Bailey (jakebailey) commented Mar 27, 2024

Uh oh!

Andrew Branch (andrewbranch) commented Mar 27, 2024

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 27, 2024

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 27, 2024

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 27, 2024

tsc

Uh oh!

Jake Bailey (jakebailey)Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

Andrew Branch (andrewbranch)Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

Jake Bailey (jakebailey)Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 28, 2024

Uh oh!

Jake Bailey (jakebailey)Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

Andrew Branch (andrewbranch)Apr 2, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jake Bailey (jakebailey) left a comment

Choose a reason for hiding this comment

Uh oh!

Titian Cernicova-Dragomir (dragomirtitian) commented Apr 3, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Andrew Branch (andrewbranch) commented Mar 27, 2024•
edited
Loading

TypeScript Bot (typescript-bot) commented Mar 27, 2024•
edited
Loading

Andrew Branch (andrewbranch)Apr 2, 2024•
edited
Loading

Titian Cernicova-Dragomir (dragomirtitian) commented Apr 3, 2024•
edited
Loading