| author |  Matthew Lugg <mlugg@mlugg.co.uk> 2022-11-27 14:17:55+00:00 |
| committer | Matthew Lugg <mlugg@mlugg.co.uk> 2023-01-22 19:00:03+00:00 |
| log | 5f9186d0ce78ce1eb7db9634849b38d2e90d071e |
| tree | ab46e6e77fe60874eb9f52779196ecbd8e4deabc |
| parent | 6d71d79dc27ddd6f66913a34fd6cd40691a8c959 |
| signature |  Commit is signed but in an unrecognized format. |
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: #136622 files changed, 118 insertions(+), 10 deletions(-)
src/Compilation.zig+61-10
| ... | ... | @@ -639,14 +639,6 @@ pub const AllErrors = struct { |
| 639 | 639 | note_i += 1; |
| 640 | 640 | } |
| 641 | 641 | } |
| 642 | if (module_err_msg.src_loc.lazy == .entire_file) { | |
| 643 | try errors.append(.{ | |
| 644 | .plain = .{ | |
| 645 | .msg = try allocator.dupe(u8, module_err_msg.msg), | |
| 646 | }, | |
| 647 | }); | |
| 648 | return; | |
| 649 | } | |
| 650 | 642 | |
| 651 | 643 | const reference_trace = try allocator.alloc(Message, module_err_msg.reference_trace.len); |
| 652 | 644 | for (reference_trace) |*reference, i| { |
| ... | ... | @@ -683,7 +675,7 @@ pub const AllErrors = struct { |
| 683 | 675 | .column = @intCast(u32, err_loc.column), |
| 684 | 676 | .notes = notes_buf[0..note_i], |
| 685 | 677 | .reference_trace = reference_trace, |
| 686 | .source_line = try allocator.dupe(u8, err_loc.source_line), | |
| 678 | .source_line = if (module_err_msg.src_loc.lazy == .entire_file) null else try allocator.dupe(u8, err_loc.source_line), | |
| 687 | 679 | }, |
| 688 | 680 | }); |
| 689 | 681 | } |
| ... | ... | @@ -3080,6 +3072,57 @@ pub fn performAllTheWork( |
| 3080 | 3072 | } |
| 3081 | 3073 | } |
| 3082 | 3074 | |
| 3075 | if (comp.bin_file.options.module) |mod| { | |
| 3076 | for (mod.import_table.values()) |file| { | |
| 3077 | if (!file.multi_pkg) continue; | |
| 3078 | const err = err_blk: { | |
| 3079 | const notes = try mod.gpa.alloc(Module.ErrorMsg, file.references.items.len); | |
| 3080 | errdefer mod.gpa.free(notes); | |
| 3081 | ||
| 3082 | for (notes) |*note, i| { | |
| 3083 | errdefer for (notes[0..i]) |*n| n.deinit(mod.gpa); | |
| 3084 | note.* = switch (file.references.items[i]) { | |
| 3085 | .import => |loc| try Module.ErrorMsg.init( | |
| 3086 | mod.gpa, | |
| 3087 | loc, | |
| 3088 | "imported from package {s}", | |
| 3089 | .{loc.file_scope.pkg.name}, | |
| 3090 | ), | |
| 3091 | .root => |pkg| try Module.ErrorMsg.init( | |
| 3092 | mod.gpa, | |
| 3093 | .{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file }, | |
| 3094 | "root of package {s}", | |
| 3095 | .{pkg.name}, | |
| 3096 | ), | |
| 3097 | }; | |
| 3098 | } | |
| 3099 | errdefer for (notes) |*n| n.deinit(mod.gpa); | |
| 3100 | ||
| 3101 | const err = try Module.ErrorMsg.create( | |
| 3102 | mod.gpa, | |
| 3103 | .{ .file_scope = file, .parent_decl_node = 0, .lazy = .entire_file }, | |
| 3104 | "file exists in multiple packages", | |
| 3105 | .{}, | |
| 3106 | ); | |
| 3107 | err.notes = notes; | |
| 3108 | break :err_blk err; | |
| 3109 | }; | |
| 3110 | errdefer err.destroy(mod.gpa); | |
| 3111 | try mod.failed_files.putNoClobber(mod.gpa, file, err); | |
| 3112 | } | |
| 3113 | ||
| 3114 | // Now that we've reported the errors, we need to deal with | |
| 3115 | // dependencies. Any file referenced by a multi_pkg file should also be | |
| 3116 | // marked multi_pkg and have its status set to astgen_failure, as it's | |
| 3117 | // ambiguous which package they should be analyzed as a part of. We need | |
| 3118 | // to add this flag after reporting the errors however, as otherwise | |
| 3119 | // we'd get an error for every single downstream file, which wouldn't be | |
| 3120 | // very useful. | |
| 3121 | for (mod.import_table.values()) |file| { | |
| 3122 | if (file.multi_pkg) file.recursiveMarkMultiPkg(mod); | |
| 3123 | } | |
| 3124 | } | |
| 3125 | ||
| 3083 | 3126 | { |
| 3084 | 3127 | const outdated_and_deleted_decls_frame = tracy.namedFrame("outdated_and_deleted_decls"); |
| 3085 | 3128 | defer outdated_and_deleted_decls_frame.end(); |
| ... | ... | @@ -3504,7 +3547,15 @@ fn workerAstGenFile( |
| 3504 | 3547 | comp.mutex.lock(); |
| 3505 | 3548 | defer comp.mutex.unlock(); |
| 3506 | 3549 | |
| 3507 | break :blk mod.importFile(file, import_path) catch continue; | |
| 3550 | const res = mod.importFile(file, import_path) catch continue; | |
| 3551 | if (!res.is_pkg) { | |
| 3552 | res.file.addReference(mod.*, .{ .import = .{ | |
| 3553 | .file_scope = file, | |
| 3554 | .parent_decl_node = 0, | |
| 3555 | .lazy = .{ .token_abs = item.data.token }, | |
| 3556 | } }) catch continue; | |
| 3557 | } | |
| 3558 | break :blk res; | |
| 3508 | 3559 | }; |
| 3509 | 3560 | if (import_result.is_new) { |
| 3510 | 3561 | log.debug("AstGen of {s} has import '{s}'; queuing AstGen of {s}", .{ |
src/Module.zig+57
| ... | ... | @@ -1943,6 +1943,10 @@ pub const File = struct { |
| 1943 | 1943 | zir: Zir, |
| 1944 | 1944 | /// Package that this file is a part of, managed externally. |
| 1945 | 1945 | pkg: *Package, |
| 1946 | /// Whether this file is a part of multiple packages. This is an error condition which will be reported after AstGen. | |
| 1947 | multi_pkg: bool = false, | |
| 1948 | /// List of references to this file, used for multi-package errors. | |
| 1949 | references: std.ArrayListUnmanaged(Reference) = .{}, | |
| 1946 | 1950 | |
| 1947 | 1951 | /// Used by change detection algorithm, after astgen, contains the |
| 1948 | 1952 | /// set of decls that existed in the previous ZIR but not in the new one. |
| ... | ... | @@ -1958,6 +1962,14 @@ pub const File = struct { |
| 1958 | 1962 | /// successful, this field is unloaded. |
| 1959 | 1963 | prev_zir: ?*Zir = null, |
| 1960 | 1964 | |
| 1965 | /// A single reference to a file. | |
| 1966 | const Reference = union(enum) { | |
| 1967 | /// The file is imported directly (i.e. not as a package) with @import. | |
| 1968 | import: SrcLoc, | |
| 1969 | /// The file is the root of a package. | |
| 1970 | root: *Package, | |
| 1971 | }; | |
| 1972 | ||
| 1961 | 1973 | pub fn unload(file: *File, gpa: Allocator) void { |
| 1962 | 1974 | file.unloadTree(gpa); |
| 1963 | 1975 | file.unloadSource(gpa); |
| ... | ... | @@ -1990,6 +2002,7 @@ pub const File = struct { |
| 1990 | 2002 | log.debug("deinit File {s}", .{file.sub_file_path}); |
| 1991 | 2003 | file.deleted_decls.deinit(gpa); |
| 1992 | 2004 | file.outdated_decls.deinit(gpa); |
| 2005 | file.references.deinit(gpa); | |
| 1993 | 2006 | if (file.root_decl.unwrap()) |root_decl| { |
| 1994 | 2007 | mod.destroyDecl(root_decl); |
| 1995 | 2008 | } |
| ... | ... | @@ -2110,6 +2123,44 @@ pub const File = struct { |
| 2110 | 2123 | else => true, |
| 2111 | 2124 | }; |
| 2112 | 2125 | } |
| 2126 | ||
| 2127 | /// Add a reference to this file during AstGen. | |
| 2128 | pub fn addReference(file: *File, mod: Module, ref: Reference) !void { | |
| 2129 | try file.references.append(mod.gpa, ref); | |
| 2130 | ||
| 2131 | const pkg = switch (ref) { | |
| 2132 | .import => |loc| loc.file_scope.pkg, | |
| 2133 | .root => |pkg| pkg, | |
| 2134 | }; | |
| 2135 | if (pkg != file.pkg) file.multi_pkg = true; | |
| 2136 | } | |
| 2137 | ||
| 2138 | /// Mark this file and every file referenced by it as multi_pkg and report an | |
| 2139 | /// astgen_failure error for them. AstGen must have completed in its entirety. | |
| 2140 | pub fn recursiveMarkMultiPkg(file: *File, mod: *Module) void { | |
| 2141 | file.multi_pkg = true; | |
| 2142 | file.status = .astgen_failure; | |
| 2143 | ||
| 2144 | std.debug.assert(file.zir_loaded); | |
| 2145 | const imports_index = file.zir.extra[@enumToInt(Zir.ExtraIndex.imports)]; | |
| 2146 | if (imports_index == 0) return; | |
| 2147 | const extra = file.zir.extraData(Zir.Inst.Imports, imports_index); | |
| 2148 | ||
| 2149 | var import_i: u32 = 0; | |
| 2150 | var extra_index = extra.end; | |
| 2151 | while (import_i < extra.data.imports_len) : (import_i += 1) { | |
| 2152 | const item = file.zir.extraData(Zir.Inst.Imports.Item, extra_index); | |
| 2153 | extra_index = item.end; | |
| 2154 | ||
| 2155 | const import_path = file.zir.nullTerminatedString(item.data.name); | |
| 2156 | if (mem.eql(u8, import_path, "builtin")) continue; | |
| 2157 | ||
| 2158 | const res = mod.importFile(file, import_path) catch continue; | |
| 2159 | if (!res.is_pkg and !res.file.multi_pkg) { | |
| 2160 | res.file.recursiveMarkMultiPkg(mod); | |
| 2161 | } | |
| 2162 | } | |
| 2163 | } | |
| 2113 | 2164 | }; |
| 2114 | 2165 | |
| 2115 | 2166 | /// Represents the contents of a file loaded with `@embedFile`. |
| ... | ... | @@ -4690,6 +4741,7 @@ pub fn declareDeclDependency(mod: *Module, depender_index: Decl.Index, dependee_ |
| 4690 | 4741 | pub const ImportFileResult = struct { |
| 4691 | 4742 | file: *File, |
| 4692 | 4743 | is_new: bool, |
| 4744 | is_pkg: bool, | |
| 4693 | 4745 | }; |
| 4694 | 4746 | |
| 4695 | 4747 | pub fn importPkg(mod: *Module, pkg: *Package) !ImportFileResult { |
| ... | ... | @@ -4709,6 +4761,7 @@ pub fn importPkg(mod: *Module, pkg: *Package) !ImportFileResult { |
| 4709 | 4761 | if (gop.found_existing) return ImportFileResult{ |
| 4710 | 4762 | .file = gop.value_ptr.*, |
| 4711 | 4763 | .is_new = false, |
| 4764 | .is_pkg = true, | |
| 4712 | 4765 | }; |
| 4713 | 4766 | |
| 4714 | 4767 | const sub_file_path = try gpa.dupe(u8, pkg.root_src_path); |
| ... | ... | @@ -4732,9 +4785,11 @@ pub fn importPkg(mod: *Module, pkg: *Package) !ImportFileResult { |
| 4732 | 4785 | .pkg = pkg, |
| 4733 | 4786 | .root_decl = .none, |
| 4734 | 4787 | }; |
| 4788 | try new_file.addReference(mod.*, .{ .root = pkg }); | |
| 4735 | 4789 | return ImportFileResult{ |
| 4736 | 4790 | .file = new_file, |
| 4737 | 4791 | .is_new = true, |
| 4792 | .is_pkg = true, | |
| 4738 | 4793 | }; |
| 4739 | 4794 | } |
| 4740 | 4795 | |
| ... | ... | @@ -4775,6 +4830,7 @@ pub fn importFile( |
| 4775 | 4830 | if (gop.found_existing) return ImportFileResult{ |
| 4776 | 4831 | .file = gop.value_ptr.*, |
| 4777 | 4832 | .is_new = false, |
| 4833 | .is_pkg = false, | |
| 4778 | 4834 | }; |
| 4779 | 4835 | |
| 4780 | 4836 | const new_file = try gpa.create(File); |
| ... | ... | @@ -4820,6 +4876,7 @@ pub fn importFile( |
| 4820 | 4876 | return ImportFileResult{ |
| 4821 | 4877 | .file = new_file, |
| 4822 | 4878 | .is_new = true, |
| 4879 | .is_pkg = false, | |
| 4823 | 4880 | }; |
| 4824 | 4881 | } |
| 4825 | 4882 |