From 705d2a3c2cd94faf8e16c660b3b342d6fe900e55 Mon Sep 17 00:00:00 2001 From: mlugg Date: Fri, 17 Feb 2023 01:44:08 +0000 Subject: Implement new module CLI --- src/Module.zig | 62 ++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 21 deletions(-) (limited to 'src/Module.zig') diff --git a/src/Module.zig b/src/Module.zig index 76777532ab..2afe3f5df1 100644 --- a/src/Module.zig +++ b/src/Module.zig @@ -144,10 +144,6 @@ stage1_flags: packed struct { } = .{}, job_queued_update_builtin_zig: bool = true, -/// This makes it so that we can run `zig test` on the standard library. -/// Otherwise, the logic for scanning test decls skips all of them because -/// `main_pkg != std_pkg`. -main_pkg_is_std: bool, compile_log_text: ArrayListUnmanaged(u8) = .{}, @@ -2113,7 +2109,27 @@ pub const File = struct { /// 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); + // Don't add the same module root twice. Note that since we always add module roots at the + // front of the references array (see below), this loop is actually O(1) on valid code. + if (ref == .root) { + for (file.references.items) |other| { + switch (other) { + .root => |r| if (ref.root == r) return, + else => break, // reached the end of the "is-root" references + } + } + } + + switch (ref) { + // We put root references at the front of the list both to make the above loop fast and + // to make multi-module errors more helpful (since "root-of" notes are generally more + // informative than "imported-from" notes). This path is hit very rarely, so the speed + // of the insert operation doesn't matter too much. + .root => try file.references.insert(mod.gpa, 0, ref), + + // Other references we'll just put at the end. + else => try file.references.append(mod.gpa, ref), + } const pkg = switch (ref) { .import => |loc| loc.file_scope.pkg, @@ -3323,10 +3339,19 @@ pub fn deinit(mod: *Module) void { // The callsite of `Compilation.create` owns the `main_pkg`, however // Module owns the builtin and std packages that it adds. if (mod.main_pkg.table.fetchRemove("builtin")) |kv| { + gpa.free(kv.key); kv.value.destroy(gpa); } if (mod.main_pkg.table.fetchRemove("std")) |kv| { - kv.value.destroy(gpa); + gpa.free(kv.key); + // It's possible for main_pkg to be std when running 'zig test'! In this case, we must not + // destroy it, since it would lead to a double-free. + if (kv.value != mod.main_pkg) { + kv.value.destroy(gpa); + } + } + if (mod.main_pkg.table.fetchRemove("root")) |kv| { + gpa.free(kv.key); } if (mod.root_pkg != mod.main_pkg) { mod.root_pkg.destroy(gpa); @@ -4808,11 +4833,14 @@ pub fn importPkg(mod: *Module, pkg: *Package) !ImportFileResult { const gop = try mod.import_table.getOrPut(gpa, resolved_path); errdefer _ = mod.import_table.pop(); - if (gop.found_existing) return ImportFileResult{ - .file = gop.value_ptr.*, - .is_new = false, - .is_pkg = true, - }; + if (gop.found_existing) { + try gop.value_ptr.*.addReference(mod.*, .{ .root = pkg }); + return ImportFileResult{ + .file = gop.value_ptr.*, + .is_new = false, + .is_pkg = true, + }; + } const sub_file_path = try gpa.dupe(u8, pkg.root_src_path); errdefer gpa.free(sub_file_path); @@ -5208,22 +5236,14 @@ fn scanDecl(iter: *ScanDeclIter, decl_sub_index: usize, flags: u4) Allocator.Err // test decl with no name. Skip the part where we check against // the test name filter. if (!comp.bin_file.options.is_test) break :blk false; - if (decl_pkg != mod.main_pkg) { - if (!mod.main_pkg_is_std) break :blk false; - const std_pkg = mod.main_pkg.table.get("std").?; - if (std_pkg != decl_pkg) break :blk false; - } + if (decl_pkg != mod.main_pkg) break :blk false; try mod.test_functions.put(gpa, new_decl_index, {}); break :blk true; }, else => blk: { if (!is_named_test) break :blk false; if (!comp.bin_file.options.is_test) break :blk false; - if (decl_pkg != mod.main_pkg) { - if (!mod.main_pkg_is_std) break :blk false; - const std_pkg = mod.main_pkg.table.get("std").?; - if (std_pkg != decl_pkg) break :blk false; - } + if (decl_pkg != mod.main_pkg) break :blk false; if (comp.test_filter) |test_filter| { if (mem.indexOf(u8, decl_name, test_filter) == null) { break :blk false; -- cgit v1.2.3 From b8a96baab887e4dcd54d49fb1bbc8294af47392e Mon Sep 17 00:00:00 2001 From: mlugg Date: Sat, 18 Feb 2023 04:10:50 +0000 Subject: Improve multi-module error messages - Fix assertion failure if AstGen failed on a multi-module file - Cap number of per-error reference notes and total multi-module errors each at 5 - Always put "root of package" reference notes first Resolves: #14499 --- src/Compilation.zig | 162 ++++++++++++++++++++++++++++++++++------------------ src/Module.zig | 7 ++- 2 files changed, 111 insertions(+), 58 deletions(-) (limited to 'src/Module.zig') diff --git a/src/Compilation.zig b/src/Compilation.zig index 9b054fbe62..717a396870 100644 --- a/src/Compilation.zig +++ b/src/Compilation.zig @@ -2773,6 +2773,111 @@ fn emitOthers(comp: *Compilation) void { } } +fn reportMultiModuleErrors(mod: *Module) !void { + // Some cases can give you a whole bunch of multi-module errors, which it's not helpful to + // print all of, so we'll cap the number of these to emit. + var num_errors: u32 = 0; + const max_errors = 5; + // Attach the "some omitted" note to the final error message + var last_err: ?*Module.ErrorMsg = null; + + for (mod.import_table.values()) |file| { + if (!file.multi_pkg) continue; + + num_errors += 1; + if (num_errors > max_errors) continue; + + const err = err_blk: { + // Like with errors, let's cap the number of notes to prevent a huge error spew. + const max_notes = 5; + const omitted = file.references.items.len -| max_notes; + const num_notes = file.references.items.len - omitted; + + const notes = try mod.gpa.alloc(Module.ErrorMsg, if (omitted > 0) num_notes + 1 else num_notes); + errdefer mod.gpa.free(notes); + + for (notes[0..num_notes], file.references.items[0..num_notes], 0..) |*note, ref, i| { + errdefer for (notes[0..i]) |*n| n.deinit(mod.gpa); + note.* = switch (ref) { + .import => |loc| blk: { + const name = try loc.file_scope.pkg.getName(mod.gpa, mod.*); + defer mod.gpa.free(name); + break :blk try Module.ErrorMsg.init( + mod.gpa, + loc, + "imported from module {s}", + .{name}, + ); + }, + .root => |pkg| blk: { + const name = try pkg.getName(mod.gpa, mod.*); + defer mod.gpa.free(name); + break :blk try Module.ErrorMsg.init( + mod.gpa, + .{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file }, + "root of module {s}", + .{name}, + ); + }, + }; + } + errdefer for (notes[0..num_notes]) |*n| n.deinit(mod.gpa); + + if (omitted > 0) { + notes[num_notes] = try Module.ErrorMsg.init( + mod.gpa, + .{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file }, + "{} more references omitted", + .{omitted}, + ); + } + errdefer if (omitted > 0) notes[num_notes].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 modules", + .{}, + ); + err.notes = notes; + break :err_blk err; + }; + errdefer err.destroy(mod.gpa); + try mod.failed_files.putNoClobber(mod.gpa, file, err); + last_err = err; + } + + // If we omitted any errors, add a note saying that + if (num_errors > max_errors) { + const err = last_err.?; + + // There isn't really any meaningful place to put this note, so just attach it to the + // last failed file + var note = try Module.ErrorMsg.init( + mod.gpa, + err.src_loc, + "{} more errors omitted", + .{num_errors - max_errors}, + ); + errdefer note.deinit(mod.gpa); + + const i = err.notes.len; + err.notes = try mod.gpa.realloc(err.notes, i + 1); + err.notes[i] = note; + } + + // 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); + } +} + /// Having the file open for writing is problematic as far as executing the /// binary is concerned. This will remove the write flag, or close the file, /// or whatever is needed so that it can be executed. @@ -3099,62 +3204,7 @@ pub fn performAllTheWork( } if (comp.bin_file.options.module) |mod| { - for (mod.import_table.values()) |file| { - if (!file.multi_pkg) continue; - const err = err_blk: { - const notes = try mod.gpa.alloc(Module.ErrorMsg, file.references.items.len); - errdefer mod.gpa.free(notes); - - for (notes, 0..) |*note, i| { - errdefer for (notes[0..i]) |*n| n.deinit(mod.gpa); - note.* = switch (file.references.items[i]) { - .import => |loc| blk: { - const name = try loc.file_scope.pkg.getName(mod.gpa, mod.*); - defer mod.gpa.free(name); - break :blk try Module.ErrorMsg.init( - mod.gpa, - loc, - "imported from package {s}", - .{name}, - ); - }, - .root => |pkg| blk: { - const name = try pkg.getName(mod.gpa, mod.*); - defer mod.gpa.free(name); - break :blk try Module.ErrorMsg.init( - mod.gpa, - .{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file }, - "root of package {s}", - .{name}, - ); - }, - }; - } - 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); - } + try reportMultiModuleErrors(mod); } { diff --git a/src/Module.zig b/src/Module.zig index 2afe3f5df1..a2502d36d3 100644 --- a/src/Module.zig +++ b/src/Module.zig @@ -1946,7 +1946,7 @@ pub const File = struct { prev_zir: ?*Zir = null, /// A single reference to a file. - const Reference = union(enum) { + pub 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. @@ -2144,7 +2144,10 @@ pub const File = struct { file.multi_pkg = true; file.status = .astgen_failure; - std.debug.assert(file.zir_loaded); + // We can only mark children as failed if the ZIR is loaded, which may not + // be the case if there were other astgen failures in this file + if (!file.zir_loaded) return; + 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); -- cgit v1.2.3