Produce a more descriptive error if an array type's bound is too large. (#3638)

toolchain/check/eval.cpp

@@ -126,12 +126,14 @@ static auto ReplaceFieldWithConstantValue(Context& context, InstT* inst,
 
 // If the specified fields of the given typed instruction have constant values,
 // replaces the fields with their constant values and builds a corresponding
-// constant value. The constant value is then checked by calling
-// `validate_fn(typed_inst)`, which should return a `bool` indicating whether
-// the new constant is valid. If validation passes, a corresponding ConstantId
-// for the new constant is returned. Otherwise returns
-// `ConstantId::NotConstant`. Returns `ConstantId::Error` if any subexpression
-// is an error.
+// constant value. Otherwise returns `ConstantId::NotConstant`. Returns
+// `ConstantId::Error` if any subexpression is an error.
+//
+// The constant value is then checked by calling `validate_fn(typed_inst)`,
+// which should return a `bool` indicating whether the new constant is valid. If
+// validation passes, a corresponding ConstantId for the new constant is
+// returned. If validation fails, it should produce a suitable error message.
+// `ConstantId::Error` is returned.
 template <typename InstT, typename ValidateFn, typename... EachFieldIdT>
 static auto RebuildAndValidateIfFieldsAreConstant(
     Context& context, SemIR::Inst inst, ValidateFn validate_fn,
@@ -142,8 +144,10 @@ static auto RebuildAndValidateIfFieldsAreConstant(
   Phase phase = Phase::Template;
   if ((ReplaceFieldWithConstantValue(context, &typed_inst, each_field_id,
                                      &phase) &&
-       ...) &&
-      validate_fn(typed_inst)) {
+       ...)) {
+    if (!validate_fn(typed_inst)) {
+      return SemIR::ConstantId::Error;
+    }
     return MakeConstantResult(context, typed_inst, phase);
   }
   return MakeNonConstantResult(phase);
@@ -277,13 +281,29 @@ auto TryEvalInst(Context& context, SemIR::InstId inst_id, SemIR::Inst inst)
       return RebuildAndValidateIfFieldsAreConstant(
           context, inst,
           [&](SemIR::ArrayType result) {
-            // TODO: If the bound doesn't fit in 64 bits or is negative,
-            // produce an error rather than a non-constant type.
-            // TODO: Support a symbolic bound here. This will require fixing
-            // callers of `GetArrayBoundValue`.
-            auto int_bound =
-                context.insts().TryGetAs<SemIR::IntLiteral>(result.bound_id);
-            return context.ints().Get(int_bound->int_id).getActiveBits() <= 64;
+            auto bound_id = inst.As<SemIR::ArrayType>().bound_id;
+            if (auto int_bound = context.insts().TryGetAs<SemIR::IntLiteral>(
+                    result.bound_id)) {
+              // TODO: We should check that the size of the resulting array type
+              // fits in 64 bits, not just that the bound does. Should we use a
+              // 32-bit limit for 32-bit targets?
+              // TODO: Also check for a negative bound, once that's something we
+              // can represent.
+              const auto& bound_val = context.ints().Get(int_bound->int_id);
+              if (bound_val.getActiveBits() > 64) {
+                CARBON_DIAGNOSTIC(ArrayBoundTooLarge, Error,
+                                  "Array bound of {0} is too large.",
+                                  llvm::APInt);
+                context.emitter().Emit(bound_id, ArrayBoundTooLarge, bound_val);
+                return false;
+              }
+            } else {
+              // TODO: Permit symbolic array bounds. This will require fixing
+              // callers of `GetArrayBoundValue`.
+              context.TODO(context.insts().GetParseNode(bound_id),
+                           "symbolic array bound");
+            }
+            return true;
           },
           &SemIR::ArrayType::bound_id);
     case SemIR::BoundMethod::Kind:

toolchain/check/handle_array.cpp

@@ -41,8 +41,7 @@ auto HandleArrayExpr(Context& context, Parse::ArrayExprId parse_node) -> bool {
   if (!bound_inst.is_constant()) {
     CARBON_DIAGNOSTIC(InvalidArrayExpr, Error,
                       "Array bound is not a constant.");
-    context.emitter().Emit(context.insts().GetParseNode(bound_inst_id),
-                           InvalidArrayExpr);
+    context.emitter().Emit(bound_inst_id, InvalidArrayExpr);
     context.node_stack().Push(parse_node, SemIR::InstId::BuiltinError);
     return true;
   }

toolchain/check/testdata/array/fail_bound_overflow.carbon

@@ -4,17 +4,17 @@
 //
 // AUTOUPDATE
 
-// CHECK:STDERR: fail_bound_overflow.carbon:[[@LINE+3]]:8: ERROR: Cannot evaluate type expression.
+// CHECK:STDERR: fail_bound_overflow.carbon:[[@LINE+3]]:14: ERROR: Array bound of 39999999999999999993 is too large.
 // CHECK:STDERR: var a: [i32; 39999999999999999993];
-// CHECK:STDERR:        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:              ^~~~~~~~~~~~~~~~~~~~
 var a: [i32; 39999999999999999993];
 
 // CHECK:STDERR: fail_bound_overflow.carbon:[[@LINE+6]]:8: ERROR: Cannot implicitly convert from `i32` to `type`.
 // CHECK:STDERR: var b: [1; 39999999999999999993];
 // CHECK:STDERR:        ^~~~~~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR: fail_bound_overflow.carbon:[[@LINE+3]]:8: ERROR: Cannot evaluate type expression.
+// CHECK:STDERR: fail_bound_overflow.carbon:[[@LINE+3]]:12: ERROR: Array bound of 39999999999999999993 is too large.
 // CHECK:STDERR: var b: [1; 39999999999999999993];
-// CHECK:STDERR:        ^~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:            ^~~~~~~~~~~~~~~~~~~~
 var b: [1; 39999999999999999993];
 
 // CHECK:STDOUT: --- fail_bound_overflow.carbon
@@ -27,12 +27,12 @@ var b: [1; 39999999999999999993];
 // CHECK:STDOUT: file {
 // CHECK:STDOUT:   package: <namespace> = namespace package, {.a = %a, .b = %b} [template]
 // CHECK:STDOUT:   %.loc10_14: i32 = int_literal 39999999999999999993 [template = constants.%.1]
-// CHECK:STDOUT:   %.loc10_34: type = array_type %.loc10_14, i32
+// CHECK:STDOUT:   %.loc10_34: type = array_type %.loc10_14, i32 [template = <error>]
 // CHECK:STDOUT:   %a.var: ref <error> = var a
 // CHECK:STDOUT:   %a: ref <error> = bind_name a, %a.var
 // CHECK:STDOUT:   %.loc18_9: i32 = int_literal 1 [template = constants.%.2]
 // CHECK:STDOUT:   %.loc18_12: i32 = int_literal 39999999999999999993 [template = constants.%.1]
-// CHECK:STDOUT:   %.loc18_32: type = array_type %.loc18_12, <error>
+// CHECK:STDOUT:   %.loc18_32: type = array_type %.loc18_12, <error> [template = <error>]
 // CHECK:STDOUT:   %b.var: ref <error> = var b
 // CHECK:STDOUT:   %b: ref <error> = bind_name b, %b.var
 // CHECK:STDOUT: }

toolchain/diagnostics/diagnostic_kind.def

@@ -166,6 +166,7 @@ CARBON_DIAGNOSTIC_KIND(InterfaceRedefinition)
 CARBON_DIAGNOSTIC_KIND(AddrOfEphemeralRef)
 CARBON_DIAGNOSTIC_KIND(AddrOfNonRef)
 CARBON_DIAGNOSTIC_KIND(AddrOnNonSelfParam)
+CARBON_DIAGNOSTIC_KIND(ArrayBoundTooLarge)
 CARBON_DIAGNOSTIC_KIND(ArrayIndexOutOfBounds)
 CARBON_DIAGNOSTIC_KIND(ArrayInitFromLiteralArgCountMismatch)
 CARBON_DIAGNOSTIC_KIND(ArrayInitFromExprArgCountMismatch)