transpileModule miscompiles const/let to var without renaming, shadowing globals

[andersk]

Bug Report

typescript.transpileModule incorrectly compiles { const console = 'local'; }, which does not shadow the global console, to { var console = 'local'; }, which does.

🔎 Search Terms

transpileModule, const, let, var, rename, scope, shadow, global

(#14542 is possibly related, but that doesn’t show a miscompilation.)

🕗 Version & Regression Information

• This is the behavior in every version I tried, and I reviewed the FAQ for entries about transpileModule

⏯ Playground Link

(Playground does not expose transpileModule.)

💻 Code

const typescript = require("typescript");
const code = "{ const console = 'local'; } console.log('ok')";
const transpiled = typescript.transpileModule(code, {}).outputText;
console.log(transpiled);
eval(transpiled);

🙁 Actual behavior

{
    var console = 'local';
}
console.log('ok');

undefined:4
console.log('ok');
        ^

TypeError: console.log is not a function
    at eval (eval at <anonymous> (/tmp/test.js:5:1), <anonymous>:4:9)
    at Object.<anonymous> (/tmp/test.js:5:1)
    at Module._compile (internal/modules/cjs/loader.js:1072:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1101:10)
    at Module.load (internal/modules/cjs/loader.js:937:32)
    at Function.Module._load (internal/modules/cjs/loader.js:778:12)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:76:12)
    at internal/main/run_main_module.js:17:47

🙂 Expected behavior

{
    var console_1 = 'local';
}
console.log('ok');

ok

[RyanCavanaugh]

It is not safe to use this function to produce a file that is loaded into a global context (thus the name transpileModule, not transpileFile)

[andersk]

It doesn't matter whether the file is loaded into the global context. There is no context in which { var console = 'local'; } console.log('ok') is a correct translation of { const console = 'local'; } console.log('ok').

(Note that, importantly, the console.log statement is part of the code being translated. If it weren't, I would agree with you.)

[RyanCavanaugh]

You're right; I misread.

This seems expensive to fix and I'm sort of curious why no one else has noticed this.

[frank-dspeed]

@RyanCavanaugh i did spot that also i often reference to that in my issues like

• https://github.com/microsoft/TypeScript/issues/48716
• https://github.com/microsoft/TypeScript/issues/47237
• posted only 2 here but i comment often in the ESM issues about that. as they use strict by default it is a no brainer to spot issues there and all are related to the fact that typescript is not aware is that code inside a module and in which scope does that module run after compilation and while in dev. typescript simply is not aware of global scope i am at present preparing even the next nightmare i would love to see support for eval() and thats why i found this issue. when Typescript would want to support eval it would be need to be context aware and target environment aware as also final module standard used aware

implemenation strategy

Typescript needs to implement ast views else it will not scale. some functions or proxys that represent the ast out of diffrent view.

const ast = {
  entry: {
    module: ..
  }
}

view representing the ast under ESM Conditions and also support inlining Scrpts typescript does know 2 kinds of js Scripts and Modules i am referencing to that. so it should get supported for modules as also eval to be driven from the same ast and show diffrent code hints based on diffrent views as JS is always intended to run in multiple environments often.

[frigus02]

I think I found another instance of this while rolling out ts.transpileModule in our code base.

Playground link

export function foo() {
  for (const name of ["from loop"]) {
    // with `noLib: false`, the variable is called `var name_1`
    // with `noLib: true`, the variable is called `var name`
    console.log(name);
  }

  console.log(name); // when `noLib: true` is set, this incorrectly logs "from loop"
}

I wonder if this sort of thing could be flagged by isolatedModules. Though I imagine it may still be expensive?

For what it's worth, I only noticed this as an emit difference. It does not actually cause an error in our code. And it's only an issue with target: es5, so maybe not worth the effort.

[frigus02]

This did now cause an issue for us. Here is the problematic piece of code:

Playground link

export function updateUrl(newHash: string) {
  if (newHash !== window.location.hash) {
    window.history.replaceState(null, '', newHash);
    for (const window of getChildWindows()) {
      window.history.replaceState(null, '', newHash);
    }
  }
}

With target: es5 and noLib: true, the window inside the loop turns into a var window. Therefore the window.location access above fails.

I kinda feel like having an error on the window loop variable under isolatedDeclarations would be quite annoying.

I tried modifying isSymbolOfDeclarationWithCollidingName in the type checker, so that block scoped names are always renamed. ~300 baselines need updates: https://github.com/microsoft/TypeScript/commit/9ffe9a061acb57c6a1ceb1ccd3717b36d1d1c61e. Is that feasible? Or is that too native or does that make the emit too expensive?