Repository navigation
feat(transpiler): lower auto-accessor class fields - #26431
Closed
Jarred-Sumner wants to merge 2 commits into
Closed
Jarred-Sumner wants to merge 2 commits into
Jarred-Sumner wants to merge 2 commits into
Conversation
Collaborator
|
Updated 12:00 AM PT - Jan 25th, 2026
❌ @Jarred-Sumner, your commit 47f1309 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 26431That installs a local version of the PR into your bun-26431 --bun |
Contributor
WalkthroughAdds TypeScript/JS class auto-accessor support: new Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/ast/P.zig`:
- Around line 4858-4929: The computed-accessor backing-name and temp symbol are
not unique: change lowerAutoAccessors so the computed backing_name generation
(currently using auto_accessor_count with 'a'..'z') produces a
collision-resistant unique string (e.g., include the auto_accessor_count numeric
suffix or a monotonically increasing id instead of limiting to 26) and increment
the counter deterministically; likewise create the temp symbol name passed to
p.newSymbol (currently "_a") to include the same unique suffix (e.g.,
"_auto_accessor_<n>") so each computed accessor gets distinct backing and temp
identifiers; update uses of auto_accessor_count, backing_name creation, and the
temp_ref/newSymbol call to use this unique naming scheme.
In `@src/ast/parseProperty.zig`:
- Around line 302-310: The parser currently allows method-form auto-accessors
(e.g., "accessor foo()") because the auto-accessor branch (.p_accessor => { ...
kind = .auto_accessor; ... }) falls through into parseMethodExpression; add a
guard before calling parseMethodExpression that detects an accessor modifier in
a class context (reuse the same condition involving opts.is_class,
opts.is_async, opts.is_generator and
js_lexer.PropertyModifierKeyword.List.get(raw) or .p_static) and if the token
stream shows an identifier followed immediately by '(' (method form) emit a
syntax error and reject the construct rather than parsing it as a method; update
the logic around kind/errors/continue :restart to ensure auto_accessor remains
only for field-form (e.g., "accessor x = ..." or accessor with get/set blocks)
and that parseMethodExpression is not invoked for method-form accessor tokens.
In `@src/ast/visit.zig`:
- Around line 847-858: The auto-accessor temp vars accumulated in
p.auto_accessor_computed_key_refs are only emitted inside the statement-visit
loop, so temps created when the current statement list is empty (e.g., while
visiting parameter defaults) never get flushed; modify the visitor to flush any
remaining entries from p.auto_accessor_computed_key_refs after the
statement-visitation loop (or whenever the current statement list becomes empty)
by allocating the Decl as done in the loop (using p.allocator.alloc, p.b for
Identifier binding, p.s to create S.Local and list.append) and then clearing
p.auto_accessor_computed_key_refs to retain capacity so temps are correctly
scoped to the immediate statement list rather than the parent scope.
Comment on lines
+4858
to
+4929
| pub fn lowerAutoAccessors(p: *P, properties: []G.Property) []G.Property { | ||
| var new_props = ListManaged(G.Property).init(p.allocator); | ||
| new_props.ensureTotalCapacity(properties.len * 3) catch unreachable; | ||
| var auto_accessor_count: u32 = 0; | ||
|
|
||
| for (properties) |prop| { | ||
| if (prop.kind != .auto_accessor) { | ||
| new_props.append(prop) catch unreachable; | ||
| continue; | ||
| } | ||
|
|
||
| const loc = if (prop.key) |k| k.loc else logger.Loc.Empty; | ||
| const is_static = prop.flags.contains(.is_static); | ||
| const is_computed = prop.flags.contains(.is_computed); | ||
|
|
||
| // Derive backing field name from the key | ||
| const backing_name: []const u8 = blk: { | ||
| if (prop.key != null and prop.key.?.data == .e_private_identifier) { | ||
| // accessor #x -> backing field #_x | ||
| const orig_name = p.loadNameFromRef(prop.key.?.data.e_private_identifier.ref); | ||
| break :blk std.fmt.allocPrint(p.allocator, "#_{s}", .{orig_name[1..]}) catch unreachable; | ||
| } else if (!is_computed) { | ||
| if (prop.key != null and prop.key.?.data == .e_string) { | ||
| // accessor x -> backing field #x | ||
| const str = prop.key.?.data.e_string; | ||
| break :blk std.fmt.allocPrint(p.allocator, "#{s}", .{str.data}) catch unreachable; | ||
| } | ||
| } | ||
| // Computed key -> counter-based name | ||
| const offset: u8 = @intCast(@min(auto_accessor_count, 25)); | ||
| const name = std.fmt.allocPrint(p.allocator, "#{c}", .{ | ||
| @as(u8, 'a' + offset), | ||
| }) catch unreachable; | ||
| auto_accessor_count += 1; | ||
| break :blk name; | ||
| }; | ||
|
|
||
| // Create backing field symbol | ||
| const backing_ref = p.newSymbol( | ||
| if (is_static) .private_static_field else .private_field, | ||
| backing_name, | ||
| ) catch unreachable; | ||
| p.recordDeclaredSymbol(backing_ref) catch unreachable; | ||
|
|
||
| // For private auto-accessors, update the original symbol kind | ||
| // from .private_field to .private_get_set_pair | ||
| if (prop.key != null and prop.key.?.data == .e_private_identifier) { | ||
| const orig_ref = prop.key.?.data.e_private_identifier.ref; | ||
| p.symbols.items[orig_ref.innerIndex()].kind = | ||
| if (is_static) .private_static_get_set_pair else .private_get_set_pair; | ||
| } | ||
|
|
||
| // For computed keys, cache in a temp variable so the expression | ||
| // is only evaluated once (getter uses _a = expr, setter uses _a) | ||
| var getter_key = prop.key; | ||
| var setter_key = prop.key; | ||
| if (is_computed) { | ||
| const temp_ref = p.newSymbol(.other, "_a") catch unreachable; | ||
| p.recordDeclaredSymbol(temp_ref) catch unreachable; | ||
| p.auto_accessor_computed_key_refs.append( | ||
| p.allocator, | ||
| LocRef{ .loc = loc, .ref = temp_ref }, | ||
| ) catch unreachable; | ||
|
|
||
| // Getter key: _a = expr (evaluate and cache) | ||
| getter_key = p.newExpr(E.Binary{ | ||
| .op = .bin_assign, | ||
| .left = p.newExpr(E.Identifier{ .ref = temp_ref }, loc), | ||
| .right = prop.key.?, | ||
| }, loc); | ||
| // Setter key: _a (reuse cached value) | ||
| setter_key = p.newExpr(E.Identifier{ .ref = temp_ref }, loc); |
Contributor
There was a problem hiding this comment.
Prevent temp/backing-name collisions for computed auto-accessors.
The computed backing name only ranges over #a..#z (Line 4887), which can collide with user private names or after 26 accessors, yielding duplicate private identifiers (syntax error). The temp name is always _a (Line 4915), which can clash with user let/const _a when renaming is off, also causing a syntax error. Please make both names collision‑resistant and unique per computed accessor.
🔧 Suggested fix (unique, collision‑resistant names)
- var auto_accessor_count: u32 = 0;
+ var auto_accessor_count: u32 = 0;
+ var computed_key_index: ?u32 = null;
@@
- const offset: u8 = `@intCast`(`@min`(auto_accessor_count, 25));
- const name = std.fmt.allocPrint(p.allocator, "#{c}", .{
- `@as`(u8, 'a' + offset),
- }) catch unreachable;
- auto_accessor_count += 1;
- break :blk name;
+ computed_key_index = auto_accessor_count;
+ auto_accessor_count += 1;
+ const name = std.fmt.allocPrint(
+ p.allocator,
+ "#_auto_accessor_{d}",
+ .{computed_key_index.?},
+ ) catch unreachable;
+ break :blk name;
@@
- const temp_ref = p.newSymbol(.other, "_a") catch unreachable;
+ const temp_name = std.fmt.allocPrint(
+ p.allocator,
+ "_auto_accessor_{d}",
+ .{computed_key_index.?},
+ ) catch unreachable;
+ const temp_ref = p.newSymbol(.other, temp_name) catch unreachable;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn lowerAutoAccessors(p: *P, properties: []G.Property) []G.Property { | |
| var new_props = ListManaged(G.Property).init(p.allocator); | |
| new_props.ensureTotalCapacity(properties.len * 3) catch unreachable; | |
| var auto_accessor_count: u32 = 0; | |
| for (properties) |prop| { | |
| if (prop.kind != .auto_accessor) { | |
| new_props.append(prop) catch unreachable; | |
| continue; | |
| } | |
| const loc = if (prop.key) |k| k.loc else logger.Loc.Empty; | |
| const is_static = prop.flags.contains(.is_static); | |
| const is_computed = prop.flags.contains(.is_computed); | |
| // Derive backing field name from the key | |
| const backing_name: []const u8 = blk: { | |
| if (prop.key != null and prop.key.?.data == .e_private_identifier) { | |
| // accessor #x -> backing field #_x | |
| const orig_name = p.loadNameFromRef(prop.key.?.data.e_private_identifier.ref); | |
| break :blk std.fmt.allocPrint(p.allocator, "#_{s}", .{orig_name[1..]}) catch unreachable; | |
| } else if (!is_computed) { | |
| if (prop.key != null and prop.key.?.data == .e_string) { | |
| // accessor x -> backing field #x | |
| const str = prop.key.?.data.e_string; | |
| break :blk std.fmt.allocPrint(p.allocator, "#{s}", .{str.data}) catch unreachable; | |
| } | |
| } | |
| // Computed key -> counter-based name | |
| const offset: u8 = @intCast(@min(auto_accessor_count, 25)); | |
| const name = std.fmt.allocPrint(p.allocator, "#{c}", .{ | |
| @as(u8, 'a' + offset), | |
| }) catch unreachable; | |
| auto_accessor_count += 1; | |
| break :blk name; | |
| }; | |
| // Create backing field symbol | |
| const backing_ref = p.newSymbol( | |
| if (is_static) .private_static_field else .private_field, | |
| backing_name, | |
| ) catch unreachable; | |
| p.recordDeclaredSymbol(backing_ref) catch unreachable; | |
| // For private auto-accessors, update the original symbol kind | |
| // from .private_field to .private_get_set_pair | |
| if (prop.key != null and prop.key.?.data == .e_private_identifier) { | |
| const orig_ref = prop.key.?.data.e_private_identifier.ref; | |
| p.symbols.items[orig_ref.innerIndex()].kind = | |
| if (is_static) .private_static_get_set_pair else .private_get_set_pair; | |
| } | |
| // For computed keys, cache in a temp variable so the expression | |
| // is only evaluated once (getter uses _a = expr, setter uses _a) | |
| var getter_key = prop.key; | |
| var setter_key = prop.key; | |
| if (is_computed) { | |
| const temp_ref = p.newSymbol(.other, "_a") catch unreachable; | |
| p.recordDeclaredSymbol(temp_ref) catch unreachable; | |
| p.auto_accessor_computed_key_refs.append( | |
| p.allocator, | |
| LocRef{ .loc = loc, .ref = temp_ref }, | |
| ) catch unreachable; | |
| // Getter key: _a = expr (evaluate and cache) | |
| getter_key = p.newExpr(E.Binary{ | |
| .op = .bin_assign, | |
| .left = p.newExpr(E.Identifier{ .ref = temp_ref }, loc), | |
| .right = prop.key.?, | |
| }, loc); | |
| // Setter key: _a (reuse cached value) | |
| setter_key = p.newExpr(E.Identifier{ .ref = temp_ref }, loc); | |
| pub fn lowerAutoAccessors(p: *P, properties: []G.Property) []G.Property { | |
| var new_props = ListManaged(G.Property).init(p.allocator); | |
| new_props.ensureTotalCapacity(properties.len * 3) catch unreachable; | |
| var auto_accessor_count: u32 = 0; | |
| var computed_key_index: ?u32 = null; | |
| for (properties) |prop| { | |
| if (prop.kind != .auto_accessor) { | |
| new_props.append(prop) catch unreachable; | |
| continue; | |
| } | |
| const loc = if (prop.key) |k| k.loc else logger.Loc.Empty; | |
| const is_static = prop.flags.contains(.is_static); | |
| const is_computed = prop.flags.contains(.is_computed); | |
| // Derive backing field name from the key | |
| const backing_name: []const u8 = blk: { | |
| if (prop.key != null and prop.key.?.data == .e_private_identifier) { | |
| // accessor `#x` -> backing field `#_x` | |
| const orig_name = p.loadNameFromRef(prop.key.?.data.e_private_identifier.ref); | |
| break :blk std.fmt.allocPrint(p.allocator, "#_{s}", .{orig_name[1..]}) catch unreachable; | |
| } else if (!is_computed) { | |
| if (prop.key != null and prop.key.?.data == .e_string) { | |
| // accessor x -> backing field `#x` | |
| const str = prop.key.?.data.e_string; | |
| break :blk std.fmt.allocPrint(p.allocator, "#{s}", .{str.data}) catch unreachable; | |
| } | |
| } | |
| // Computed key -> counter-based name | |
| computed_key_index = auto_accessor_count; | |
| auto_accessor_count += 1; | |
| const name = std.fmt.allocPrint( | |
| p.allocator, | |
| "#_auto_accessor_{d}", | |
| .{computed_key_index.?}, | |
| ) catch unreachable; | |
| break :blk name; | |
| }; | |
| // Create backing field symbol | |
| const backing_ref = p.newSymbol( | |
| if (is_static) .private_static_field else .private_field, | |
| backing_name, | |
| ) catch unreachable; | |
| p.recordDeclaredSymbol(backing_ref) catch unreachable; | |
| // For private auto-accessors, update the original symbol kind | |
| // from .private_field to .private_get_set_pair | |
| if (prop.key != null and prop.key.?.data == .e_private_identifier) { | |
| const orig_ref = prop.key.?.data.e_private_identifier.ref; | |
| p.symbols.items[orig_ref.innerIndex()].kind = | |
| if (is_static) .private_static_get_set_pair else .private_get_set_pair; | |
| } | |
| // For computed keys, cache in a temp variable so the expression | |
| // is only evaluated once (getter uses _a = expr, setter uses _a) | |
| var getter_key = prop.key; | |
| var setter_key = prop.key; | |
| if (is_computed) { | |
| const temp_name = std.fmt.allocPrint( | |
| p.allocator, | |
| "_auto_accessor_{d}", | |
| .{computed_key_index.?}, | |
| ) catch unreachable; | |
| const temp_ref = p.newSymbol(.other, temp_name) catch unreachable; | |
| p.recordDeclaredSymbol(temp_ref) catch unreachable; | |
| p.auto_accessor_computed_key_refs.append( | |
| p.allocator, | |
| LocRef{ .loc = loc, .ref = temp_ref }, | |
| ) catch unreachable; | |
| // Getter key: _a = expr (evaluate and cache) | |
| getter_key = p.newExpr(E.Binary{ | |
| .op = .bin_assign, | |
| .left = p.newExpr(E.Identifier{ .ref = temp_ref }, loc), | |
| .right = prop.key.?, | |
| }, loc); | |
| // Setter key: _a (reuse cached value) | |
| setter_key = p.newExpr(E.Identifier{ .ref = temp_ref }, loc); |
🤖 Prompt for AI Agents
In `@src/ast/P.zig` around lines 4858 - 4929, The computed-accessor backing-name
and temp symbol are not unique: change lowerAutoAccessors so the computed
backing_name generation (currently using auto_accessor_count with 'a'..'z')
produces a collision-resistant unique string (e.g., include the
auto_accessor_count numeric suffix or a monotonically increasing id instead of
limiting to 26) and increment the counter deterministically; likewise create the
temp symbol name passed to p.newSymbol (currently "_a") to include the same
unique suffix (e.g., "_auto_accessor_<n>") so each computed accessor gets
distinct backing and temp identifiers; update uses of auto_accessor_count,
backing_name creation, and the temp_ref/newSymbol call to use this unique naming
scheme.
Lower `accessor` class fields into a backing private field + getter/setter
pair, since JavaScriptCore does not support `accessor` natively.
- `accessor x = 1` → `#x = 1; get x() { return this.#x; } set x(_) { this.#x = _; }`
- `accessor #x` → `#_x; get #x() { return this.#_x; } set #x(_) { this.#_x = _; }`
- `accessor [expr]` → `#a; get [_a = expr]() { return this.#a; } set [_a](_) { this.#a = _; }`
Computed keys are cached in temp variables to avoid evaluating
side-effecting expressions twice, matching esbuild's behavior.
Co-Authored-By: Claude <noreply@anthropic.com>
Jarred-Sumner
force-pushed
the
jarred/auto-accessor
branch
from
January 25, 2026 02:49
2017ee0 to
351a2ff
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js_printer.zig (1)
3488-3499: Keep JSON-mode invariant for auto_accessor.
In JSON mode, auto-accessors should be impossible; add an explicit unreachable to avoid silently emitting invalid JSON if the AST leaks this kind.🛠️ Proposed safeguard
- if (item.kind != .normal and item.kind != .auto_accessor) { + if (comptime is_json and item.kind == .auto_accessor) { + bun.unreachablePanic("item.kind must be normal in json", .{}); + } + if (item.kind != .normal and item.kind != .auto_accessor) {
🤖 Fix all issues with AI agents
In `@src/ast/visit.zig`:
- Around line 848-856: The loop emitting temps for
auto_accessor_computed_key_refs relies on S.Local's default kind but should set
the Var kind explicitly; update the creation of the G.Decl for each temp in the
loop that builds p.s(S.Local{ .decls = G.Decl.List.fromOwnedSlice(decls) },
temp.loc) so the decl includes the explicit kind `.k_var` (i.e., set the
Decl.kind field when constructing the G.Decl for each temp), matching nearby
patterns that explicitly set `.kind` for local declarations and ensuring
consistency for the auto_accessor_computed_key_refs handling.
In `@src/ast/visitStmt.zig`:
- Around line 590-595: The loop emits local declarations using p.s(S.Local{
.decls = ... }) but relies on the default S.Local.kind; update the S.Local
construction to explicitly set .kind = .k_var to match project conventions
(i.e., call p.s with S.Local{ .kind = .k_var, .decls =
Decl.List.fromOwnedSlice(decls) }) when appending stmts for
p.auto_accessor_computed_key_refs so intent and consistency with other sites
like convertStmtsForChunkForDevServer.zig and visit.zig are clear.
♻️ Duplicate comments (2)
src/ast/P.zig (1)
4861-4920: Make computed auto‑accessor names collision‑resistant.
Computed backing names are limited to#a..#z, and the temp symbol is always_a. This can collide with user private names or multiple computed accessors, causing syntax errors. Please make both names unique per computed accessor.🔧 Suggested fix (unique names)
- var auto_accessor_count: u32 = 0; + var auto_accessor_count: u32 = 0; + var computed_key_index: ?u32 = null; @@ - const offset: u8 = `@intCast`(`@min`(auto_accessor_count, 25)); - const name = std.fmt.allocPrint(p.allocator, "#{c}", .{ - `@as`(u8, 'a' + offset), - }) catch unreachable; - auto_accessor_count += 1; + computed_key_index = auto_accessor_count; + auto_accessor_count += 1; + const name = std.fmt.allocPrint( + p.allocator, + "#_auto_accessor_{d}", + .{computed_key_index.?}, + ) catch unreachable; break :blk name; @@ - const temp_ref = p.newSymbol(.other, "_a") catch unreachable; + const temp_name = std.fmt.allocPrint( + p.allocator, + "_auto_accessor_{d}", + .{computed_key_index.?}, + ) catch unreachable; + const temp_ref = p.newSymbol(.other, temp_name) catch unreachable;src/ast/parseProperty.zig (1)
302-310: Add validation to reject method-form auto-accessors (accessor foo()).The parser correctly prevents field parsing when
p.lexer.token == .t_open_paren(line 444), but it still callsparseMethodExpression()withkind == .auto_accessor. This function preserves thekindfield (line 114) and marksis_method = true, creating a property with both flags set. ThelowerAutoAccessors()function lacks a guard for this case and would attempt to lower a method as an auto-accessor field, producing invalid output.Add a validation in
parseMethodExpression()to reject whenkind == .auto_accessor(since auto-accessors must be field declarations, not methods):if (kind == .auto_accessor) { p.log.addRangeError(p.source, key_range, "Auto-accessor cannot be a method") catch unreachable; // Return appropriate error property or null }Alternatively, add a guard in
lowerAutoAccessors()to skip properties withis_method == true.Also add a test case to verify
accessor foo()is rejected as invalid syntax.
Comment on lines
+848
to
+856
| // Emit var declarations for auto-accessor computed key temps | ||
| // that were generated from class expressions during this statement. | ||
| // (Class statement temps are handled in visitAndAppendStmt directly.) | ||
| // var is hoisted so placement within the statement list is fine. | ||
| for (p.auto_accessor_computed_key_refs.items) |temp| { | ||
| const decls = p.allocator.alloc(G.Decl, 1) catch unreachable; | ||
| decls[0] = .{ .binding = p.b(B.Identifier{ .ref = temp.ref.? }, temp.loc) }; | ||
| try list.append(p.s(S.Local{ .decls = G.Decl.List.fromOwnedSlice(decls) }, temp.loc)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find S.Local struct definition
rg -n "struct Local|pub const Local" -g '*.zig' src --max-count=20Repository: oven-sh/bun
Length of output: 686
🏁 Script executed:
#!/bin/bash
# Search for S.Local usage patterns in visit.zig to see if .kind is typically explicit
rg -n "S\.Local\{" -g '*.zig' src/ast/visit.zig -A 3 | head -60Repository: oven-sh/bun
Length of output: 683
🏁 Script executed:
#!/bin/bash
# Look for the definition of S namespace and Local struct more broadly
fd -g '*.zig' src | xargs grep -l "pub.*struct.*Local" | head -5Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Get the S.Local struct definition from src/ast/S.zig
sed -n '171,200p' src/ast/S.zigRepository: oven-sh/bun
Length of output: 964
🏁 Script executed:
#!/bin/bash
# Get more context around S.Local definition
sed -n '165,210p' src/ast/S.zigRepository: oven-sh/bun
Length of output: 1295
Make temp decl kind explicit (.k_var).
These computed-key temps shouldn't depend on S.Local defaults. The default is .k_var, which is the intended value here, but explicit declaration aligns with how .kind is set in nearby code (lines 944–946 and 961–963).
♻️ Proposed tweak
- try list.append(p.s(S.Local{ .decls = G.Decl.List.fromOwnedSlice(decls) }, temp.loc));
+ try list.append(p.s(S.Local{
+ .kind = .k_var,
+ .decls = G.Decl.List.fromOwnedSlice(decls),
+ }, temp.loc));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Emit var declarations for auto-accessor computed key temps | |
| // that were generated from class expressions during this statement. | |
| // (Class statement temps are handled in visitAndAppendStmt directly.) | |
| // var is hoisted so placement within the statement list is fine. | |
| for (p.auto_accessor_computed_key_refs.items) |temp| { | |
| const decls = p.allocator.alloc(G.Decl, 1) catch unreachable; | |
| decls[0] = .{ .binding = p.b(B.Identifier{ .ref = temp.ref.? }, temp.loc) }; | |
| try list.append(p.s(S.Local{ .decls = G.Decl.List.fromOwnedSlice(decls) }, temp.loc)); | |
| } | |
| // Emit var declarations for auto-accessor computed key temps | |
| // that were generated from class expressions during this statement. | |
| // (Class statement temps are handled in visitAndAppendStmt directly.) | |
| // var is hoisted so placement within the statement list is fine. | |
| for (p.auto_accessor_computed_key_refs.items) |temp| { | |
| const decls = p.allocator.alloc(G.Decl, 1) catch unreachable; | |
| decls[0] = .{ .binding = p.b(B.Identifier{ .ref = temp.ref.? }, temp.loc) }; | |
| try list.append(p.s(S.Local{ | |
| .kind = .k_var, | |
| .decls = G.Decl.List.fromOwnedSlice(decls), | |
| }, temp.loc)); | |
| } |
🤖 Prompt for AI Agents
In `@src/ast/visit.zig` around lines 848 - 856, The loop emitting temps for
auto_accessor_computed_key_refs relies on S.Local's default kind but should set
the Var kind explicitly; update the creation of the G.Decl for each temp in the
loop that builds p.s(S.Local{ .decls = G.Decl.List.fromOwnedSlice(decls) },
temp.loc) so the decl includes the explicit kind `.k_var` (i.e., set the
Decl.kind field when constructing the G.Decl for each temp), matching nearby
patterns that explicitly set `.kind` for local declarations and ensuring
consistency for the auto_accessor_computed_key_refs handling.
Comment on lines
+590
to
+595
| // Emit var declarations for auto-accessor computed key temps | ||
| for (p.auto_accessor_computed_key_refs.items) |temp| { | ||
| const decls = p.allocator.alloc(Decl, 1) catch unreachable; | ||
| decls[0] = .{ .binding = Binding.alloc(p.allocator, B.Identifier{ .ref = temp.ref.? }, temp.loc) }; | ||
| stmts.append(p.s(S.Local{ .decls = Decl.List.fromOwnedSlice(decls) }, temp.loc)) catch unreachable; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Find S.Local struct definition
rg -n "pub const Local\|struct Local" -A 10 -g '*.zig' src | head -50Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Search for k_var enumeration
rg -n "k_var" -B 2 -A 2 -g '*.zig' src | head -50Repository: oven-sh/bun
Length of output: 3675
🏁 Script executed:
# Look at the broader context of S.Local usage in the file
sed -n '585,600p' src/ast/visitStmt.zigRepository: oven-sh/bun
Length of output: 949
🏁 Script executed:
# Search for where S is defined or S.Local is declared
rg -n "^pub const S\|^pub struct S" -A 5 -g '*.zig' src | head -100Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Try searching in js_ast file
fd -n "js_ast" -g '*.zig' src | xargs rg -l "Local"Repository: oven-sh/bun
Length of output: 17217
🏁 Script executed:
# Look for where Local kind enum is defined
rg -n "kind.*:" -g '*.zig' src/ast | grep -i localRepository: oven-sh/bun
Length of output: 367
🏁 Script executed:
# Find the Local struct in S.zig
rg -n "pub const Local\|^ Local" -A 15 src/ast/S.zig | head -80Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check S.zig and search for Local more broadly
grep -n "Local\|kind" src/ast/S.zig | head -50Repository: oven-sh/bun
Length of output: 385
🏁 Script executed:
# Let's read S.zig to find Local definition
wc -l src/ast/S.zigRepository: oven-sh/bun
Length of output: 72
Consider explicitly setting .kind = .k_var to match codebase patterns.
While S.Local.kind defaults to .k_var (as defined in S.zig), other parts of the codebase explicitly set this field when creating var declarations (e.g., convertStmtsForChunkForDevServer.zig, visit.zig). Making it explicit here would clarify intent and improve consistency.
♻️ Suggested improvement
- stmts.append(p.s(S.Local{ .decls = Decl.List.fromOwnedSlice(decls) }, temp.loc)) catch unreachable;
+ stmts.append(p.s(S.Local{
+ .kind = .k_var,
+ .decls = Decl.List.fromOwnedSlice(decls),
+ }, temp.loc)) catch unreachable;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Emit var declarations for auto-accessor computed key temps | |
| for (p.auto_accessor_computed_key_refs.items) |temp| { | |
| const decls = p.allocator.alloc(Decl, 1) catch unreachable; | |
| decls[0] = .{ .binding = Binding.alloc(p.allocator, B.Identifier{ .ref = temp.ref.? }, temp.loc) }; | |
| stmts.append(p.s(S.Local{ .decls = Decl.List.fromOwnedSlice(decls) }, temp.loc)) catch unreachable; | |
| } | |
| // Emit var declarations for auto-accessor computed key temps | |
| for (p.auto_accessor_computed_key_refs.items) |temp| { | |
| const decls = p.allocator.alloc(Decl, 1) catch unreachable; | |
| decls[0] = .{ .binding = Binding.alloc(p.allocator, B.Identifier{ .ref = temp.ref.? }, temp.loc) }; | |
| stmts.append(p.s(S.Local{ | |
| .kind = .k_var, | |
| .decls = Decl.List.fromOwnedSlice(decls), | |
| }, temp.loc)) catch unreachable; | |
| } |
🤖 Prompt for AI Agents
In `@src/ast/visitStmt.zig` around lines 590 - 595, The loop emits local
declarations using p.s(S.Local{ .decls = ... }) but relies on the default
S.Local.kind; update the S.Local construction to explicitly set .kind = .k_var
to match project conventions (i.e., call p.s with S.Local{ .kind = .k_var,
.decls = Decl.List.fromOwnedSlice(decls) }) when appending stmts for
p.auto_accessor_computed_key_refs so intent and consistency with other sites
like convertStmtsForChunkForDevServer.zig and visit.zig are clear.
Generated backing field names for computed auto-accessors (#a, #b, ...) now skip names already used by existing private fields in the class. Private names are not subject to automatic renaming, so collisions would produce duplicate private name errors at runtime. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/ast/P.zig`:
- Around line 4873-4884: The generated backing field names for non-computed
accessors (code around lowerAutoAccessors producing backing_name from prop.key
and patterns like "#_{s}" and "#{s}") can collide with user-declared private
identifiers; add a collision check that scans the current props for existing
.e_private_identifier names (use p.loadNameFromRef to read each private name)
and, when a candidate backing_name matches an existing private identifier, fall
back to creating a unique name (e.g., append a suffix or generate a unique id)
instead of returning the colliding "#..." name; implement this as a small helper
(e.g., privateNameCollides) and invoke it in the branches that produce
backing_name for private_identifier and .e_string keys.
♻️ Duplicate comments (1)
src/ast/P.zig (1)
4873-4909: Fix computed auto-accessor name generation to avoid hangs/duplicates.Computed backing names are capped at
#a..#zand don’t account for previously generated names. If#zexists, the loop can become infinite; if there are >26 computed accessors, you’ll emit duplicate private names. The temp ref_ais also constant, so it can collide when the renamer is off. This is still the same collision class flagged earlier.🔧 Proposed fix (unique backing + temp names)
- var auto_accessor_count: u32 = 0; + var auto_accessor_count: u32 = 0; for (properties) |prop| { + var computed_key_index: ?u32 = null; ... // Computed key -> counter-based name, skipping any // that collide with existing private names in the class // (private names are not subject to automatic renaming). - while (true) : (auto_accessor_count += 1) { - const offset: u8 = `@intCast`(`@min`(auto_accessor_count, 25)); - const candidate = std.fmt.allocPrint(p.allocator, "#{c}", .{ - `@as`(u8, 'a' + offset), - }) catch unreachable; + while (true) { + const candidate_index = auto_accessor_count; + auto_accessor_count += 1; + const candidate = std.fmt.allocPrint( + p.allocator, + "#_auto_accessor_{d}", + .{candidate_index}, + ) catch unreachable; var collides = false; for (properties) |other| { if (other.key != null and other.key.?.data == .e_private_identifier) { const other_name = p.loadNameFromRef(other.key.?.data.e_private_identifier.ref); if (strings.eql(candidate, other_name)) { collides = true; break; } } } if (!collides) { - auto_accessor_count += 1; + computed_key_index = candidate_index; break :blk candidate; } } ... if (is_computed) { - const temp_ref = p.newSymbol(.other, "_a") catch unreachable; + const temp_name = std.fmt.allocPrint( + p.allocator, + "_auto_accessor_{d}", + .{computed_key_index.?}, + ) catch unreachable; + const temp_ref = p.newSymbol(.other, temp_name) catch unreachable;Also applies to: 4932-4947
Comment on lines
+4873
to
+4884
| // Derive backing field name from the key | ||
| const backing_name: []const u8 = blk: { | ||
| if (prop.key != null and prop.key.?.data == .e_private_identifier) { | ||
| // accessor #x -> backing field #_x | ||
| const orig_name = p.loadNameFromRef(prop.key.?.data.e_private_identifier.ref); | ||
| break :blk std.fmt.allocPrint(p.allocator, "#_{s}", .{orig_name[1..]}) catch unreachable; | ||
| } else if (!is_computed) { | ||
| if (prop.key != null and prop.key.?.data == .e_string) { | ||
| // accessor x -> backing field #x | ||
| const str = prop.key.?.data.e_string; | ||
| break :blk std.fmt.allocPrint(p.allocator, "#{s}", .{str.data}) catch unreachable; | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid backing-field collisions with existing private names.
For non-computed accessors, #${name} (and #_${name} for private accessors) can collide with user-declared private fields, producing invalid JS due to duplicate private identifiers. Please guard these paths and fall back to a unique name when a collision is detected.
🔧 Example fix (fallback to unique name on collision)
const backing_name: []const u8 = blk: {
if (prop.key != null and prop.key.?.data == .e_private_identifier) {
// accessor `#x` -> backing field `#_x`
const orig_name = p.loadNameFromRef(prop.key.?.data.e_private_identifier.ref);
- break :blk std.fmt.allocPrint(p.allocator, "#_{s}", .{orig_name[1..]}) catch unreachable;
+ const candidate = std.fmt.allocPrint(p.allocator, "#_{s}", .{orig_name[1..]}) catch unreachable;
+ if (!privateNameCollides(p, properties, candidate)) break :blk candidate;
} else if (!is_computed) {
if (prop.key != null and prop.key.?.data == .e_string) {
// accessor x -> backing field `#x`
const str = prop.key.?.data.e_string;
- break :blk std.fmt.allocPrint(p.allocator, "#{s}", .{str.data}) catch unreachable;
+ const candidate = std.fmt.allocPrint(p.allocator, "#{s}", .{str.data}) catch unreachable;
+ if (!privateNameCollides(p, properties, candidate)) break :blk candidate;
}
}
// Computed key -> counter-based name, skipping any
// that collide with existing private names in the class// add a small local helper near the top of lowerAutoAccessors
const privateNameCollides = struct {
fn check(p: *P, props: []G.Property, candidate: []const u8) bool {
for (props) |other| {
if (other.key != null and other.key.?.data == .e_private_identifier) {
const other_name = p.loadNameFromRef(other.key.?.data.e_private_identifier.ref);
if (strings.eql(candidate, other_name)) return true;
}
}
return false;
}
}.check;🤖 Prompt for AI Agents
In `@src/ast/P.zig` around lines 4873 - 4884, The generated backing field names
for non-computed accessors (code around lowerAutoAccessors producing
backing_name from prop.key and patterns like "#_{s}" and "#{s}") can collide
with user-declared private identifiers; add a collision check that scans the
current props for existing .e_private_identifier names (use p.loadNameFromRef to
read each private name) and, when a candidate backing_name matches an existing
private identifier, fall back to creating a unique name (e.g., append a suffix
or generate a unique id) instead of returning the colliding "#..." name;
implement this as a small helper (e.g., privateNameCollides) and invoke it in
the branches that produce backing_name for private_identifier and .e_string
keys.
Collaborator
|
Closing as stale: this PR predates the Rust rewrite. Every If the underlying change is still wanted, it will need to be redone against the current Rust/C++ tree. Apologies for the churn, and thank you for the contribution. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6051
Summary
accessorclass fields into a backing private field + getter/setter pair, since JavaScriptCore does not supportaccessornativelyaccessor #x→ backing field#_xwith private getter/setter pair)Transformations
Files changed
src/ast/G.zig— Addauto_accessorproperty kindsrc/ast/P.zig— AddlowerAutoAccessorsfunctionsrc/ast/parseProperty.zig— Parseaccessormodifier keywordsrc/ast/visit.zig— Call lowering invisitClass, emit computed key temp varssrc/ast/visitStmt.zig— Emit computed key temp var declarations for class statementssrc/js_lexer_tables.zig— Addaccessorto property modifier keywordssrc/js_printer.zig— Printaccessorkeyword for pass-through casesTest plan
TestLowerAutoAccessorscases ported (tests 9-14 involve private field polyfilling which JSC doesn't need)declare accessor,abstract accessor, type annotations, visibility modifiers)accessor()as method,accessor = 1as property)🤖 Generated with Claude Code