[jbarrieault]

What does this PR do?

Fixes #14585

This fixes a bundling bug in which the parsing step fails to hoist block-scoped vars, which can result in identifier collisions.

Implementation

I did a fair amount of debugging in bundler_v2.zig and renamer.zig to understand the issue well enough to identify the issue being in the parsing step. Namely, the discovery that at identifier rename time, d was not a top-level symbol in any of foo.js's chunks. It was instead in a child scope. Thus, the performing renaming on bar.js wasn't detecting that the identifier d was already taken by the block-scoped var in foo.js.

That said, I spent much less time stepping through the js_parser.zig stack frames and have a very limited understanding of it. The implementation may be less than ideal for all I know.

How did you verify your code works?

• I included a test for the new code, or existing tests cover it
• I ran my tests locally and they pass (bun-debug test test-file-name.test)

I added the test @Jarred-Sumner provided based on my initial reproduction of the issue. I observed the test failing initially, and passing after the the change. Additionally, manually verifying the output looks as expected:

Zig change PR template checkboxes

I don't know if these apply or how to do/answer them.

• I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
• I included a test for the new code, or an existing test covers it
• JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed
• I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test)

[jbarrieault]

This is used a few lines below, which is then used by the bundler immediately prior to renaming.

[paperclover]

This is not the correct fix. It is a workaround, and actually the same bug and workaround I ran into somewhere else.

                // TODO(paperclover): fix the renamer bug. this bug
                // theoretically affects all usages of temp refs, but i cannot
                // find another example of it breaking (like with `using`)
                p.declared_symbols.append(p.allocator, .{
                    .is_top_level = true,
                    .ref = ctx_storage.*.?.signature_cb,
                }) catch bun.outOfMemory();

It may be worthwhile to compare esbuild's code around this point. Note how it does not append to declared_symbols in their code.

If this PR is blocking something important, i'm ok with adding a TODO comment here. but the real bug is in renaming, hoisting, or symbol merging code. I haven't investigated enough to know.

[paperclover]

by the way this is a good and correct test