Change IndexWith to use a standard binary operator setup (carbon-language#6127)

This is closer to [the
design](https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/expressions/indexing.md?plain=1#L55-L64),
just lacking `ref`, but does remove a lot of special-casing done for the
lookup.

The `ErrorInst` changes in `Build*Operator` are to align with what was
being done for `IndexWith`; don't do an interface lookup if the relevant
operand is an error. Otherwise, that becomes visible because some files
have an error operand and don't provide the interface.

Author: jonmeow

core/prelude/operators/index.carbon

@@ -4,6 +4,7 @@
 
 package Core library "prelude/operators/index";
 
-interface IndexWith(SubscriptType:! type, ElementType:! type) {
+interface IndexWith(SubscriptType:! type) {
+  let ElementType:! type;
   fn At[self: Self](subscript: SubscriptType) -> ElementType;
 }

toolchain/check/handle_index.cpp

@@ -26,105 +26,20 @@ auto HandleParseNode(Context& /*context*/, Parse::IndexExprStartId /*node_id*/)
   return true;
 }
 
-// Returns the argument values of the `IndexWith` interface. Arguments
-// correspond to the `SubscriptType` and the `ElementType`. If no arguments are
-// used to define `IndexWith`, this returns an empty array reference. If the
-// class does not implement the said interface, this returns a `std::nullopt`.
-// TODO: Switch to using an associated type instead of a parameter for the
-// `ElementType`.
-static auto GetIndexWithArgs(Context& context, Parse::NodeId node_id,
-                             SemIR::TypeId self_id)
-    -> std::optional<llvm::ArrayRef<SemIR::InstId>> {
-  auto index_with_inst_id = LookupNameInCore(context, node_id, "IndexWith");
-  // If the `IndexWith` interface doesn't have generic arguments then return an
-  // empty reference.
-  if (context.insts().Is<SemIR::FacetType>(index_with_inst_id)) {
-    return llvm::ArrayRef<SemIR::InstId>();
-  }
-
-  auto index_with_inst =
-      context.insts().TryGetAsIfValid<SemIR::StructValue>(index_with_inst_id);
-  if (!index_with_inst) {
-    return std::nullopt;
-  }
-
-  auto index_with_interface =
-      context.types().TryGetAs<SemIR::GenericInterfaceType>(
-          index_with_inst->type_id);
-  if (!index_with_interface) {
-    return std::nullopt;
-  }
-
-  for (const auto& impl : context.impls().values()) {
-    auto impl_self_type_id =
-        context.types().GetTypeIdForTypeInstId(impl.self_id);
-    auto impl_constraint_type_id =
-        context.types().GetTypeIdForTypeInstId(impl.constraint_id);
-
-    if (impl_self_type_id != self_id) {
-      continue;
-    }
-    auto facet_type =
-        context.types().TryGetAs<SemIR::FacetType>(impl_constraint_type_id);
-    if (!facet_type) {
-      continue;
-    }
-    const auto& facet_type_info =
-        context.facet_types().Get(facet_type->facet_type_id);
-    auto interface_type = facet_type_info.TryAsSingleInterface();
-    if (!interface_type) {
-      continue;
-    }
-    if (index_with_interface->interface_id != interface_type->interface_id) {
-      continue;
-    }
-
-    return context.inst_blocks().GetOrEmpty(
-        context.specifics().Get(interface_type->specific_id).args_id);
-  }
-
-  return std::nullopt;
-}
-
 // Performs an index with base expression `operand_inst_id` and
 // `operand_type_id` for types that are not an array. This checks if
 // the base expression implements the `IndexWith` interface; if so, uses the
 // `At` associative method, otherwise prints a diagnostic.
 static auto PerformIndexWith(Context& context, Parse::NodeId node_id,
                              SemIR::InstId operand_inst_id,
-                             SemIR::TypeId operand_type_id,
                              SemIR::InstId index_inst_id) -> SemIR::InstId {
-  auto args = GetIndexWithArgs(context, node_id, operand_type_id);
-
-  // If the type does not implement the `IndexWith` interface, then return
-  // an error.
-  if (!args) {
-    CARBON_DIAGNOSTIC(TypeNotIndexable, Error,
-                      "type {0} does not support indexing", SemIR::TypeId);
-    context.emitter().Emit(node_id, TypeNotIndexable, operand_type_id);
-    return SemIR::ErrorInst::InstId;
-  }
-
-  Operator op{
-      .interface_name = "IndexWith",
-      .interface_args_ref = *args,
-      .op_name = "At",
-  };
-
-  // IndexWith is defined without generic arguments.
-  if (args->empty()) {
-    return BuildBinaryOperator(context, node_id, op, operand_inst_id,
-                               index_inst_id);
-  }
-
-  // The first argument of the `IndexWith` interface corresponds to the
-  // `SubscriptType`, so first cast `index_inst_id` to that type.
-  auto subscript_type_id = context.types().GetTypeIdForTypeInstId((*args)[0]);
-  auto cast_index_id =
-      ConvertToValueOfType(context, node_id, index_inst_id, subscript_type_id);
-
+  SemIR::InstId args[] = {
+      context.types().GetInstId(context.insts().Get(index_inst_id).type_id())};
+  Operator op{.interface_name = "IndexWith",
+              .interface_args_ref = args,
+              .op_name = "At"};
   return BuildBinaryOperator(context, node_id, op, operand_inst_id,
-                             cast_index_id);
+                             index_inst_id);
 }
 
 auto HandleParseNode(Context& context, Parse::IndexExprId node_id) -> bool {
@@ -170,11 +85,8 @@ auto HandleParseNode(Context& context, Parse::IndexExprId node_id) -> bool {
     }
 
     default: {
-      auto elem_id = SemIR::ErrorInst::InstId;
-      if (operand_type_id != SemIR::ErrorInst::TypeId) {
-        elem_id = PerformIndexWith(context, node_id, operand_inst_id,
-                                   operand_type_id, index_inst_id);
-      }
+      auto elem_id =
+          PerformIndexWith(context, node_id, operand_inst_id, index_inst_id);
       context.node_stack().Push(node_id, elem_id);
       return true;
     }

toolchain/check/operator.cpp

@@ -25,6 +25,7 @@ static auto GetOperatorOpFunction(Context& context, SemIR::LocId loc_id,
   auto implicit_loc_id = context.insts().GetLocIdForDesugaring(loc_id);
 
   // Look up the interface, and pass it any generic arguments.
+  // TODO: Improve diagnostics when the found `interface_id` isn't callable.
   auto interface_id =
       LookupNameInCore(context, implicit_loc_id, op.interface_name);
   if (!op.interface_args_ref.empty()) {
@@ -58,6 +59,11 @@ auto BuildUnaryOperator(Context& context, SemIR::LocId loc_id, Operator op,
                         SemIR::InstId operand_id,
                         MakeDiagnosticBuilderFn missing_impl_diagnoser)
     -> SemIR::InstId {
+  if (operand_id == SemIR::ErrorInst::InstId) {
+    // Exit early for errors, which prevent forming an `Op` function.
+    return SemIR::ErrorInst::InstId;
+  }
+
   // For unary operators with a C++ class as the operand, try to import and call
   // the C++ operator.
   // TODO: Change impl lookup instead. See
@@ -91,6 +97,11 @@ auto BuildBinaryOperator(Context& context, SemIR::LocId loc_id, Operator op,
                          SemIR::InstId lhs_id, SemIR::InstId rhs_id,
                          MakeDiagnosticBuilderFn missing_impl_diagnoser)
     -> SemIR::InstId {
+  if (lhs_id == SemIR::ErrorInst::InstId) {
+    // Exit early for errors, which prevent forming an `Op` function.
+    return SemIR::ErrorInst::InstId;
+  }
+
   // For binary operators with a C++ class as at least one of the operands, try
   // to import and call the C++ operator.
   // TODO: Instead of hooking this here, change impl lookup, so that a generic

toolchain/check/testdata/facet/aggregate_through_access.carbon

@@ -47,7 +47,7 @@ interface Z {
   let X:! type;
 }
 
-// CHECK:STDERR: fail_todo_array_access_through_witness.carbon:[[@LINE+4]]:40: error: type `type` does not support indexing [TypeNotIndexable]
+// CHECK:STDERR: fail_todo_array_access_through_witness.carbon:[[@LINE+4]]:40: error: cannot access member of interface `Core.IndexWith(Core.IntLiteral)` in type `type` that does not implement that interface [MissingImplInMemberAccess]
 // CHECK:STDERR: fn F(T:! Z where .X = array({}, 1)) -> T.X[0] {
 // CHECK:STDERR:                                        ^~~~~~
 // CHECK:STDERR:

toolchain/check/testdata/index/fail_invalid_base.carbon

@@ -27,13 +27,13 @@ fn F();
 // CHECK:STDERR:
 var b: i32 = F[1];
 
-// CHECK:STDERR: fail_invalid_base.carbon:[[@LINE+4]]:14: error: type `{.a: Core.IntLiteral, .b: Core.IntLiteral}` does not support indexing [TypeNotIndexable]
+// CHECK:STDERR: fail_invalid_base.carbon:[[@LINE+4]]:14: error: cannot access member of interface `Core.IndexWith(Core.IntLiteral)` in type `{.a: Core.IntLiteral, .b: Core.IntLiteral}` that does not implement that interface [MissingImplInMemberAccess]
 // CHECK:STDERR: var c: i32 = {.a = 1, .b = 2}[0];
 // CHECK:STDERR:              ^~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:
 var c: i32 = {.a = 1, .b = 2}[0];
 
-// CHECK:STDERR: fail_invalid_base.carbon:[[@LINE+4]]:14: error: type `type` does not support indexing [TypeNotIndexable]
+// CHECK:STDERR: fail_invalid_base.carbon:[[@LINE+4]]:14: error: cannot access member of interface `Core.IndexWith(Core.IntLiteral)` in type `type` that does not implement that interface [MissingImplInMemberAccess]
 // CHECK:STDERR: var d: i32 = {.a: i32, .b: i32}[0];
 // CHECK:STDERR:              ^~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:

toolchain/check/testdata/index/fail_non_tuple_access.carbon

@@ -13,7 +13,7 @@
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/index/fail_non_tuple_access.carbon
 
 fn Main() {
-  // CHECK:STDERR: fail_non_tuple_access.carbon:[[@LINE+4]]:3: error: type `Core.IntLiteral` does not support indexing [TypeNotIndexable]
+  // CHECK:STDERR: fail_non_tuple_access.carbon:[[@LINE+4]]:3: error: cannot access member of interface `Core.IndexWith(Core.IntLiteral)` in type `Core.IntLiteral` that does not implement that interface [MissingImplInMemberAccess]
   // CHECK:STDERR:   0[1];
   // CHECK:STDERR:   ^~~~
   // CHECK:STDERR:

toolchain/check/testdata/operators/builtin/fail_type_mismatch_once.carbon

@@ -72,7 +72,6 @@ fn Main() -> i32 {
 // CHECK:STDOUT:   %.loc22: %empty_struct_type = struct_literal ()
 // CHECK:STDOUT:   %float: Core.FloatLiteral = float_literal_value 34e-1 [concrete = constants.%float]
 // CHECK:STDOUT:   %int_12: Core.IntLiteral = int_value 12 [concrete = constants.%int_12]
-// CHECK:STDOUT:   %impl.elem1: <error> = impl_witness_access <error>, element1 [concrete = <error>]
 // CHECK:STDOUT:   return <error> to %return
 // CHECK:STDOUT: }
 // CHECK:STDOUT: