Detect duplicate member names in struct and struct type literals (#3395)

A struct with the same member name twice can cause a `CARBON_CHECK`
failure later when it is used (problem found by fuzzing).

---------

Co-authored-by: Richard Smith <richard@metafoo.co.uk>

toolchain/check/handle_struct.cpp

@@ -39,19 +39,47 @@ auto HandleStructFieldUnknown(Context& context, Parse::Node parse_node)
 }
 
 auto HandleStructFieldValue(Context& context, Parse::Node parse_node) -> bool {
-  auto [value_parse_node, value_inst_id] =
-      context.node_stack().PopExprWithParseNode();
-  SemIR::NameId name_id = context.node_stack().Pop<Parse::NodeKind::Name>();
+  auto value_inst_id = context.node_stack().PopExpr();
+  auto [name_node, name_id] =
+      context.node_stack().PopWithParseNode<Parse::NodeKind::Name>();
 
   // Store the name for the type.
   context.args_type_info_stack().AddInst(SemIR::StructTypeField{
-      parse_node, name_id, context.insts().Get(value_inst_id).type_id()});
+      name_node, name_id, context.insts().Get(value_inst_id).type_id()});
 
   // Push the value back on the stack as an argument.
   context.node_stack().Push(parse_node, value_inst_id);
   return true;
 }
 
