Clean up clang-tidy issues in explorer. (#2621)

google-readability-function-size and readability-function-size were _both_ triggering on TypeCheckExp. It looks like the Google version may be a subset of the general version, so I've disabled the Google version while keeping the general version and adding a NOLINT for it.

I manually removed the `const` in cases like `Nonnull<const VTable* const>` based on the readability-const-return-type warning. i.e., where a return type is a pointer, the `const` isn't meaningful and the tidy check was warning about that.

Added a NOLINT for misc-definitions-in-headers on IsRecursivelyTransformable. I think that's the right choice for the `constexpr`, the warning didn't feel accurate and may be getting confused by the templating.

I changed the structure of `carbon_files` in the fuzzer because the `new` was causing a warning about exceptions. However, also disabling bugprone-exception-escape because it's what was flagging this and it's not really a helpful warning.

Other changes were automated.

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

.clang-tidy

@@ -3,13 +3,17 @@
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 ---
+# - bugprone-exception-escape finds issues like out-of-memory in main(). We
+#   don't use exceptions, so it's unlikely to find real issues.
+# - google-readability-function-size overlaps with readability-function-size.
 # - modernize-use-nodiscard is disabled because it only fixes const methods,
 #   not non-const, which yields distracting results on accessors.
 # - performance-unnecessary-value-param is disabled because it duplicate
 #   modernize-pass-by-value.
 Checks:
   -*, bugprone-*, -bugprone-branch-clone, -bugprone-easily-swappable-parameters,
-  -bugprone-narrowing-conversions, google-*, -google-readability-todo,
+  -bugprone-exception-escape, -bugprone-narrowing-conversions, google-*,
+  -google-readability-function-size, -google-readability-todo,
   misc-definitions-in-headers, misc-misplaced-const, misc-redundant-expression,
   misc-static-assert, misc-unconventional-assign-operator,
   misc-uniqueptr-reset-release, misc-unused-*, modernize-*,

explorer/ast/declaration.cpp

@@ -441,7 +441,7 @@ void AlternativeSignature::PrintID(llvm::raw_ostream& out) const {
 
 auto ChoiceDeclaration::FindAlternative(std::string_view name) const
     -> std::optional<const AlternativeSignature*> {
-  for (auto* alt : alternatives()) {
+  for (const auto* alt : alternatives()) {
     if (alt->name() == name) {
       return alt;
     }

explorer/fuzzing/ast_to_proto.cpp

@@ -618,7 +618,7 @@ static auto DeclaredNameToProto(const DeclaredName& name)
     -> Fuzzing::DeclaredName {
   Fuzzing::DeclaredName name_proto;
   name_proto.set_name(std::string(name.inner_name()));
-  for (auto& [loc, qual] : name.qualifiers()) {
+  for (const auto& [loc, qual] : name.qualifiers()) {
     name_proto.add_qualifiers(qual);
   }
   return name_proto;

explorer/fuzzing/ast_to_proto_test.cpp

@@ -124,7 +124,7 @@ TEST(AstToProtoTest, SetsAllProtoFields) {
 auto main(int argc, char** argv) -> int {
   ::testing::InitGoogleTest(&argc, argv);
   // gtest should remove flags, leaving just input files.
-  Carbon::Testing::carbon_files =
-      new std::vector<llvm::StringRef>(&argv[1], &argv[argc]);
+  std::vector<llvm::StringRef> carbon_files(&argv[1], &argv[argc]);
+  Carbon::Testing::carbon_files = &carbon_files;
   return RUN_ALL_TESTS();
 }

explorer/fuzzing/proto_to_carbon_test.cpp

@@ -57,7 +57,8 @@ TEST(ProtoToCarbonTest, Roundtrip) {
 
 auto main(int argc, char** argv) -> int {
   ::testing::InitGoogleTest(&argc, argv);
-  Carbon::Testing::carbon_files =
-      new std::vector<llvm::StringRef>(&argv[1], &argv[argc]);
+  // gtest should remove flags, leaving just input files.
+  std::vector<llvm::StringRef> carbon_files(&argv[1], &argv[argc]);
+  Carbon::Testing::carbon_files = &carbon_files;
   return RUN_ALL_TESTS();
 }

explorer/interpreter/matching_impl_set.cpp

@@ -17,7 +17,7 @@ namespace Carbon {
 // and adds them to the signature of a `Match` object.
 class MatchingImplSet::LeafCollector {
  public:
-  LeafCollector(Match* match) : match_(match) {}
+  explicit LeafCollector(Match* match) : match_(match) {}
 
   void Collect(const Value* value) {
     value->Visit<void>(
@@ -28,15 +28,15 @@ class MatchingImplSet::LeafCollector {
 
  private:
   // Most kinds of value don't contribute to the signature.
-  void VisitValue(const Value*) {}
+  void VisitValue(const Value* /*unused*/) {}
 
-  void VisitValue(const TypeType*) { Collect(Label::TypeType); }
+  void VisitValue(const TypeType* /*unused*/) { Collect(Label::TypeType); }
 
-  void VisitValue(const BoolType*) { Collect(Label::BoolType); }
+  void VisitValue(const BoolType* /*unused*/) { Collect(Label::BoolType); }
 
-  void VisitValue(const IntType*) { Collect(Label::IntType); }
+  void VisitValue(const IntType* /*unused*/) { Collect(Label::IntType); }
 
-  void VisitValue(const StringType*) { Collect(Label::StringType); }
+  void VisitValue(const StringType* /*unused*/) { Collect(Label::StringType); }
 
   void VisitValue(const StaticArrayType* array) {
     Collect(Label::ArrayType);
@@ -57,7 +57,7 @@ class MatchingImplSet::LeafCollector {
 
   void VisitValue(const TupleType* tuple_type) {
     Collect(Label::TupleType);
-    for (auto* elem_type : tuple_type->elements()) {
+    for (const auto* elem_type : tuple_type->elements()) {
       Collect(elem_type);
     }
   }
@@ -92,7 +92,6 @@ class MatchingImplSet::LeafCollector {
     }
   }
 
- private:
   Match* match_;
 };
 
@@ -100,7 +99,8 @@ auto MatchingImplSet::GetLabelForDeclaration(const Declaration& declaration)
     -> Label {
   auto [it, added] = declaration_labels_.insert(
       {&declaration,
-       Label(int(Label::FirstDeclarationLabel) + declaration_labels_.size())});
+       static_cast<Label>(static_cast<int>(Label::FirstDeclarationLabel) +
+                          declaration_labels_.size())});
   return it->second;
 }
 

explorer/interpreter/matching_impl_set.h

@@ -49,7 +49,7 @@ class MatchingImplSet {
     ~Match();
 
     Match(const Match&) = delete;
-    Match& operator=(const Match&) = delete;
+    auto operator=(const Match&) -> Match& = delete;
 
     // Check to see if this match duplicates any prior one within the same set,
     // or if there's a simpler form of this match in the set. If so, returns a
@@ -121,13 +121,13 @@ struct llvm::DenseMapInfo<Carbon::MatchingImplSet::Label> {
   using Base = llvm::DenseMapInfo<int>;
   using Label = Carbon::MatchingImplSet::Label;
   static inline auto getEmptyKey() -> Label {
-    return Label(Base::getEmptyKey());
+    return static_cast<Label>(Base::getEmptyKey());
   }
   static inline auto getTombstoneKey() -> Label {
-    return Label(Base::getTombstoneKey());
+    return static_cast<Label>(Base::getTombstoneKey());
   }
   static inline auto getHashValue(Label label) -> unsigned {
-    return Base::getHashValue(int(label));
+    return Base::getHashValue(static_cast<int>(label));
   }
   static auto isEqual(Label a, Label b) -> bool { return a == b; }
 };

explorer/interpreter/type_checker.cpp

@@ -948,7 +948,7 @@ auto TypeChecker::ArgumentDeduction::Deduce(Nonnull<const Value*> param,
   }
 
   // If param is the name of a variable we're deducing, then deduce it.
-  if (auto* var_type = dyn_cast<VariableType>(param)) {
+  if (const auto* var_type = dyn_cast<VariableType>(param)) {
     const auto& binding = var_type->binding();
     if (auto it = deduced_values_.find(&binding); it != deduced_values_.end()) {
       it->second.push_back(arg);
@@ -2512,6 +2512,7 @@ auto TypeChecker::CheckAddrMeAccess(
   return Success();
 }
 
+// NOLINTNEXTLINE(readability-function-size)
 auto TypeChecker::TypeCheckExp(Nonnull<Expression*> e,
                                const ImplScope& impl_scope)
     -> ErrorOr<Success> {

explorer/interpreter/value.h

@@ -357,7 +357,7 @@ class NominalClassValue : public Value {
     return base_;
   }
   // Returns a pointer of pointer to the child-most class value.
-  auto class_value_ptr() const -> Nonnull<const NominalClassValue** const> {
+  auto class_value_ptr() const -> Nonnull<const NominalClassValue**> {
     return class_value_ptr_;
   }
 

explorer/interpreter/value_transform.h

@@ -29,6 +29,7 @@ struct IsRecursivelyTransformableVisitor {
 template <typename T, typename = std::true_type>
 constexpr bool IsRecursivelyTransformable = false;
 template <typename T>
+// NOLINTNEXTLINE(misc-definitions-in-headers)
 constexpr bool IsRecursivelyTransformable<
     T, decltype(std::declval<const T>().Decompose(
            IsRecursivelyTransformableVisitor<T>{}))> = true;
@@ -178,14 +179,13 @@ class ValueTransform : public TransformBase<Derived> {
   }
 
   // Preserve vtable during transformation.
-  auto operator()(Nonnull<const VTable* const> vtable)
-      -> Nonnull<const VTable* const> {
+  auto operator()(Nonnull<const VTable*> vtable) -> Nonnull<const VTable*> {
     return vtable;
   }
 
   // Preserve class value ptr during transformation.
-  auto operator()(Nonnull<const NominalClassValue** const> value_ptr)
-      -> Nonnull<const NominalClassValue** const> {
+  auto operator()(Nonnull<const NominalClassValue**> value_ptr)
+      -> Nonnull<const NominalClassValue**> {
     return value_ptr;
   }
 };