Skip to content

fix: const enums + isolatedModules emit invalid code #41933

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jan 13, 2021

Conversation

FauxFaux
Copy link
Contributor

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 #40499.

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Dec 11, 2020
@ghost
Copy link

ghost commented Dec 11, 2020

CLA assistant check
All CLA requirements met.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; }

@andrewbranch
Copy link
Member

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.

@FauxFaux
Copy link
Contributor Author

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.

@FauxFaux FauxFaux force-pushed the fix/const-enum branch 2 times, most recently from fe99edd to b39e650 Compare December 15, 2020 06:58
FauxFaux and others added 7 commits December 18, 2020 12:11
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.
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @FauxFaux!

@sandersn
Copy link
Member

@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.

@andrewbranch
Copy link
Member

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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@andrewbranch
Copy link
Member

Thanks @FauxFaux! Sorry this ended up being more complicated than you bargained for 😅

@andrewbranch andrewbranch merged commit 368cdfd into microsoft:master Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

const enum imports are stripped even with isolatedModules
5 participants