From 5f06b6504fbb898a1c70eabfee4fdc466bdba615 Mon Sep 17 00:00:00 2001 From: agra Date: Sat, 6 Jun 2026 23:54:51 +0300 Subject: [PATCH] fix(imports): diagnose namespace-alias dup + propagate buildImportFacts errors [stdlib A attempt-2] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects from the Phase A attempt-1 review. F1 — duplicate-name diagnostic missed NAMESPACE ALIASES (silent first-win). `addNamespace` unconditionally put the alias into scope/own_decls, so a same-module collision between an authored decl and a `dup :: #import "…"` alias compiled clean in the fn-then-alias order (the scalar ModuleRawDeclIndex silently first-won). Now `addNamespace` returns a bool and refuses a same-module duplicate (mirroring addOwnDecl); the call site surfaces it via the new `reportDuplicateName` (the import_decl node has no declName, so the alias name is passed explicitly). The C-import namespace site gets the same guard. Both orders now emit "duplicate top-level declaration 'X'" and exit nonzero (alias-then-fn was already caught by addOwnDecl seeing the alias in scope). F2 — buildImportFacts errors were swallowed by `else |_| {}` in core.zig (REJECTED-PATTERN catch-all leaving the borrowed store silently empty). `resolveImports` returns !void, so the call is now a plain `try` and a build failure propagates instead of producing a stale/empty store. Tests: extend the dup-name regression with fn-vs-namespace-alias collisions in both orders. No resolution behavior change (no lower.zig edits; run_examples 471 byte-identical); m3te ios-sim builds via the worktree binary. --- src/core.zig | 11 +++++---- src/imports.test.zig | 54 ++++++++++++++++++++++++++++++++++++++++++++ src/imports.zig | 47 ++++++++++++++++++++++++++++---------- 3 files changed, 95 insertions(+), 17 deletions(-) diff --git a/src/core.zig b/src/core.zig index b92fb5b..f73c141 100644 --- a/src/core.zig +++ b/src/core.zig @@ -136,11 +136,12 @@ pub const Compilation = struct { // Raw import facts (the unified-resolver store): scalar per-module // raw-decl index + namespace edges, built from the SAME modules. Nothing // consumes these yet — they are borrowed by `ProgramIndex` for later - // phases (and the LSP). Built without IR lowering. - if (imports.buildImportFacts(self.allocator, self.file_path, mod, &cache)) |facts| { - self.module_decls = facts.decls; - self.namespace_edges = facts.ns_edges; - } else |_| {} + // phases (and the LSP). Built without IR lowering. A build failure here + // (allocation) is the Phase A deliverable failing — propagate it rather + // than leaving the borrowed views silently empty/stale. + const facts = try imports.buildImportFacts(self.allocator, self.file_path, mod, &cache); + self.module_decls = facts.decls; + self.namespace_edges = facts.ns_edges; // Store main file source in import_sources so error reporting can find it self.import_sources.put(self.file_path, self.source) catch {}; diff --git a/src/imports.test.zig b/src/imports.test.zig index 662e623..99f06f6 100644 --- a/src/imports.test.zig +++ b/src/imports.test.zig @@ -449,3 +449,57 @@ test "buildImportFacts: same-module duplicate top-level name is diagnosed" { const m_idx = facts.decls.get(main_path) orelse return error.MissingMainIndex; try expectTag(m_idx.names.get("foo") orelse return error.MissingFoo, .fn_decl); } + +// F1: the duplicate-name invariant must also cover NAMESPACE ALIASES. A +// `dup :: #import "…"` alias colliding with a same-module authored name is a +// duplicate in EITHER order — `addNamespace` (alias second) and `addOwnDecl` +// (alias first) each refuse the second author and the site diagnoses it. Before +// the fix the fn-then-alias order compiled clean (silent first-win in the scalar +// index). Surviving author is whichever came FIRST. +test "buildImportFacts: fn-then-namespace-alias same-module collision is diagnosed" { + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + const io = testIo(); + + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + + try tmp.dir.writeFile(io, .{ .sub_path = "lib.sx", .data = "helper :: () -> s64 { 9 }\n" }); + try tmp.dir.writeFile(io, .{ .sub_path = "main.sx", .data = "dup :: () -> s64 { 1 }\ndup :: #import \"lib.sx\";\nmain :: () -> s32 { 0 }\n" }); + + var dirbuf: [4096]u8 = undefined; + const absdir = dirbuf[0..try tmp.dir.realPath(io, &dirbuf)]; + const main_path = try std.fmt.allocPrint(alloc, "{s}/main.sx", .{absdir}); + + var facts = try buildFacts(alloc, io, absdir, main_path); + + try std.testing.expect(hasErr(&facts.diags, "duplicate top-level declaration 'dup'")); + // The fn came first, so it survives in the scalar index; the alias dropped. + const m_idx = facts.decls.get(main_path) orelse return error.MissingMainIndex; + try expectTag(m_idx.names.get("dup") orelse return error.MissingDup, .fn_decl); +} + +test "buildImportFacts: namespace-alias-then-fn same-module collision is diagnosed" { + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + const io = testIo(); + + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + + try tmp.dir.writeFile(io, .{ .sub_path = "lib.sx", .data = "helper :: () -> s64 { 9 }\n" }); + try tmp.dir.writeFile(io, .{ .sub_path = "main.sx", .data = "dup :: #import \"lib.sx\";\ndup :: () -> s64 { 1 }\nmain :: () -> s32 { 0 }\n" }); + + var dirbuf: [4096]u8 = undefined; + const absdir = dirbuf[0..try tmp.dir.realPath(io, &dirbuf)]; + const main_path = try std.fmt.allocPrint(alloc, "{s}/main.sx", .{absdir}); + + var facts = try buildFacts(alloc, io, absdir, main_path); + + try std.testing.expect(hasErr(&facts.diags, "duplicate top-level declaration 'dup'")); + // The alias came first, so the namespace_decl survives; the fn dropped. + const m_idx = facts.decls.get(main_path) orelse return error.MissingMainIndex; + try expectTag(m_idx.names.get("dup") orelse return error.MissingDup, .namespace_decl); +} diff --git a/src/imports.zig b/src/imports.zig index 76401ee..98bca89 100644 --- a/src/imports.zig +++ b/src/imports.zig @@ -357,6 +357,13 @@ pub const ResolvedModule = struct { /// Add another module as a namespaced import. The alias `name` becomes /// part of this module's own decls (so a flat-importer of this module /// sees the alias one hop out — matching authored names). + /// + /// Returns `false` (and adds nothing) when `name` already names a decl + /// authored in THIS module — symmetric to `addOwnDecl`, so a namespace + /// alias colliding with a prior same-module name is dropped rather than + /// shadowing it. The caller surfaces the drop via `reportDuplicateName`; + /// the reverse order (alias first, then a same-name authored decl) is + /// caught by `addOwnDecl` seeing the alias already in `scope`. pub fn addNamespace( self: *ResolvedModule, allocator: std.mem.Allocator, @@ -367,7 +374,8 @@ pub const ResolvedModule = struct { other: ResolvedModule, span: ast.Span, is_raw: bool, - ) !void { + ) !bool { + if (self.scope.contains(name)) return false; const ns_node = try allocator.create(Node); ns_node.* = .{ .span = span, @@ -391,6 +399,7 @@ pub const ResolvedModule = struct { try seen_list.put(name, {}); try list.append(allocator, ns_node); try own_list.append(allocator, ns_node); + return true; } pub fn finalize( @@ -570,15 +579,24 @@ pub fn buildImportFacts( return .{ .decls = decls, .ns_edges = ns_edges }; } -/// Surface a same-module duplicate top-level declaration as a hard error. -/// `addOwnDecl` returns `false` when the name is already in this module's scope -/// and drops the second author; without this the drop is silent, and the scalar -/// `ModuleRawDeclIndex` would lose an authored name with no diagnostic. -fn reportDuplicateDecl(diagnostics: ?*errors.DiagnosticList, added: bool, decl: *const Node) void { +/// Surface a same-module duplicate top-level declaration as a hard error at an +/// explicit name + span. `addOwnDecl` / `addNamespace` return `false` when the +/// name is already in this module's scope and drop the second author; without +/// this the drop is silent, and the scalar `ModuleRawDeclIndex` would lose an +/// authored name with no diagnostic. +fn reportDuplicateName(diagnostics: ?*errors.DiagnosticList, added: bool, name: []const u8, span: ast.Span) void { if (added) return; const diags = diagnostics orelse return; + diags.addFmt(.err, span, "duplicate top-level declaration '{s}'", .{name}); +} + +/// `reportDuplicateName` keyed off a node whose `declName()` carries the name +/// (the regular authored-decl sites; an `import_decl` has no `declName`, so a +/// namespace alias must use `reportDuplicateName` with the alias directly). +fn reportDuplicateDecl(diagnostics: ?*errors.DiagnosticList, added: bool, decl: *const Node) void { + if (added) return; const name = decl.data.declName() orelse return; - diags.addFmt(.err, decl.span, "duplicate top-level declaration '{s}'", .{name}); + reportDuplicateName(diagnostics, added, name, decl.span); } pub fn resolveImports( @@ -706,10 +724,14 @@ pub fn resolveImports( } }, }; ns_node.source_file = file_path; - try mod.scope.put(ns_name, {}); - try seen_in_list.put(ns_name, {}); - try decl_list.append(allocator, ns_node); - try own_decl_list.append(allocator, ns_node); + if (mod.scope.contains(ns_name)) { + reportDuplicateName(diagnostics, false, ns_name, decl.span); + } else { + try mod.scope.put(ns_name, {}); + try seen_in_list.put(ns_name, {}); + try decl_list.append(allocator, ns_node); + try own_decl_list.append(allocator, ns_node); + } } else { // Flat: add fn_decls directly + keep c_import_decl for (result.fn_decls) |fd| { @@ -796,7 +818,8 @@ 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, imp.is_raw); + const added = try mod.addNamespace(allocator, &decl_list, &own_decl_list, &seen_in_list, ns_name, imported_mod, decl.span, imp.is_raw); + reportDuplicateName(diagnostics, added, ns_name, decl.span); } else { try mod.mergeFlat(allocator, &decl_list, &seen_in_list, &seen_nodes, imported_mod); }