Don't look up variables past hoisting scope

[indutny-signal]

When we are renaming variables - we don't need to check for the conflicts past the closest scope that stops the hoisting. Additionally, function expression's name should be put into function's scope, and not outside of the function.

Fix: https://github.com/evanw/esbuild/issues/1147

~~# Caveat~~

~~I might need some guidance with regards to generateTempRef and generateTopLevelTempRef. They rely on the current behavior of the parser and it breaks tests for private properties because of the name clash. Given this the PR is in a draft state, even though all other tests pass.~~

cc @evanw

[indutny]

(This is actually still me, above is my work account)

[evanw]

This PR introduces name collisions into the bundler. Here's an example, which should print 444 when run:

import { bar as baz } from 'data:text/javascript,export let bar = 123'
function foo(bar) {
  return bar + baz
}
console.log(foo(321))

Bundling this with the latest release of esbuild results in the following, which prints 444 (correct):

(() => {
  // <data:text/javascript,export let bar = 123>
  var bar = 123;

  // example.ts
  function foo(bar2) {
    return bar2 + bar;
  }
  console.log(foo(321));
})();

Bundling this with your PR gives this, which prints 642 instead (incorrect):

(() => {
  // <data:text/javascript,export let bar = 123>
  var bar = 123;

  // example.ts
  function foo(bar) {
    return bar + bar;
  }
  console.log(foo(321));
})();

[indutny-signal]

Sorry about that. Let me see...

[indutny-signal]

Alright, it took me awhile to find a way, but I think I got something far enough to request additional feedback @evanw .

The idea is to mark symbols post merge with WasMerged flag and place them in a numberScope's newly introduced map. If we ever find a merged symbol in the scope-chain that has the same name, but different source id - we do the rename.

It is possible to drop WasMerged flag, but at the cost of introducing extraneous renames. As an example:

--- FAIL: TestPackageJsonBrowserMapModuleDisabled (0.00s)
    --- FAIL: TestPackageJsonBrowserMapModuleDisabled/#00 (0.00s)
        bundler_packagejson_test.go:372:  ---------- /Users/user/project/out.js ----------
             // (disabled):Users/user/project/node_modules/node-pkg/index.js
             var require_node_pkg = __commonJS({
               "(disabled):Users/user/project/node_modules/node-pkg/index.js"() {
               }
             });
             
             // Users/user/project/node_modules/demo-pkg/index.js                  7/package.json imported from /Users/indutny/Code/evanw/esbuild/scripts/.end-to-end-tests/1187/node.js.
             var require_demo_pkg = __commonJS({
               "Users/user/project/node_modules/demo-pkg/index.js"(exports, module) {
            -    var fn = require_node_pkg();
            +    var fn2 = require_node_pkg();
                 module.exports = function() {
            -      return fn();
            +      return fn2();
                 };
               }
             });
             
             // Users/user/project/src/entry.js
             var import_demo_pkg = __toESM(require_demo_pkg());
             console.log((0, import_demo_pkg.default)());

I think this flag is very much needed because it tells that the symbol is used outside of the source where it was defined. Maybe I should rename flag to UsedExternally?

[kaatt]

@evanw please take another look?

[jakebailey]

I merged master into this PR to give it a test on some codebases; I found this case to be one that is also not handled.

// index.ts
import * as mod from "./mod";

export function doSomething() {
    return foo();
    function foo() {
        return mod.foo();
    }
}

// mod.ts
export function foo() {}

The correct output would be something like:

// project/mod.ts
function foo() {
}

// project/index.ts
function doSomething() {
  return foo2();
  function foo2() {
    return foo();
  }
}

However, this PR changes it to:

// project/mod.ts
function foo() {
}

// project/index.ts
function doSomething() {
  return foo();
  function foo() {
    return foo();
  }
}

That being said, I really like the affect this PR has on my outputs; it gets rid of a load of renames, mainly due to parameters not being renamed (since they are in a different scope entirely).

[kaatt]

@evanw please take another look?