From 1b8f7e46fa33cda57caf39c5a5a0f5f52335a99b Mon Sep 17 00:00:00 2001
From: David Rubin <87927264+Rexicon226@users.noreply.github.com>
Date: Sat, 20 Jan 2024 09:23:47 -0800
Subject: [PATCH] AstGen: detect duplicate field names

This logic was previously in Sema, which was unnecessary complexity, and meant the issue was not detected unless the declaration was semantically analyzed. This commit finishes the work which 941090d started.

Resolves: #17916
---
 src/AstGen.zig                                | 112 ++++++++++++++++--
 src/Sema.zig                                  |  53 +--------
 .../compile_errors/duplicate_enum_field.zig   |   5 +-
 ...cate_field_in_anonymous_struct_literal.zig |   5 +-
 ...duplicate_field_in_discarded_anon_init.zig |   5 +-
 ...icate_field_in_struct_value_expression.zig |   5 +-
 .../compile_errors/duplicate_struct_field.zig |  10 +-
 .../compile_errors/duplicate_union_field.zig  |   4 +-
 ..._initializer_doesnt_crash_the_compiler.zig |   4 +-
 .../struct_duplicate_field_name.zig           |   4 +-
 .../union_duplicate_enum_field.zig            |   4 +-
 .../union_duplicate_field_definition.zig      |   4 +-
 test/cbe.zig                                  |  10 +-
 13 files changed, 138 insertions(+), 87 deletions(-)

