Ensure enum members syntactically determinable to be strings do not get reverse mappings

[Andrew Branch (andrewbranch)]

Unblocks #56736?

In #56164, we decided that under isolatedModules, (from the notes) “enum members referencing another external enum must always be numeric, or explicitly converted to a string. Otherwise, it is an error.” This proposed rule has since undergone a number of interpretations on its way to an attempt to implement it. In #56153 (comment), Ryan stated that we should issue an error when “an enum initializer expression that isn't a string literal has a string type.” In #56736, Jan Kühle (@frigus02) worked off this statement, but interpreted “string literal” as any expression that could be evaluated to a known constant string value with single-file syntactic information only.

An invariant guiding the decision is that error-free (under --isolatedModules) code should produce semantically equivalent JS between ts.transpileModule and tsc. Currently, that’s violated by this test case:

// @filename: ./foo.tsimport{BAR}from'./bar';enumFoo{A=`${BAR}`}// @filename: ./bar.tsexportconstBAR='bar';

This is currently error-free, but the emit varies. tsc:

import{BAR}from'./bar';varFoo;(function(Foo){Foo["A"]="bar";})(Foo||(Foo={}));

ts.transpileModule:

import{BAR}from'./bar';varFoo;(function(Foo){Foo[Foo["A"]=`${BAR}`]="A";})(Foo||(Foo={}));

The problematic part isn’t the inlining of the value "bar"; it’s the fact that the transpiled output includes a reverse mapping when it absolutely shouldn’t.

To fix this, we either need an error under isolatedModules or we need to fix the ts.transpileModule emit. Both Ryan’s formulation of the rule proposed in #56164 and Jan Kühle (@frigus02)’s expanded interpretation in #56736 make this code an error. This PR fixes the emit by determining that the initializer is definitely going to be a string without looking at any type or extra-file info.

I’m fine with adopting Ryan’s very strict rule instead of this, but it does seem like fairly low hanging fruit here. I’m less of a fan of the approach currently in #56736, since constant evaluation feels like the wrong thing to control whether a reverse mapping gets created, even though that’s what actually controls the emit today.

[Jan Kühle (frigus02)]

Thank you, Andrew. I'm personally in favor of this approach.

[Jake Bailey (jakebailey)]

since constant evaluation feels like the wrong thing to control whether a reverse mapping gets created

I personally agree with this, and so think I prefer this PR.

even though that’s what actually controls the emit today.

Is there some way we can instead use isSyntacticallyString to do that, and we stop using constant eval entirely for the other determinations?

[Andrew Branch (andrewbranch)]

This PR moves in that direction, but we can't fully control emit by isSyntacticallyString because that would break the case

import{SomeOtherStringEnum}from'./m';enumFoo{A=SomeOtherStringEnum.StringValue}

This works in non-isolatedModules tsc. #56736 will make it an error under isolatedModules. Basically, you're asking for the isolatedModules rules to apply to everyone, which might be the design choice we would make starting fresh, but would be a big breaking change now.

[Andrew Branch (andrewbranch)]

TypeScript Bot (@typescript-bot) pack this

[TypeScript Bot (typescript-bot)]

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

Command	Status	Results
pack this	✅ Started	✅ Results

[TypeScript Bot (typescript-bot)]

Hey Andrew Branch (@andrewbranch), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
"devDependencies": {
"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/160329/artifacts?artifactName=tgz&fileId=869CEB102DBC9A8715F1606BF689837B82FDE0677C3EC0E8EEE6C0299669009802&fileName=/typescript-5.5.0-insiders.20240312.tgz"
}
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.5.0-pr-57686-4".;

[Jake Bailey (jakebailey)]

FWIW esbuild does it the same way this PR does so I am all for it: https://jakebailey.dev/esbuild-playground/#v3=PTAEAEDMEsBsFMB2BDAtvAXKAxge0TAOYB0AVgM74BQA3laKAERIAuATgJ4AKu0iL5RlgDajYHwAm8AB7EB0xgF0ANPSaRcbVMhZCm8cqkZUAvlSogIMBCnRZJMueWlVoqAA6aWoGqABCAIIASqAmoJBsuKigAOQARshsMQDcVEgArtEAYri4PqABoAC8oAAGACQ0gUEmpaFUQA

[Andrew Branch (andrewbranch)]

Jake and Ryan have expressed support for this. Can I have a ✅? 😄