Look at flipping clang-tidy's misc-* to enable-by-default (#4699)

I was wondering, instead of treating `misc` differently and enabling
specific checks, maybe we can flip that since we actually seem okay with
most of the checks?

The main check I'm enabling, with significant edits here, is
`misc-no-recursion`. But maybe this is helpful to enable, even with the
necessary NOLINTs, since we want to avoid recursion in the toolchain?
This PR shows some example fixes in subst.cpp (which are more stylistic,
since the code shouldn't actually have recursed due to its structure; I
think we could remove the warning on TryResolveInst the same way). Some
also just don't seem worth fixing, like those in tests files (I didn't
see a way to exclude files in .clang-tidy, so instead I'm using
NOLINTBEGIN). But I think we might actually want to fix inst_namer, and
there's enough in convert that I didn't look closely.

Also, I made some protected -> private style fixes based on
`misc-non-private-member-variables-in-classes` (this is also how I
noticed `class Real` versus `struct Real`). With node_stack, it looks
like the `protected` wasn't even used. [Per
style](https://google.github.io/styleguide/cppguide.html#Access_Control),
data members should be private outside tests. But since we can't
trivially exclude `protected` members in tests, I'm turning it off -- I
don't view it as offering enough benefit on the whole.

migrate_cpp issues are preexisting (I believe we just aren't monitoring
it), but changes there make `bazel build --config=clang-tidy -k //...`
work cleanly.

---------

Co-authored-by: Richard Smith <richard@metafoo.co.uk>

.clang-tidy

@@ -13,20 +13,15 @@ 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-use-anonymous-namespace'
   - '-modernize-return-braced-init-list'
   - '-modernize-use-default-member-init'
   - '-modernize-use-integer-sign-comparison'
@@ -68,6 +63,11 @@ Checks:
   - '-google-readability-function-size'
   # Suggests usernames on TODOs, which we don't want.
   - '-google-readability-todo'
+  # Even with `IgnoreClassesWithAllMemberVariablesBeingPublic` to allow structs,
+  # we use `protected` members in too many tests.
+  - '-misc-non-private-member-variables-in-classes'
+  # 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)
@@ -1311,41 +1315,41 @@ void ArgBuilder::HelpHidden(bool is_help_hidden) {
 ArgBuilder::ArgBuilder(Arg* arg) : arg_(arg) {}
 
 void FlagBuilder::Default(bool flag_value) {
-  arg_->has_default = true;
-  arg_->default_flag = flag_value;
+  arg()->has_default = true;
+  arg()->default_flag = flag_value;
 }
 
-void FlagBuilder::Set(bool* flag) { arg_->flag_storage = flag; }
+void FlagBuilder::Set(bool* flag) { arg()->flag_storage = flag; }
 
 void IntegerArgBuilder::Default(int integer_value) {
-  arg_->has_default = true;
-  arg_->default_integer = integer_value;
+  arg()->has_default = true;
+  arg()->default_integer = integer_value;
 }
 
 void IntegerArgBuilder::Set(int* integer) {
-  arg_->is_append = false;
-  arg_->integer_storage = integer;
+  arg()->is_append = false;
+  arg()->integer_storage = integer;
 }
 
 void IntegerArgBuilder::Append(llvm::SmallVectorImpl<int>* sequence) {
-  arg_->is_append = true;
-  arg_->integer_sequence = sequence;
+  arg()->is_append = true;
+  arg()->integer_sequence = sequence;
 }
 
 void StringArgBuilder::Default(llvm::StringRef string_value) {
-  arg_->has_default = true;
-  arg_->default_string = string_value;
+  arg()->has_default = true;
+  arg()->default_string = string_value;
 }
 
 void StringArgBuilder::Set(llvm::StringRef* string) {
-  arg_->is_append = false;
-  arg_->string_storage = string;
+  arg()->is_append = false;
+  arg()->string_storage = string;
 }
 
 void StringArgBuilder::Append(
     llvm::SmallVectorImpl<llvm::StringRef>* sequence) {
-  arg_->is_append = true;
-  arg_->string_sequence = sequence;
+  arg()->is_append = true;
+  arg()->string_sequence = sequence;
 }
 
 static auto IsValidName(llvm::StringRef name) -> bool {
@@ -1525,3 +1529,5 @@ auto Parse(llvm::ArrayRef<llvm::StringRef> unparsed_args,
 }
 
 }  // namespace Carbon::CommandLine
+
+// NOLINTEND(misc-no-recursion)

common/command_line.h

@@ -307,10 +307,13 @@ class ArgBuilder {
   void MetaAction(T action);
 
  protected:
-  friend CommandBuilder;
+  friend class CommandBuilder;
   // `arg` must not be null.
   explicit ArgBuilder(Arg* arg);
 
+  auto arg() -> Arg* { return arg_; }
+
+ private:
   Arg* arg_;
 };
 
@@ -756,7 +759,7 @@ auto OneOfArgBuilder::OneOfValue(llvm::StringRef str, T value)
 template <typename T, typename U, size_t N>
 void OneOfArgBuilder::SetOneOf(const OneOfValueT<U> (&values)[N], T* result) {
   static_assert(N > 0, "Must include at least one value.");
-  arg_->is_append = false;
+  arg()->is_append = false;
   OneOfImpl(
       values, [result](T value) { *result = value; },
       std::make_index_sequence<N>{});
@@ -766,7 +769,7 @@ template <typename T, typename U, size_t N>
 void OneOfArgBuilder::AppendOneOf(const OneOfValueT<U> (&values)[N],
                                   llvm::SmallVectorImpl<T>* sequence) {
   static_assert(N > 0, "Must include at least one value.");
-  arg_->is_append = true;
+  arg()->is_append = true;
   OneOfImpl(
       values, [sequence](T value) { sequence->push_back(value); },
       std::make_index_sequence<N>{});
@@ -792,12 +795,12 @@ void OneOfArgBuilder::OneOfImpl(const OneOfValueT<U> (&input_values)[N],
 
   // Directly copy the value strings into a heap-allocated array in the
   // argument.
-  new (&arg_->value_strings)
+  new (&arg()->value_strings)
       llvm::OwningArrayRef<llvm::StringRef>(value_strings);
 
   // And build a type-erased action that maps a specific value string to a value
   // by index.
-  new (&arg_->value_action) Arg::ValueActionT(
+  new (&arg()->value_action) Arg::ValueActionT(
       [values, match](const Arg& arg, llvm::StringRef value_string) -> bool {
         for (int i : llvm::seq<int>(0, N)) {
           if (value_string == arg.value_strings[i]) {
@@ -810,11 +813,11 @@ void OneOfArgBuilder::OneOfImpl(const OneOfValueT<U> (&input_values)[N],
 
   // Fold over all the input values to see if there is a default.
   if ((input_values[Indices].is_default || ...)) {
-    CARBON_CHECK(!arg_->is_append, "Can't append default.");
+    CARBON_CHECK(!arg()->is_append, "Can't append default.");
     CARBON_CHECK((input_values[Indices].is_default + ... + 0) == 1,
                  "Cannot default more than one value.");
 
-    arg_->has_default = true;
+    arg()->has_default = true;
 
     // First build a lambda that configures the default using an index. We'll
     // call this below, this lambda isn't the one that is stored.
@@ -822,12 +825,12 @@ void OneOfArgBuilder::OneOfImpl(const OneOfValueT<U> (&input_values)[N],
       // Now that we have the desired default index, build a lambda and store it
       // as the default action. This lambda is stored and so it captures the
       // necessary information explicitly and by value.
-      new (&arg_->default_action)
+      new (&arg()->default_action)
           Arg::DefaultActionT([value = default_value.value,
                                match](const Arg& /*arg*/) { match(value); });
 
       // Also store the index itself for use when printing help.
-      arg_->default_value_index = index;
+      arg()->default_value_index = index;
     };
 
     // Now we fold across the inputs and in the one case that is the default, we

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

@@ -342,6 +342,14 @@ class ImportContext {
   }
 
  protected:
+  auto pending_generics() -> llvm::SmallVector<PendingGeneric>& {
+    return pending_generics_;
+  }
+  auto pending_specifics() -> llvm::SmallVector<PendingSpecific>& {
+    return pending_specifics_;
+  }
+
+ private:
   Context& context_;
   SemIR::ImportIRId import_ir_id_;
   const SemIR::File& import_ir_;
@@ -2526,6 +2534,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)
@@ -2850,7 +2862,7 @@ static auto FinishPendingSpecific(ImportRefResolver& resolver,
 auto ImportRefResolver::PerformPendingWork() -> void {
   // Note that the individual Finish steps can add new pending work, so keep
   // going until we have no more work to do.
-  while (!pending_generics_.empty() || !pending_specifics_.empty()) {
+  while (!pending_generics().empty() || !pending_specifics().empty()) {
     // Process generics in the order that we added them because a later
     // generic might refer to an earlier one, and the calls to
     // RebuildGenericEvalBlock assume that the reachable SemIR is in a valid
@@ -2858,13 +2870,13 @@ auto ImportRefResolver::PerformPendingWork() -> void {
     // TODO: Import the generic eval block rather than calling
     // RebuildGenericEvalBlock to rebuild it so that order doesn't matter.
     // NOLINTNEXTLINE(modernize-loop-convert)
-    for (size_t i = 0; i != pending_generics_.size(); ++i) {
-      FinishPendingGeneric(*this, pending_generics_[i]);
+    for (size_t i = 0; i != pending_generics().size(); ++i) {
+      FinishPendingGeneric(*this, pending_generics()[i]);
     }
-    pending_generics_.clear();
+    pending_generics().clear();
 
-    while (!pending_specifics_.empty()) {
-      FinishPendingSpecific(*this, pending_specifics_.pop_back_val());
+    while (!pending_specifics().empty()) {
+      FinishPendingSpecific(*this, pending_specifics().pop_back_val());
     }
   }
 }

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.
@@ -1383,3 +1388,5 @@ auto Formatter::Print(llvm::raw_ostream& out) -> void {
 }
 
 }  // namespace Carbon::SemIR
+
+// NOLINTEND(misc-no-recursion)

toolchain/sem_ir/inst_namer.cpp

@@ -355,6 +355,9 @@ 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()) {
@@ -362,6 +365,7 @@ auto InstNamer::CollectNamesInBlock(ScopeId scope_id, InstBlockId 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);