From 14a213d095b922390ace5187c4967810cd0d40ce Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Sat, 30 May 2026 01:26:09 -0700 Subject: [PATCH] Apply the unused-without-definition check to implicit parameters (#7270) The `unused` modifier is rejected on parameters of a function declaration, but the check only covered the explicit parameter list, so an implicit parameter (such as self or a compile-time binding) could carry unused without a definition. Check the implicit parameter list too. The code changes and the test updates are split into two commits for easier review. Assisted-by: Claude Code with Claude Opus 4.7 --------- Co-authored-by: Christopher Di Bella --- toolchain/check/handle_function.cpp | 33 +++++++++++-------- .../testdata/basics/include_in_dumps.carbon | 32 +++++++++--------- .../testdata/class/fail_modifiers.carbon | 2 +- toolchain/check/testdata/impl/basic.carbon | 2 +- .../testdata/impl/use_assoc_entity.carbon | 2 +- .../check/testdata/patterns/unused.carbon | 22 ++++++++++++- toolchain/diagnostics/kind.def | 2 +- .../lower/testdata/impl/import_facet.carbon | 2 +- 8 files changed, 61 insertions(+), 36 deletions(-) diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index 120faad239da..c50b11f099cc 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -609,8 +609,8 @@ static auto BuildFunctionDecl(Context& context, return {function_decl.function_id, decl_id}; } -// Checks that "unused" marker is only used in definitions, and emits a -// diagnostic for every binding that is marked unused. +// Checks that the `unused` modifier is only used when there is a definition, +// and emits a diagnostic for every binding that is marked `unused`. static auto CheckUnusedBindingsInPattern(Context& context, SemIR::InstId pattern_id) -> void { llvm::SmallVector work_list; @@ -626,12 +626,12 @@ static auto CheckUnusedBindingsInPattern(Context& context, case CARBON_KIND_ANY(SemIR::AnyBindingPattern, bind): { auto& entity_name = context.entity_names().Get(bind.entity_name_id); // We need special treatment for the name "_" which is implicitly - // unused but actually permitted in declarations. + // unused but actually permitted without a definition. if (entity_name.is_unused && entity_name.name_id != SemIR::NameId::Underscore) { - CARBON_DIAGNOSTIC(UnusedModifierOnDeclaration, Error, - "`unused` modifier on declaration"); - context.emitter().Emit(current_id, UnusedModifierOnDeclaration); + CARBON_DIAGNOSTIC(UnusedModifierWithoutDefinition, Error, + "`unused` modifier without a definition"); + context.emitter().Emit(current_id, UnusedModifierWithoutDefinition); } if (bind.kind == SemIR::WrapperBindingPattern::Kind) { work_list.push_back(bind.subpattern_id); @@ -655,14 +655,19 @@ static auto CheckUnusedBindingsInPattern(Context& context, } } -static auto DiagnoseUnusedMarkersInDeclaration(Context& context, - SemIR::FunctionId function_id) - -> void { +static auto DiagnoseUnusedMarkersWithoutDefinition( + Context& context, SemIR::FunctionId function_id) -> void { const auto& function = context.functions().Get(function_id); - if (function.param_patterns_id.has_value()) { - for (auto pattern_id : - context.inst_blocks().Get(function.param_patterns_id)) { - CheckUnusedBindingsInPattern(context, pattern_id); + // The `unused` modifier requires a definition, so it is not valid on any + // parameter when there is none. This applies to implicit parameters (such as + // `self`) too, so check the implicit parameter list as well as the explicit + // one. + for (auto param_patterns_id : + {function.implicit_param_patterns_id, function.param_patterns_id}) { + if (param_patterns_id.has_value()) { + for (auto pattern_id : context.inst_blocks().Get(param_patterns_id)) { + CheckUnusedBindingsInPattern(context, pattern_id); + } } } } @@ -670,7 +675,7 @@ static auto DiagnoseUnusedMarkersInDeclaration(Context& context, auto HandleParseNode(Context& context, Parse::FunctionDeclId node_id) -> bool { auto [function_id, decl_id] = BuildFunctionDecl(context, node_id, /*is_definition=*/false); - DiagnoseUnusedMarkersInDeclaration(context, function_id); + DiagnoseUnusedMarkersWithoutDefinition(context, function_id); context.decl_name_stack().PopScope(); return true; } diff --git a/toolchain/check/testdata/basics/include_in_dumps.carbon b/toolchain/check/testdata/basics/include_in_dumps.carbon index 7fe6cd64c673..633d0e80391c 100644 --- a/toolchain/check/testdata/basics/include_in_dumps.carbon +++ b/toolchain/check/testdata/basics/include_in_dumps.carbon @@ -31,7 +31,7 @@ library "[[@TEST_NAME]]"; //@dump-sem-ir-begin interface I { - fn Op[unused self: Self](); + fn Op[self: Self](); } //@dump-sem-ir-end @@ -61,7 +61,7 @@ library "[[@TEST_NAME]]"; //@dump-sem-ir-begin interface I { - fn Op[unused self: Self](); + fn Op[self: Self](); } //@dump-sem-ir-end @@ -147,13 +147,13 @@ fn F(c: C) { c.(I.Op)(); } // CHECK:STDOUT: %self.param_patt: @I.WithSelf.Op.%pattern_type (%pattern_type.d72) = value_param_pattern [concrete] // CHECK:STDOUT: %self.patt: @I.WithSelf.Op.%pattern_type (%pattern_type.d72) = at_binding_pattern self, %self.param_patt [concrete] // CHECK:STDOUT: } { -// CHECK:STDOUT: %self.param: @I.WithSelf.Op.%Self.as_type.loc8_22.1 (%Self.as_type) = value_param call_param0 -// CHECK:STDOUT: %.loc8_22.1: type = splice_block %.loc8_22.2 [symbolic = %Self.as_type.loc8_22.1 (constants.%Self.as_type)] { +// CHECK:STDOUT: %self.param: @I.WithSelf.Op.%Self.as_type.loc8_15.1 (%Self.as_type) = value_param call_param0 +// CHECK:STDOUT: %.loc8_15.1: type = splice_block %.loc8_15.2 [symbolic = %Self.as_type.loc8_15.1 (constants.%Self.as_type)] { // CHECK:STDOUT: %Self.ref: %I.type = name_ref Self, @I.%Self [symbolic = %Self (constants.%Self)] -// CHECK:STDOUT: %Self.as_type.loc8_22.2: type = facet_access_type %Self.ref [symbolic = %Self.as_type.loc8_22.1 (constants.%Self.as_type)] -// CHECK:STDOUT: %.loc8_22.2: type = converted %Self.ref, %Self.as_type.loc8_22.2 [symbolic = %Self.as_type.loc8_22.1 (constants.%Self.as_type)] +// CHECK:STDOUT: %Self.as_type.loc8_15.2: type = facet_access_type %Self.ref [symbolic = %Self.as_type.loc8_15.1 (constants.%Self.as_type)] +// CHECK:STDOUT: %.loc8_15.2: type = converted %Self.ref, %Self.as_type.loc8_15.2 [symbolic = %Self.as_type.loc8_15.1 (constants.%Self.as_type)] // CHECK:STDOUT: } -// CHECK:STDOUT: %self: @I.WithSelf.Op.%Self.as_type.loc8_22.1 (%Self.as_type) = value_binding self, %self.param +// CHECK:STDOUT: %self: @I.WithSelf.Op.%Self.as_type.loc8_15.1 (%Self.as_type) = value_binding self, %self.param // CHECK:STDOUT: } // CHECK:STDOUT: %assoc0: %I.assoc_type = assoc_entity element0, %I.WithSelf.Op.decl [concrete = constants.%assoc0] // CHECK:STDOUT: @@ -167,10 +167,10 @@ fn F(c: C) { c.(I.Op)(); } // CHECK:STDOUT: // CHECK:STDOUT: generic fn @I.WithSelf.Op(@I.%Self: %I.type) { // CHECK:STDOUT: %Self: %I.type = symbolic_binding Self, 0 [symbolic = %Self (constants.%Self)] -// CHECK:STDOUT: %Self.as_type.loc8_22.1: type = facet_access_type %Self [symbolic = %Self.as_type.loc8_22.1 (constants.%Self.as_type)] -// CHECK:STDOUT: %pattern_type: type = pattern_type %Self.as_type.loc8_22.1 [symbolic = %pattern_type (constants.%pattern_type.d72)] +// CHECK:STDOUT: %Self.as_type.loc8_15.1: type = facet_access_type %Self [symbolic = %Self.as_type.loc8_15.1 (constants.%Self.as_type)] +// CHECK:STDOUT: %pattern_type: type = pattern_type %Self.as_type.loc8_15.1 [symbolic = %pattern_type (constants.%pattern_type.d72)] // CHECK:STDOUT: -// CHECK:STDOUT: fn(%self.param: @I.WithSelf.Op.%Self.as_type.loc8_22.1 (%Self.as_type)); +// CHECK:STDOUT: fn(%self.param: @I.WithSelf.Op.%Self.as_type.loc8_15.1 (%Self.as_type)); // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: specific @I.WithSelf(constants.%Self) { @@ -182,7 +182,7 @@ fn F(c: C) { c.(I.Op)(); } // CHECK:STDOUT: // CHECK:STDOUT: specific @I.WithSelf.Op(constants.%Self) { // CHECK:STDOUT: %Self => constants.%Self -// CHECK:STDOUT: %Self.as_type.loc8_22.1 => constants.%Self.as_type +// CHECK:STDOUT: %Self.as_type.loc8_15.1 => constants.%Self.as_type // CHECK:STDOUT: %pattern_type => constants.%pattern_type.d72 // CHECK:STDOUT: } // CHECK:STDOUT: @@ -195,7 +195,7 @@ fn F(c: C) { c.(I.Op)(); } // CHECK:STDOUT: // CHECK:STDOUT: specific @I.WithSelf.Op(constants.%I.facet) { // CHECK:STDOUT: %Self => constants.%I.facet -// CHECK:STDOUT: %Self.as_type.loc8_22.1 => constants.%C +// CHECK:STDOUT: %Self.as_type.loc8_15.1 => constants.%C // CHECK:STDOUT: %pattern_type => constants.%pattern_type.da6 // CHECK:STDOUT: } // CHECK:STDOUT: @@ -235,11 +235,11 @@ fn F(c: C) { c.(I.Op)(); } // CHECK:STDOUT: } // CHECK:STDOUT: %Main.import_ref.8f2: = import_ref Main//included_with_range, loc16_1, loaded [concrete = constants.%complete_type] // CHECK:STDOUT: %Main.import_ref.cb7 = import_ref Main//included_with_range, inst{{[0-9A-F]+}} [no loc], unloaded -// CHECK:STDOUT: %Main.import_ref.e95: %I.assoc_type = import_ref Main//included_with_range, loc8_29, loaded [concrete = constants.%assoc0] +// CHECK:STDOUT: %Main.import_ref.e95: %I.assoc_type = import_ref Main//included_with_range, loc8_22, loaded [concrete = constants.%assoc0] // CHECK:STDOUT: %Main.Op = import_ref Main//included_with_range, Op, unloaded // CHECK:STDOUT: %Main.import_ref.da386d.2: %I.type = import_ref Main//included_with_range, loc7_13, loaded [symbolic = constants.%Self] // CHECK:STDOUT: %Main.import_ref.d94 = import_ref Main//included_with_range, loc7_13, unloaded -// CHECK:STDOUT: %Main.import_ref.e14: @I.WithSelf.%I.WithSelf.Op.type (%I.WithSelf.Op.type.47c) = import_ref Main//included_with_range, loc8_29, loaded [symbolic = @I.WithSelf.%I.WithSelf.Op (constants.%I.WithSelf.Op.0b4)] +// CHECK:STDOUT: %Main.import_ref.e14: @I.WithSelf.%I.WithSelf.Op.type (%I.WithSelf.Op.type.47c) = import_ref Main//included_with_range, loc8_22, loaded [symbolic = @I.WithSelf.%I.WithSelf.Op (constants.%I.WithSelf.Op.0b4)] // CHECK:STDOUT: %Main.import_ref.4b7: = import_ref Main//included_with_range, loc13_15, loaded [concrete = constants.%I.impl_witness] // CHECK:STDOUT: %Main.import_ref.b9c: type = import_ref Main//included_with_range, loc13_8, loaded [concrete = constants.%C] // CHECK:STDOUT: %Main.import_ref.4ec: type = import_ref Main//included_with_range, loc13_13, loaded [concrete = constants.%I.type] @@ -357,8 +357,8 @@ fn F(c: C) { c.(I.Op)(); } // CHECK:STDOUT: import Core//prelude // CHECK:STDOUT: import Core//prelude/... // CHECK:STDOUT: } -// CHECK:STDOUT: %Main.import_ref.bfd: %I.assoc_type = import_ref Main//excluded_with_range, loc6_29, loaded [concrete = constants.%assoc0] -// CHECK:STDOUT: %Main.import_ref.c83: @I.WithSelf.%I.WithSelf.Op.type (%I.WithSelf.Op.type.360) = import_ref Main//excluded_with_range, loc6_29, loaded [symbolic = @I.WithSelf.%I.WithSelf.Op (constants.%I.WithSelf.Op.e20)] +// CHECK:STDOUT: %Main.import_ref.bfd: %I.assoc_type = import_ref Main//excluded_with_range, loc6_22, loaded [concrete = constants.%assoc0] +// CHECK:STDOUT: %Main.import_ref.c83: @I.WithSelf.%I.WithSelf.Op.type (%I.WithSelf.Op.type.360) = import_ref Main//excluded_with_range, loc6_22, loaded [symbolic = @I.WithSelf.%I.WithSelf.Op (constants.%I.WithSelf.Op.e20)] // CHECK:STDOUT: %Main.import_ref.4cc: %C.as.I.impl.Op.type = import_ref Main//excluded_with_range, loc12_32, loaded [concrete = constants.%C.as.I.impl.Op] // CHECK:STDOUT: %I.impl_witness_table = impl_witness_table (%Main.import_ref.4cc), @C.as.I.impl [concrete] // CHECK:STDOUT: } diff --git a/toolchain/check/testdata/class/fail_modifiers.carbon b/toolchain/check/testdata/class/fail_modifiers.carbon index 84c7cd2c2004..1a743d7c7c2f 100644 --- a/toolchain/check/testdata/class/fail_modifiers.carbon +++ b/toolchain/check/testdata/class/fail_modifiers.carbon @@ -90,7 +90,7 @@ abstract class AbstractWithDefinition { // CHECK:STDERR: ^ // CHECK:STDERR: abstract fn F[unused self: Self]() {} - abstract fn G[unused self: Self](); + abstract fn G[self: Self](); } // CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:50: error: definition of `abstract` function [DefinedAbstractFunction] // CHECK:STDERR: fn AbstractWithDefinition.G[unused self: Self]() { diff --git a/toolchain/check/testdata/impl/basic.carbon b/toolchain/check/testdata/impl/basic.carbon index a1f324d557a1..ec90cc3db32a 100644 --- a/toolchain/check/testdata/impl/basic.carbon +++ b/toolchain/check/testdata/impl/basic.carbon @@ -31,7 +31,7 @@ impl C as Simple { library "[[@TEST_NAME]]"; interface I { - fn Op[unused self: Self](); + fn Op[self: Self](); } class C {} diff --git a/toolchain/check/testdata/impl/use_assoc_entity.carbon b/toolchain/check/testdata/impl/use_assoc_entity.carbon index 776d1efdbb16..02b25b893d2e 100644 --- a/toolchain/check/testdata/impl/use_assoc_entity.carbon +++ b/toolchain/check/testdata/impl/use_assoc_entity.carbon @@ -197,7 +197,7 @@ class E { library "[[@TEST_NAME]]"; interface J { - fn F[unused self: Self](); + fn F[self: Self](); } class E { diff --git a/toolchain/check/testdata/patterns/unused.carbon b/toolchain/check/testdata/patterns/unused.carbon index 23284aecbe73..da90f5ece8fb 100644 --- a/toolchain/check/testdata/patterns/unused.carbon +++ b/toolchain/check/testdata/patterns/unused.carbon @@ -178,12 +178,32 @@ fn F() -> i32 { // --- fail_unused_decl.carbon library "[[@TEST_NAME]]"; -// CHECK:STDERR: fail_unused_decl.carbon:[[@LINE+4]]:13: error: `unused` modifier on declaration [UnusedModifierOnDeclaration] +// CHECK:STDERR: fail_unused_decl.carbon:[[@LINE+4]]:13: error: `unused` modifier without a definition [UnusedModifierWithoutDefinition] // CHECK:STDERR: fn F(unused x: i32); // CHECK:STDERR: ^~~~~~ // CHECK:STDERR: fn F(unused x: i32); +// --- fail_unused_implicit_param_decl.carbon +library "[[@TEST_NAME]]"; + +// The `unused` modifier is rejected without a definition for any implicit +// parameter, both a compile-time binding and `self`. + +// CHECK:STDERR: fail_unused_implicit_param_decl.carbon:[[@LINE+4]]:13: error: `unused` modifier without a definition [UnusedModifierWithoutDefinition] +// CHECK:STDERR: fn F[unused T:! type](); +// CHECK:STDERR: ^~~~~~~~ +// CHECK:STDERR: +fn F[unused T:! type](); + +class C { + // CHECK:STDERR: fail_unused_implicit_param_decl.carbon:[[@LINE+4]]:15: error: `unused` modifier without a definition [UnusedModifierWithoutDefinition] + // CHECK:STDERR: fn G[unused self: Self](); + // CHECK:STDERR: ^~~~~~~~~~ + // CHECK:STDERR: + fn G[unused self: Self](); +} + // --- fail_multiple_uses_of_unused.carbon library "[[@TEST_NAME]]"; diff --git a/toolchain/diagnostics/kind.def b/toolchain/diagnostics/kind.def index 382d96037739..ec7f727840ca 100644 --- a/toolchain/diagnostics/kind.def +++ b/toolchain/diagnostics/kind.def @@ -570,7 +570,7 @@ CARBON_DIAGNOSTIC_KIND(TuplePatternSizeDoesntMatchLiteral) // Unused diagnostics. CARBON_DIAGNOSTIC_KIND(UnusedButUsed) CARBON_DIAGNOSTIC_KIND(UnusedButUsedHere) -CARBON_DIAGNOSTIC_KIND(UnusedModifierOnDeclaration) +CARBON_DIAGNOSTIC_KIND(UnusedModifierWithoutDefinition) CARBON_DIAGNOSTIC_KIND(UnusedPatternNoBindings) CARBON_DIAGNOSTIC_KIND(UnusedBinding) diff --git a/toolchain/lower/testdata/impl/import_facet.carbon b/toolchain/lower/testdata/impl/import_facet.carbon index ef8d58bdee8f..0445ac21f29d 100644 --- a/toolchain/lower/testdata/impl/import_facet.carbon +++ b/toolchain/lower/testdata/impl/import_facet.carbon @@ -29,7 +29,7 @@ export import library "a"; interface I { let T:! Core.Copy & Core.UnformedInit & Core.Destroy; - fn GetI[unused self: Self]() -> C(T); + fn GetI[self: Self]() -> C(T); } // --- c.carbon