+static auto DiagnoseDuplicateNames(Context& context,
+                                   SemIR::InstBlockId type_block_id,
+                                   llvm::StringRef construct) -> bool {
+  auto& sem_ir = context.sem_ir();
+  auto fields = sem_ir.inst_blocks().Get(type_block_id);
+  llvm::SmallDenseMap<SemIR::NameId, Parse::Node> names;
+  auto& insts = sem_ir.insts();
+  for (SemIR::InstId field_inst_id : fields) {
+    auto field_inst = insts.GetAs<SemIR::StructTypeField>(field_inst_id);
+    auto [it, added] =
+        names.insert({field_inst.name_id, field_inst.parse_node});
+    if (!added) {
+      CARBON_DIAGNOSTIC(StructNameDuplicate, Error,
+                        "Duplicated field name `{1}` in {0}.", llvm::StringRef,
+                        llvm::StringRef);
+      CARBON_DIAGNOSTIC(StructNamePrevious, Note,
+                        "Field with the same name here.");
+      context.emitter()
+          .Build(field_inst.parse_node, StructNameDuplicate, construct,
+                 sem_ir.names().GetFormatted(field_inst.name_id))
+          .Note(it->second, StructNamePrevious)
+          .Emit();
+      return true;
+    }
+  }
+  return false;
+}
+
 auto HandleStructLiteral(Context& context, Parse::Node parse_node) -> bool {
   auto refs_id = context.ParamOrArgEnd(
       Parse::NodeKind::StructLiteralOrStructTypeLiteralStart);
@@ -61,6 +89,10 @@ auto HandleStructLiteral(Context& context, Parse::Node parse_node) -> bool {
       .PopAndDiscardSoloParseNode<
           Parse::NodeKind::StructLiteralOrStructTypeLiteralStart>();
   auto type_block_id = context.args_type_info_stack().Pop();
+  if (DiagnoseDuplicateNames(context, type_block_id, "struct literal")) {
+    context.node_stack().Push(parse_node, SemIR::InstId::BuiltinError);
+    return true;
+  }
 
   auto type_id = context.CanonicalizeStructType(parse_node, type_block_id);
 
@@ -97,6 +129,10 @@ auto HandleStructTypeLiteral(Context& context, Parse::Node parse_node) -> bool {
   CARBON_CHECK(refs_id != SemIR::InstBlockId::Empty)
       << "{} is handled by StructLiteral.";
 
+  if (DiagnoseDuplicateNames(context, refs_id, "struct type literal")) {
+    context.node_stack().Push(parse_node, SemIR::InstId::BuiltinError);
+    return true;
+  }
   context.AddInstAndPush(
       parse_node,
       SemIR::StructType{parse_node, SemIR::TypeId::TypeType, refs_id});

toolchain/check/testdata/struct/fail_duplicate_name.carbon

@@ -0,0 +1,74 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// AUTOUPDATE
+
+// CHECK:STDERR: fail_duplicate_name.carbon:[[@LINE+6]]:42: ERROR: Duplicated field name `abc` in struct type literal.
+// CHECK:STDERR: fn F() -> {.d: i32, .abc: i32, .e: i32, .abc: i32, .f: i32};
+// CHECK:STDERR:                                          ^
+// CHECK:STDERR: fail_duplicate_name.carbon:[[@LINE+3]]:22: Field with the same name here.
+// CHECK:STDERR: fn F() -> {.d: i32, .abc: i32, .e: i32, .abc: i32, .f: i32};
+// CHECK:STDERR:                      ^
+fn F() -> {.d: i32, .abc: i32, .e: i32, .abc: i32, .f: i32};
+
+// CHECK:STDERR: fail_duplicate_name.carbon:[[@LINE+6]]:19: ERROR: Duplicated field name `a` in struct type literal.
+// CHECK:STDERR: let v: {.a: i32, .a: i32} = {.a = 1};
+// CHECK:STDERR:                   ^
+// CHECK:STDERR: fail_duplicate_name.carbon:[[@LINE+3]]:10: Field with the same name here.
+// CHECK:STDERR: let v: {.a: i32, .a: i32} = {.a = 1};
+// CHECK:STDERR:          ^
+let v: {.a: i32, .a: i32} = {.a = 1};
+
+// CHECK:STDERR: fail_duplicate_name.carbon:[[@LINE+6]]:26: ERROR: Duplicated field name `def` in struct literal.
+// CHECK:STDERR: let w: i32 = {.def = 1, .def = 2}.def;
+// CHECK:STDERR:                          ^
+// CHECK:STDERR: fail_duplicate_name.carbon:[[@LINE+3]]:16: Field with the same name here.
+// CHECK:STDERR: let w: i32 = {.def = 1, .def = 2}.def;
+// CHECK:STDERR:                ^
+let w: i32 = {.def = 1, .def = 2}.def;
+
+// CHECK:STDERR: fail_duplicate_name.carbon:[[@LINE+6]]:30: ERROR: Duplicated field name `a` in struct literal.
+// CHECK:STDERR: var x: {.a: i32} = {.a = 1, .a = 2};
+// CHECK:STDERR:                              ^
+// CHECK:STDERR: fail_duplicate_name.carbon:[[@LINE+3]]:22: Field with the same name here.
+// CHECK:STDERR: var x: {.a: i32} = {.a = 1, .a = 2};
+// CHECK:STDERR:                      ^
+var x: {.a: i32} = {.a = 1, .a = 2};
+
+// CHECK:STDERR: fail_duplicate_name.carbon:[[@LINE+6]]:39: ERROR: Duplicated field name `b` in struct literal.
+// CHECK:STDERR: var y: {.b: i32, .c: i32} = {.b = 3, .b = 4};
+// CHECK:STDERR:                                       ^
+// CHECK:STDERR: fail_duplicate_name.carbon:[[@LINE+3]]:31: Field with the same name here.
+// CHECK:STDERR: var y: {.b: i32, .c: i32} = {.b = 3, .b = 4};
+// CHECK:STDERR:                               ^
+var y: {.b: i32, .c: i32} = {.b = 3, .b = 4};
+
+// CHECK:STDOUT: constants {
+// CHECK:STDOUT:   %.loc21: type = struct_type {.a: i32}
+// CHECK:STDOUT:   %.loc45: type = ptr_type {.b: i32, .c: i32}
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: file "fail_duplicate_name.carbon" {
+// CHECK:STDOUT:   %F: <function> = fn_decl @F
+// CHECK:STDOUT:   %.loc21_35: i32 = int_literal 1
+// CHECK:STDOUT:   %.loc21_36: {.a: i32} = struct_literal (%.loc21_35)
+// CHECK:STDOUT:   %v: <error> = bind_name v, <error>
+// CHECK:STDOUT:   %.loc29_22: i32 = int_literal 1
+// CHECK:STDOUT:   %.loc29_32: i32 = int_literal 2
+// CHECK:STDOUT:   %w: i32 = bind_name w, <error>
+// CHECK:STDOUT:   %.loc37_16: type = struct_type {.a: i32}
+// CHECK:STDOUT:   %x.var: ref {.a: i32} = var x
+// CHECK:STDOUT:   %x: ref {.a: i32} = bind_name x, %x.var
+// CHECK:STDOUT:   %.loc37_26: i32 = int_literal 1
+// CHECK:STDOUT:   %.loc37_34: i32 = int_literal 2
+// CHECK:STDOUT:   assign %x.var, <error>
+// CHECK:STDOUT:   %.loc45_25: type = struct_type {.b: i32, .c: i32}
+// CHECK:STDOUT:   %y.var: ref {.b: i32, .c: i32} = var y
+// CHECK:STDOUT:   %y: ref {.b: i32, .c: i32} = bind_name y, %y.var
+// CHECK:STDOUT:   %.loc45_35: i32 = int_literal 3
+// CHECK:STDOUT:   %.loc45_43: i32 = int_literal 4
+// CHECK:STDOUT:   assign %y.var, <error>
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: fn @F() -> <error>;

toolchain/diagnostics/diagnostic_kind.def

@@ -177,6 +177,8 @@ CARBON_DIAGNOSTIC_KIND(SelfOutsideImplicitParamList)
 CARBON_DIAGNOSTIC_KIND(StructInitElementCountMismatch)
 CARBON_DIAGNOSTIC_KIND(StructInitMissingFieldInLiteral)
 CARBON_DIAGNOSTIC_KIND(StructInitMissingFieldInConversion)
+CARBON_DIAGNOSTIC_KIND(StructNameDuplicate)
+CARBON_DIAGNOSTIC_KIND(StructNamePrevious)
 CARBON_DIAGNOSTIC_KIND(TupleIndexIntegerLiteral)
 CARBON_DIAGNOSTIC_KIND(TupleInitElementCountMismatch)
 CARBON_DIAGNOSTIC_KIND(ReturnedVarHere)