[compiler] Avoid failing builds when import specifiers conflict or shadow vars

[mofeiZ]

Avoid failing builds when imported function specifiers conflict by using babel's generateUid. Failing a build is very disruptive, as it usually presents to developers similar to a javascript parse error.

import {logRender as _logRender} from 'instrument-runtime';

const logRender = () => { /* local conflicting implementation */ }

function Component_optimized() {
  _logRender(); // inserted by compiler
}

Currently, we fail builds (even in panicThreshold:none cases) when import specifiers are detected to conflict with existing local variables. The reason we destructively throw (instead of bailing out) is because (1) we first generate identifier references to the conflicting name in compiled functions, (2) replaced original functions with compiled functions, and then (3) finally check for conflicts.

When we finally check for conflicts, it's too late to bail out.

// import {logRender} from 'instrument-runtime';

const logRender = () => { /* local conflicting implementation */ }

function Component_optimized() {
  logRender(); // inserted by compiler
}

[mofeiZ]

This function is the primary change in this PR. When collecting external functions imports to insert into the program, we also generate uids for each import.

In cases we cannot generate uids (e.g. program shadows global variables), we bail out without modifying the program (see error.emit-freeze-conflicting-global.js)

[josephsavona]

This makes sense but I wonder if there's something a bit simpler. An idea I had played around with is creating an ImportContext at the program level to track all imports we need to create, and then passing this context to each environment that we create to compile functions. The ImportContext would hold a reference to the program's Scope, such that it could be used to call Babel's helper to generate unique identifier ids. Individual passes can then do env.importContext.addImport(module, importedName, localNameHint): NonLocalBinding. The return type is the binding type used by the LoadGlobal instruction, so you can easily synthesize a LoadGlobal to refer to the imported value. The addImport() function would record the import into a map — reusing existing identifiers if they are already known imported, adding a new entry if its the first import. All those identifiers would be known unique. Then at the end of Program, we actually generate all the import statements.

As an extension, we could also traverse top-level imports and pre-populate the import context map, so that we can reuse existing imports and don't have to recreate those.

What do you think?

[mofeiZ]

This makes sense but I wonder if there's something a bit simpler. An idea I had played around with is creating an ImportContext at the program level to track all imports we need to create, and then passing this context to each environment that we create to compile functions. The ImportContext would hold a reference to the program's Scope, such that it could be used to call Babel's helper to generate unique identifier ids. Individual passes can then do env.importContext.addImport(module, importedName, localNameHint): NonLocalBinding. The return type is the binding type used by the LoadGlobal instruction, so you can easily synthesize a LoadGlobal to refer to the imported value. The addImport() function would record the import into a map — reusing existing identifiers if they are already known imported, adding a new entry if its the first import. All those identifiers would be known unique. Then at the end of Program, we actually generate all the import statements.

As an extension, we could also traverse top-level imports and pre-populate the import context map, so that we can reuse existing imports and don't have to recreate those.

What do you think?

Yeah that makes a lot of sense! That would also help clean up the logic in Program.ts to detect whether to insert useMemoCache/c as _c and useFire etc.

It could also generalize as a generated variable name registry -- one thing I didn't love about this approach was that it renames hook imports to _useHook (e.g. the useFire hook). If we had a registry of all identifiers created by react compiler, we could confidently generate our own names for useHook.

class ProgramContext {
  uids: Set<string> = new Set();
  hookNameConfig: string;
  scope: t.Scope;
  constructor(program: NodePath<t.Program>, hookNameConfig: string) {
    this.hookNameConfig = hookNameConfig;
    this.scope = program.scope;
  }
  addImportSpecifier(module: string, specifier: string): NonLocalBinding {
     if (isHookName(specifier, this.hookNameConfig)) {
       let maybeName = specifier;
       let i = 0;
       while (scope.hasReference(maybeName)) {
         maybeName = `${specifier}_${i++}`;
       }
     }
     ...
  }
  makeUid(nameHint?: string): string {
    const name = .generateUid(nameHint);
  }
}

[josephsavona]

yesss this is awesome

[josephsavona]

this also prevents names that might be shadowed in local scopes, right?

[mofeiZ]

Yep, I took this from Babel's implementation (https://github.com/babel/babel/blob/main/packages/babel-traverse/src/scope/index.ts#L510-L513).

[josephsavona]

why is this one an error rather than renaming?

[mofeiZ]

This one is trickier -- we could synthesize a read off of globalThis or something similar, but I'm not familiar with browser / node version support. Note that this is a normal bailout (not a build failure)

[mofeiZ]

Updates:

• fixed failing e2e tests in [compiler][be] Move e2e tests to BabelPlugin transformer #32706. Previously, our e2e tests circumvented Program-level transform logic by directly calling compile(fn). This means that imports etc were never added

• added ProgramContext.addReference calls for all generated identifier names