fix: const enums + isolatedModules emit invalid code#41933
fix: const enums + isolatedModules emit invalid code#41933andrewbranch merged 9 commits intomicrosoft:masterfrom
Conversation
|
|
andrewbranch
left a comment
There was a problem hiding this comment.
Can we add a test for only using the type side of a const enum too? Like
// @isolatedModules: true
// @Filename: /enum.ts
export const enum Foo { Bar }
// @Filename: /index.ts
import { Foo } from "./enum";
function f(foo: Foo) { return; }|
I think it’s worth considering adding an error too, but that can be done separately. I see no reason not to take this fix regardless of whether we add an error in the future. |
860dab4 to
7012959
Compare
|
That test is simple enough, and does what I expect (the import, which is only used in type context, is deleted). It doesn't even run the new code. Added. |
7012959 to
372bbfd
Compare
fe99edd to
b39e650
Compare
In `isolatedModules` mode, the compiler does not inline const enums,
but also decides not to `import` them, leaving invalid code that
throws a `ReferenceError` at runtime.
This code:
```
import { SomeEnum } from './bar';
sink(SomeEnum.VALUE);
```
..should compile to either:
```
var { SomeEnum } = require('./bar');
sink(SomeEnum.VALUE);
```
..or (with const enum inlining):
```
sink(1 /* VALUE */);
```
..but actually compiles to:
```
sink(SomeEnum.VALUE);
```
..with no imports, which throws a ReferenceError at runtime.
---
The compiler has already realised that the symbol is a referenced const
enum, it just doesn't use this information when it comes to deciding
whether to emit an import. This commit additionally checks that
information, if we are compiling in isolatedModules mode.
---
In my opinion, this is not the correct fix, but it is the simplest. In
`isolatedModules` mode, `const enum` should probably be a compile error
(because there are no benefits and the complexity is high, and,
apparently, buggy). If not, the compiler should not be checking whether
something is a const enum, and should just be treating it as a regular
enum, and checking as if it was?
Fixes microsoft#40499.
Co-authored-by: Andrew Branch <andrewbranch@users.noreply.github.com>
b39e650 to
38cef4b
Compare
|
@andrewbranch @sheetalkamat is this ready to go in? From the comment history, it looks like @sheetalkamat 's comments were addressed and @andrewbranch reviewed them, but I want to make sure before merging. |
|
I believe it’s ready, just wanted another team member to review. |
| })(ConstEnumOnlyModule = exports.ConstEnumOnlyModule || (exports.ConstEnumOnlyModule = {})); | ||
| //// [reexport.js] | ||
| "use strict"; | ||
| var Foo = require("./foo"); |
There was a problem hiding this comment.
A bit of a separate bug, but closely inspecting checkPropertyAccessExpressionOrQualifiedName led me to discover that this import was missing until I added the check for isExportOrExportExpression.
|
Thanks @FauxFaux! Sorry this ended up being more complicated than you bargained for 😅 |
In
isolatedModulesmode, the compiler does not inline const enums, but also decides not toimportthem, leaving invalid code that throws aReferenceErrorat runtime.This code:
..should compile to either:
..or (with const enum inlining):
..but actually compiles to:
..with no imports, which throws a ReferenceError at runtime.
The compiler has already realised that the symbol is a referenced const enum, it just doesn't use this information when it comes to deciding whether to emit an import. This commit additionally checks that information, if we are compiling in isolatedModules mode.
In my opinion, this is not the correct fix, but it is the simplest. In
isolatedModulesmode,const enumshould probably be a compile error (because there are no benefits and the complexity is high, and, apparently, buggy). If not, the compiler should not be checking whether something is a const enum, and should just be treating it as a regular enum, and checking as if it was?Fixes #40499.