Inline and erase local const enums

[Gelio]

The aim of this PR is to partially address #128

This PR changes the behavior of handling const enums:

1. For unexported const enums:
   1. Inline and erase if they are used only as known property access
   2. Inline values, but do not erase the enum if it is used in some other way than known property access (e.g. using the enum object in a function call)
2. For exported const enums and const enums inside namespaces:
   1. Preserve them, i.e. keep the existing logic

Changes in this PR:

1. Detect const enums (IsConst field on SEnum)

2. Display errors when non-literals are used in a const enum

3. Add warnings when an unknown member of an enum is referenced (this is for both const and regular enums)

4. Mark local const enums as CanBeRemovedIfUnused and DoesNotAffectTreeShaking. Combined with calling not calling recordUsage on the enum, this lets the existing mangler (I think) erase the declaration if it is not used in any other way.

   I thought this way is probably the most elegant.

Keep in mind this is one of my first contributions to esbuild, and one of my first lines of go code, so please point out any mistakes/suggestions 😄

[Gelio]

Thanks for the esprima test, I noticed there is a problem with importing the const enum. I added a test case to capture the behavior. I do not have a clue how to fix it, though. Any pointers to places I would need to change would be awesome

[evanw]

Unfortunately this isn't simple to accomplish, which is the reason why I haven't attempted this yet. Here are some of the trickier cases:

export namespace N {
  export const enum E {
    X = 123
  }
}
console.log(N.E.X)

import {N} from './file' // where "file" is the code written above
console.log(N.E.X)

Doing this transformation correctly in all cases requires a type system, and it's a non-goal for esbuild to replicate TypeScript's type system due to the complexity and maintenance overhead. That's why I currently fall back to having const enum behave like enum.

There is an approach where you try to do this when you can and fall back to regular enums when you can't. Doing that means detecting whether or not the identifier for the enum is ever used for something other than a known property access. This is doable for a non-exported enum within a single file, and is hard to do (but not impossible) across multiple files.

[Gelio]

Thanks for the detailed description of the problem. I'll try to experiment a bit more over the next few days

[Gelio]

Hey! Sorry for the lack of activity 😟

I rebased the MR on the latest master and adjusted the code a bit.

Right now:

1. export const enum are preserved
2. Some values (numbers and strings) in export const enums are inlined in the same module/namespace they are defined
3. Values of export const enum are not inlined outside of the enum's namespace
4. Not exported const enum are skipped, no matter how they are used. Their values are inlined

The TestTSImportConstEnumInsideNamespace test should show most of the results when combining namespaces and const enums.

The doubts that I have:

1. When a const enum is preserved (e.g. because it is exported), should we inline string values (as if the const enum would be skipped)? I believe that those string values should remain enum property access, so then those const enums behave like regular enums

2. When an unexported const enum is used in a way other than known property access, there will possibly be a warning, and the enum would still be skipped. This is what you mentioned in an earlier comment. For example:

   const enum Foo {
     A = 1,
   }
   
   (Foo as any).B = 2;
   let usage = bar(Foo.A, Foo.B)

   We could try to detect such an unexpected use and decide to keep the enum. Given this input, TypeScript skips the enum (TS playground). As far as I see, esbuild detects that (Foo as any).B is still property access on an enum, so I reckon there would be more changes necessary to detect this scenario.

   One approach I was thinking about was to count the number of known property accesses for an enum (similarly to parser.recordUsage), and compare it with the number of times the enum was used. If they are not equal, then the enum should be preserved.

   If I'm not mistaken, this would require to know all the uses of the enum (also those below the enum declaration) when the enum is scanned. Is this possible without scanning twice? Unless exprCanBeRemovedIfUnused happens after the file is already parsed, in which case, I could try using that

   Do you think this is the right approach?

[jods4]

Thanks for working on that! We have tons of const enums and that's a lot of dead weight if they aren't "const".

Are there many cases other than namespaces that are troublesome?
If esbuild supported compiling away const enums outside namespaces, that would already be a big improvement for many if not most real-world use-cases.

Keeping const enums in namespaces as plain enums could remain a documented limitation.

Full support of namespaced enums is out of question anyway because as you noted, namespaces can be aliased and then require full typing to know what is what.

On the other hand I believe you can't alias a const enum itself, so that's probably doable.

• You can alias the enum as a type but we don't care for codegen: const enum E {}; type F = E
• You can alias an import but that's something you can track without too much trouble import { E as F } from "./enums"

[Gelio]

@jods4 I appreciate the support 🙂 As far as I know, the 2 cases in which const enums are hard to erase are

1. Namespaces
2. export const enum (I believe this is an error with the --isolatedModules flag, which is recommended when using esbuild)

In the solution I prepared, in both cases the const enum will be left intact as if it was a regular enum.

Only unexported const enums (used only inside of a single module) will be erased 🙂

[jods4]

I think 2. is doable and is an important use case as enums are often used across files.

For example in our codebase we have lots of enums auto-generated in a single file that is just const enums exports and nothing else.

I believe that:

• export const enum is actually treated as export enum with the --isolatedModules flag. As I said I use const enums a lot and they still work in snowpack, vite or webpack w/ fork-ts-checker-plugin. It's just that the codegen is much bigger than it should.
• You can't reference a const enum value any other way than directly on the declared enum name. So it's actually trackable without full typing. You just need to know which imports are consts and substitute the values. This still requires inter-module analysis but that's the point of [Feature] Inline const enum values in TypeScript #128.

This last bullet is not true once a const enum is exported from a namespace, because the namespace itself can be aliased into new variables, parameters, destructured, etc. etc. This means 1. is not doable without full typing, which esbuild is not going to do.

[Gelio]

What you are saying about point 2 makes sense. I got fooled by thinking esbuild does not have access to symbols exported in a dependency module when parsing another file, but that would not make sense when bundling (I hope I am not mistaken here 😄 )

Back to the drawing board with this MR then, I guess 🙂 I'll try to experiment a bit more with erasing exported const enums, but leaving the ones inside namespaces intact.

[Gelio]

I've got some sad news. I give up on inlining const enum values across files. I've been trying and failing to do that for the last 9h and it looks like a major endeavor 😄

Some findings:

1. The problem is that parser.knownEnumValues is local to only 1 file. There is no way to know if an identifier imported from another file is an enum, and what values it has.
2. We could probably implement some global store for knownEnumValues, or attach known enum values to each exported identifier, so it is accessible from its dependents. However, that seems like a major task, so I did not try to do that
3. I tried to make it so that instead of transpiling the enum statement right away in the parser, we do it lazily, in the printer. That did not work too well, because then the linker did not pick up that the enum is exported, and did not see the identifier. I abandoned that path since I thought it may be more time-consuming.

All in all, @evanw was definitely right and I underestimated the effort 😅

I managed to (hopefully) implement the easier solution that @evanw suggested in #534 (comment). I will update the description of the PR

@evanw This is a small enhancement, as it only provides value for local const enums, but I reckon we still could try to merge it. Let me know what you think

[Gelio]

Hey @evanw, I'm wondering whether you are still interested in this addition to esbuild, as there's been some period of inactivity in the PR. My intent is not to apply any pressure, but just want to be sure if I should be hoping to get this landed any time soon 🙂

[kzc]

TS has an odd feature where new members can be added to pre-existing enums:

$ cat enum.ts 
enum Foo {BAR=1, BAZ=2};
enum Foo {BOO=3};
console.log(Foo.BAR, Foo.BAZ, Foo.BOO);

Notice that no warnings are generated by tsc:

$ node_modules/.bin/tsc enum.ts --outFile enum.js && node enum.js
1 2 3

This PR produces:

$ esbuild enum.ts | node
 > enum.ts:3:12: warning: Unknown member BAR on enum Foo
    3 │ console.log(Foo.BAR, Foo.BAZ, Foo.BOO);
      ╵             ~~~

 > enum.ts:3:21: warning: Unknown member BAZ on enum Foo
    3 │ console.log(Foo.BAR, Foo.BAZ, Foo.BOO);
      ╵                      ~~~

2 warnings
1 2 3

[Gelio]

@kzc Oh, that's surprising 😄 Thanks for pointing it out! I'll try to incorporate it into the solution and add a test for this case.

I guess it's a matter of not creating a new knownEnumValues every time.

[kzc]

Here's a test case variation using const enum that generates incorrect pure annotations...

Given:

