diff --git a/examples/issue-0056-diamond-param-impl.sx b/examples/issue-0056-diamond-param-impl.sx new file mode 100644 index 0000000..ddcf0c6 --- /dev/null +++ b/examples/issue-0056-diamond-param-impl.sx @@ -0,0 +1,19 @@ +// Regression (issue 0056): a parameterised-protocol (`Into`) impl living in a +// module reached through more than one import path must register exactly once. +// +// main ─┬─ issue-0056/mid_a ─┐ +// └─ issue-0056/mid_b ─┴─ issue-0056/common (holds `impl Into(Wrapped) for s64`) +// +// Impl blocks are anonymous (no `declName`), so before the fix the diamond +// dedup in imports.zig appended the cached node once per path and the second +// registration tripped: `duplicate impl 'Into' for source 's64'`. Now the flat +// decl list also dedups by node identity, so this builds and prints 7. +#import "modules/std.sx"; +#import "issue-0056/mid_a.sx"; +#import "issue-0056/mid_b.sx"; + +main :: () -> s32 { + w : Wrapped = xx 7; + print("{}\n", w.v); + 0; +} diff --git a/examples/issue-0056/common.sx b/examples/issue-0056/common.sx new file mode 100644 index 0000000..6c18d61 --- /dev/null +++ b/examples/issue-0056/common.sx @@ -0,0 +1,12 @@ +// A parameterised-protocol (`Into`) impl living in a module reachable through +// more than one import path. Each path must NOT re-register the impl. +// Impl blocks are anonymous (`declName() == null`), so the diamond-import +// dedup in imports.zig (`mergeFlat`) skips them and appends the node once per +// path — `registerParamImpl` then trips its same-file duplicate check. +Wrapped :: struct { v: s64; } + +impl Into(Wrapped) for s64 { + convert :: (self: s64) -> Wrapped { + return .{ v = self }; + } +} diff --git a/examples/issue-0056/mid_a.sx b/examples/issue-0056/mid_a.sx new file mode 100644 index 0000000..a7a5ea8 --- /dev/null +++ b/examples/issue-0056/mid_a.sx @@ -0,0 +1,3 @@ +#import "common.sx"; + +mid_a_marker :: () -> s64 { 1; } diff --git a/examples/issue-0056/mid_b.sx b/examples/issue-0056/mid_b.sx new file mode 100644 index 0000000..4708cfe --- /dev/null +++ b/examples/issue-0056/mid_b.sx @@ -0,0 +1,3 @@ +#import "common.sx"; + +mid_b_marker :: () -> s64 { 2; } diff --git a/issues/0056-param-impl-not-deduped-across-diamond-import.md b/issues/0056-param-impl-not-deduped-across-diamond-import.md new file mode 100644 index 0000000..9457cf4 --- /dev/null +++ b/issues/0056-param-impl-not-deduped-across-diamond-import.md @@ -0,0 +1,79 @@ +# 0056 — parameterised-protocol impl not deduped across a diamond import + +**FIXED** (`examples/issue-0056-diamond-param-impl.sx`, helpers in +`examples/issue-0056/`). The flat decl list in +[src/imports.zig](../src/imports.zig) now dedups by **node identity** as well +as by name. `ResolvedModule.mergeFlat` and the directory-import merge loop each +carry a `seen_nodes: std.AutoHashMap(*Node, void)` alongside the existing +`seen_in_list` name set, and skip a decl whose pointer was already appended. + +## Symptom + +A module containing a parameterised-protocol impl (`impl Into(T) for S`) could +not be imported through more than one path. Under a diamond — + +``` +main ─┬─ mid_a ─┐ + └─ mid_b ─┴─ common (holds `impl Into(Wrapped) for s64`) +``` + +— compilation failed with: + +``` +error: duplicate impl 'Into' for source 's64' in .../common.sx +``` + +This bit the moment `modules/std/objc.sx` (imported by `main.sx`, +`platform/uikit.sx`, and `gpu/metal.sx` — a diamond) gained an +`impl Into(*NSString) for string`. + +## Root cause + +`mergeFlat`/`addOwnDecl` dedup the global flat decl list by +`decl.data.declName()`. Named decls (structs, fns, foreign classes) dedup +fine across diamonds. But `impl_block` is anonymous — `declName()` returns +`null` (see [src/ast.zig](../src/ast.zig) `Data.declName`) — so the dedup +guard was skipped and the **same cached** impl node (modules are cached in +`ModuleCache`, so both paths share one node pointer) was appended once per +path. `registerParamImpl` in [src/ir/lower.zig](../src/ir/lower.zig) then saw +two entries with the same `defining_module` and raised the same-file +"duplicate impl" diagnostic. + +The pre-existing `impl Into(Block) for Closure(...)` in +`modules/std/objc_block.sx` never tripped this because that module is not +imported through any diamond. + +## Reproduction + +`examples/issue-0056/common.sx`: + +```sx +Wrapped :: struct { v: s64; } + +impl Into(Wrapped) for s64 { + convert :: (self: s64) -> Wrapped { + return .{ v = self }; + } +} +``` + +`mid_a.sx` / `mid_b.sx` each `#import "common.sx";`. The diamond main: + +```sx +#import "modules/std.sx"; +#import "issue-0056/mid_a.sx"; +#import "issue-0056/mid_b.sx"; + +main :: () -> s32 { + w : Wrapped = xx 7; // pre-fix: duplicate impl 'Into' for source 's64' + print("{}\n", w.v); // post-fix: prints 7 + 0; +} +``` + +## Fix + +Dedup the flat decl list by node identity in addition to name — a physical +AST node must be lowered once regardless of how many import paths reach it. +Named-decl first-wins behaviour is unchanged. Regression test: +`examples/issue-0056-diamond-param-impl.sx` (in `tests/run_examples.sh`). diff --git a/src/imports.zig b/src/imports.zig index 4d158c0..5ce9de5 100644 --- a/src/imports.zig +++ b/src/imports.zig @@ -315,20 +315,29 @@ pub const ResolvedModule = struct { /// `self.scope` — visibility joins per-file scopes at lookup time via /// `import_graph`. We only need to append `other.decls` (the full /// transitive list) to the global `list` so the lowering pass can - /// still resolve transitively-imported callees. Deduped by name. + /// still resolve transitively-imported callees. + /// + /// Deduped two ways: named decls by name (first-wins on cross-module + /// collisions), and EVERY decl by node identity. The latter matters for + /// anonymous decls — `impl` blocks have no `declName`, so under a diamond + /// import the same cached node would otherwise be appended once per path + /// and registered twice (e.g. `duplicate impl 'Into'`). pub fn mergeFlat( self: *ResolvedModule, allocator: std.mem.Allocator, list: *std.ArrayList(*Node), seen_list: *std.StringHashMap(void), + seen_nodes: *std.AutoHashMap(*Node, void), other: ResolvedModule, ) !void { _ = self; for (other.decls) |decl| { + if (seen_nodes.contains(decl)) continue; if (decl.data.declName()) |name| { if (seen_list.contains(name)) continue; try seen_list.put(name, {}); } + try seen_nodes.put(decl, {}); try list.append(allocator, decl); } } @@ -420,6 +429,9 @@ pub fn resolveImports( // by `mergeFlat` to dedupe across diamond imports now that `mod.scope` // is non-transitive and can no longer serve as the dedup key. var seen_in_list = std.StringHashMap(void).init(allocator); + // Node-identity set for the same purpose, covering anonymous decls + // (impl blocks) that carry no name to dedupe on. + var seen_nodes = std.AutoHashMap(*Node, void).init(allocator); for (flat_decls) |decl| { if (decl.data == .c_import_decl) { @@ -559,7 +571,7 @@ pub fn resolveImports( if (imp.name) |ns_name| { try mod.addNamespace(allocator, &decl_list, &own_decl_list, &seen_in_list, ns_name, imported_mod, decl.span); } else { - try mod.mergeFlat(allocator, &decl_list, &seen_in_list, imported_mod); + try mod.mergeFlat(allocator, &decl_list, &seen_in_list, &seen_nodes, imported_mod); } } @@ -622,6 +634,7 @@ fn resolveDirectoryImport( var decl_list = std.ArrayList(*Node).empty; var own_decl_list = std.ArrayList(*Node).empty; var seen_in_list = std.StringHashMap(void).init(allocator); + var seen_nodes = std.AutoHashMap(*Node, void).init(allocator); for (file_names.items) |file_name| { const file_path = try std.fmt.allocPrint(allocator, "{s}/{s}", .{ dir_path, file_name }); @@ -668,10 +681,12 @@ fn resolveDirectoryImport( // register `Event` (a tagged_union) before `handle_event(e: *Event)` // triggers the placeholder-struct fallback in `resolveTypeName`. for (file_mod.decls) |decl| { + if (seen_nodes.contains(decl)) continue; if (decl.data.declName()) |name| { if (seen_in_list.contains(name)) continue; try seen_in_list.put(name, {}); } + try seen_nodes.put(decl, {}); try decl_list.append(allocator, decl); } // Separately track which decls the directory `re-exports` to its diff --git a/tests/expected/issue-0056-diamond-param-impl.exit b/tests/expected/issue-0056-diamond-param-impl.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/expected/issue-0056-diamond-param-impl.exit @@ -0,0 +1 @@ +0 diff --git a/tests/expected/issue-0056-diamond-param-impl.txt b/tests/expected/issue-0056-diamond-param-impl.txt new file mode 100644 index 0000000..7f8f011 --- /dev/null +++ b/tests/expected/issue-0056-diamond-param-impl.txt @@ -0,0 +1 @@ +7