diff --git a/src/AstGen.zig b/src/AstGen.zig
index b613a6b6774ebdde31d49bf5ff56cbe835dbe621..f709751fde3c0d752b5bbe25f53c6a6bdf5caac0 100644
--- a/src/AstGen.zig
+++ b/src/AstGen.zig
@@ -1753,7 +1753,6 @@ fn structInitExpr(
         const sfba_allocator = sfba.get();
 
         var duplicate_names = std.AutoArrayHashMap(Zir.NullTerminatedString, ArrayListUnmanaged(Ast.TokenIndex)).init(sfba_allocator);
-        defer duplicate_names.deinit();
         try duplicate_names.ensureTotalCapacity(@intCast(struct_init.ast.fields.len));
 
         // When there aren't errors, use this to avoid a second iteration.
@@ -1783,12 +1782,14 @@ fn structInitExpr(
                     var error_notes = std.ArrayList(u32).init(astgen.arena);
 
                     for (record.items[1..]) |duplicate| {
-                        try error_notes.append(try astgen.errNoteTok(duplicate, "other field here", .{}));
+                        try error_notes.append(try astgen.errNoteTok(duplicate, "duplicate name here", .{}));
                     }
 
+                    try error_notes.append(try astgen.errNoteNode(node, "struct declared here", .{}));
+
                     try astgen.appendErrorTokNotes(
                         record.items[0],
-                        "duplicate field",
+                        "duplicate struct field name",
                         .{},
                         error_notes.items,
                     );
@@ -1815,9 +1816,10 @@ fn structInitExpr(
     switch (ri.rl) {
         .none => return structInitExprAnon(gz, scope, node, struct_init),
         .discard => {
-            // Even if discarding we must perform an anonymous init to check for duplicate field names.
-            // TODO: should duplicate field names be caught in AstGen?
-            _ = try structInitExprAnon(gz, scope, node, struct_init);
+            // Even if discarding we must perform side-effects.
+            for (struct_init.ast.fields) |field_init| {
+                _ = try expr(gz, scope, .{ .rl = .discard }, field_init);
+            }
             return .void_value;
         },
         .ref => {
@@ -5101,15 +5103,15 @@ fn structDeclInner(
                 var error_notes = std.ArrayList(u32).init(astgen.arena);
 
                 for (record.items[1..]) |duplicate| {
-                    try error_notes.append(try astgen.errNoteTok(duplicate, "other field here", .{}));
+                    try error_notes.append(try astgen.errNoteTok(duplicate, "duplicate field here", .{}));
                 }
 
                 try error_notes.append(try astgen.errNoteNode(node, "struct declared here", .{}));
 
                 try astgen.appendErrorTokNotes(
                     record.items[0],
-                    "duplicate struct field: '{s}'",
-                    .{try astgen.identifierTokenString(record.items[0])},
+                    "duplicate struct field name",
+                    .{},
                     error_notes.items,
                 );
             }
@@ -5118,8 +5120,6 @@ fn structDeclInner(
         return error.AnalysisFail;
     }
 
-    duplicate_names.deinit();
-
     try gz.setStruct(decl_inst, .{
         .src_node = node,
         .layout = layout,
@@ -5211,6 +5211,15 @@ fn unionDeclInner(
     var wip_members = try WipMembers.init(gpa, &astgen.scratch, decl_count, field_count, bits_per_field, max_field_size);
     defer wip_members.deinit();
 
+    var sfba = std.heap.stackFallback(256, astgen.arena);
+    const sfba_allocator = sfba.get();
+
+    var duplicate_names = std.AutoArrayHashMap(Zir.NullTerminatedString, std.ArrayListUnmanaged(Ast.TokenIndex)).init(sfba_allocator);
+    try duplicate_names.ensureTotalCapacity(field_count);
+
+    // When there aren't errors, use this to avoid a second iteration.
+    var any_duplicate = false;
+
     for (members) |member_node| {
         var member = switch (try containerMember(&block_scope, &namespace.base, &wip_members, member_node)) {
             .decl => continue,
@@ -5227,6 +5236,16 @@ fn unionDeclInner(
         const field_name = try astgen.identAsString(member.ast.main_token);
         wip_members.appendToField(@intFromEnum(field_name));
 
+        const gop = try duplicate_names.getOrPut(field_name);
+
+        if (gop.found_existing) {
+            try gop.value_ptr.append(sfba_allocator, member.ast.main_token);
+            any_duplicate = true;
+        } else {
+            gop.value_ptr.* = .{};
+            try gop.value_ptr.append(sfba_allocator, member.ast.main_token);
+        }
+
         const doc_comment_index = try astgen.docCommentAsString(member.firstToken());
         wip_members.appendToField(@intFromEnum(doc_comment_index));
 
@@ -5281,6 +5300,32 @@ fn unionDeclInner(
         }
     }
 
+    if (any_duplicate) {
+        var it = duplicate_names.iterator();
+
+        while (it.next()) |entry| {
+            const record = entry.value_ptr.*;
+            if (record.items.len > 1) {
+                var error_notes = std.ArrayList(u32).init(astgen.arena);
+
+                for (record.items[1..]) |duplicate| {
+                    try error_notes.append(try astgen.errNoteTok(duplicate, "duplicate field here", .{}));
+                }
+
+                try error_notes.append(try astgen.errNoteNode(node, "union declared here", .{}));
+
+                try astgen.appendErrorTokNotes(
+                    record.items[0],
+                    "duplicate union field name",
+                    .{},
+                    error_notes.items,
+                );
+            }
+        }
+
+        return error.AnalysisFail;
+    }
+
     if (!block_scope.isEmpty()) {
         _ = try block_scope.addBreak(.break_inline, decl_inst, .void_value);
     }
@@ -5490,6 +5535,15 @@ fn containerDecl(
             var wip_members = try WipMembers.init(gpa, &astgen.scratch, @intCast(counts.decls), @intCast(counts.total_fields), bits_per_field, max_field_size);
             defer wip_members.deinit();
 
+            var sfba = std.heap.stackFallback(256, astgen.arena);
+            const sfba_allocator = sfba.get();
+
+            var duplicate_names = std.AutoArrayHashMap(Zir.NullTerminatedString, std.ArrayListUnmanaged(Ast.TokenIndex)).init(sfba_allocator);
+            try duplicate_names.ensureTotalCapacity(counts.total_fields);
+
+            // When there aren't errors, use this to avoid a second iteration.
+            var any_duplicate = false;
+
             for (container_decl.ast.members) |member_node| {
                 if (member_node == counts.nonexhaustive_node)
                     continue;
@@ -5506,6 +5560,16 @@ fn containerDecl(
                 const field_name = try astgen.identAsString(member.ast.main_token);
                 wip_members.appendToField(@intFromEnum(field_name));
 
+                const gop = try duplicate_names.getOrPut(field_name);
+
+                if (gop.found_existing) {
+                    try gop.value_ptr.append(sfba_allocator, member.ast.main_token);
+                    any_duplicate = true;
+                } else {
+                    gop.value_ptr.* = .{};
+                    try gop.value_ptr.append(sfba_allocator, member.ast.main_token);
+                }
+
                 const doc_comment_index = try astgen.docCommentAsString(member.firstToken());
                 wip_members.appendToField(@intFromEnum(doc_comment_index));
 
@@ -5533,6 +5597,32 @@ fn containerDecl(
                 }
             }
 
+            if (any_duplicate) {
+                var it = duplicate_names.iterator();
+
+                while (it.next()) |entry| {
+                    const record = entry.value_ptr.*;
+                    if (record.items.len > 1) {
+                        var error_notes = std.ArrayList(u32).init(astgen.arena);
+
+                        for (record.items[1..]) |duplicate| {
+                            try error_notes.append(try astgen.errNoteTok(duplicate, "duplicate field here", .{}));
+                        }
+
+                        try error_notes.append(try astgen.errNoteNode(node, "enum declared here", .{}));
+
+                        try astgen.appendErrorTokNotes(
+                            record.items[0],
+                            "duplicate enum field name",
+                            .{},
+                            error_notes.items,
+                        );
+                    }
+                }
+
+                return error.AnalysisFail;
+            }
+
             if (!block_scope.isEmpty()) {
                 _ = try block_scope.addBreak(.break_inline, decl_inst, .void_value);
             }
diff --git a/src/Sema.zig b/src/Sema.zig
index a38da0adefe92b63a9a8a4edff32064316a8a213..96be1a441fac930930441bcfe2d5dcff4605a7c8 100644
--- a/src/Sema.zig
+++ b/src/Sema.zig
@@ -3046,23 +3046,10 @@ fn zirEnumDecl(
 
         const field_name_index: Zir.NullTerminatedString = @enumFromInt(sema.code.extra[extra_index]);
         const field_name_zir = sema.code.nullTerminatedString(field_name_index);
-        extra_index += 1;
-
-        // doc comment
-        extra_index += 1;
+        extra_index += 2; // field name, doc comment
 
         const field_name = try mod.intern_pool.getOrPutString(gpa, field_name_zir);
-        if (incomplete_enum.addFieldName(&mod.intern_pool, field_name)) |other_index| {
-            const field_src = mod.fieldSrcLoc(new_decl_index, .{ .index = field_i }).lazy;
-            const other_field_src = mod.fieldSrcLoc(new_decl_index, .{ .index = other_index }).lazy;
-            const msg = msg: {
-                const msg = try sema.errMsg(block, field_src, "duplicate enum field '{s}'", .{field_name_zir});
-                errdefer msg.destroy(gpa);
-                try sema.errNote(block, other_field_src, msg, "other field here", .{});
-                break :msg msg;
-            };
-            return sema.failWithOwnedErrorMsg(block, msg);
-        }
+        assert(incomplete_enum.addFieldName(&mod.intern_pool, field_name) == null);
 
         const tag_overflow = if (has_tag_value) overflow: {
             const tag_val_ref: Zir.Inst.Ref = @enumFromInt(sema.code.extra[extra_index]);
@@ -21732,7 +21719,7 @@ fn reifyStruct(
             }
         } else if (struct_type.addFieldName(ip, field_name)) |prev_index| {
             _ = prev_index; // TODO: better source location
-            return sema.fail(block, src, "duplicate struct field {}", .{field_name.fmt(ip)});
+            return sema.fail(block, src, "duplicate struct field name {}", .{field_name.fmt(ip)});
         }
 
         const field_ty = type_val.toType();
@@ -36284,7 +36271,6 @@ fn semaStructFields(
     const zir = mod.namespacePtr(namespace_index).file_scope.zir;
     const zir_index = struct_type.zir_index;
 
-    const src = LazySrcLoc.nodeOffset(0);
     const fields_len, const small, var extra_index = structZirInfo(zir, zir_index);
 
     if (fields_len == 0) switch (struct_type.layout) {
@@ -36384,19 +36370,7 @@ fn semaStructFields(
             // This string needs to outlive the ZIR code.
             if (opt_field_name_zir) |field_name_zir| {
                 const field_name = try ip.getOrPutString(gpa, field_name_zir);
-                if (struct_type.addFieldName(ip, field_name)) |other_index| {
-                    const msg = msg: {
-                        const field_src = mod.fieldSrcLoc(decl_index, .{ .index = field_i }).lazy;
-                        const msg = try sema.errMsg(&block_scope, field_src, "duplicate struct field: '{}'", .{field_name.fmt(ip)});
-                        errdefer msg.destroy(gpa);
-
-                        const prev_field_src = mod.fieldSrcLoc(decl_index, .{ .index = other_index });
-                        try mod.errNoteNonLazy(prev_field_src, msg, "other field here", .{});
-                        try sema.errNote(&block_scope, src, msg, "struct declared here", .{});
-                        break :msg msg;
-                    };
-                    return sema.failWithOwnedErrorMsg(&block_scope, msg);
-                }
+                assert(struct_type.addFieldName(ip, field_name) == null);
             }
 
             if (has_align) {
@@ -36840,12 +36814,10 @@ fn semaUnionFields(mod: *Module, arena: Allocator, union_type: InternPool.Key.Un
 
     var field_types: std.ArrayListUnmanaged(InternPool.Index) = .{};
     var field_aligns: std.ArrayListUnmanaged(InternPool.Alignment) = .{};
-    var field_name_table: std.AutoArrayHashMapUnmanaged(InternPool.NullTerminatedString, void) = .{};
 
     try field_types.ensureTotalCapacityPrecise(sema.arena, fields_len);
     if (small.any_aligned_fields)
         try field_aligns.ensureTotalCapacityPrecise(sema.arena, fields_len);
-    try field_name_table.ensureTotalCapacity(sema.arena, fields_len);
 
     const bits_per_field = 4;
     const fields_per_u32 = 32 / bits_per_field;
@@ -36961,23 +36933,6 @@ fn semaUnionFields(mod: *Module, arena: Allocator, union_type: InternPool.Key.Un
             return error.GenericPoison;
         }
 
-        const gop = field_name_table.getOrPutAssumeCapacity(field_name);
-        if (gop.found_existing) {
-            const msg = msg: {
-                const field_src = mod.fieldSrcLoc(union_type.decl, .{ .index = field_i }).lazy;
-                const msg = try sema.errMsg(&block_scope, field_src, "duplicate union field: '{}'", .{
-                    field_name.fmt(ip),
-                });
-                errdefer msg.destroy(gpa);
-
-                const prev_field_src = mod.fieldSrcLoc(union_type.decl, .{ .index = gop.index }).lazy;
-                try mod.errNoteNonLazy(prev_field_src.toSrcLoc(decl, mod), msg, "other field here", .{});
-                try sema.errNote(&block_scope, src, msg, "union declared here", .{});
-                break :msg msg;
-            };
-            return sema.failWithOwnedErrorMsg(&block_scope, msg);
-        }
-
         if (explicit_tags_seen.len > 0) {
             const tag_info = ip.indexToKey(union_type.tagTypePtr(ip).*).enum_type;
             const enum_index = tag_info.nameIndex(ip, field_name) orelse {
diff --git a/test/cases/compile_errors/duplicate_enum_field.zig b/test/cases/compile_errors/duplicate_enum_field.zig
index ebf01f99a70f89d7fb41ce0cbd66586960a2f917..02738f1b37430b23a1da463a1c89256fba4589c8 100644
--- a/test/cases/compile_errors/duplicate_enum_field.zig
+++ b/test/cases/compile_errors/duplicate_enum_field.zig
@@ -12,5 +12,6 @@ export fn entry() void {
 // backend=stage2
 // target=native
 //
-// :3:5: error: duplicate enum field 'Bar'
-// :2:5: note: other field here
+// :2:5: error: duplicate enum field name
+// :3:5: note: duplicate field here
+// :1:13: note: enum declared here
diff --git a/test/cases/compile_errors/duplicate_field_in_anonymous_struct_literal.zig b/test/cases/compile_errors/duplicate_field_in_anonymous_struct_literal.zig
index ee81f72b1db5be344ee6d0825d311fdc04e2b51b..e1e220e34622318e53f26361111015f16a187d45 100644
--- a/test/cases/compile_errors/duplicate_field_in_anonymous_struct_literal.zig
+++ b/test/cases/compile_errors/duplicate_field_in_anonymous_struct_literal.zig
@@ -14,5 +14,6 @@ export fn entry() void {
 // backend=stage2
 // target=native
 //
-// :4:14: error: duplicate field
-// :7:14: note: other field here
+// :4:14: error: duplicate struct field name
+// :7:14: note: duplicate name here
+// :3:19: note: struct declared here
diff --git a/test/cases/compile_errors/duplicate_field_in_discarded_anon_init.zig b/test/cases/compile_errors/duplicate_field_in_discarded_anon_init.zig
index ff1912d27bbde8518a4fe6f33bd8ea686ae6c748..913100f79b9d180599912e843d193c55a4176b6e 100644
--- a/test/cases/compile_errors/duplicate_field_in_discarded_anon_init.zig
+++ b/test/cases/compile_errors/duplicate_field_in_discarded_anon_init.zig
@@ -6,5 +6,6 @@ pub export fn entry() void {
 // backend=stage2
 // target=native
 //
-// :2:13: error: duplicate field
-// :2:21: note: other field here
+// :2:13: error: duplicate struct field name
+// :2:21: note: duplicate name here
+// :2:10: note: struct declared here
diff --git a/test/cases/compile_errors/duplicate_field_in_struct_value_expression.zig b/test/cases/compile_errors/duplicate_field_in_struct_value_expression.zig
index e52f835146e76b70a9a509511b87684cbb2478ee..b0f656b3c46f83a3ca24814495a1874efe400df0 100644
--- a/test/cases/compile_errors/duplicate_field_in_struct_value_expression.zig
+++ b/test/cases/compile_errors/duplicate_field_in_struct_value_expression.zig
@@ -17,5 +17,6 @@ export fn f() void {
 // backend=stage2
 // target=native
 //
-// :8:10: error: duplicate field
-// :11:10: note: other field here
+// :8:10: error: duplicate struct field name
+// :11:10: note: duplicate name here
+// :7:16: note: struct declared here
diff --git a/test/cases/compile_errors/duplicate_struct_field.zig b/test/cases/compile_errors/duplicate_struct_field.zig
index 61340a423ed952c29b07cac11cf97d2c124ff878..954ed24e920f0659353ea6224d8ffb4c20f3bdd6 100644
--- a/test/cases/compile_errors/duplicate_struct_field.zig
+++ b/test/cases/compile_errors/duplicate_struct_field.zig
@@ -24,10 +24,10 @@ export fn b() void {
 // backend=stage2
 // target=native
 //
-// :2:5: error: duplicate struct field: 'Bar'
-// :3:5: note: other field here
+// :2:5: error: duplicate struct field name
+// :3:5: note: duplicate field here
 // :1:13: note: struct declared here
-// :7:5: error: duplicate struct field: 'a'
-// :9:5: note: other field here
-// :10:5: note: other field here
+// :7:5: error: duplicate struct field name
+// :9:5: note: duplicate field here
+// :10:5: note: duplicate field here
 // :6:11: note: struct declared here
diff --git a/test/cases/compile_errors/duplicate_union_field.zig b/test/cases/compile_errors/duplicate_union_field.zig
index 6a0265dd0294d5c6ba8dd334eea382cfa3363087..b57842c1f79869f171d8194251a7519189c52653 100644
--- a/test/cases/compile_errors/duplicate_union_field.zig
+++ b/test/cases/compile_errors/duplicate_union_field.zig
@@ -11,6 +11,6 @@ export fn entry() void {
 // backend=stage2
 // target=native
 //
-// :3:5: error: duplicate union field: 'Bar'
-// :2:5: note: other field here
+// :2:5: error: duplicate union field name
+// :3:5: note: duplicate field here
 // :1:13: note: union declared here
diff --git a/test/cases/compile_errors/error_in_struct_initializer_doesnt_crash_the_compiler.zig b/test/cases/compile_errors/error_in_struct_initializer_doesnt_crash_the_compiler.zig
index 515ac855ce2ca6956eacaa5759bdb3aa238d5088..ea799c561aafadb935bcb70aa013cca494a4a7c9 100644
--- a/test/cases/compile_errors/error_in_struct_initializer_doesnt_crash_the_compiler.zig
+++ b/test/cases/compile_errors/error_in_struct_initializer_doesnt_crash_the_compiler.zig
@@ -11,6 +11,6 @@ pub export fn entry() void {
 // backend=stage2
 // target=native
 //
-// :3:9: error: duplicate struct field: 'e'
-// :4:9: note: other field here
+// :3:9: error: duplicate struct field name
+// :4:9: note: duplicate field here
 // :2:22: note: struct declared here
diff --git a/test/cases/compile_errors/struct_duplicate_field_name.zig b/test/cases/compile_errors/struct_duplicate_field_name.zig
index d19e94b9d228c843ea1e66817628527d73f258dc..057083c3cb5723105352cf3cee7fc7e71cf1c39f 100644
--- a/test/cases/compile_errors/struct_duplicate_field_name.zig
+++ b/test/cases/compile_errors/struct_duplicate_field_name.zig
@@ -11,6 +11,6 @@ export fn entry() void {
 // error
 // target=native
 //
-// :2:5: error: duplicate struct field: 'foo'
-// :3:5: note: other field here
+// :2:5: error: duplicate struct field name
+// :3:5: note: duplicate field here
 // :1:11: note: struct declared here
diff --git a/test/cases/compile_errors/union_duplicate_enum_field.zig b/test/cases/compile_errors/union_duplicate_enum_field.zig
index 6a5fafbb2580da3bce182fbe43e195489ec41768..21c307755be00069a169ccaba1fc3e6b314fc624 100644
--- a/test/cases/compile_errors/union_duplicate_enum_field.zig
+++ b/test/cases/compile_errors/union_duplicate_enum_field.zig
@@ -12,6 +12,6 @@ export fn foo() void {
 // error
 // target=native
 //
-// :4:5: error: duplicate union field: 'a'
-// :3:5: note: other field here
+// :3:5: error: duplicate union field name
+// :4:5: note: duplicate field here
 // :2:11: note: union declared here
diff --git a/test/cases/compile_errors/union_duplicate_field_definition.zig b/test/cases/compile_errors/union_duplicate_field_definition.zig
index 7993f27141f54f163bc29e32dfe0ed46054c67c4..e0866964eb36494816d6a7652c170bab0ff71a8f 100644
--- a/test/cases/compile_errors/union_duplicate_field_definition.zig
+++ b/test/cases/compile_errors/union_duplicate_field_definition.zig
@@ -11,6 +11,6 @@ export fn entry() void {
 // error
 // target=native
 //
-// :3:5: error: duplicate union field: 'foo'
-// :2:5: note: other field here
+// :2:5: error: duplicate union field name
+// :3:5: note: duplicate field here
 // :1:11: note: union declared here
diff --git a/test/cbe.zig b/test/cbe.zig
index 26724a1df00072a2dbeffd9c2746e29907ecc548..35c2d8e8f9d137a3d15d9447934d0005f54eab6b 100644
--- a/test/cbe.zig
+++ b/test/cbe.zig
@@ -507,8 +507,9 @@ pub fn addCases(ctx: *Cases, b: *std.Build) !void {
             \\    return p.y - p.x - p.x;
             \\}
         , &.{
-            ":4:10: error: duplicate field",
-            ":6:10: note: other field here",
+            ":4:10: error: duplicate struct field name",
+            ":6:10: note: duplicate name here",
+            ":3:21: note: struct declared here",
         });
         case.addError(
             \\const Point = struct { x: i32, y: i32 };
@@ -722,8 +723,9 @@ pub fn addCases(ctx: *Cases, b: *std.Build) !void {
             \\    _ = E1.a;
             \\}
         , &.{
-            ":1:28: error: duplicate enum field 'b'",
-            ":1:22: note: other field here",
+            ":1:22: error: duplicate enum field name",
+            ":1:28: note: duplicate field here",
+            ":1:12: note: enum declared here",
         });
 
         case.addError(
-- 
2.54.0