$ cat const_enum.ts
const enum Foo {BAR=1, BAZ=2};
const enum Foo {BOO=3};
console.log(Foo.BAR, Foo.BAZ, Foo.BOO);

Expected:

$ node_modules/.bin/tsc const_enum.ts --outFile const_enum.js && node const_enum.js
1 2 3

With this PR:

$ esbuild const_enum.ts --log-level=error
var Foo;
/* @__PURE__ */ (function(Foo2) {
  Foo2[Foo2["BAR"] = 1] = "BAR";
  Foo2[Foo2["BAZ"] = 2] = "BAZ";
})(Foo || (Foo = {}));
;
/* @__PURE__ */ (function(Foo2) {
  Foo2[Foo2["BOO"] = 3] = "BOO";
})(Foo || (Foo = {}));
;
console.log(Foo.BAR, Foo.BAZ, 3);

it's fine without minification:

$ esbuild const_enum.ts --log-level=error | node
1 2 3

but when run through terser the incorrect annotations produce:

$ esbuild const_enum.ts --log-level=error | node_modules/.bin/terser -bc
var Foo;

Foo || (Foo = {}), Foo || (Foo = {}), console.log(Foo.BAR, Foo.BAZ, 3);

which leads to the incorrect result:

$ esbuild const_enum.ts --log-level=error | node_modules/.bin/terser -bc | node
undefined undefined 3

Likewise with esbuild:

$ esbuild const_enum.ts --log-level=error --bundle --minify-syntax
(() => {
  // const_enum.ts
  /* @__PURE__ */ (function(Foo2) {
    Foo2[Foo2.BOO = 3] = "BOO";
  })(Foo || (Foo = {}));
  console.log(Foo.BAR, Foo.BAZ, 3);
})();

$ esbuild const_enum.ts --log-level=error --bundle --minify-syntax | node
[stdin]:5
  })(Foo || (Foo = {}));
     ^

ReferenceError: Foo is not defined

[kzc]

Side note - not specific to this PR...

In my opinion instead of the following esbuild generated output:

$ echo 'export enum E {A, B};' | esbuild --loader=ts
export var E;
(function(E2) {
  E2[E2["A"] = 0] = "A";
  E2[E2["B"] = 1] = "B";
})(E || (E = {}));
;

this ought to be generated:

$ cat proposed.js 
// export enum E {A, B};
var E = /*@__PURE__*/ (function(E2) {
  return E2[E2["A"] = 0] = "A", E2[E2["B"] = 1] = "B", E2;
})(E || {});
export {E};

or the more compact arrow form:

// export enum E {A, B};
var E = /*@__PURE__*/ (E2 => (E2[E2["A"] = 0] = "A", E2[E2["B"] = 1] = "B", E2))(E || {});
export {E};

since it can be dropped altogether if not used:

$ echo 'import {E} from "./proposed.js";' | esbuild --bundle --minify
(()=>{})();

$ echo 'import {E} from "./proposed.js";' | rollup --silent
// no output

and still function correctly if used:

$ echo 'import {E} from "./proposed.js"; console.log(E.A, E.B);' | esbuild --bundle --minify | node
0 1

$ echo 'import {E} from "./proposed.js"; console.log(E.A, E.B);' | rollup --silent | node
0 1

[gregveres]

The suggested output that @kzc suggests seems to be the best approach. In the issue thread, we said that the real issue of not getting rid of the enums was the code that got generated. If the code gen was changed to output the suggested code, and it was removed by rollup, that seems like the perfect solution.

[Gelio]

I've rebased the branch on the latest master and added 2 new commits that fix the issues mentioned by @kzc above. I have also added tests that are based on the snippets you provided, so thanks 🙂

It turns out that I had to change the symbol merge strategy for enums - it was mergeReplaceWithNew, and it needs mergeKeepExisting. The tests passed, but I am not 100% certain why the strategy was different initially.

One behavior that is different from tsc is error reporting for prematurely used enum members. In the following snippet:

enum Foo {BAR=1, BAZ=2};
console.log(Foo.BAR, Foo.BAZ, Foo.BOO);
enum Foo {BOO=3};

tsc is fine with using Foo.BOO before it's defined. The current behavior of esbuild will print a warning here (validated by a test), which IMO is a useful enhancement.

