From 00969062a9d5e8229737f0ec38a8af91822caf31 Mon Sep 17 00:00:00 2001 From: mlugg Date: Fri, 8 Mar 2024 10:01:33 +0000 Subject: [PATCH] compiler: detect duplicate test names in AstGen There is no reason to perform this detection during semantic analysis. In fact, doing so is problematic, because we wish to utilize detection of existing decls in a namespace in incremental compilation. --- lib/std/zig/AstGen.zig | 50 ++++++++++++++++++- src/Module.zig | 23 ++------- .../invalid_duplicate_test_decl_name.zig | 4 +- 3 files changed, 56 insertions(+), 21 deletions(-) diff --git a/lib/std/zig/AstGen.zig b/lib/std/zig/AstGen.zig index e20925adbc..364e49ae8f 100644 --- a/lib/std/zig/AstGen.zig +++ b/lib/std/zig/AstGen.zig @@ -13496,6 +13496,15 @@ fn scanDecls(astgen: *AstGen, namespace: *Scope.Namespace, members: []const Ast. const node_tags = tree.nodes.items(.tag); const main_tokens = tree.nodes.items(.main_token); const token_tags = tree.tokens.items(.tag); + + // We don't have shadowing for test names, so we just track those for duplicate reporting locally. + var named_tests: std.AutoHashMapUnmanaged(Zir.NullTerminatedString, Ast.Node.Index) = .{}; + var decltests: std.AutoHashMapUnmanaged(Zir.NullTerminatedString, Ast.Node.Index) = .{}; + defer { + named_tests.deinit(gpa); + decltests.deinit(gpa); + } + var decl_count: u32 = 0; for (members) |member_node| { const name_token = switch (node_tags[member_node]) { @@ -13525,11 +13534,50 @@ fn scanDecls(astgen: *AstGen, namespace: *Scope.Namespace, members: []const Ast. break :blk ident; }, - .@"comptime", .@"usingnamespace", .test_decl => { + .@"comptime", .@"usingnamespace" => { decl_count += 1; continue; }, + .test_decl => { + decl_count += 1; + // We don't want shadowing detection here, and test names work a bit differently, so + // we must do the redeclaration detection ourselves. + const test_name_token = main_tokens[member_node] + 1; + switch (token_tags[test_name_token]) { + else => {}, // unnamed test + .string_literal => { + const name = try astgen.strLitAsString(test_name_token); + const gop = try named_tests.getOrPut(gpa, name.index); + if (gop.found_existing) { + const name_slice = astgen.string_bytes.items[@intFromEnum(name.index)..][0..name.len]; + const name_duped = try gpa.dupe(u8, name_slice); + defer gpa.free(name_duped); + try astgen.appendErrorNodeNotes(member_node, "duplicate test name '{s}'", .{name_duped}, &.{ + try astgen.errNoteNode(gop.value_ptr.*, "other test here", .{}), + }); + } else { + gop.value_ptr.* = member_node; + } + }, + .identifier => { + const name = try astgen.identAsString(test_name_token); + const gop = try decltests.getOrPut(gpa, name); + if (gop.found_existing) { + const name_slice = mem.span(astgen.nullTerminatedString(name)); + const name_duped = try gpa.dupe(u8, name_slice); + defer gpa.free(name_duped); + try astgen.appendErrorNodeNotes(member_node, "duplicate decltest '{s}'", .{name_duped}, &.{ + try astgen.errNoteNode(gop.value_ptr.*, "other decltest here", .{}), + }); + } else { + gop.value_ptr.* = member_node; + } + }, + } + continue; + }, + else => continue, }; diff --git a/src/Module.zig b/src/Module.zig index 7d20c0f023..c64b6cb2ef 100644 --- a/src/Module.zig +++ b/src/Module.zig @@ -4128,6 +4128,7 @@ fn scanDecl(iter: *ScanDeclIter, decl_inst: Zir.Inst.Index) Allocator.Error!void .@"comptime" => info: { const i = iter.comptime_index; iter.comptime_index += 1; + // TODO: avoid collisions with named decls with this name break :info .{ try ip.getOrPutStringFmt(gpa, "comptime_{d}", .{i}), .@"comptime", @@ -4137,6 +4138,7 @@ fn scanDecl(iter: *ScanDeclIter, decl_inst: Zir.Inst.Index) Allocator.Error!void .@"usingnamespace" => info: { const i = iter.usingnamespace_index; iter.usingnamespace_index += 1; + // TODO: avoid collisions with named decls with this name break :info .{ try ip.getOrPutStringFmt(gpa, "usingnamespace_{d}", .{i}), .@"usingnamespace", @@ -4146,6 +4148,7 @@ fn scanDecl(iter: *ScanDeclIter, decl_inst: Zir.Inst.Index) Allocator.Error!void .unnamed_test => info: { const i = iter.unnamed_test_index; iter.unnamed_test_index += 1; + // TODO: avoid collisions with named decls with this name break :info .{ try ip.getOrPutStringFmt(gpa, "test_{d}", .{i}), .@"test", @@ -4155,6 +4158,7 @@ fn scanDecl(iter: *ScanDeclIter, decl_inst: Zir.Inst.Index) Allocator.Error!void .decltest => info: { assert(declaration.flags.has_doc_comment); const name = zir.nullTerminatedString(@enumFromInt(zir.extra[extra.end])); + // TODO: avoid collisions with named decls with this name break :info .{ try ip.getOrPutStringFmt(gpa, "decltest.{s}", .{name}), .@"test", @@ -4162,6 +4166,7 @@ fn scanDecl(iter: *ScanDeclIter, decl_inst: Zir.Inst.Index) Allocator.Error!void }; }, _ => if (declaration.name.isNamedTest(zir)) .{ + // TODO: avoid collisions with named decls with this name try ip.getOrPutStringFmt(gpa, "test.{s}", .{zir.nullTerminatedString(declaration.name.toString(zir).?)}), .@"test", true, @@ -4226,24 +4231,6 @@ fn scanDecl(iter: *ScanDeclIter, decl_inst: Zir.Inst.Index) Allocator.Error!void } const decl_index = gop.key_ptr.*; const decl = zcu.declPtr(decl_index); - if (kind == .@"test") { - const src_loc = SrcLoc{ - .file_scope = decl.getFileScope(zcu), - .parent_decl_node = decl.src_node, - .lazy = .{ .token_offset = 1 }, - }; - const msg = try ErrorMsg.create(gpa, src_loc, "duplicate test name: {}", .{ - decl_name.fmt(ip), - }); - errdefer msg.destroy(gpa); - try zcu.failed_decls.putNoClobber(gpa, decl_index, msg); - const other_src_loc = SrcLoc{ - .file_scope = namespace.file_scope, - .parent_decl_node = decl_node, - .lazy = .{ .token_offset = 1 }, - }; - try zcu.errNoteNonLazy(other_src_loc, msg, "other test here", .{}); - } // Update the AST node of the decl; even if its contents are unchanged, it may // have been re-ordered. decl.src_node = decl_node; diff --git a/test/cases/compile_errors/invalid_duplicate_test_decl_name.zig b/test/cases/compile_errors/invalid_duplicate_test_decl_name.zig index 48008f970d..2dd330912a 100644 --- a/test/cases/compile_errors/invalid_duplicate_test_decl_name.zig +++ b/test/cases/compile_errors/invalid_duplicate_test_decl_name.zig @@ -6,5 +6,5 @@ test "thingy" {} // target=native // is_test=true // -// :1:6: error: duplicate test name: test.thingy -// :2:6: note: other test here +// :2:1: error: duplicate test name 'thingy' +// :1:1: note: other test here