From 6608b65100648a1ef5ac1951a744b4f73756378d Mon Sep 17 00:00:00 2001 From: Matthew Lugg Date: Wed, 15 Apr 2026 23:55:52 +0100 Subject: [PATCH] incremental: fix tracking of nested container declarations ...but not in the way you'd expect. We were actually tracking them in cases where we shouldn't have been! We cannot track a declaration if its parent namespace has been lost, because that will cause analysis failures immediately, but if we excluded a type from the mapping due to a major change (such as a struct turning into a union, or a field being added), we were still including any trackable instructions inside the container's field expressions (e.g. struct field type expressions). This meant we were tracking a type declaration while losing tracking on its parent namespace, with predictably disastrous results. Oh, also, tracking for opaque types was just totally wrong (I think this was a typo from a while back). We could map it to things other than opaque declarations, and we never mapped declarations inside opaques. So, uh, I fixed that too. --- lib/std/zig/Zir.zig | 97 ++++++++++--------- src/Zcu.zig | 90 ++++++++--------- ..._field_and_nested_struct_uses_changed_decl | 25 +++++ test/incremental/do_nothing | 41 ++++++++ 4 files changed, 162 insertions(+), 91 deletions(-) create mode 100644 test/incremental/add_field_and_nested_struct_uses_changed_decl create mode 100644 test/incremental/do_nothing diff --git a/lib/std/zig/Zir.zig b/lib/std/zig/Zir.zig index bcded94045..4b683e2762 100644 --- a/lib/std/zig/Zir.zig +++ b/lib/std/zig/Zir.zig @@ -4033,30 +4033,30 @@ pub const DeclContents = struct { /// This is a simple optional because ZIR guarantees that a `func`/`func_inferred`/`func_fancy` instruction /// can only occur once per `declaration`. func_decl: ?Inst.Index, - explicit_types: std.ArrayList(Inst.Index), + type_decls: std.ArrayList(Inst.Index), other: std.ArrayList(Inst.Index), pub const init: DeclContents = .{ .func_decl = null, - .explicit_types = .empty, + .type_decls = .empty, .other = .empty, }; pub fn clear(contents: *DeclContents) void { contents.func_decl = null; - contents.explicit_types.clearRetainingCapacity(); + contents.type_decls.clearRetainingCapacity(); contents.other.clearRetainingCapacity(); } pub fn deinit(contents: *DeclContents, gpa: Allocator) void { - contents.explicit_types.deinit(gpa); + contents.type_decls.deinit(gpa); contents.other.deinit(gpa); } }; /// Find all tracked ZIR instructions, recursively, within a `declaration` instruction. Does not recurse through /// nested declarations; to find all declarations, call this function recursively on the type declarations discovered -/// in `contents.explicit_types`. +/// in `contents.type_decls`. /// /// This populates an `ArrayList` because an iterator would need to allocate memory anyway. pub fn findTrackable(zir: Zir, gpa: Allocator, contents: *DeclContents, decl_inst: Zir.Inst.Index) !void { @@ -4076,15 +4076,49 @@ pub fn findTrackable(zir: Zir, gpa: Allocator, contents: *DeclContents, decl_ins if (decl.value_body) |b| try zir.findTrackableBody(gpa, contents, &found_defers, b); } -/// Like `findTrackable`, but only considers the `main_struct_inst` instruction. This may return more than -/// just that instruction because it will also traverse fields. -pub fn findTrackableRoot(zir: Zir, gpa: Allocator, contents: *DeclContents) !void { +/// `findTrackable` does not recurse into field expressions in a type. Instead, this function will +/// scan specifically field expressions in a given type declaration for trackable ZIR instructions. +pub fn findTrackableFields( + zir: *const Zir, + gpa: Allocator, + contents: *DeclContents, + type_decl_inst: Zir.Inst.Index, +) Allocator.Error!void { contents.clear(); var found_defers: std.AutoHashMapUnmanaged(u32, void) = .empty; defer found_defers.deinit(gpa); - try zir.findTrackableInner(gpa, contents, &found_defers, .main_struct_inst); + assert(zir.instructions.items(.tag)[@intFromEnum(type_decl_inst)] == .extended); + switch (zir.instructions.items(.data)[@intFromEnum(type_decl_inst)].extended.opcode) { + .struct_decl => { + const struct_decl = zir.getStructDecl(type_decl_inst); + var it = struct_decl.iterateFields(); + while (it.next()) |field| { + try zir.findTrackableBody(gpa, contents, &found_defers, field.type_body); + if (field.align_body) |b| try zir.findTrackableBody(gpa, contents, &found_defers, b); + if (field.default_body) |b| try zir.findTrackableBody(gpa, contents, &found_defers, b); + } + }, + .union_decl => { + const union_decl = zir.getUnionDecl(type_decl_inst); + var it = union_decl.iterateFields(); + while (it.next()) |field| { + if (field.type_body) |b| try zir.findTrackableBody(gpa, contents, &found_defers, b); + if (field.align_body) |b| try zir.findTrackableBody(gpa, contents, &found_defers, b); + if (field.value_body) |b| try zir.findTrackableBody(gpa, contents, &found_defers, b); + } + }, + .enum_decl => { + const enum_decl = zir.getEnumDecl(type_decl_inst); + var it = enum_decl.iterateFields(); + while (it.next()) |field| { + if (field.value_body) |b| try zir.findTrackableBody(gpa, contents, &found_defers, b); + } + }, + .opaque_decl => {}, + else => unreachable, + } } fn findTrackableInner( @@ -4411,49 +4445,18 @@ fn findTrackableInner( try zir.findTrackableBody(gpa, contents, defers, body); }, - // Reifications and opaque declarations need tracking, but have no bodies. + // Reifications need tracking. .reify_enum, .reify_struct, .reify_union, - .opaque_decl, => return contents.other.append(gpa, inst), - // Struct declarations need tracking and have bodies. - .struct_decl => { - try contents.explicit_types.append(gpa, inst); - - const struct_decl = zir.getStructDecl(inst); - var it = struct_decl.iterateFields(); - while (it.next()) |field| { - try zir.findTrackableBody(gpa, contents, defers, field.type_body); - if (field.align_body) |b| try zir.findTrackableBody(gpa, contents, defers, b); - if (field.default_body) |b| try zir.findTrackableBody(gpa, contents, defers, b); - } - }, - - // Union declarations need tracking and have bodies. - .union_decl => { - try contents.explicit_types.append(gpa, inst); - - const union_decl = zir.getUnionDecl(inst); - var it = union_decl.iterateFields(); - while (it.next()) |field| { - if (field.type_body) |b| try zir.findTrackableBody(gpa, contents, defers, b); - if (field.align_body) |b| try zir.findTrackableBody(gpa, contents, defers, b); - if (field.value_body) |b| try zir.findTrackableBody(gpa, contents, defers, b); - } - }, - - // Enum declarations need tracking and have bodies. - .enum_decl => { - try contents.explicit_types.append(gpa, inst); - - const enum_decl = zir.getEnumDecl(inst); - var it = enum_decl.iterateFields(); - while (it.next()) |field| { - if (field.value_body) |b| try zir.findTrackableBody(gpa, contents, defers, b); - } - }, + // Type declarations need tracking. + .struct_decl, + .union_decl, + .enum_decl, + .opaque_decl, + => return contents.type_decls.append(gpa, inst), } }, diff --git a/src/Zcu.zig b/src/Zcu.zig index 88258b236a..9dae765341 100644 --- a/src/Zcu.zig +++ b/src/Zcu.zig @@ -3361,8 +3361,8 @@ pub fn mapOldZirToNew( old_inst: Zir.Inst.Index, new_inst: Zir.Inst.Index, }; - var match_stack: std.ArrayList(MatchedZirDecl) = .empty; - defer match_stack.deinit(gpa); + var pending_matched_type_decls: std.ArrayList(MatchedZirDecl) = .empty; + defer pending_matched_type_decls.deinit(gpa); // Used as temporary buffers for namespace declaration instructions var old_contents: Zir.DeclContents = .init; @@ -3370,42 +3370,13 @@ pub fn mapOldZirToNew( var new_contents: Zir.DeclContents = .init; defer new_contents.deinit(gpa); - // Map the main struct inst (and anything in its fields) - { - try old_zir.findTrackableRoot(gpa, &old_contents); - try new_zir.findTrackableRoot(gpa, &new_contents); + // Map the main struct inst to start off with. + try pending_matched_type_decls.append(gpa, .{ + .old_inst = .main_struct_inst, + .new_inst = .main_struct_inst, + }); - assert(old_contents.explicit_types.items[0] == .main_struct_inst); - assert(new_contents.explicit_types.items[0] == .main_struct_inst); - - assert(old_contents.func_decl == null); - assert(new_contents.func_decl == null); - - // We don't have any smart way of matching up these instructions, so we correlate them based on source order - // in their respective arrays. - - const num_explicit_types = @min(old_contents.explicit_types.items.len, new_contents.explicit_types.items.len); - try match_stack.ensureUnusedCapacity(gpa, @intCast(num_explicit_types)); - for ( - old_contents.explicit_types.items[0..num_explicit_types], - new_contents.explicit_types.items[0..num_explicit_types], - ) |old_inst, new_inst| { - // Here we use `match_stack`, so that we will recursively consider declarations on these types. - match_stack.appendAssumeCapacity(.{ .old_inst = old_inst, .new_inst = new_inst }); - } - - const num_other = @min(old_contents.other.items.len, new_contents.other.items.len); - try inst_map.ensureUnusedCapacity(gpa, @intCast(num_other)); - for ( - old_contents.other.items[0..num_other], - new_contents.other.items[0..num_other], - ) |old_inst, new_inst| { - // These instructions don't have declarations, so we just modify `inst_map` directly. - inst_map.putAssumeCapacity(old_inst, new_inst); - } - } - - while (match_stack.pop()) |match_item| { + while (pending_matched_type_decls.pop()) |match_item| { // There are some properties of type declarations which cannot change across incremental // updates. If they have, we need to ignore this mapping. These properties are essentially // everything passed into `InternPool.getDeclaredStructType` (likewise for unions, enums, @@ -3461,9 +3432,41 @@ pub fn mapOldZirToNew( else => unreachable, } - // Match the namespace declaration itself + // Match the container declaration itself try inst_map.put(gpa, match_item.old_inst, match_item.new_inst); + { + // First, map the fields... + try old_zir.findTrackableFields(gpa, &old_contents, match_item.old_inst); + try new_zir.findTrackableFields(gpa, &new_contents, match_item.new_inst); + + // This isn't a `.declaration`, so we shouldn't see a function declaration. + assert(old_contents.func_decl == null); + assert(new_contents.func_decl == null); + + // We don't have any smart way of matching up these instructions, so we correlate them based on source order + // in their respective arrays. + + const num_type_decls = @min(old_contents.type_decls.items.len, new_contents.type_decls.items.len); + try pending_matched_type_decls.ensureUnusedCapacity(gpa, @intCast(num_type_decls)); + for ( + old_contents.type_decls.items[0..num_type_decls], + new_contents.type_decls.items[0..num_type_decls], + ) |old_inst, new_inst| { + pending_matched_type_decls.appendAssumeCapacity(.{ .old_inst = old_inst, .new_inst = new_inst }); + } + + const num_other = @min(old_contents.other.items.len, new_contents.other.items.len); + try inst_map.ensureUnusedCapacity(gpa, @intCast(num_other)); + for ( + old_contents.other.items[0..num_other], + new_contents.other.items[0..num_other], + ) |old_inst, new_inst| { + // These instructions don't have declarations, so we just modify `inst_map` directly. + inst_map.putAssumeCapacity(old_inst, new_inst); + } + } + // Maps decl name to `declaration` instruction. var named_decls: std.StringHashMapUnmanaged(Zir.Inst.Index) = .empty; defer named_decls.deinit(gpa); @@ -3537,14 +3540,13 @@ pub fn mapOldZirToNew( // We don't have any smart way of matching up these instructions, so we correlate them based on source order // in their respective arrays. - const num_explicit_types = @min(old_contents.explicit_types.items.len, new_contents.explicit_types.items.len); - try match_stack.ensureUnusedCapacity(gpa, @intCast(num_explicit_types)); + const num_type_decls = @min(old_contents.type_decls.items.len, new_contents.type_decls.items.len); + try pending_matched_type_decls.ensureUnusedCapacity(gpa, @intCast(num_type_decls)); for ( - old_contents.explicit_types.items[0..num_explicit_types], - new_contents.explicit_types.items[0..num_explicit_types], + old_contents.type_decls.items[0..num_type_decls], + new_contents.type_decls.items[0..num_type_decls], ) |old_inst, new_inst| { - // Here we use `match_stack`, so that we will recursively consider declarations on these types. - match_stack.appendAssumeCapacity(.{ .old_inst = old_inst, .new_inst = new_inst }); + pending_matched_type_decls.appendAssumeCapacity(.{ .old_inst = old_inst, .new_inst = new_inst }); } const num_other = @min(old_contents.other.items.len, new_contents.other.items.len); diff --git a/test/incremental/add_field_and_nested_struct_uses_changed_decl b/test/incremental/add_field_and_nested_struct_uses_changed_decl new file mode 100644 index 0000000000..35882c10a3 --- /dev/null +++ b/test/incremental/add_field_and_nested_struct_uses_changed_decl @@ -0,0 +1,25 @@ +#update=initial version +#file=main.zig +pub fn main() void { + _ = @as(S, undefined); +} +// To reproduce the original bug, the inner struct must perform a namespace lookup +// or a scope lookup when resolving its field type. +const SomeType = u8; +const S = struct { + foo: struct { inner: SomeType }, +}; +#expect_stdout="" +#update=add field to outer struct, change decl used by inner struct +#file=main.zig +pub fn main() void { + _ = @as(S, undefined); +} +// To reproduce the original bug, the inner struct must perform a namespace lookup +// or a scope lookup when resolving its field type. +const SomeType = u16; +const S = struct { + foo: struct { inner: SomeType }, + bar: u32, +}; +#expect_stdout="" diff --git a/test/incremental/do_nothing b/test/incremental/do_nothing new file mode 100644 index 0000000000..6908984019 --- /dev/null +++ b/test/incremental/do_nothing @@ -0,0 +1,41 @@ +// TODO: it'd be great if we could actually check that no analysis happened! +#update=initial version +#file=main.zig +pub fn main() void { + const ptr: *const O = @ptrFromInt(0x1000); + _ = ptr; +} +const S = struct { foo: u32, nested: struct { x: u16 } }; +const U = union(enum) { a, b, c: S }; +const E = enum(u8) { a = @typeInfo(U).@"union".fields.len, b = 0, c }; +const O = opaque { + comptime { + _ = @as(S, undefined); + _ = @as(U, undefined); + _ = @as(E, undefined); + const Wrapper = struct { val: S }; + const wrapper: Wrapper = .{ .val = .{ .foo = 123, .nested = .{ .x = 456 } } }; + _ = wrapper; + } +}; +#expect_stdout="" +#update=do literally nothing +#file=main.zig +pub fn main() void { + const ptr: *const O = @ptrFromInt(0x1000); + _ = ptr; +} +const S = struct { foo: u32, nested: struct { x: u16 } }; +const U = union(enum) { a, b, c: S }; +const E = enum(u8) { a = @typeInfo(U).@"union".fields.len, b = 0, c }; +const O = opaque { + comptime { + _ = @as(S, undefined); + _ = @as(U, undefined); + _ = @as(E, undefined); + const Wrapper = struct { val: S }; + const wrapper: Wrapper = .{ .val = .{ .foo = 123, .nested = .{ .x = 456 } } }; + _ = wrapper; + } +}; +#expect_stdout=""