Unfortunately, due to the single-pass parsing nature of esbuild, the following valid code also reports a warning and Foo.ZOO is not inlined 😟:

const enum Foo {BAR=1, BAZ=2};
function falsePositiveWarningInsideFunction() {
	console.log(Foo.BAR, Foo.BAZ, Foo.BOO);
}
const enum Foo {BOO=3};
falsePositiveWarningInsideFunction();

This will likely be a problem with any use of an enum above the place of its declaration.

---

On the output proposed by @kzc - yeah, that would do the job. However, esbuild's output is similar to tsc's one, which also defines a variable in a separate statement, and assigns to it in an IIFE. See this TS playground. I agree that having the syntax you proposed when it's possible (probably when there is a single enum declaration, without any enum merging) would be ideal. I won't pursue that in this PR, though

---

I see that the CI for windows failed, but that looks to be related to the failure on master caused by 471e743 (for which I have raised a PR #1074)

[Gelio]

Yet another rebase on master, since #1074 is now merged and the CI should start passing

[kzc]

While the const enum values are indeed inlined, the unused pure annotated generated IIFEs for the enums are not always dropped by esbuild:

With the latest PR:

$ cat common.ts 
export namespace NS {
  const enum Foo {
    A = 1,
    B = A * 2,
    C,
    D = 'stringsWorkToo',
  }
  export let usage = bar(Foo.A, Foo.B, Foo.C, Foo.D)
}

$ cat entry.ts 
import { NS } from './common.ts';
NS.usage();

$ esbuild entry.ts --bundle --minify-syntax
(() => {
  // common.ts
  var NS;
  (function(NS2) {
    let Foo;
    /* @__PURE__ */ (function(Foo2) {
      Foo2[Foo2.A = 1] = "A", Foo2[Foo2.B = 2] = "B", Foo2[Foo2.C = 3] = "C", Foo2.D = "stringsWorkToo";
    })(Foo || (Foo = {})), NS2.usage = bar(1, 2, 3, "stringsWorkToo");
  })(NS || (NS = {}));

  // entry.ts
  NS.usage();
})();

esbuild is unable to drop the unused pure annotated IIFE above because it is not top level.

Even latest rollup (for whatever reason) is unable to drop the unused pure annotated IIFE generated by esbuild:

$ esbuild entry.ts --bundle --minify-syntax | rollup --silent
(() => {
  // common.ts
  var NS;
  (function(NS2) {
    let Foo;
    /* @__PURE__ */ (function(Foo2) {
      Foo2[Foo2.A = 1] = "A", Foo2[Foo2.B = 2] = "B", Foo2[Foo2.C = 3] = "C", Foo2.D = "stringsWorkToo";
    })(Foo || (Foo = {})), NS2.usage = bar(1, 2, 3, "stringsWorkToo");
  })(NS || (NS = {}));

  // entry.ts
  NS.usage();
})();

Although uglify-js and terser can drop the unused pure annotated IIFEs in question, most esbuild users would probably not elect to use them:

$ esbuild entry.ts --bundle --minify-syntax | npx uglify-js --toplevel -mc
var a;(a||(a={})).usage=bar(1,2,3,"stringsWorkToo"),a.usage();

$ esbuild entry.ts --bundle --minify-syntax | npx terser --toplevel -mc
var a;!function(a){let o;o||(o={}),a.usage=bar(1,2,3,"stringsWorkToo")}(a||(a={})),a.usage();

Using the pure annotated IIFE approach outlined in #534 (comment) would allow esbuild (and rollup) to drop these unused enum IIFEs.

Ideally, this PR would not emit the unused enum IIFEs at all and avoid this issue altogether.

[Gelio]

Thanks for the followup and more suggestions. I have been testing out various approaches now to get the output to match your suggestion @kzc.

For the following input files:

			"/common.ts": `
				const enum Foo {
					A = 1,
					B = A * 2,
					C,
					D = 'stringsWorkToo',
				}

				export let usage = bar(Foo.A, Foo.B, Foo.C, Foo.D)
			`,
			"/entry.ts": `
				import { usage } from './common.ts';

				usage();
			`,

I have managed to get the following esbuild's output:

// common.ts
var usage = bar(1, 2, 3, "stringsWorkToo");

// entry.ts
usage();

which looks pretty good.

I'll need to try some more approaches. Also, the change is quite messy at the moment, so I'll take some time to improve it.

However, as it does seem to be a different way to tackle the problem with const enums, I plan on releasing it as a separate PR. Both PRs could be merged independently, as IMO they introduce different ideas. Also, I don't want to get this one unstable.

A couple of notes that I have on my mind:

1. Is it common to run files through rollup after esbuild? esbuild is not as good as cutting out unused PURE-annotated expressions, so the benefit may not be noticeable for most users.

2. Is the benefit of this approach only related to removing unused enums? And in the context of const enums, the benefit would be that the closure generated by const enums that are inside of namespaces would be erased.

   If so, that seems pretty marginal to me.

   I'm not certain how often namespaces are used, but I'd say pretty rarely. At least I have not seen one used outside of declaration files in the last 2 years. ES modules are used more often, and in the case of ES modules, the generated closures are erased correctly (see the output for TestTSInlinableLocalConstEnum)

   If that is the case, I'm on the fence about pursuing that idea, as it involves more work, and is a breaking change

Let me know if I'm mistaken in some point

[kzc]

I have managed to get the following esbuild's output:
var usage=bar(1,2,3,"stringsWorkToo");usage();

That's great.

Is it common to run files through rollup after esbuild?

For applications, probably not. But if esbuild is used to produce library bundles then it would be common for those library packages to be consumed by other bundlers.

Is the benefit of this approach only related to removing unused enums?

If you already know which enums are in use, and they are not exported from the source file, then there's no advantage to emitting them. And if an enum is exported or if the enum identifier itself is used in a non-enumeration way then it would be a good thing if it could be tree shakable if not necessary.

I'm not certain how often namespaces are used, but I'd say pretty rarely

100% agreement on that point.

and is a breaking change

Not to my knowledge. The esbuild user would not notice the missing unused enum IIFEs.

[kzc]

@Gelio Wanted to mention that my comments are from the perspective of a user, and I was just listing what features that I'd like to see. But feel free to ignore my input. Ultimately it's up to you and @evanw to hash out.

[Gelio]

Thanks for the answers again. To my mind, the issue about changing the emitted syntax for enums is orthogonal to this PR and the point about inlining const enum values (which was the main point of #128).

If you reckon changing the output syntax could have benefits, it deserves a separate issue, that may catch the attention of @evanw who definitely has moree knowledge than me about the possible implications of such a change (and does not seem to be active in this PR anymore, unfortunately). If you create an issue for it, I may chime in with some comments and findings I came across during the experiment yesterday 🙂

I want to keep the discussion in this PR related to the PR and the issue it tries to solve

[kzc]

Changing the enum code generation was just mentioned as a generalization because this PR was emitting /*@__PURE__*/ annotated IIFEs that were not assigned to a variable - meaning they ought to be unconditionally dropped, aside from the IIFE argument Enum || (Enum = {}). Because esbuild only performs tree shaking on top level code, problems resulting from an incorrectly annotated IIFE would only be evident by subsequently using another tool like uglify-js, terser or rollup.

[AlonMiz]

@Gelio thanks a lot. i wish we can merge this soon.
what's holding you back?

[Gelio]

@AlonMiz This PR has not been reviewed by @evanw yet at all, which I believe is a prerequisite to getting it merged. I am waiting for his input (or any other review, for that matter).

I don't think there is much for me to do specifically, aside from maybe resolving merge conflicts and remembering the changes. Since there is no clear direction from @evanw, I don't want to spend time on this PR now only to have it be forgotten for a few months and require a revisit in the future

[kzc]

Looks like #534 (comment) was just implemented and merged: 731f559

[jods4]

Nice improvement, and great to see that const enums are not set in stone yet.

In my projects we generate tons of enums from server-side model and reference them in other files, so I'm still hoping for basic direct reference simplification, i.e.:

// Direct import of an exported enum const
import { E } from "./enums"
console.log(E.One)

// Compiles to:
console.log(1)

Hopefully, this simplification will eventually be doable without full type analysis as it only requires to mark exports that are const enums.

That still doesn't support more convoluted cases but it'd be good enough as it covers 99% cases for me.

[evanw]

Closing this PR now that #128 has been fixed.