Look at flipping clang-tidy's misc-* to enable-by-default

.clang-tidy

@@ -13,20 +13,16 @@ Checks:
   - '-*'
   - 'bugprone-*'
   - 'google-*'
+  - 'misc-*'
   - 'modernize-*'
   - 'performance-*'
   - 'readability-*'
 
-  # `misc` is selectively enabled because we want a minority of them.
-  - 'misc-definitions-in-headers'
-  - 'misc-misplaced-const'
-  - 'misc-redundant-expression'
-  - 'misc-static-assert'
-  - 'misc-unconventional-assign-operator'
-  - 'misc-uniqueptr-reset-release'
-  - 'misc-unused-*'
-
   # Disabled due to the implied style choices.
+  - '-misc-const-correctness'
+  - '-misc-include-cleaner'
+  - '-misc-non-private-member-variables-in-classes'
+  - '-misc-use-anonymous-namespace'
   - '-modernize-return-braced-init-list'
   - '-modernize-use-default-member-init'
   - '-modernize-use-integer-sign-comparison'
@@ -68,6 +64,8 @@ Checks:
   - '-google-readability-function-size'
   # Suggests usernames on TODOs, which we don't want.
   - '-google-readability-todo'
+  # Overlaps with `-Wno-missing-prototypes`.
+  - '-misc-use-internal-linkage'
   # Suggests `std::array`, which we could migrate to, but conflicts with the
   # status quo.
   - '-modernize-avoid-c-arrays'

common/command_line.cpp

@@ -10,6 +10,10 @@
 #include "llvm/ADT/PointerIntPair.h"
 #include "llvm/Support/FormatVariadic.h"
 
+// Recursion is used for subcommands. This should be okay since recursion is
+// limited by command line architecture.
+// NOLINTBEGIN(misc-no-recursion)
+
 namespace Carbon::CommandLine {
 
 auto operator<<(llvm::raw_ostream& output, ParseResult result)
@@ -1525,3 +1529,5 @@ auto Parse(llvm::ArrayRef<llvm::StringRef> unparsed_args,
 }
 
 }  // namespace Carbon::CommandLine
+
+// NOLINTEND(misc-no-recursion)

migrate_cpp/cpp_refactoring/fn_inserter.cpp

@@ -6,6 +6,7 @@
 
 #include "clang/ASTMatchers/ASTMatchers.h"
 
