Skip to content

Commit b8a96ba

Browse files
committed
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
1 parent 09a84c8 commit b8a96ba

File tree

2 files changed

+111
-58
lines changed

2 files changed

+111
-58
lines changed

src/Compilation.zig

Lines changed: 106 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -2773,6 +2773,111 @@ fn emitOthers(comp: *Compilation) void {
27732773
}
27742774
}
27752775

2776+
fn reportMultiModuleErrors(mod: *Module) !void {
2777+
// Some cases can give you a whole bunch of multi-module errors, which it's not helpful to
2778+
// print all of, so we'll cap the number of these to emit.
2779+
var num_errors: u32 = 0;
2780+
const max_errors = 5;
2781+
// Attach the "some omitted" note to the final error message
2782+
var last_err: ?*Module.ErrorMsg = null;
2783+
2784+
for (mod.import_table.values()) |file| {
2785+
if (!file.multi_pkg) continue;
2786+
2787+
num_errors += 1;
2788+
if (num_errors > max_errors) continue;
2789+
2790+
const err = err_blk: {
2791+
// Like with errors, let's cap the number of notes to prevent a huge error spew.
2792+
const max_notes = 5;
2793+
const omitted = file.references.items.len -| max_notes;
2794+
const num_notes = file.references.items.len - omitted;
2795+
2796+
const notes = try mod.gpa.alloc(Module.ErrorMsg, if (omitted > 0) num_notes + 1 else num_notes);
2797+
errdefer mod.gpa.free(notes);
2798+
2799+
for (notes[0..num_notes], file.references.items[0..num_notes], 0..) |*note, ref, i| {
2800+
errdefer for (notes[0..i]) |*n| n.deinit(mod.gpa);
2801+
note.* = switch (ref) {
2802+
.import => |loc| blk: {
2803+
const name = try loc.file_scope.pkg.getName(mod.gpa, mod.*);
2804+
defer mod.gpa.free(name);
2805+
break :blk try Module.ErrorMsg.init(
2806+
mod.gpa,
2807+
loc,
2808+
"imported from module {s}",
2809+
.{name},
2810+
);
2811+
},
2812+
.root => |pkg| blk: {
2813+
const name = try pkg.getName(mod.gpa, mod.*);
2814+
defer mod.gpa.free(name);
2815+
break :blk try Module.ErrorMsg.init(
2816+
mod.gpa,
2817+
.{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file },
2818+
"root of module {s}",
2819+
.{name},
2820+
);
2821+
},
2822+
};
2823+
}
2824+
errdefer for (notes[0..num_notes]) |*n| n.deinit(mod.gpa);
2825+
2826+
if (omitted > 0) {
2827+
notes[num_notes] = try Module.ErrorMsg.init(
2828+
mod.gpa,
2829+
.{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file },
2830+
"{} more references omitted",
2831+
.{omitted},
2832+
);
2833+
}
2834+
errdefer if (omitted > 0) notes[num_notes].deinit(mod.gpa);
2835+
2836+
const err = try Module.ErrorMsg.create(
2837+
mod.gpa,
2838+
.{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file },
2839+
"file exists in multiple modules",
2840+
.{},
2841+
);
2842+
err.notes = notes;
2843+
break :err_blk err;
2844+
};
2845+
errdefer err.destroy(mod.gpa);
2846+
try mod.failed_files.putNoClobber(mod.gpa, file, err);
2847+
last_err = err;
2848+
}
2849+
2850+
// If we omitted any errors, add a note saying that
2851+
if (num_errors > max_errors) {
2852+
const err = last_err.?;
2853+
2854+
// There isn't really any meaningful place to put this note, so just attach it to the
2855+
// last failed file
2856+
var note = try Module.ErrorMsg.init(
2857+
mod.gpa,
2858+
err.src_loc,
2859+
"{} more errors omitted",
2860+
.{num_errors - max_errors},
2861+
);
2862+
errdefer note.deinit(mod.gpa);
2863+
2864+
const i = err.notes.len;
2865+
err.notes = try mod.gpa.realloc(err.notes, i + 1);
2866+
err.notes[i] = note;
2867+
}
2868+
2869+
// Now that we've reported the errors, we need to deal with
2870+
// dependencies. Any file referenced by a multi_pkg file should also be
2871+
// marked multi_pkg and have its status set to astgen_failure, as it's
2872+
// ambiguous which package they should be analyzed as a part of. We need
2873+
// to add this flag after reporting the errors however, as otherwise
2874+
// we'd get an error for every single downstream file, which wouldn't be
2875+
// very useful.
2876+
for (mod.import_table.values()) |file| {
2877+
if (file.multi_pkg) file.recursiveMarkMultiPkg(mod);
2878+
}
2879+
}
2880+
27762881
/// Having the file open for writing is problematic as far as executing the
27772882
/// binary is concerned. This will remove the write flag, or close the file,
27782883
/// or whatever is needed so that it can be executed.
@@ -3099,62 +3204,7 @@ pub fn performAllTheWork(
30993204
}
31003205

