From d21e7b4f149b9ec413efa96a73dd0e11f15a2fea Mon Sep 17 00:00:00 2001 From: josh11b Date: Tue, 14 Nov 2023 22:12:03 -0800 Subject: [PATCH] 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 --- toolchain/check/handle_struct.cpp | 44 ++++++++++- .../struct/fail_duplicate_name.carbon | 74 +++++++++++++++++++ toolchain/diagnostics/diagnostic_kind.def | 2 + 3 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 toolchain/check/testdata/struct/fail_duplicate_name.carbon diff --git a/toolchain/check/handle_struct.cpp b/toolchain/check/handle_struct.cpp index e515620e174d..665dbc9999a1 100644 --- a/toolchain/check/handle_struct.cpp +++ b/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(); + auto value_inst_id = context.node_stack().PopExpr(); + auto [name_node, name_id] = + context.node_stack().PopWithParseNode(); // 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 names; + auto& insts = sem_ir.insts(); + for (SemIR::InstId field_inst_id : fields) { + auto field_inst = insts.GetAs(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}); diff --git a/toolchain/check/testdata/struct/fail_duplicate_name.carbon b/toolchain/check/testdata/struct/fail_duplicate_name.carbon new file mode 100644 index 000000000000..47f50696c49f --- /dev/null +++ b/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: = fn_decl @F +// CHECK:STDOUT: %.loc21_35: i32 = int_literal 1 +// CHECK:STDOUT: %.loc21_36: {.a: i32} = struct_literal (%.loc21_35) +// CHECK:STDOUT: %v: = bind_name v, +// CHECK:STDOUT: %.loc29_22: i32 = int_literal 1 +// CHECK:STDOUT: %.loc29_32: i32 = int_literal 2 +// CHECK:STDOUT: %w: i32 = bind_name w, +// 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, +// 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, +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @F() -> ; diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index 8feba807e7a6..760df86293bd 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/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)