+// NOLINTNEXTLINE(readability-identifier-naming)
 namespace cam = ::clang::ast_matchers;
 
 namespace Carbon {

migrate_cpp/cpp_refactoring/for_range.cpp

@@ -6,6 +6,7 @@
 
 #include "clang/ASTMatchers/ASTMatchers.h"
 
+// NOLINTNEXTLINE(readability-identifier-naming)
 namespace cam = ::clang::ast_matchers;
 
 namespace Carbon {

migrate_cpp/cpp_refactoring/var_decl.cpp

@@ -7,6 +7,7 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "llvm/Support/FormatVariadic.h"
 
+// NOLINTNEXTLINE(readability-identifier-naming)
 namespace cam = ::clang::ast_matchers;
 
 namespace Carbon {
@@ -36,7 +37,6 @@ auto VarDecl::GetTypeStr(const clang::VarDecl& decl) -> std::string {
   auto type_loc = decl.getTypeSourceInfo()->getTypeLoc();
   std::vector<std::pair<clang::TypeLoc::TypeLocClass, std::string>> segments;
   while (!type_loc.isNull()) {
-    std::string text;
     auto qualifiers = type_loc.getType().getLocalQualifiers();
     std::string qual_str;
     if (!qualifiers.empty()) {

migrate_cpp/rewriter.cpp

@@ -297,6 +297,7 @@ auto RewriteBuilder::VisitUnaryOperator(clang::UnaryOperator* expr) -> bool {
   return true;
 }
 
+// NOLINTNEXTLINE(misc-no-recursion): Recursion may be okay for migration.
 auto RewriteBuilder::TraverseFunctionDecl(clang::FunctionDecl* decl) -> bool {
   clang::TypeLoc return_type_loc = decl->getFunctionTypeLoc().getReturnLoc();
   if (!TraverseTypeLoc(return_type_loc)) {
@@ -345,6 +346,7 @@ auto RewriteBuilder::TraverseFunctionDecl(clang::FunctionDecl* decl) -> bool {
   return true;
 }
 
+// NOLINTNEXTLINE(misc-no-recursion): Recursion may be okay for migration.
 auto RewriteBuilder::TraverseVarDecl(clang::VarDecl* decl) -> bool {
   clang::TypeLoc loc = decl->getTypeSourceInfo()->getTypeLoc();
   if (!TraverseTypeLoc(loc)) {

testing/fuzzing/proto_to_carbon.cpp

@@ -12,6 +12,9 @@
 #include "llvm/Support/raw_ostream.h"
 #include "testing/fuzzing/carbon.pb.h"
 
+// This is a test file, so the recursion is okay.
+// NOLINTBEGIN(misc-no-recursion)
+
 namespace Carbon {
 
 static auto ExpressionToCarbon(const Fuzzing::Expression& expression,
@@ -1023,3 +1026,5 @@ auto ParseCarbonTextProto(const std::string& contents)
 }
 
 }  // namespace Carbon
+
+// NOLINTEND(misc-no-recursion)

toolchain/base/value_ids.h

@@ -21,8 +21,7 @@ namespace Carbon {
 //
 // These values are not canonicalized, because we don't expect them to repeat
 // and don't use them in SemIR values.
-class Real : public Printable<Real> {
- public:
+struct Real : public Printable<Real> {
   auto Print(llvm::raw_ostream& output_stream) const -> void {
     mantissa.print(output_stream, /*isSigned=*/false);
     output_stream << "*" << (is_decimal ? "10" : "2") << "^" << exponent;

toolchain/check/convert.cpp

@@ -22,6 +22,10 @@
 #include "toolchain/sem_ir/inst.h"
 #include "toolchain/sem_ir/typed_insts.h"
 
+// TODO: This contains a lot of recursion. Consider removing it in order to
+// prevent accidents.
+// NOLINTBEGIN(misc-no-recursion)
+
 namespace Carbon::Check {
 
 // Given an initializing expression, find its return slot argument. Returns
@@ -1245,3 +1249,5 @@ auto ExprAsType(Context& context, SemIR::LocId loc_id, SemIR::InstId value_id)
 }
 
 }  // namespace Carbon::Check
+
+// NOLINTEND(misc-no-recursion)

toolchain/check/import_ref.cpp

@@ -2523,6 +2523,10 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
 // Invalid if more has been added to the stack. This is the same as
 // TryResolveInst, except that it may resolve symbolic constants as canonical
 // constants instead of as constants associated with a particular generic.
+//
+// TODO: Consider refactoring the body to a helper in order to eliminate
+// recursion.
+// NOLINTNEXTLINE(misc-no-recursion)
 static auto TryResolveInstCanonical(ImportRefResolver& resolver,
                                     SemIR::InstId inst_id,
                                     SemIR::ConstantId const_id)

toolchain/check/subst.cpp

@@ -61,6 +61,19 @@ class Worklist {
 // Pushes the specified operand onto the worklist.
 static auto PushOperand(Context& context, Worklist& worklist,
                         SemIR::IdKind kind, int32_t arg) -> void {
+  auto push_block = [&](SemIR::InstBlockId block_id) {
+    for (auto inst_id :
+         context.inst_blocks().Get(SemIR::InstBlockId(block_id))) {
+      worklist.Push(inst_id);
+    }
+  };
+
+  auto push_specific = [&](SemIR::SpecificId specific_id) {
+    if (specific_id.is_valid()) {
+      push_block(context.specifics().Get(specific_id).args_id);
+    }
+  };
+
   switch (kind) {
     case SemIR::IdKind::For<SemIR::InstId>:
       if (SemIR::InstId inst_id(arg); inst_id.is_valid()) {
@@ -73,9 +86,7 @@ static auto PushOperand(Context& context, Worklist& worklist,
       }
       break;
     case SemIR::IdKind::For<SemIR::InstBlockId>:
-      for (auto inst_id : context.inst_blocks().Get(SemIR::InstBlockId(arg))) {
-        worklist.Push(inst_id);
-      }
+      push_block(SemIR::InstBlockId(arg));
       break;
     case SemIR::IdKind::For<SemIR::StructTypeFieldsId>: {
       for (auto field :
@@ -90,18 +101,13 @@ static auto PushOperand(Context& context, Worklist& worklist,
       }
       break;
     case SemIR::IdKind::For<SemIR::SpecificId>:
-      if (auto specific_id = static_cast<SemIR::SpecificId>(arg);
-          specific_id.is_valid()) {
-        PushOperand(context, worklist, SemIR::IdKind::For<SemIR::InstBlockId>,
-                    context.specifics().Get(specific_id).args_id.index);
-      }
+      push_specific(SemIR::SpecificId(arg));
       break;
     case SemIR::IdKind::For<SemIR::FacetTypeId>: {
       const auto& facet_type_info =
           context.facet_types().Get(SemIR::FacetTypeId(arg));
       for (auto interface : facet_type_info.impls_constraints) {
-        PushOperand(context, worklist, SemIR::IdKind::For<SemIR::SpecificId>,
-                    interface.specific_id.index);
+        push_specific(interface.specific_id);
       }
       for (auto rewrite : facet_type_info.rewrite_constraints) {
         auto lhs_inst_id =
@@ -134,6 +140,25 @@ static auto ExpandOperands(Context& context, Worklist& worklist,
 // Pops the specified operand from the worklist and returns it.
 static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind,
                        int32_t arg) -> int32_t {
+  auto pop_block_id = [&](SemIR::InstBlockId old_inst_block_id) {
+    auto size = context.inst_blocks().Get(old_inst_block_id).size();
+    SemIR::CopyOnWriteInstBlock new_inst_block(context.sem_ir(),
+                                               old_inst_block_id);
+    for (auto i : llvm::reverse(llvm::seq(size))) {
+      new_inst_block.Set(i, worklist.Pop());
+    }
+    return new_inst_block.GetCanonical();
+  };
+
+  auto pop_specific = [&](SemIR::SpecificId specific_id) {
+    if (!specific_id.is_valid()) {
+      return specific_id;
+    }
+    auto& specific = context.specifics().Get(specific_id);
+    auto args_id = pop_block_id(specific.args_id);
+    return context.specifics().GetOrAdd(specific.generic_id, args_id);
+  };
+
   switch (kind) {
     case SemIR::IdKind::For<SemIR::InstId>: {
       SemIR::InstId inst_id(arg);
@@ -150,14 +175,7 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind,
       return context.GetTypeIdForTypeInst(worklist.Pop()).index;
     }
     case SemIR::IdKind::For<SemIR::InstBlockId>: {
-      SemIR::InstBlockId old_inst_block_id(arg);
-      auto size = context.inst_blocks().Get(old_inst_block_id).size();
-      SemIR::CopyOnWriteInstBlock new_inst_block(context.sem_ir(),
-                                                 old_inst_block_id);
-      for (auto i : llvm::reverse(llvm::seq(size))) {
-        new_inst_block.Set(i, worklist.Pop());
-      }
-      return new_inst_block.GetCanonical().index;
+      return pop_block_id(SemIR::InstBlockId(arg)).index;
     }
     case SemIR::IdKind::For<SemIR::StructTypeFieldsId>: {
       SemIR::StructTypeFieldsId old_fields_id(arg);
@@ -182,15 +200,7 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind,
       return new_type_block.GetCanonical().index;
     }
     case SemIR::IdKind::For<SemIR::SpecificId>: {
-      SemIR::SpecificId specific_id(arg);
-      if (!specific_id.is_valid()) {
-        return arg;
-      }
-      auto& specific = context.specifics().Get(specific_id);
-      auto args_id = SemIR::InstBlockId(
-          PopOperand(context, worklist, SemIR::IdKind::For<SemIR::InstBlockId>,
-                     specific.args_id.index));
-      return context.specifics().GetOrAdd(specific.generic_id, args_id).index;
+      return pop_specific(SemIR::SpecificId(arg)).index;
     }
     case SemIR::IdKind::For<SemIR::FacetTypeId>: {
       const auto& old_facet_type_info =
@@ -206,11 +216,8 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::IdKind kind,
       }
       for (auto i : llvm::reverse(
                llvm::seq(old_facet_type_info.impls_constraints.size()))) {
-        auto specific_id = PopOperand(
-            context, worklist, SemIR::IdKind::For<SemIR::SpecificId>,
-            old_facet_type_info.impls_constraints[i].specific_id.index);
         new_facet_type_info.impls_constraints[i].specific_id =
-            SemIR::SpecificId(specific_id);
+            pop_specific(old_facet_type_info.impls_constraints[i].specific_id);
       }
       new_facet_type_info.Canonicalize();
       return context.facet_types().Add(new_facet_type_info).index;

toolchain/sem_ir/formatter.cpp

@@ -20,6 +20,11 @@
 #include "toolchain/sem_ir/name_scope.h"
 #include "toolchain/sem_ir/typed_insts.h"
 
+// TODO: Consider addressing recursion here, although it's not critical because
+// the formatter isn't required to work on arbitrary code. Still, it may help
+// in the future to debug complex code.
+// NOLINTBEGIN(misc-no-recursion)
+
 namespace Carbon::SemIR {
 
 // Formatter for printing textual Semantics IR.
@@ -1379,3 +1384,5 @@ auto Formatter::Print(llvm::raw_ostream& out) -> void {
 }
 
 }  // namespace Carbon::SemIR
+
+// NOLINTEND(misc-no-recursion)

toolchain/sem_ir/inst_namer.cpp

@@ -355,13 +355,17 @@ auto InstNamer::AddBlockLabel(ScopeId scope_id, SemIR::LocId loc_id,
   AddBlockLabel(scope_id, branch.target_id, name.str(), loc_id);
 }
 
+// TODO: Consider addressing recursion here. It may be important because
+// InstNamer is used for debug info.
+// NOLINTNEXTLINE(misc-no-recursion)
 auto InstNamer::CollectNamesInBlock(ScopeId scope_id, InstBlockId block_id)
     -> void {
   if (block_id.is_valid()) {
     CollectNamesInBlock(scope_id, sem_ir_->inst_blocks().Get(block_id));
   }
 }
 
+// NOLINTNEXTLINE(misc-no-recursion): See above TODO.
 auto InstNamer::CollectNamesInBlock(ScopeId scope_id,
                                     llvm::ArrayRef<InstId> block) -> void {
   Scope& scope = GetScopeInfo(scope_id);

toolchain/testing/yaml_test_helpers.cpp

@@ -9,6 +9,8 @@
 
 namespace Carbon::Testing::Yaml {
 
+// This is for tests, so we should be okay with the recursion here.
+// NOLINTNEXTLINE(misc-no-recursion)
 static auto Parse(llvm::yaml::Node* node) -> Value {
   CARBON_CHECK(node != nullptr);