Fix isolatedModules check for export assignments #57148

andrewbranch · 2024-01-23T22:22:31Z

Originally added in #55097, this was missing both the non-ambient check and a check that the resolved symbol has no value meaning:

We only issue isolatedModules errors on non-ambient code, because the goal of all isolatedModules errors to ensure that code can be transpiled to JS. Ambient code produces no JS, so it’s exempt from checks.
These errors are only supposed to occur when some re-export (events in the test and linked issue) resolves to something that doesn’t really exist at runtime (i.e., something that’s only a type, or something whose value meaning comes from a type-only import/export). The point is it needs to be easy for a transpiler to tell whether the re-export should be emitted or not—if it resolves to something real, it can and must be emitted; otherwise, it must be marked as type-only somewhere in the file so it can be removed.

To try to determine if events resolved to anything real, the implementation was only checking that the re-export resolves to something with a type meaning before issuing an error. But events has both a value meaning and a type meaning (since it’s a namespace that contains one value and two types). So the implementation needed to check that the resolved symbol doesn’t also have a value meaning in order to issue an error.

andrewbranch · 2024-01-23T22:36:25Z

src/compiler/checker.ts

                    if (
                        sym.flags & SymbolFlags.Alias
-                        && resolveAlias(sym) !== unknownSymbol


unknownSymbol.flags === SymbolFlags.All, so while this check was necessary before, it’s now redundant with !(nonLocalMeanings & SymbolFlags.Value).

They're actually equal to All? I thought that it was SymbolFlags.Property for this symbol.

Huh, oops, you’re right. At any rate, it’s still redundant with !(nonLocalMeanings & SymbolFlags.Value), but I’m glad to fix that memory

jakebailey · 2024-01-23T22:39:58Z

tests/cases/compiler/isolatedModulesReExportAlias.ts

+export = events;
+
+// @Filename: /boo.ts
+// Bad test runner (ignores stuff when last file contains the string r-e-q-u-i-r-e)


I am definitely curious about this one... 😄

The absolute worst

TypeScript/src/testRunner/compilerRunner.ts

Lines 238 to 242 in 25a4f9e

// We need to assemble the list of input files for the compiler and other related files on the 'filesystem' (ie in a multi-file test)

// If the last file in a test uses require or a triple slash reference we'll assume all other files will be brought in via references,

// otherwise, assume all files are just meant to be in the same compilation session without explicit references to one another.

if (testCaseContent.settings.noImplicitReferences || /require\(/.test(lastUnit.content) || /reference\spath/.test(lastUnit.content)) {

Wow, this is bogus

jakebailey · 2024-01-23T22:41:06Z

src/compiler/checker.ts

                    if (
                        sym.flags & SymbolFlags.Alias
-                        && resolveAlias(sym) !== unknownSymbol


They're actually equal to All? I thought that it was SymbolFlags.Property for this symbol.

jakebailey · 2024-01-23T22:41:41Z

@typescript-bot test top200
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot perf test this faster

typescript-bot · 2024-01-23T22:41:44Z

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

Update: The results are in!

typescript-bot · 2024-01-23T22:41:44Z

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

Update: The results are in!

typescript-bot · 2024-01-23T22:41:44Z

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

Update: The results are in!

typescript-bot · 2024-01-23T22:41:44Z

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

Update: The results are in!

typescript-bot · 2024-01-23T23:02:57Z

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

There were infrastructure failures potentially unrelated to your change:

1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

[MISSING] error TS2307: Cannot find module '@pptr/testserver' or its corresponding type declarations.
- packages/browsers/test/src/utils.ts#L13 in packages/browsers/test/src/tsconfig.json

typescript-bot · 2024-01-23T23:11:16Z

@jakebailey
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,649k (± 0.01%)}	_{295,658k (± 0.01%)}	_~	_295,608k	_295,693k	_{p=0.470 n=6}
_{Parse Time}	_{2.66s (± 0.28%)}	_{2.66s (± 0.50%)}	_~	_2.64s	_2.68s	_{p=0.548 n=6}
_{Bind Time}	_{0.83s (± 1.25%)}	_{0.83s (± 0.62%)}	_~	_0.82s	_0.83s	_{p=0.142 n=6}
_{Check Time}	_{8.19s (± 0.25%)}	_{8.19s (± 0.47%)}	_~	_8.15s	_8.24s	_{p=0.466 n=6}
_{Emit Time}	_{7.10s (± 0.24%)}	_{7.10s (± 0.19%)}	_~	_7.08s	_7.11s	_{p=0.676 n=6}
_{Total Time}	_{18.78s (± 0.16%)}	_{18.78s (± 0.24%)}	_~	_18.73s	_18.85s	_{p=0.935 n=6}
_{Compiler-Unions - node (v18.15.0, x64)}
_{Memory used}	_{191,969k (± 0.55%)}	_{192,552k (± 1.26%)}	_~	_191,485k	_197,489k	_{p=0.689 n=6}
_{Parse Time}	_{1.37s (± 1.07%)}	_{1.36s (± 1.21%)}	_~	_1.33s	_1.38s	_{p=0.120 n=6}
_{Bind Time}	_{0.72s (± 0.00%)}	_{0.72s (± 0.00%)}	_~	_0.72s	_0.72s	_{p=1.000 n=6}
_{Check Time}	_{9.35s (± 0.59%)}	_{9.34s (± 0.34%)}	_~	_9.29s	_9.38s	_{p=0.747 n=6}
_{Emit Time}	_{2.62s (± 0.58%)}	_{2.63s (± 0.59%)}	_~	_2.61s	_2.65s	_{p=0.166 n=6}
_{Total Time}	_{14.05s (± 0.31%)}	_{14.05s (± 0.18%)}	_~	_14.01s	_14.08s	_{p=0.808 n=6}
_{Monaco - node (v18.15.0, x64)}
_{Memory used}	_{347,460k (± 0.00%)}	_{347,466k (± 0.00%)}	_~	_347,436k	_347,483k	_{p=0.521 n=6}
_{Parse Time}	_{2.48s (± 0.59%)}	_{2.48s (± 0.47%)}	_~	_2.47s	_2.50s	_{p=1.000 n=6}
_{Bind Time}	_{0.93s (± 0.56%)}	_{0.93s (± 0.68%)}	_~	_0.92s	_0.94s	_{p=0.386 n=6}
_{Check Time}	_{6.94s (± 0.58%)}	_{6.93s (± 0.22%)}	_~	_6.91s	_6.95s	_{p=0.571 n=6}
_{Emit Time}	_{4.05s (± 0.41%)}	_{4.06s (± 0.48%)}	_~	_4.04s	_4.09s	_{p=0.415 n=6}
_{Total Time}	_{14.39s (± 0.25%)}	_{14.40s (± 0.19%)}	_~	_14.37s	_14.45s	_{p=0.807 n=6}
_{TFS - node (v18.15.0, x64)}
_{Memory used}	_{302,839k (± 0.01%)}	_{302,842k (± 0.01%)}	_~	_302,819k	_302,896k	_{p=0.810 n=6}
_{Parse Time}	_{2.02s (± 0.85%)}	_{2.01s (± 0.81%)}	_~	_1.99s	_2.03s	_{p=0.287 n=6}
_{Bind Time}	_{1.01s (± 0.81%)}	_{1.00s (± 1.03%)}	_~	_0.99s	_1.02s	_{p=0.546 n=6}
_{Check Time}	_{6.33s (± 0.54%)}	_{6.32s (± 0.34%)}	_~	_6.29s	_6.35s	_{p=0.808 n=6}
_{Emit Time}	_{3.59s (± 0.73%)}	_{3.62s (± 0.45%)}	_~	_3.60s	_3.64s	_{p=0.143 n=6}
_{Total Time}	_{12.94s (± 0.29%)}	_{12.94s (± 0.37%)}	_~	_12.87s	_13.00s	_{p=1.000 n=6}
_{material-ui - node (v18.15.0, x64)}
_{Memory used}	_{511,306k (± 0.00%)}	_{511,277k (± 0.00%)}	_~	_511,230k	_511,301k	_{p=0.054 n=6}
_{Parse Time}	_{2.65s (± 0.60%)}	_{2.65s (± 0.86%)}	_~	_2.63s	_2.68s	_{p=0.870 n=6}
_{Bind Time}	_{1.00s (± 0.75%)}	_{1.00s (± 0.84%)}	_~	_0.98s	_1.00s	_{p=0.652 n=6}
_{Check Time}	_{17.19s (± 0.21%)}	_{17.22s (± 0.45%)}	_~	_17.12s	_17.31s	_{p=0.574 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{20.83s (± 0.21%)}	_{20.86s (± 0.27%)}	_~	_20.78s	_20.92s	_{p=0.520 n=6}
_{mui-docs - node (v18.15.0, x64)}
_{Memory used}	_{1,696,405k (± 0.00%)}	_{1,696,413k (± 0.00%)}	_~	_1,696,353k	_1,696,438k	_{p=0.575 n=6}
_{Parse Time}	_{6.54s (± 0.23%)}	_{6.55s (± 0.66%)}	_~	_6.47s	_6.59s	_{p=0.462 n=6}
_{Bind Time}	_{2.36s (± 0.82%)}	_{2.36s (± 0.73%)}	_~	_2.34s	_2.39s	_{p=0.744 n=6}
_{Check Time}	_{55.59s (± 0.50%)}	_{55.45s (± 0.42%)}	_~	_55.14s	_55.74s	_{p=0.521 n=6}
_{Emit Time}	_{0.16s (± 0.00%)}	_{0.16s (± 0.00%)}	_~	_0.16s	_0.16s	_{p=1.000 n=6}
_{Total Time}	_{64.65s (± 0.43%)}	_{64.52s (± 0.40%)}	_~	_64.22s	_64.87s	_{p=0.689 n=6}
_{self-build-src - node (v18.15.0, x64)}
_{Memory used}	_{2,413,432k (± 0.03%)}	_{2,413,410k (± 0.04%)}	_~	_2,412,350k	_2,414,357k	_{p=0.936 n=6}
_{Parse Time}	_{4.95s (± 0.49%)}	_{4.92s (± 1.14%)}	_~	_4.86s	_4.99s	_{p=0.471 n=6}
_{Bind Time}	_{1.87s (± 0.89%)}	_{1.88s (± 1.03%)}	_~	_1.84s	_1.89s	_{p=0.363 n=6}
_{Check Time}	_{33.45s (± 0.38%)}	_{33.45s (± 0.45%)}	_~	_33.25s	_33.66s	_{p=0.810 n=6}
_{Emit Time}	_{2.67s (± 1.23%)}	_{2.72s (± 1.33%)}	_{+0.05s (+ 1.75%)}	_2.68s	_2.78s	_{p=0.045 n=6}
_{Total Time}	_{42.98s (± 0.34%)}	_{42.99s (± 0.41%)}	_~	_42.77s	_43.21s	_{p=0.936 n=6}
_{self-compiler - node (v18.15.0, x64)}
_{Memory used}	_{419,704k (± 0.01%)}	_{419,754k (± 0.01%)}	_~	_419,671k	_419,847k	_{p=0.093 n=6}
_{Parse Time}	_{2.72s (± 3.34%)}	_{2.75s (± 2.81%)}	_~	_2.66s	_2.82s	_{p=0.573 n=6}
_{Bind Time}	_{1.18s (± 6.60%)}	_{1.14s (± 6.65%)}	_~	_1.07s	_1.23s	_{p=0.413 n=6}
_{Check Time}	_{15.14s (± 0.47%)}	_{15.13s (± 0.45%)}	_~	_15.02s	_15.22s	_{p=0.872 n=6}
_{Emit Time}	_{1.16s (± 1.67%)}	_{1.15s (± 0.90%)}	_~	_1.13s	_1.16s	_{p=0.192 n=6}
_{Total Time}	_{20.19s (± 0.42%)}	_{20.16s (± 0.31%)}	_~	_20.09s	_20.27s	_{p=0.688 n=6}
_{vscode - node (v18.15.0, x64)}
_{Memory used}	_{2,808,007k (± 0.00%)}	_{2,807,956k (± 0.00%)}	_~	_2,807,853k	_2,808,019k	_{p=0.128 n=6}
_{Parse Time}	_{10.62s (± 0.37%)}	_{10.61s (± 0.28%)}	_~	_10.56s	_10.65s	_{p=0.221 n=6}
_{Bind Time}	_{3.39s (± 0.42%)}	_{3.39s (± 0.43%)}	_~	_3.37s	_3.41s	_{p=0.870 n=6}
_{Check Time}	_{59.65s (± 0.44%)}	_{59.95s (± 0.45%)}	_~	_59.71s	_60.39s	_{p=0.090 n=6}
_{Emit Time}	_{16.13s (± 0.35%)}	_{16.18s (± 0.54%)}	_~	_16.06s	_16.29s	_{p=0.336 n=6}
_{Total Time}	_{89.80s (± 0.29%)}	_{90.13s (± 0.35%)}	_~	_89.71s	_90.54s	_{p=0.109 n=6}
_{webpack - node (v18.15.0, x64)}
_{Memory used}	_{392,529k (± 0.01%)}	_{392,553k (± 0.02%)}	_~	_392,468k	_392,620k	_{p=0.298 n=6}
_{Parse Time}	_{3.05s (± 0.94%)}	_{3.05s (± 0.56%)}	_~	_3.02s	_3.07s	_{p=0.504 n=6}
_{Bind Time}	_{1.39s (± 0.99%)}	_{1.40s (± 0.29%)}	_~	_1.39s	_1.40s	_{p=0.796 n=6}
_{Check Time}	_{13.95s (± 0.20%)}	_{13.96s (± 0.31%)}	_~	_13.91s	_14.02s	_{p=1.000 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{18.39s (± 0.29%)}	_{18.41s (± 0.25%)}	_~	_18.36s	_18.48s	_{p=0.572 n=6}
_{xstate - node (v18.15.0, x64)}
_{Memory used}	_{513,393k (± 0.01%)}	_{513,397k (± 0.01%)}	_~	_513,344k	_513,431k	_{p=1.000 n=6}
_{Parse Time}	_{3.28s (± 0.31%)}	_{3.29s (± 0.25%)}	_~	_3.27s	_3.29s	_{p=0.149 n=6}
_{Bind Time}	_{1.54s (± 0.26%)}	_{1.54s (± 0.00%)}	_~	_1.54s	_1.54s	_{p=0.405 n=6}
_{Check Time}	_{2.86s (± 0.65%)}	_{2.84s (± 0.35%)}	_~	_2.83s	_2.85s	_{p=0.217 n=6}
_{Emit Time}	_{0.08s (± 0.00%)}	_{0.08s (± 6.19%)}	_~	_0.08s	_0.09s	_{p=0.174 n=6}
_{Total Time}	_{7.75s (± 0.16%)}	_{7.75s (± 0.18%)}	_~	_7.73s	_7.76s	_{p=0.453 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

typescript-bot · 2024-01-23T23:27:18Z

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

typescript-bot · 2024-01-24T00:58:36Z

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

Something interesting changed - please have a look.

Details

chakra-ui/chakra-ui

4 of 28 projects failed to build with the old tsc and were ignored

packages/components/tsconfig.build.json

error TS5056: Cannot write file '/mnt/ts_downloads/chakra-ui/packages/components/dist/types/menu/menu.stories.d.ts' because it would be overwritten by multiple input files.
- Project Scope

andrewbranch · 2024-01-25T00:28:06Z

@typescript-bot cherry-pick to release-5.4

typescript-bot · 2024-01-25T00:28:13Z

Heya @andrewbranch, I've started to cherry-pick this into release-5.4 for you. Here's the link to my best guess at the log.

typescript-bot · 2024-01-25T00:29:41Z

Hey @andrewbranch, I've created #57161 for you.

Fix isolatedModules check for export assignments

f457d72

typescript-bot assigned andrewbranch Jan 23, 2024

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jan 23, 2024

andrewbranch commented Jan 23, 2024

View reviewed changes

andrewbranch requested review from jakebailey and gabritto January 23, 2024 22:36

jakebailey reviewed Jan 23, 2024

View reviewed changes

jakebailey approved these changes Jan 24, 2024

View reviewed changes

gabritto approved these changes Jan 24, 2024

View reviewed changes

andrewbranch merged commit 94f4379 into microsoft:main Jan 24, 2024

andrewbranch deleted the bug/57136 branch January 24, 2024 20:03

nicolo-ribaudo mentioned this pull request Jan 24, 2024

Migrate eslint-parser to cts babel/babel#16222

Merged

typescript-bot pushed a commit that referenced this pull request Jan 25, 2024

Fix isolatedModules check for export assignments (#57148)

b8ca76f

typescript-bot mentioned this pull request Jan 25, 2024

🤖 Pick PR #57148 (Fix isolatedModules check for expor...) into release-5.4 #57161

Closed

jakebailey mentioned this pull request Jan 31, 2024

[@types/node] Fix types for TS-next DefinitelyTyped/DefinitelyTyped#68291

Closed

8 tasks

	// We need to assemble the list of input files for the compiler and other related files on the 'filesystem' (ie in a multi-file test)
	// If the last file in a test uses require or a triple slash reference we'll assume all other files will be brought in via references,
	// otherwise, assume all files are just meant to be in the same compilation session without explicit references to one another.

	if (testCaseContent.settings.noImplicitReferences \|\| /require\(/.test(lastUnit.content) \|\| /reference\spath/.test(lastUnit.content)) {

Fix isolatedModules check for export assignments #57148

Fix isolatedModules check for export assignments #57148

Uh oh!

Conversation

andrewbranch commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Jan 23, 2024

Uh oh!

typescript-bot commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jan 23, 2024

Uh oh!

typescript-bot commented Jan 23, 2024

tsc

Uh oh!

typescript-bot commented Jan 23, 2024

Uh oh!

typescript-bot commented Jan 24, 2024

Uh oh!

andrewbranch commented Jan 25, 2024

Uh oh!

typescript-bot commented Jan 25, 2024

Uh oh!

typescript-bot commented Jan 25, 2024

Uh oh!

Uh oh!

andrewbranch commented Jan 23, 2024 •

edited

Loading

typescript-bot commented Jan 23, 2024 •

edited

Loading

typescript-bot commented Jan 23, 2024 •

edited

Loading

typescript-bot commented Jan 23, 2024 •

edited

Loading

typescript-bot commented Jan 23, 2024 •

edited

Loading