AstGen: detect and error on files included in multiple packages

Previously, if a source file was referenced from multiple packages, it
just became owned by the first one AstGen happened to reach; this was a
problem, because it could lead to inconsistent behaviour in the compiler
based on a race condition. This could be fixed by just analyzing such
files multiple times - however, it was pointed out by Andrew that it
might make more sense to enforce files being part of at most a single
package. Having a file in multiple packages would not only impact
compile times (due to Sema having to run multiple times on potentially a
lot of code) but is also a confusing anti-pattern which more often than
not is a mistake on the part of the user.

Resolves: ziglang#13662

Author: mlugg

src/Compilation.zig

@@ -617,14 +617,6 @@ pub const AllErrors = struct {
                 note_i += 1;
             }
         }
-        if (module_err_msg.src_loc.lazy == .entire_file) {
-            try errors.append(.{
-                .plain = .{
-                    .msg = try allocator.dupe(u8, module_err_msg.msg),
-                },
-            });
-            return;
-        }
 
         const reference_trace = try allocator.alloc(Message, module_err_msg.reference_trace.len);
         for (reference_trace) |*reference, i| {
@@ -661,7 +653,7 @@ pub const AllErrors = struct {
                 .column = @intCast(u32, err_loc.column),
                 .notes = notes_buf[0..note_i],
                 .reference_trace = reference_trace,
-                .source_line = try allocator.dupe(u8, err_loc.source_line),
+                .source_line = if (module_err_msg.src_loc.lazy == .entire_file) null else try allocator.dupe(u8, err_loc.source_line),
             },
         });
     }
@@ -3034,6 +3026,58 @@ pub fn performAllTheWork(
         }
     }
 