31013206
if (comp.bin_file.options.module) |mod| {
3102-
for (mod.import_table.values()) |file| {
3103-
if (!file.multi_pkg) continue;
3104-
const err = err_blk: {
3105-
const notes = try mod.gpa.alloc(Module.ErrorMsg, file.references.items.len);
3106-
errdefer mod.gpa.free(notes);
3107-
3108-
for (notes, 0..) |*note, i| {
3109-
errdefer for (notes[0..i]) |*n| n.deinit(mod.gpa);
3110-
note.* = switch (file.references.items[i]) {
3111-
.import => |loc| blk: {
3112-
const name = try loc.file_scope.pkg.getName(mod.gpa, mod.*);
3113-
defer mod.gpa.free(name);
3114-
break :blk try Module.ErrorMsg.init(
3115-
mod.gpa,
3116-
loc,
3117-
"imported from package {s}",
3118-
.{name},
3119-
);
3120-
},
3121-
.root => |pkg| blk: {
3122-
const name = try pkg.getName(mod.gpa, mod.*);
3123-
defer mod.gpa.free(name);
3124-
break :blk try Module.ErrorMsg.init(
3125-
mod.gpa,
3126-
.{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file },
3127-
"root of package {s}",
3128-
.{name},
3129-
);
3130-
},
3131-
};
3132-
}
3133-
errdefer for (notes) |*n| n.deinit(mod.gpa);
3134-
3135-
const err = try Module.ErrorMsg.create(
3136-
mod.gpa,
3137-
.{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file },
3138-
"file exists in multiple packages",
3139-
.{},
3140-
);
3141-
err.notes = notes;
3142-
break :err_blk err;
3143-
};
3144-
errdefer err.destroy(mod.gpa);
3145-
try mod.failed_files.putNoClobber(mod.gpa, file, err);
3146-
}
3147-
3148-
// Now that we've reported the errors, we need to deal with
3149-
// dependencies. Any file referenced by a multi_pkg file should also be
3150-
// marked multi_pkg and have its status set to astgen_failure, as it's
3151-
// ambiguous which package they should be analyzed as a part of. We need
3152-
// to add this flag after reporting the errors however, as otherwise
3153-
// we'd get an error for every single downstream file, which wouldn't be
3154-
// very useful.
3155-
for (mod.import_table.values()) |file| {
3156-
if (file.multi_pkg) file.recursiveMarkMultiPkg(mod);
3157-
}
3207+
try reportMultiModuleErrors(mod);
31583208
}
31593209

31603210
{

src/Module.zig

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1946,7 +1946,7 @@ pub const File = struct {
19461946
prev_zir: ?*Zir = null,
19471947

19481948
/// A single reference to a file.
1949-
const Reference = union(enum) {
1949+
pub const Reference = union(enum) {
19501950
/// The file is imported directly (i.e. not as a package) with @import.
19511951
import: SrcLoc,
19521952
/// The file is the root of a package.
@@ -2144,7 +2144,10 @@ pub const File = struct {
21442144
file.multi_pkg = true;
21452145
file.status = .astgen_failure;
21462146

2147-
std.debug.assert(file.zir_loaded);
2147+
// We can only mark children as failed if the ZIR is loaded, which may not
2148+
// be the case if there were other astgen failures in this file
2149+
if (!file.zir_loaded) return;
2150+
21482151
const imports_index = file.zir.extra[@enumToInt(Zir.ExtraIndex.imports)];
21492152
if (imports_index == 0) return;
21502153
const extra = file.zir.extraData(Zir.Inst.Imports, imports_index);

0 commit comments

Comments
 (0)