Change isolatedModules to allow const enum declaration and disallow access

[alangpierce]

Fixes #20703 with solution suggested in #20703 (comment)

Previously, --isolatedModules gave an error for any ambient const enum, which
meant that some third-party libraries would always give errors even if the
ambient const enums they declare were never used. Now, we only give an error
when an ambient const enum is referenced, which allows such libraries to still
be used as long as the const enums are never accessed.

Some nuances:

• As before, the error is only surfaced for ambient const enums. With
  non-ambient const enums, we know that an isolatedModules build will emit the
  enum and produce a plain reference rather than inlining the constant, so
  everything will still work.
• I originally planned to do this check in the code path that inlines the
  constant, but that code is only exercised at emit time, so, for example, the
  TS language service wasn't giving an error in my editor. Instead, I do the
  check at typecheck time next to another const-enum-related check.
• This can be a breaking change when using skipLibCheck because the error is
  typically moved from a .d.ts file to a .ts file.

Testing done:
I ran this TS build on a large project of mine that previously had disabled
isolatedModules so I could use the chalk library. With isolatedModules
enabled, there was no longer an error in the chalk typedefs, and a reference to
the Level const enum produced an error in my editor.

[RyanCavanaugh]

LGTM, just one thing - can you please address the merge conflict in diagnosticMessages.json? Thanks!

[sheetalkamat]

Should this error be any other file than the declaration of const enum file?

[alangpierce]

The intention with this change is for the error to be on the access, not on the declaration, so in practice it could be in a completely different file than the const enum declaration. In a typical case, I might import the chalk library, which shouldn't error, but any attempt to use the Level enum should error.

I could see an argument for putting an additional error on the const enum (like before), but the point of this change is that simply declaring a const enum is not an error anymore; the error is if you try to use it.

[sheetalkamat]

Yes the error is on access but declaring const enum a {} and using a in module1 shouldn't cause error. It should cause when accessing in module2...n instead?

[alangpierce]

Got it. I just tested it a bit more, and I believe the error still makes sense even within the same file. To be clear, this only happens with isolatedModules enabled and only happens for ambient const enums (the declare is necessary in the code above example).

From my testing, it looks like with isolatedModules enabled, const enums are always compiled as plain enums and const enum accesses are always compiled as plain enum accesses, even when the const enum is in the same file. That means that in this code snippet, the first line is erased (since it's a declare) and the second line is compiled as-is as export var y = E.X. With isolatedModules, TypeScript will not inline the constant even within the same file, so this code would crash at runtime (because E doesn't exist) even though it would work at runtime without isolatedModules. So I think it's fair for TS to error here.

It certainly seems like a reasonable future improvement to implement inlining within the same file even when isolatedModules is on. In that case, this error could be limited to cross-file accesses, but for now, even same-file ambient const enum accesses need to be errors. This would also require the Babel implementation to implement same-file constant inlining.

[alangpierce]

@RyanCavanaugh

can you please address the merge conflict in diagnosticMessages.json

Done! Thanks for taking a look.

[RyanCavanaugh]

Sorry, we're trying to merge a ton of PRs right now and it looks like there's one in checker.ts now

[alangpierce]

@RyanCavanaugh Ok, just updated to resolve the conflict!

[alangpierce]

Thanks!