+    if (comp.bin_file.options.module) |mod| {
+        for (mod.import_table.values()) |file| {
+            if (file.multi_pkg) {
+                const err = err_blk: {
+                    const notes = try mod.gpa.alloc(Module.ErrorMsg, file.references.items.len);
+                    errdefer mod.gpa.free(notes);
+
+                    for (notes) |*note, i| {
+                        errdefer for (notes[0..i]) |*n| n.deinit(mod.gpa);
+                        note.* = switch (file.references.items[i]) {
+                            .import => |loc| try Module.ErrorMsg.init(
+                                mod.gpa,
+                                loc,
+                                "imported from package {s}",
+                                .{loc.file_scope.pkg.getName()},
+                            ),
+                            .root => |pkg| try Module.ErrorMsg.init(
+                                mod.gpa,
+                                .{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file },
+                                "root of package {s}",
+                                .{pkg.getName()},
+                            ),
+                        };
+                    }
+                    errdefer for (notes) |*n| n.deinit(mod.gpa);
+
+                    const err = try Module.ErrorMsg.create(
+                        mod.gpa,
+                        .{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file },
+                        "file exists in multiple packages",
+                        .{},
+                    );
+                    err.notes = notes;
+                    break :err_blk err;
+                };
+                errdefer err.destroy(mod.gpa);
+                try mod.failed_files.putNoClobber(mod.gpa, file, err);
+            }
+        }
+
+        // Now that we've reported the errors, we need to deal with
+        // dependencies. Any file referenced by a multi_pkg file should also be
+        // marked multi_pkg and have its status set to astgen_failure, as it's
+        // ambiguous which package they should be analyzed as a part of. We need
+        // to add this flag after reporting the errors however, as otherwise
+        // we'd get an error for every single downstream file, which wouldn't be
+        // very useful.
+        for (mod.import_table.values()) |file| {
+            if (file.multi_pkg) file.recursiveMarkMultiPkg(mod);
+        }
+    }
+
     {
         const outdated_and_deleted_decls_frame = tracy.namedFrame("outdated_and_deleted_decls");
         defer outdated_and_deleted_decls_frame.end();
@@ -3458,7 +3502,15 @@ fn workerAstGenFile(
                 comp.mutex.lock();
                 defer comp.mutex.unlock();
 
-                break :blk mod.importFile(file, import_path) catch continue;
+                const res = mod.importFile(file, import_path) catch continue;
+                if (res.is_file) {
+                    res.file.addReference(mod.*, .{ .import = .{
+                        .file_scope = file,
+                        .parent_decl_node = 0,
+                        .lazy = .{ .token_abs = item.data.token },
+                    } }) catch continue;
+                }
+                break :blk res;
             };
             if (import_result.is_new) {
                 log.debug("AstGen of {s} has import '{s}'; queuing AstGen of {s}", .{

src/Module.zig

@@ -1895,6 +1895,10 @@ pub const File = struct {
     zir: Zir,
     /// Package that this file is a part of, managed externally.
     pkg: *Package,
+    /// Whether this file is a part of multiple packages. This is an error condition which will be reported after AstGen.
+    multi_pkg: bool = false,
+    /// List of references to this file, used for multi-package errors.
+    references: std.ArrayListUnmanaged(Reference) = .{},
 
     /// Used by change detection algorithm, after astgen, contains the
     /// set of decls that existed in the previous ZIR but not in the new one.
@@ -1910,6 +1914,14 @@ pub const File = struct {
     /// successful, this field is unloaded.
     prev_zir: ?*Zir = null,
 
+    /// A single reference to a file.
+    const Reference = union(enum) {
+        /// The file is imported directly (i.e. not as a package) with @import.
+        import: SrcLoc,
+        /// The file is the root of a package.
+        root: *Package,
+    };
+
     pub fn unload(file: *File, gpa: Allocator) void {
         file.unloadTree(gpa);
         file.unloadSource(gpa);
@@ -1942,6 +1954,7 @@ pub const File = struct {
         log.debug("deinit File {s}", .{file.sub_file_path});
         file.deleted_decls.deinit(gpa);
         file.outdated_decls.deinit(gpa);
+        file.references.deinit(gpa);
         if (file.root_decl.unwrap()) |root_decl| {
             mod.destroyDecl(root_decl);
         }
@@ -2062,6 +2075,44 @@ pub const File = struct {
             else => true,
         };
     }
+
+    /// Add a reference to this file during AstGen.
+    pub fn addReference(file: *File, mod: Module, ref: Reference) !void {
+        try file.references.append(mod.gpa, ref);
+
+        const pkg = switch (ref) {
+            .import => |loc| loc.file_scope.pkg,
+            .root => |pkg| pkg,
+        };
+        if (pkg != file.pkg) file.multi_pkg = true;
+    }
+
+    /// Mark this file and every file referenced by it as multi_pkg and report an
+    /// astgen_failure error for them. AstGen must have completed in its entirety.
+    pub fn recursiveMarkMultiPkg(file: *File, mod: *Module) void {
+        file.multi_pkg = true;
+        file.status = .astgen_failure;
+
+        std.debug.assert(file.zir_loaded);
+        const imports_index = file.zir.extra[@enumToInt(Zir.ExtraIndex.imports)];
+        if (imports_index == 0) return;
+        const extra = file.zir.extraData(Zir.Inst.Imports, imports_index);
+
+        var import_i: u32 = 0;
+        var extra_index = extra.end;
+        while (import_i < extra.data.imports_len) : (import_i += 1) {
+            const item = file.zir.extraData(Zir.Inst.Imports.Item, extra_index);
+            extra_index = item.end;
+
+            const import_path = file.zir.nullTerminatedString(item.data.name);
+            if (mem.eql(u8, import_path, "builtin")) continue;
+
+            const res = mod.importFile(file, import_path) catch continue;
+            if (res.is_file and !res.file.multi_pkg) {
+                res.file.recursiveMarkMultiPkg(mod);
+            }
+        }
+    }
 };
 
 /// Represents the contents of a file loaded with `@embedFile`.
@@ -4867,6 +4918,7 @@ pub fn declareDeclDependency(mod: *Module, depender_index: Decl.Index, dependee_
 pub const ImportFileResult = struct {
     file: *File,
     is_new: bool,
+    is_file: bool,
 };
 
 pub fn importPkg(mod: *Module, pkg: *Package) !ImportFileResult {
@@ -4886,6 +4938,7 @@ pub fn importPkg(mod: *Module, pkg: *Package) !ImportFileResult {
     if (gop.found_existing) return ImportFileResult{
         .file = gop.value_ptr.*,
         .is_new = false,
+        .is_file = false,
     };
 
     const sub_file_path = try gpa.dupe(u8, pkg.root_src_path);
@@ -4909,9 +4962,11 @@ pub fn importPkg(mod: *Module, pkg: *Package) !ImportFileResult {
         .pkg = pkg,
         .root_decl = .none,
     };
+    try new_file.addReference(mod.*, .{ .root = pkg });
     return ImportFileResult{
         .file = new_file,
         .is_new = true,
+        .is_file = false,
     };
 }
 
@@ -4952,6 +5007,7 @@ pub fn importFile(
     if (gop.found_existing) return ImportFileResult{
         .file = gop.value_ptr.*,
         .is_new = false,
+        .is_file = true,
     };
 
     const new_file = try gpa.create(File);
@@ -4997,6 +5053,7 @@ pub fn importFile(
     return ImportFileResult{
         .file = new_file,
         .is_new = true,
+        .is_file = true,
     };
 }
 

src/Package.zig

@@ -124,3 +124,13 @@ pub fn addAndAdopt(parent: *Package, gpa: Allocator, name: []const u8, child: *P
     child.parent = parent;
     return parent.add(gpa, name, child);
 }
+
+pub fn getName(pkg: *const Package) []const u8 {
+    if (pkg.parent) |parent| {
+        var it = parent.table.iterator();
+        while (it.next()) |p| {
+            if (p.value_ptr.* == pkg) return p.key_ptr.*;
+        }
+        unreachable;
+    } else return "root";
+}