Diagnose the unused generic params on an impl decl (#5189)

These parameters are never deduced, so they prevent the impl decl from
ever being used. But we also don't emit diagnostics inside impl lookup,
so there's nothing provided to the user explaining that they made an
impl that is useless.

toolchain/check/handle_impl.cpp

@@ -5,6 +5,7 @@
 #include "toolchain/check/context.h"
 #include "toolchain/check/convert.h"
 #include "toolchain/check/decl_name_stack.h"
+#include "toolchain/check/deduce.h"
 #include "toolchain/check/generic.h"
 #include "toolchain/check/handle.h"
 #include "toolchain/check/impl.h"
@@ -428,6 +429,53 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
     FinishGenericDecl(context, impl_decl_id, impl_info.generic_id);
     impl_decl.impl_id = context.impls().Add(impl_info);
     lookup_bucket_ref.push_back(impl_decl.impl_id);
+
+    // Looking to see if there are any generic bindings on the `impl`
+    // declaration that are not deducible. If so, and the `impl` does not
+    // actually use all its generic bindings, and will never be matched. This
+    // should be diagnossed to the user.
+    bool has_error_in_implicit_pattern = false;
+    if (name.implicit_param_patterns_id.has_value()) {
+      for (auto inst_id :
+           context.inst_blocks().Get(name.implicit_param_patterns_id)) {
+        if (inst_id == SemIR::ErrorInst::SingletonInstId) {
+          has_error_in_implicit_pattern = true;
+          break;
+        }
+      }
+    }
+    if (impl_info.generic_id.has_value() && !has_error_in_implicit_pattern &&
+        impl_info.witness_id != SemIR::ErrorInst::SingletonInstId) {
+      context.inst_block_stack().Push();
+      auto deduced_specific_id = DeduceImplArguments(
+          context, node_id,
+          DeduceImpl{.self_id = impl_info.self_id,
+                     .generic_id = impl_info.generic_id,
+                     .specific_id = impl_info.interface.specific_id},
+          context.constant_values().Get(impl_info.self_id),
+          impl_info.interface.specific_id);
+      // TODO: Deduce has side effects in the semir by generating `Converted`
+      // instructions which we will not use here. We should stop generating
+      // those when deducing for impl lookup, but for now we discard them by
+      // pushing an InstBlock on the stack and dropping it here.
+      context.inst_block_stack().PopAndDiscard();
+      if (!deduced_specific_id.has_value()) {
+        CARBON_DIAGNOSTIC(ImplUnusedBinding, Error,
+                          "`impl` with unused generic binding");
+        // TODO: This location may be incorrect, the binding may be inherited
+        // from an outer declaration. It would be nice to get the particular
+        // binding that was undeducible back from DeduceImplArguments here and
+        // use that.
+        auto loc = name.implicit_params_loc_id.has_value()
+                       ? name.implicit_params_loc_id
+                       : node_id;
+        context.emitter().Emit(loc, ImplUnusedBinding);
+        // Don't try to match the impl at all, save us work and possible future
+        // diagnostics.
+        context.impls().Get(impl_decl.impl_id).witness_id =
+            SemIR::ErrorInst::SingletonInstId;
+      }
+    }
   } else {
     auto prev_decl_generic_id =
         context.impls().Get(impl_decl.impl_id).generic_id;

toolchain/check/testdata/impl/lookup/generic.carbon

@@ -82,14 +82,15 @@ interface HasF {
   fn F[self: Self]();
 }
 
-// TODO: Reject this declaration because U cannot be deduced.
+// CHECK:STDERR: fail_incomplete_deduction.carbon:[[@LINE+4]]:13: error: `impl` with unused generic binding [ImplUnusedBinding]
+// CHECK:STDERR: impl forall [T:! type, U:! type] T as HasF {
+// CHECK:STDERR:             ^~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
 impl forall [T:! type, U:! type] T as HasF {
   fn F[self: Self]() {}
 }
 
 fn G(x: {}) {
-  // TODO: It'd be nice to include a note here saying that deduction failed because
-  // we couldn't deduce a value for 'U'.
   // CHECK:STDERR: fail_incomplete_deduction.carbon:[[@LINE+4]]:3: error: cannot access member of interface `HasF` in type `{}` that does not implement that interface [MissingImplInMemberAccess]
   // CHECK:STDERR:   x.(HasF.F)();
   // CHECK:STDERR:   ^~~~~~~~~~
@@ -1034,10 +1035,9 @@ fn G(x: A) {
 // CHECK:STDOUT:   %T.patt: type = symbolic_binding_pattern T, 0 [symbolic]
 // CHECK:STDOUT:   %U: type = bind_symbolic_name U, 1 [symbolic]
 // CHECK:STDOUT:   %U.patt: type = symbolic_binding_pattern U, 1 [symbolic]
-// CHECK:STDOUT:   %impl_witness: <witness> = impl_witness (@impl.%F.decl), @impl(%T, %U) [symbolic]
+// CHECK:STDOUT:   %impl_witness: <witness> = impl_witness (invalid), @impl(%T, %U) [symbolic]
 // CHECK:STDOUT:   %F.type.56e: type = fn_type @F.2, @impl(%T, %U) [symbolic]
 // CHECK:STDOUT:   %F.9b8: %F.type.56e = struct_value () [symbolic]
-// CHECK:STDOUT:   %HasF.facet: %HasF.type = facet_value %T, (%impl_witness) [symbolic]
 // CHECK:STDOUT:   %require_complete: <witness> = require_complete_type %T [symbolic]
 // CHECK:STDOUT:   %empty_struct_type: type = struct_type {} [concrete]
 // CHECK:STDOUT:   %G.type: type = fn_type @G [concrete]
@@ -1060,23 +1060,23 @@ fn G(x: A) {
 // CHECK:STDOUT:   %Core.import = import Core
 // CHECK:STDOUT:   %HasF.decl: type = interface_decl @HasF [concrete = constants.%HasF.type] {} {}
 // CHECK:STDOUT:   impl_decl @impl [concrete] {
-// CHECK:STDOUT:     %T.patt.loc9_14.1: type = symbolic_binding_pattern T, 0 [symbolic = %T.patt.loc9_14.2 (constants.%T.patt)]
-// CHECK:STDOUT:     %U.patt.loc9_24.1: type = symbolic_binding_pattern U, 1 [symbolic = %U.patt.loc9_24.2 (constants.%U.patt)]
+// CHECK:STDOUT:     %T.patt.loc12_14.1: type = symbolic_binding_pattern T, 0 [symbolic = %T.patt.loc12_14.2 (constants.%T.patt)]
+// CHECK:STDOUT:     %U.patt.loc12_24.1: type = symbolic_binding_pattern U, 1 [symbolic = %U.patt.loc12_24.2 (constants.%U.patt)]
 // CHECK:STDOUT:   } {
-// CHECK:STDOUT:     %T.ref: type = name_ref T, %T.loc9_14.1 [symbolic = %T.loc9_14.2 (constants.%T)]
+// CHECK:STDOUT:     %T.ref: type = name_ref T, %T.loc12_14.1 [symbolic = %T.loc12_14.2 (constants.%T)]
 // CHECK:STDOUT:     %HasF.ref: type = name_ref HasF, file.%HasF.decl [concrete = constants.%HasF.type]
-// CHECK:STDOUT:     %T.loc9_14.1: type = bind_symbolic_name T, 0 [symbolic = %T.loc9_14.2 (constants.%T)]
-// CHECK:STDOUT:     %U.loc9_24.1: type = bind_symbolic_name U, 1 [symbolic = %U.loc9_24.2 (constants.%U)]
+// CHECK:STDOUT:     %T.loc12_14.1: type = bind_symbolic_name T, 0 [symbolic = %T.loc12_14.2 (constants.%T)]
+// CHECK:STDOUT:     %U.loc12_24.1: type = bind_symbolic_name U, 1 [symbolic = %U.loc12_24.2 (constants.%U)]
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %impl_witness: <witness> = impl_witness (@impl.%F.decl), @impl(constants.%T, constants.%U) [symbolic = @impl.%impl_witness (constants.%impl_witness)]
+// CHECK:STDOUT:   %impl_witness: <witness> = impl_witness (invalid), @impl(constants.%T, constants.%U) [symbolic = @impl.%impl_witness (constants.%impl_witness)]
 // CHECK:STDOUT:   %G.decl: %G.type = fn_decl @G [concrete = constants.%G] {
 // CHECK:STDOUT:     %x.patt: %empty_struct_type = binding_pattern x
 // CHECK:STDOUT:     %x.param_patt: %empty_struct_type = value_param_pattern %x.patt, call_param0
 // CHECK:STDOUT:   } {
 // CHECK:STDOUT:     %x.param: %empty_struct_type = value_param call_param0
-// CHECK:STDOUT:     %.loc13_10.1: type = splice_block %.loc13_10.3 [concrete = constants.%empty_struct_type] {
-// CHECK:STDOUT:       %.loc13_10.2: %empty_struct_type = struct_literal ()
-// CHECK:STDOUT:       %.loc13_10.3: type = converted %.loc13_10.2, constants.%empty_struct_type [concrete = constants.%empty_struct_type]
+// CHECK:STDOUT:     %.loc16_10.1: type = splice_block %.loc16_10.3 [concrete = constants.%empty_struct_type] {
+// CHECK:STDOUT:       %.loc16_10.2: %empty_struct_type = struct_literal ()
+// CHECK:STDOUT:       %.loc16_10.3: type = converted %.loc16_10.2, constants.%empty_struct_type [concrete = constants.%empty_struct_type]
 // CHECK:STDOUT:     }
 // CHECK:STDOUT:     %x: %empty_struct_type = bind_name x, %x.param
 // CHECK:STDOUT:   }
@@ -1104,15 +1104,15 @@ fn G(x: A) {
 // CHECK:STDOUT:   witness = (%F.decl)
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: generic impl @impl(%T.loc9_14.1: type, %U.loc9_24.1: type) {
-// CHECK:STDOUT:   %T.loc9_14.2: type = bind_symbolic_name T, 0 [symbolic = %T.loc9_14.2 (constants.%T)]
-// CHECK:STDOUT:   %T.patt.loc9_14.2: type = symbolic_binding_pattern T, 0 [symbolic = %T.patt.loc9_14.2 (constants.%T.patt)]
-// CHECK:STDOUT:   %U.loc9_24.2: type = bind_symbolic_name U, 1 [symbolic = %U.loc9_24.2 (constants.%U)]
-// CHECK:STDOUT:   %U.patt.loc9_24.2: type = symbolic_binding_pattern U, 1 [symbolic = %U.patt.loc9_24.2 (constants.%U.patt)]
-// CHECK:STDOUT:   %impl_witness: <witness> = impl_witness (%F.decl), @impl(%T.loc9_14.2, %U.loc9_24.2) [symbolic = %impl_witness (constants.%impl_witness)]
+// CHECK:STDOUT: generic impl @impl(%T.loc12_14.1: type, %U.loc12_24.1: type) {
+// CHECK:STDOUT:   %T.loc12_14.2: type = bind_symbolic_name T, 0 [symbolic = %T.loc12_14.2 (constants.%T)]
+// CHECK:STDOUT:   %T.patt.loc12_14.2: type = symbolic_binding_pattern T, 0 [symbolic = %T.patt.loc12_14.2 (constants.%T.patt)]
+// CHECK:STDOUT:   %U.loc12_24.2: type = bind_symbolic_name U, 1 [symbolic = %U.loc12_24.2 (constants.%U)]
+// CHECK:STDOUT:   %U.patt.loc12_24.2: type = symbolic_binding_pattern U, 1 [symbolic = %U.patt.loc12_24.2 (constants.%U.patt)]
+// CHECK:STDOUT:   %impl_witness: <witness> = impl_witness (invalid), @impl(%T.loc12_14.2, %U.loc12_24.2) [symbolic = %impl_witness (constants.%impl_witness)]
 // CHECK:STDOUT:
 // CHECK:STDOUT: !definition:
-// CHECK:STDOUT:   %F.type: type = fn_type @F.2, @impl(%T.loc9_14.2, %U.loc9_24.2) [symbolic = %F.type (constants.%F.type.56e)]
+// CHECK:STDOUT:   %F.type: type = fn_type @F.2, @impl(%T.loc12_14.2, %U.loc12_24.2) [symbolic = %F.type (constants.%F.type.56e)]
 // CHECK:STDOUT:   %F: @impl.%F.type (%F.type.56e) = struct_value () [symbolic = %F (constants.%F.9b8)]
 // CHECK:STDOUT:
 // CHECK:STDOUT:   impl: %T.ref as %HasF.ref {
@@ -1127,7 +1127,7 @@ fn G(x: A) {
 // CHECK:STDOUT:
 // CHECK:STDOUT:   !members:
 // CHECK:STDOUT:     .F = %F.decl
-// CHECK:STDOUT:     witness = file.%impl_witness
+// CHECK:STDOUT:     witness = <error>
 // CHECK:STDOUT:   }
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
@@ -1138,7 +1138,7 @@ fn G(x: A) {
 // CHECK:STDOUT:   fn[%self.param_patt: @F.1.%Self.as_type.loc5_14.1 (%Self.as_type)]();
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: generic fn @F.2(@impl.%T.loc9_14.1: type, @impl.%U.loc9_24.1: type) {
+// CHECK:STDOUT: generic fn @F.2(@impl.%T.loc12_14.1: type, @impl.%U.loc12_24.1: type) {
 // CHECK:STDOUT:   %T: type = bind_symbolic_name T, 0 [symbolic = %T (constants.%T)]
 // CHECK:STDOUT:
 // CHECK:STDOUT: !definition:
@@ -1164,10 +1164,10 @@ fn G(x: A) {
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: specific @impl(constants.%T, constants.%U) {
-// CHECK:STDOUT:   %T.loc9_14.2 => constants.%T
-// CHECK:STDOUT:   %T.patt.loc9_14.2 => constants.%T
-// CHECK:STDOUT:   %U.loc9_24.2 => constants.%U
-// CHECK:STDOUT:   %U.patt.loc9_24.2 => constants.%U
+// CHECK:STDOUT:   %T.loc12_14.2 => constants.%T
+// CHECK:STDOUT:   %T.patt.loc12_14.2 => constants.%T
+// CHECK:STDOUT:   %U.loc12_24.2 => constants.%U
+// CHECK:STDOUT:   %U.patt.loc12_24.2 => constants.%U
 // CHECK:STDOUT:   %impl_witness => constants.%impl_witness
 // CHECK:STDOUT:
 // CHECK:STDOUT: !definition:
@@ -1175,17 +1175,12 @@ fn G(x: A) {
 // CHECK:STDOUT:   %F => constants.%F.9b8
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: specific @impl(%T.loc9_14.2, %U.loc9_24.2) {}
+// CHECK:STDOUT: specific @impl(%T.loc12_14.2, %U.loc12_24.2) {}
 // CHECK:STDOUT:
 // CHECK:STDOUT: specific @F.2(constants.%T, constants.%U) {
 // CHECK:STDOUT:   %T => constants.%T
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: specific @F.1(constants.%HasF.facet) {
-// CHECK:STDOUT:   %Self => constants.%HasF.facet
-// CHECK:STDOUT:   %Self.as_type.loc5_14.1 => constants.%T
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
 // CHECK:STDOUT: --- fail_inconsistent_deduction.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {

toolchain/check/testdata/impl/lookup/min_prelude/unused_generic_binding.carbon

@@ -0,0 +1,158 @@
+// 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
+//
+// INCLUDE-FILE: toolchain/testing/min_prelude/facet_types.carbon
+// EXTRA-ARGS: --no-dump-sem-ir --custom-core
+//
+// AUTOUPDATE
+// TIP: To test this file alone, run:
+// TIP:   bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/impl/lookup/min_prelude/unused_generic_binding.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/lookup/min_prelude/unused_generic_binding.carbon
+
+// --- fail_no_binding_used.carbon
+library "[[@TEST_NAME]]";
+
+interface I {
+  let T:! type;
+}
+
+impl forall [U:! type] U as I where .T = () {}
+
+class C {}
+
+// This impl can never deduce its implicit generic parameter `V`, which
+// should be diagnosed.
+//
+// CHECK:STDERR: fail_no_binding_used.carbon:[[@LINE+4]]:13: error: `impl` with unused generic binding [ImplUnusedBinding]
+// CHECK:STDERR: impl forall [V:! type] C as I where .T = {.unmatched: ()} {}
+// CHECK:STDERR:             ^~~~~~~~~~
+// CHECK:STDERR:
+impl forall [V:! type] C as I where .T = {.unmatched: ()} {}
+
+fn F() {
+  // This shows that the first impl declaration was matched even though the
+  // second is more specific. But since it has an unused generic parameter, it
+  // will never match. If the second was matched, this line would not type
+  // check.
+  let x: C.(I.T) = ();
+}
+
+// --- fail_one_binding_unused.carbon
+library "[[@TEST_NAME]]";
+
+interface I {
+  let T:! type;
+}
+
+impl forall [U:! type] U as I where .T = () {}
+
+class C {}
+
+// This impl can never deduce its implicit generic parameter `W`, which
+// should be diagnosed.
+// https://discord.com/channels/655572317891461132/941071822756143115/1354563497731686550
+//
+// CHECK:STDERR: fail_one_binding_unused.carbon:[[@LINE+4]]:13: error: `impl` with unused generic binding [ImplUnusedBinding]
+// CHECK:STDERR: impl forall [V:! type, W:! type] V as I where .T = {.unmatched: ()} {}
+// CHECK:STDERR:             ^~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+impl forall [V:! type, W:! type] V as I where .T = {.unmatched: ()} {}
+
+fn F() {
+  // This shows that the first impl declaration was matched even though the
+  // second is more specific. If the second was matched, it would not type
+  // check.
+  let x: C.(I.T) = ();
+}
+
+// --- fail_inherited_binding_unused.carbon
+library "[[@TEST_NAME]]";
+
+interface I {
+  let T:! type;
+}
+
+impl forall [U:! type] U as I where .T = () {}
+
+class C(U:! type) {
+  // This impl can never deduce its inherited generic binding `U`, which
+  // should be diagnosed.
+  //
+  // TODO: We should point the diagnostic at the binding `U` in C.
+  //
+  // CHECK:STDERR: fail_inherited_binding_unused.carbon:[[@LINE+4]]:3: error: `impl` with unused generic binding [ImplUnusedBinding]
+  // CHECK:STDERR:   impl C(()) as I where .T = {.unmatched: ()} {}
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl C(()) as I where .T = {.unmatched: ()} {}
+}
+
+fn F() {
+  let x: C(()).(I.T) = ();
+}
+
+// --- fail_inherited_binding_in_forall_unused.carbon
+library "[[@TEST_NAME]]";
+
+interface I {
+  let T:! type;
+}
+
+impl forall [U:! type] U as I where .T = () {}
+
+class C(U:! type) {
+  // This impl can never deduce its inherited generic binding `U`, which
+  // should be diagnosed.
+  //
+  // TODO: We should point the diagnostic at the binding `U` in C.
+  //
+  // CHECK:STDERR: fail_inherited_binding_in_forall_unused.carbon:[[@LINE+4]]:15: error: `impl` with unused generic binding [ImplUnusedBinding]
+  // CHECK:STDERR:   impl forall [V:! type] C(V) as I where .T = {.unmatched: ()} {}
+  // CHECK:STDERR:               ^~~~~~~~~~
+  // CHECK:STDERR:
+  impl forall [V:! type] C(V) as I where .T = {.unmatched: ()} {}
+}
+
+fn F() {
+  let x: C(()).(I.T) = ();
+}
+
+// --- inherited_binding_used.carbon
+library "[[@TEST_NAME]]";
+
+interface I {
+  let T:! type;
+}
+
+impl forall [U:! type] U as I where .T = {.unmatched: ()} {}
+
+class C(U:! type) {
+  // `U` can be deduced here, so no diagnostic issued.
+  impl C(U) as I where .T = () {}
+}
+
+fn F() {
+  // The `impl` inside C is matched here.
+  let x: C(()).(I.T) = ();
+}
+
+// --- inherited_binding_in_forall_used.carbon
+library "[[@TEST_NAME]]";
+
+interface I(W:! type) {
+  let T:! type;
+}
+
+impl forall [U:! type] U as I(U) where .T = {.unmatched: ()} {}
+
+class C(U:! type) {
+  // `U` can be deduced here, so no diagnostic issued.
+  impl forall [V:! type] C(U) as I(V) where .T = () {}
+}
+
+fn F() {
+  // The `impl` inside C is matched here.
+  let x: C(()).(I({}).T) = ();
+}

toolchain/diagnostics/diagnostic_kind.def

@@ -303,6 +303,7 @@ CARBON_DIAGNOSTIC_KIND(ImplMissingFunction)
 CARBON_DIAGNOSTIC_KIND(ImplPreviousDefinition)
 CARBON_DIAGNOSTIC_KIND(ImplRedefinition)
 CARBON_DIAGNOSTIC_KIND(ImplOfNotOneInterface)
+CARBON_DIAGNOSTIC_KIND(ImplUnusedBinding)
 
 // Impl lookup.
 CARBON_DIAGNOSTIC_KIND(MissingImplInMemberAccess)