Add a compile-time check that the condition of a CHECK is not constant. (#4628)

Inspired by #4624.

common/check.h

@@ -20,9 +20,9 @@ namespace Carbon {
 // example in a template argument list:
 //   CARBON_CHECK((inst.IsOneOf<Call, TupleLiteral>()),
 //                "Unexpected inst {0}", inst);
-#define CARBON_CHECK(condition, ...) \
-  (condition) ? (void)0              \
-              : CARBON_INTERNAL_CHECK(condition __VA_OPT__(, ) __VA_ARGS__)
+#define CARBON_CHECK(condition, ...)         \
+  CARBON_INTERNAL_CHECK_CONDITION(condition) \
+  ? (void)0 : CARBON_INTERNAL_CHECK(condition __VA_OPT__(, ) __VA_ARGS__)
 
 // DCHECK calls CHECK in debug mode, and does nothing otherwise.
 #ifndef NDEBUG
@@ -35,9 +35,9 @@ namespace Carbon {
 // prefix it with a short-circuit operator, and we still emit the (dead) call to
 // the check implementation. But we use a special implementation that reduces
 // the compile time cost.
-#define CARBON_DCHECK(condition, ...) \
-  (true || (condition))               \
-      ? (void)0                       \
+#define CARBON_DCHECK(condition, ...)                  \
+  (true || CARBON_INTERNAL_CHECK_CONDITION(condition)) \
+      ? (void)0                                        \
       : CARBON_INTERNAL_DEAD_DCHECK(condition __VA_OPT__(, ) __VA_ARGS__)
 #endif
 

common/check_internal.h

@@ -10,6 +10,24 @@
 
 namespace Carbon::Internal {
 
+// Evaluates a condition in a CHECK. This diagnoses if the condition evaluates
+// to the constant `true` or `false`.
+[[clang::always_inline]] constexpr bool
+// Trailing GNU function attributes are incompatible with trailing return types.
+// Filed as https://github.com/llvm/llvm-project/issues/118697
+// NOLINTNEXTLINE(modernize-use-trailing-return-type)
+CheckCondition(bool condition)
+    __attribute__((diagnose_if(condition,
+                               "CHECK condition is always true; replace with "
+                               "static_assert if this is intended",
+                               "error")))
+    __attribute__((diagnose_if(!condition,
+                               "CHECK condition is always false; replace with "
+                               "CARBON_FATAL if this is intended",
+                               "error"))) {
+  return condition;
+}
+
 // Implements the check failure message printing.
 //
 // This is out-of-line and will arrange to stop the program, print any debugging
@@ -54,6 +72,13 @@ template <TemplateString Kind, TemplateString File, int Line,
 
 }  // namespace Carbon::Internal
 
+// Evaluates the condition of a CHECK as a boolean value.
+//
+// This performs a contextual conversion to bool, diagnoses if the condition is
+// always true or always false, and returns its value.
+#define CARBON_INTERNAL_CHECK_CONDITION(cond) \
+  (Carbon::Internal::CheckCondition(true && (cond)))
+
 // Implements check messages without any formatted values.
 //
 // Passes each of the provided components of the message to the template

common/check_test.cpp

@@ -9,15 +9,22 @@
 namespace Carbon {
 namespace {
 
-TEST(CheckTest, CheckTrue) { CARBON_CHECK(true); }
+// Non-constexpr functions that always return true and false, to bypass constant
+// condition checking.
+auto AlwaysTrue() -> bool { return true; }
+auto AlwaysFalse() -> bool { return false; }
+
+TEST(CheckTest, CheckTrue) { CARBON_CHECK(AlwaysTrue()); }
 
 TEST(CheckTest, CheckFalse) {
-  ASSERT_DEATH({ CARBON_CHECK(false); },
-               "\nCHECK failure at common/check_test.cpp:\\d+: false\n");
+  ASSERT_DEATH({ CARBON_CHECK(AlwaysFalse()); },
+               R"(
+CHECK failure at common/check_test.cpp:\d+: AlwaysFalse\(\)
+)");
 }
 
 TEST(CheckTest, CheckFalseHasStackDump) {
-  ASSERT_DEATH({ CARBON_CHECK(false); }, "\nStack dump:\n");
+  ASSERT_DEATH({ CARBON_CHECK(AlwaysFalse()); }, "\nStack dump:\n");
 }
 
 TEST(CheckTest, CheckTrueCallbackNotUsed) {
@@ -26,13 +33,15 @@ TEST(CheckTest, CheckTrueCallbackNotUsed) {
     called = true;
     return "called";
   };
-  CARBON_CHECK(true, "{0}", callback());
+  CARBON_CHECK(AlwaysTrue(), "{0}", callback());
   EXPECT_FALSE(called);
 }
 
 TEST(CheckTest, CheckFalseMessage) {
-  ASSERT_DEATH({ CARBON_CHECK(false, "msg"); },
-               "\nCHECK failure at common/check_test.cpp:.+: false: msg\n");
+  ASSERT_DEATH({ CARBON_CHECK(AlwaysFalse(), "msg"); },
+               R"(
+CHECK failure at common/check_test.cpp:.+: AlwaysFalse\(\): msg
+)");
 }
 
 TEST(CheckTest, CheckFalseFormattedMessage) {
@@ -40,15 +49,17 @@ TEST(CheckTest, CheckFalseFormattedMessage) {
   std::string str = "str";
   int i = 1;
   ASSERT_DEATH(
-      { CARBON_CHECK(false, "{0} {1} {2} {3}", msg, str, i, 0); },
-      "\nCHECK failure at common/check_test.cpp:.+: false: msg str 1 0\n");
+      { CARBON_CHECK(AlwaysFalse(), "{0} {1} {2} {3}", msg, str, i, 0); },
+      R"(
+CHECK failure at common/check_test.cpp:.+: AlwaysFalse\(\): msg str 1 0
+)");
 }
 
 TEST(CheckTest, CheckOutputForms) {
   const char msg[] = "msg";
   std::string str = "str";
   int i = 1;
-  CARBON_CHECK(true, "{0} {1} {2} {3}", msg, str, i, 0);
+  CARBON_CHECK(AlwaysTrue(), "{0} {1} {2} {3}", msg, str, i, 0);
 }
 
 TEST(CheckTest, Fatal) {
@@ -67,5 +78,16 @@ TEST(ErrorTest, FatalNoReturnRequired) {
                "\nFATAL failure at common/check_test.cpp:.+: msg\n");
 }
 
+// Detects whether `CARBON_CHECK(F())` compiles.
+template <auto F>
+concept CheckCompilesWithCondition = requires { CARBON_CHECK(F()); };
+
+TEST(CheckTest, CheckConstantCondition) {
+  EXPECT_TRUE(CheckCompilesWithCondition<[] { return AlwaysTrue(); }>);
+  EXPECT_TRUE(CheckCompilesWithCondition<[] { return AlwaysFalse(); }>);
+  EXPECT_FALSE(CheckCompilesWithCondition<[] { return true; }>);
+  EXPECT_FALSE(CheckCompilesWithCondition<[] { return false; }>);
+}
+
 }  // namespace
 }  // namespace Carbon

common/raw_hashtable_metadata_group.h

@@ -366,9 +366,14 @@ class MetadataGroup : public Printable<MetadataGroup> {
   auto Store(uint8_t* metadata, ssize_t index) const -> void;
 
   // Clear a byte of this group's metadata at the provided `byte_index` to the
-  // empty value. Note that this must only be called when `FastByteClear` is
-  // true -- in all other cases users of this class should arrange to clear
-  // individual bytes in the underlying array rather than using the group API.
+  // empty value.
+  //
+  // Note that this must only be called when `FastByteClear` is true -- in all
+  // other cases users of this class should arrange to clear individual bytes in
+  // the underlying array rather than using the group API. This is checked by a
+  // static_assert, and the function is templated so that it is not instantiated
+  // in the cases where it would not be valid.
+  template <bool IsCalled = true>
   auto ClearByte(ssize_t byte_index) -> void;
 
   // Clear all of this group's metadata bytes that indicate a deleted slot to
@@ -533,9 +538,12 @@ inline auto MetadataGroup::Store(uint8_t* metadata, ssize_t index) const
   CARBON_DCHECK(0 == std::memcmp(metadata + index, &metadata_bytes, Size));
 }
 
+template <bool IsCalled>
 inline auto MetadataGroup::ClearByte(ssize_t byte_index) -> void {
-  CARBON_DCHECK(FastByteClear, "Only use byte clearing when fast!");
-  CARBON_DCHECK(Size == 8, "The clear implementation assumes an 8-byte group.");
+  static_assert(!IsCalled || FastByteClear,
+                "Only use byte clearing when fast!");
+  static_assert(!IsCalled || Size == 8,
+                "The clear implementation assumes an 8-byte group.");
 
   metadata_ints[0] &= ~(static_cast<uint64_t>(0xff) << (byte_index * 8));
 }

explorer/interpreter/type_checker.cpp

@@ -1351,7 +1351,7 @@ auto TypeChecker::ArgumentDeduction::Deduce(Nonnull<const Value*> param,
     }
     case Value::Kind::MixinPseudoType:
     case Value::Kind::TypeOfMixinPseudoType:
-      CARBON_CHECK(false, "Type expression must not contain Mixin types");
+      CARBON_FATAL("Type expression must not contain Mixin types");
   }
 }