Add braces for CARBON_KIND uses that lack them (#5882)

Also document why these are expected on `CARBON_KIND`. In
`kind_switch_test.cpp`, drop the `str` variable.

My recollection of the original discussion of `CARBON_KIND` is that it
should always have braces due to the risk of confusion for statement
interpretation, similar to a typical `if`/`else` but more subtle due to
the macro.

For example:

```
      case CARBON_KIND(int n):
        str << "int = " << n;
        return str.TakeStr();
```

is equivalent to:

```
      case CARBON_KIND(int n): {
          str << "int = " << n;
        }
        return str.TakeStr();
```

This happens to work in context because `str` isn't scoped, but a
trivial refactoring to move `RawStringOstream str;` the first statement
of the `case` would probably have non-obvious results. For example:

```
      case CARBON_KIND(int n):
        RawStringOstream str; // Valid name shadowing, destructed without use.
        str << "int = " << n; // Name lookup error on `n`.
        return str.TakeStr();
```

toolchain/base/kind_switch.h

@@ -253,6 +253,11 @@ auto Cast(SwitchT&& kind_switch_value) -> decltype(auto) {
 // This uses `if` to scope the variable, and provides a dangling `else` in order
 // to prevent accidental `else` use. The label allows `:` to follow the macro
 // name, making it look more like a typical `case`.
+//
+// Because of the dangling else, braces should always be used with a `case
+// CARBON_KIND`. Otherwise, only the first statement is going to see the
+// variable. Even if that sometimes works, it can lead to confusing issues when
+// statements are added, and `if`/`else` coding style already requires braces.
 #define CARBON_KIND(typed_variable_decl)                                   \
   ::Carbon::Internal::Kind::ForCase<                                       \
       decltype(carbon_internal_kind_switch_value),                         \

toolchain/base/kind_switch_test.cpp

@@ -16,36 +16,33 @@ namespace {
 
 TEST(KindSwitch, Variant) {
   auto f = [](std::variant<int, float, char> v) -> std::string {
-    RawStringOstream str;
     CARBON_KIND_SWITCH(v) {
-      case CARBON_KIND(int n):
-        str << "int = " << n;
-        return str.TakeStr();
-      case CARBON_KIND(float f):
-        str << "float = " << f;
-        return str.TakeStr();
-      case CARBON_KIND(char c):
-        str << "char = " << c;
-        return str.TakeStr();
+      case CARBON_KIND(int n): {
+        return llvm::formatv("int = {0}", n);
+      }
+      case CARBON_KIND(float f): {
+        return llvm::formatv("float = {0}", f);
+      }
+      case CARBON_KIND(char c): {
+        return llvm::formatv("char = {0}", c);
+      }
     }
   };
 
   EXPECT_EQ(f(int{1}), "int = 1");
-  EXPECT_EQ(f(float{2}), "float = 2.000000e+00");
+  EXPECT_EQ(f(float{2}), "float = 2.00");
   EXPECT_EQ(f(char{'h'}), "char = h");
 }
 
 TEST(KindSwitch, VariantUnusedValue) {
   auto f = [](std::variant<int, float> v) -> std::string {
-    RawStringOstream str;
     CARBON_KIND_SWITCH(v) {
-      case CARBON_KIND(int n):
-        str << "int = " << n;
-        return str.TakeStr();
+      case CARBON_KIND(int n): {
+        return llvm::formatv("int = {0}", n);
+      }
       case CARBON_KIND(float _):
         // The float value is not used, we see that using `_` works.
-        str << "float";
-        return str.TakeStr();
+        return "float";
     }
   };
 

toolchain/check/impl_validation.cpp

@@ -92,8 +92,9 @@ static auto DiagnoseFinalImplNotInSameFileAsRootSelfTypeOrInterface(
       break;
     }
 
-    case CARBON_KIND(Step::Done _):
+    case CARBON_KIND(Step::Done _): {
       CARBON_FATAL("self type is empty?");
+    }
 
     default:
       break;

toolchain/check/type_structure.cpp

@@ -206,67 +206,86 @@ auto TypeStructureBuilder::Build(SemIR::TypeIterator type_iter)
   while (true) {
     using Step = SemIR::TypeIterator::Step;
     CARBON_KIND_SWITCH(type_iter.Next().any) {
-      case CARBON_KIND(Step::Done _):
+      case CARBON_KIND(Step::Done _): {
         // TODO: This requires 4 SmallVector moves (two here and two in the
         // constructor). Find a way to reduce that.
         return TypeStructure(std::exchange(structure_, {}),
                              std::exchange(symbolic_type_indices_, {}),
                              std::exchange(concrete_types_, {}));
-      case CARBON_KIND(Step::End _):
+      }
+      case CARBON_KIND(Step::End _): {
         AppendStructuralConcreteCloseParen();
         break;
-      case CARBON_KIND(Step::ConcreteType concrete):
+      }
+      case CARBON_KIND(Step::ConcreteType concrete): {
         AppendStructuralConcrete(concrete.type_id);
         break;
-      case CARBON_KIND(Step::SymbolicType _):
+      }
+      case CARBON_KIND(Step::SymbolicType _): {
         AppendStructuralSymbolic();
         break;
-      case CARBON_KIND(Step::TemplateType _):
+      }
+      case CARBON_KIND(Step::TemplateType _): {
         AppendStructuralSymbolic();
         break;
-      case CARBON_KIND(Step::ConcreteValue value):
+      }
+      case CARBON_KIND(Step::ConcreteValue value): {
         AppendStructuralConcrete(
             context_->constant_values().Get(value.inst_id));
         break;
-      case CARBON_KIND(Step::SymbolicValue _):
+      }
+      case CARBON_KIND(Step::SymbolicValue _): {
         AppendStructuralSymbolic();
         break;
-      case CARBON_KIND(Step::StructFieldName field_name):
+      }
+      case CARBON_KIND(Step::StructFieldName field_name): {
         AppendStructuralConcrete(field_name.name_id);
         break;
-      case CARBON_KIND(Step::ClassStartOnly class_start):
+      }
+      case CARBON_KIND(Step::ClassStartOnly class_start): {
         AppendStructuralConcrete(class_start.class_id);
         break;
-      case CARBON_KIND(Step::ClassStart class_start):
+      }
+      case CARBON_KIND(Step::ClassStart class_start): {
         AppendStructuralConcreteOpenParen(class_start.class_id);
         break;
-      case CARBON_KIND(Step::StructStartOnly _):
+      }
+      case CARBON_KIND(Step::StructStartOnly _): {
         AppendStructuralConcrete(TypeStructure::ConcreteStructType());
         break;
-      case CARBON_KIND(Step::StructStart _):
+      }
+      case CARBON_KIND(Step::StructStart _): {
         AppendStructuralConcreteOpenParen(TypeStructure::ConcreteStructType());
         break;
-      case CARBON_KIND(Step::TupleStartOnly _):
+      }
+      case CARBON_KIND(Step::TupleStartOnly _): {
         AppendStructuralConcrete(TypeStructure::ConcreteTupleType());
         break;
-      case CARBON_KIND(Step::TupleStart _):
+      }
+      case CARBON_KIND(Step::TupleStart _): {
         AppendStructuralConcreteOpenParen(TypeStructure::ConcreteTupleType());
         break;
-      case CARBON_KIND(Step::InterfaceStartOnly interface_start):
+      }
+      case CARBON_KIND(Step::InterfaceStartOnly interface_start): {
         AppendStructuralConcrete(interface_start.interface_id);
         break;
-      case CARBON_KIND(Step::InterfaceStart interface_start):
+      }
+      case CARBON_KIND(Step::InterfaceStart interface_start): {
         AppendStructuralConcreteOpenParen(interface_start.interface_id);
         break;
-      case CARBON_KIND(Step::IntStart int_start):
+      }
+      case CARBON_KIND(Step::IntStart int_start): {
         AppendStructuralConcreteOpenParen(int_start.type_id);
         break;
-      case CARBON_KIND(Step::ArrayStart _):
+      }
+      case CARBON_KIND(Step::ArrayStart _): {
         AppendStructuralConcreteOpenParen(TypeStructure::ConcreteArrayType());
         break;
-      case CARBON_KIND(Step::PointerStart _):
+      }
+      case CARBON_KIND(Step::PointerStart _): {
         AppendStructuralConcreteOpenParen(TypeStructure::ConcretePointerType());
         break;
+      }
     }
   }
 }

toolchain/lex/lex.cpp

@@ -1105,12 +1105,13 @@ auto Lexer::LexNumericLiteral(llvm::StringRef source_text, ssize_t& position)
   position += token_size;
 
   CARBON_KIND_SWITCH(literal->ComputeValue(emitter_)) {
-    case CARBON_KIND(NumericLiteral::IntValue && value):
+    case CARBON_KIND(NumericLiteral::IntValue && value): {
       return LexTokenWithPayload(TokenKind::IntLiteral,
                                  buffer_.value_stores_->ints()
                                      .AddUnsigned(std::move(value.value))
                                      .AsTokenPayload(),
                                  byte_offset);
+    }
     case CARBON_KIND(NumericLiteral::RealValue && value): {
       auto real_id = buffer_.value_stores_->reals().Add(
           Real{.mantissa = value.mantissa,
@@ -1119,8 +1120,9 @@ auto Lexer::LexNumericLiteral(llvm::StringRef source_text, ssize_t& position)
       return LexTokenWithPayload(TokenKind::RealLiteral, real_id.index,
                                  byte_offset);
     }
-    case CARBON_KIND(NumericLiteral::UnrecoverableError _):
+    case CARBON_KIND(NumericLiteral::UnrecoverableError _): {
       return LexTokenWithPayload(TokenKind::Error, token_size, byte_offset);
+    }
   }
 }
 

toolchain/sem_ir/stringify.cpp

@@ -123,36 +123,46 @@ class StepStack {
   auto PushArray(llvm::ArrayRef<PushItem> items) -> void {
     for (auto item : llvm::reverse(items)) {
       CARBON_KIND_SWITCH(item) {
-        case CARBON_KIND(InstId inst_id):
+        case CARBON_KIND(InstId inst_id): {
           PushInstId(inst_id);
           break;
-        case CARBON_KIND(llvm::StringRef string):
+        }
+        case CARBON_KIND(llvm::StringRef string): {
           PushString(string);
           break;
-        case CARBON_KIND(NameId name_id):
+        }
+        case CARBON_KIND(NameId name_id): {
           PushNameId(name_id);
           break;
-        case CARBON_KIND(ElementIndex element_index):
+        }
+        case CARBON_KIND(ElementIndex element_index): {
           PushElementIndex(element_index);
           break;
-        case CARBON_KIND(QualifiedNameItem qualified_name):
+        }
+        case CARBON_KIND(QualifiedNameItem qualified_name): {
           PushQualifiedName(qualified_name.first, qualified_name.second);
           break;
-        case CARBON_KIND(EntityNameItem entity_name):
+        }
+        case CARBON_KIND(EntityNameItem entity_name): {
           PushEntityName(entity_name.first, entity_name.second);
           break;
-        case CARBON_KIND(EntityNameId entity_name_id):
+        }
+        case CARBON_KIND(EntityNameId entity_name_id): {
           PushEntityNameId(entity_name_id);
           break;
-        case CARBON_KIND(TypeId type_id):
+        }
+        case CARBON_KIND(TypeId type_id): {
           PushTypeId(type_id);
           break;
-        case CARBON_KIND(SpecificInterface specific_interface):
+        }
+        case CARBON_KIND(SpecificInterface specific_interface): {
           PushSpecificInterface(specific_interface);
           break;
-        case CARBON_KIND(llvm::ListSeparator * sep):
+        }
+        case CARBON_KIND(llvm::ListSeparator * sep): {
           PushString(*sep);
           break;
+        }
       }
     }
   }