clang-tidy with readability checks (#1148)

.clang-tidy

@@ -16,7 +16,12 @@ Checks:
   -modernize-avoid-c-arrays, -modernize-return-braced-init-list,
   -modernize-use-default-member-init, -modernize-use-emplace,
   -modernize-use-nodiscard, performance-*, -performance-unnecessary-value-param,
-  readability-braces-around-statements, readability-identifier-naming
+  readability-*, -readability-convert-member-functions-to-static,
+  -readability-function-cognitive-complexity, -readability-else-after-return,
+  -readability-implicit-bool-conversion, -readability-magic-numbers,
+  -readability-make-member-function-const,
+  -readability-qualified-auto,  -readability-static-definition-in-anonymous-namespace,
+  -readability-suspicious-call-argument, -readability-use-anyofallof
 WarningsAsErrors: true
 CheckOptions:
   - { key: readability-identifier-naming.ClassCase, value: CamelCase }

common/check_internal.h

@@ -52,7 +52,7 @@ class ExitingStream {
     return *this;
   }
 
-  auto operator<<(AddSeparator /*unused*/) -> ExitingStream& {
+  auto operator<<(AddSeparator /*discarded*/) -> ExitingStream& {
     separator_ = true;
     return *this;
   }
@@ -60,7 +60,7 @@ class ExitingStream {
   // Low-precedence binary operator overload used in check.h macros to flush the
   // output and exit the program. We do this in a binary operator rather than
   // the destructor to ensure good debug info and backtraces for errors.
-  [[noreturn]] friend auto operator|(Helper /*unused*/, ExitingStream& rhs) {
+  [[noreturn]] friend auto operator|(Helper /*discarded*/, ExitingStream& rhs) {
     // Finish with a newline.
     llvm::errs() << "\n";
     if (rhs.treat_as_bug_) {

executable_semantics/common/error.h

@@ -33,9 +33,11 @@ class ErrorBuilder {
     return *this;
   }
 
+  // NOLINTNEXTLINE(google-explicit-constructor): Implicit cast for returns.
   operator Error() { return Error(message_); }
 
   template <typename V>
+  // NOLINTNEXTLINE(google-explicit-constructor): Implicit cast for returns.
   operator ErrorOr<V>() {
     return Error(message_);
   }
@@ -81,21 +83,22 @@ class ErrorBuilder {
 #define MAKE_UNIQUE_NAME_IMPL(a, b, c) a##b##c
 #define MAKE_UNIQUE_NAME(a, b, c) MAKE_UNIQUE_NAME_IMPL(a, b, c)
 
-#define RETURN_IF_ERROR_IMPL(unique_name, expr)       \
-  if (auto unique_name = (expr); !unique_name.ok()) { \
-    return std::move(unique_name).error();            \
+#define RETURN_IF_ERROR_IMPL(unique_name, expr)                           \
+  if (auto unique_name = (expr); /* NOLINT(bugprone-macro-parentheses) */ \
+      !(unique_name).ok()) {                                              \
+    return std::move(unique_name).error();                                \
   }
 
 #define RETURN_IF_ERROR(expr) \
   RETURN_IF_ERROR_IMPL(       \
       MAKE_UNIQUE_NAME(_llvm_error_line, __LINE__, __COUNTER__), expr)
 
-#define ASSIGN_OR_RETURN_IMPL(unique_name, var, expr) \
-  auto unique_name = (expr);                          \
-  if (!unique_name.ok()) {                            \
-    return std::move(unique_name).error();            \
-  }                                                   \
-  var = std::move(*unique_name);
+#define ASSIGN_OR_RETURN_IMPL(unique_name, var, expr)                 \
+  auto unique_name = (expr); /* NOLINT(bugprone-macro-parentheses) */ \
+  if (!(unique_name).ok()) {                                          \
+    return std::move(unique_name).error();                            \
+  }                                                                   \
+  var = std::move(*(unique_name)); /* NOLINT(bugprone-macro-parentheses) */
 
 #define ASSIGN_OR_RETURN(var, expr) \
   ASSIGN_OR_RETURN_IMPL(            \

executable_semantics/common/error_test.cpp

@@ -9,8 +9,6 @@
 namespace Carbon::Testing {
 namespace {
 
-using ::testing::Eq;
-
 auto MakeSuccess() -> ErrorOr<Success> { return Success(); }
 
 auto MakeError(std::string_view message) -> ErrorOr<Success> {

executable_semantics/interpreter/action.cpp

@@ -116,7 +116,7 @@ void Action::Print(llvm::raw_ostream& out) const {
       out << "ScopeAction";
   }
   out << "<" << pos_ << ">";
-  if (results_.size() > 0) {
+  if (!results_.empty()) {
     out << "(";
     llvm::ListSeparator sep;
     for (auto& result : results_) {

executable_semantics/interpreter/type_checker.cpp

@@ -1183,14 +1183,14 @@ auto TypeChecker::ExpectReturnOnAllPaths(
 // TODO: Add checking to function definitions to ensure that
 //   all deduced type parameters will be deduced.
 auto TypeChecker::DeclareFunctionDeclaration(Nonnull<FunctionDeclaration*> f,
-                                             const ImplScope& impl_scope)
+                                             const ImplScope& enclosing_scope)
     -> ErrorOr<Success> {
   if (trace_) {
     llvm::outs() << "** declaring function " << f->name() << "\n";
   }
   // Bring the deduced parameters into scope
   for (Nonnull<GenericBinding*> deduced : f->deduced_parameters()) {
-    RETURN_IF_ERROR(TypeCheckExp(&deduced->type(), impl_scope));
+    RETURN_IF_ERROR(TypeCheckExp(&deduced->type(), enclosing_scope));
     SetConstantValue(deduced, arena_->New<VariableType>(deduced));
     ASSIGN_OR_RETURN(Nonnull<const Value*> deduced_type,
                      InterpExp(&deduced->type(), arena_, trace_));
@@ -1198,12 +1198,12 @@ auto TypeChecker::DeclareFunctionDeclaration(Nonnull<FunctionDeclaration*> f,
   }
   // Type check the receiver pattern
   if (f->is_method()) {
-    RETURN_IF_ERROR(TypeCheckPattern(&f->me_pattern(), std::nullopt, impl_scope,
-                                     ValueCategory::Let));
+    RETURN_IF_ERROR(TypeCheckPattern(&f->me_pattern(), std::nullopt,
+                                     enclosing_scope, ValueCategory::Let));
   }
   // Type check the parameter pattern
   RETURN_IF_ERROR(TypeCheckPattern(&f->param_pattern(), std::nullopt,
-                                   impl_scope, ValueCategory::Let));
+                                   enclosing_scope, ValueCategory::Let));
 
   // Create the impl_bindings
   std::vector<Nonnull<const ImplBinding*>> impl_bindings;
@@ -1221,7 +1221,7 @@ auto TypeChecker::DeclareFunctionDeclaration(Nonnull<FunctionDeclaration*> f,
       return_expression.has_value()) {
     // We ignore the return value because return type expressions can't bring
     // new types into scope.
-    RETURN_IF_ERROR(TypeCheckExp(*return_expression, impl_scope));
+    RETURN_IF_ERROR(TypeCheckExp(*return_expression, enclosing_scope));
     // Should we be doing SetConstantValue instead? -Jeremy
     // And shouldn't the type of this be Type?
     ASSIGN_OR_RETURN(Nonnull<const Value*> ret_type,
@@ -1237,13 +1237,13 @@ auto TypeChecker::DeclareFunctionDeclaration(Nonnull<FunctionDeclaration*> f,
     }
     // Bring the impl bindings into scope
     ImplScope function_scope;
-    function_scope.AddParent(&impl_scope);
+    function_scope.AddParent(&enclosing_scope);
     for (Nonnull<const ImplBinding*> impl_binding : impl_bindings) {
       function_scope.Add(impl_binding->interface(),
                          *impl_binding->type_var()->constant_value(),
                          impl_binding);
     }
-    RETURN_IF_ERROR(TypeCheckStmt(*f->body(), impl_scope));
+    RETURN_IF_ERROR(TypeCheckStmt(*f->body(), enclosing_scope));
     if (!f->return_term().is_omitted()) {
       RETURN_IF_ERROR(ExpectReturnOnAllPaths(f->body(), f->source_loc()));
     }
@@ -1417,11 +1417,11 @@ auto TypeChecker::TypeCheckImplDeclaration(Nonnull<ImplDeclaration*> impl_decl,
 }
 
 auto TypeChecker::DeclareChoiceDeclaration(Nonnull<ChoiceDeclaration*> choice,
-                                           const ImplScope& impl_scope)
+                                           const ImplScope& enclosing_scope)
     -> ErrorOr<Success> {
   std::vector<NamedValue> alternatives;
   for (Nonnull<AlternativeSignature*> alternative : choice->alternatives()) {
-    RETURN_IF_ERROR(TypeCheckExp(&alternative->signature(), impl_scope));
+    RETURN_IF_ERROR(TypeCheckExp(&alternative->signature(), enclosing_scope));
     ASSIGN_OR_RETURN(auto signature,
                      InterpExp(&alternative->signature(), arena_, trace_));
     alternatives.push_back({.name = alternative->name(), .value = signature});
@@ -1432,8 +1432,8 @@ auto TypeChecker::DeclareChoiceDeclaration(Nonnull<ChoiceDeclaration*> choice,
   return Success();
 }
 
-auto TypeChecker::TypeCheckChoiceDeclaration(Nonnull<ChoiceDeclaration*> choice,
-                                             const ImplScope& impl_scope)
+auto TypeChecker::TypeCheckChoiceDeclaration(
+    Nonnull<ChoiceDeclaration*> /*choice*/, const ImplScope& /*impl_scope*/)
     -> ErrorOr<Success> {
   // Nothing to do here, but perhaps that will change in the future?
   return Success();
@@ -1504,34 +1504,35 @@ auto TypeChecker::TypeCheckDeclaration(Nonnull<Declaration*> d,
 }
 
 auto TypeChecker::DeclareDeclaration(Nonnull<Declaration*> d,
-                                     ImplScope& impl_scope)
+                                     ImplScope& enclosing_scope)
     -> ErrorOr<Success> {
   switch (d->kind()) {
     case DeclarationKind::InterfaceDeclaration: {
       auto& iface_decl = cast<InterfaceDeclaration>(*d);
-      RETURN_IF_ERROR(DeclareInterfaceDeclaration(&iface_decl, impl_scope));
+      RETURN_IF_ERROR(
+          DeclareInterfaceDeclaration(&iface_decl, enclosing_scope));
       break;
     }
     case DeclarationKind::ImplDeclaration: {
       auto& impl_decl = cast<ImplDeclaration>(*d);
-      RETURN_IF_ERROR(DeclareImplDeclaration(&impl_decl, impl_scope));
+      RETURN_IF_ERROR(DeclareImplDeclaration(&impl_decl, enclosing_scope));
       break;
     }
     case DeclarationKind::FunctionDeclaration: {
       auto& func_def = cast<FunctionDeclaration>(*d);
-      RETURN_IF_ERROR(DeclareFunctionDeclaration(&func_def, impl_scope));
+      RETURN_IF_ERROR(DeclareFunctionDeclaration(&func_def, enclosing_scope));
       break;
     }
 
     case DeclarationKind::ClassDeclaration: {
       auto& class_decl = cast<ClassDeclaration>(*d);
-      RETURN_IF_ERROR(DeclareClassDeclaration(&class_decl, impl_scope));
+      RETURN_IF_ERROR(DeclareClassDeclaration(&class_decl, enclosing_scope));
       break;
     }
 
     case DeclarationKind::ChoiceDeclaration: {
       auto& choice = cast<ChoiceDeclaration>(*d);
-      RETURN_IF_ERROR(DeclareChoiceDeclaration(&choice, impl_scope));
+      RETURN_IF_ERROR(DeclareChoiceDeclaration(&choice, enclosing_scope));
       break;
     }
 
@@ -1545,8 +1546,8 @@ auto TypeChecker::DeclareDeclaration(Nonnull<Declaration*> d,
       }
       Expression& type =
           cast<ExpressionPattern>(var.binding().type()).expression();
-      RETURN_IF_ERROR(TypeCheckPattern(&var.binding(), std::nullopt, impl_scope,
-                                       var.value_category()));
+      RETURN_IF_ERROR(TypeCheckPattern(&var.binding(), std::nullopt,
+                                       enclosing_scope, var.value_category()));
       ASSIGN_OR_RETURN(Nonnull<const Value*> declared_type,
                        InterpExp(&type, arena_, trace_));
       var.set_static_type(declared_type);

executable_semantics/interpreter/type_checker.h

@@ -139,11 +139,11 @@ class TypeChecker {
                                  Nonnull<const Value*>>& dict,
                   Nonnull<const Value*> type) -> Nonnull<const Value*>;
 
-  // Sets named_entity.constant_value() to `value`. Can be called multiple
-  // times on the same named_entity, so long as it is always called with
+  // Sets value_node.constant_value() to `value`. Can be called multiple
+  // times on the same value_node, so long as it is always called with
   // the same value.
   template <typename T>
-  void SetConstantValue(Nonnull<T*> named_entity, Nonnull<const Value*> value);
+  void SetConstantValue(Nonnull<T*> value_node, Nonnull<const Value*> value);
 
   void PrintConstants(llvm::raw_ostream& out);
 

executable_semantics/interpreter/value.cpp

@@ -266,7 +266,7 @@ void Value::Print(llvm::raw_ostream& out) const {
     case Value::Kind::FunctionType: {
       const auto& fn_type = cast<FunctionType>(*this);
       out << "fn ";
-      if (fn_type.deduced().size() > 0) {
+      if (!fn_type.deduced().empty()) {
         out << "[";
         unsigned int i = 0;
         for (Nonnull<const GenericBinding*> deduced : fn_type.deduced()) {

executable_semantics/interpreter/value.h

@@ -525,8 +525,6 @@ class Witness : public Value {
   Nonnull<const ImplDeclaration*> declaration_;
 };
 
-auto FieldTypes(const NominalClassType&) -> std::vector<NamedValue>;
-
 // A choice type.
 class ChoiceType : public Value {
  public:

toolchain/common/yaml_test_helpers.cpp

@@ -72,9 +72,9 @@ auto operator<<(std::ostream& os, const Value& v) -> std::ostream& {
   // Variant visitor that prints the value in the form of code to recreate the
   // value.
   struct Printer {
-    auto operator()(NullValue) -> void { out << "Yaml::NullValue()"; }
-    auto operator()(AliasValue) -> void { out << "Yaml::AliasValue()"; }
-    auto operator()(ErrorValue) -> void { out << "Yaml::ErrorValue()"; }
+    auto operator()(NullValue /*v*/) -> void { out << "Yaml::NullValue()"; }
+    auto operator()(AliasValue /*v*/) -> void { out << "Yaml::AliasValue()"; }
+    auto operator()(ErrorValue /*v*/) -> void { out << "Yaml::ErrorValue()"; }
     auto operator()(const ScalarValue& v) -> void { out << std::quoted(v); }
     auto operator()(const MappingValue& v) -> void {
       out << "Yaml::MappingValue{";

toolchain/common/yaml_test_helpers.h

@@ -60,10 +60,12 @@
 namespace Carbon::Testing::Yaml {
 
 struct EmptyComparable {
-  friend auto operator==(EmptyComparable, EmptyComparable) -> bool {
+  friend auto operator==(EmptyComparable /*lhs*/, EmptyComparable /*rhs*/)
+      -> bool {
     return true;
   }
-  friend auto operator!=(EmptyComparable, EmptyComparable) -> bool {
+  friend auto operator!=(EmptyComparable /*lhs*/, EmptyComparable /*rhs*/)
+      -> bool {
     return false;
   }
 };

toolchain/diagnostics/null_diagnostics.h

@@ -13,7 +13,9 @@ template <typename LocationT>
 inline auto NullDiagnosticLocationTranslator()
     -> DiagnosticLocationTranslator<LocationT>& {
   struct Translator : DiagnosticLocationTranslator<LocationT> {
-    auto GetLocation(LocationT) -> Diagnostic::Location override { return {}; }
+    auto GetLocation(LocationT /*loc*/) -> Diagnostic::Location override {
+      return {};
+    }
   };
   static auto* translator = new Translator;
   return *translator;

toolchain/lexer/numeric_literal.cpp

@@ -118,7 +118,8 @@ auto LexedNumericLiteral::Lex(llvm::StringRef source_text)
   //
   // TODO(zygoloid): Update lexical rules to specify that a numeric literal
   // cannot be immediately followed by an alphanumeric character.
-  int i = 1, n = source_text.size();
+  int i = 1;
+  int n = source_text.size();
   for (; i != n; ++i) {
     char c = source_text[i];
     if (IsAlnum(c) || c == '_') {
@@ -305,7 +306,7 @@ auto LexedNumericLiteral::Parser::GetExponent() -> llvm::APInt {
     // include a sign bit. Also make sure the exponent isn't too narrow so
     // the calculation below can't lose information through overflow.
     if (exponent.isSignBitSet() || exponent.getBitWidth() < 64) {
-      exponent = exponent.zext(std::max(64u, exponent.getBitWidth() + 1));
+      exponent = exponent.zext(std::max(64U, exponent.getBitWidth() + 1));
     }
     if (exponent_is_negative_) {
       exponent.negate();

toolchain/lexer/tokenized_buffer.cpp

@@ -106,7 +106,7 @@ class TokenizedBuffer::Lexer {
     // Consumes (and discard) a valid token to construct a result
     // indicating a token has been produced. Relies on implicit conversions.
     // NOLINTNEXTLINE(google-explicit-constructor)
-    LexResult(Token) : LexResult(true) {}
+    LexResult(Token /*discarded_token*/) : LexResult(true) {}
 
     // Returns a result indicating no token was produced.
     static auto NoMatch() -> LexResult { return LexResult(false); }

toolchain/lexer/tokenized_buffer.h

@@ -382,7 +382,7 @@ class TokenizedBuffer {
 
     // Map the given position within the source buffer into a diagnostic
     // location.
-    auto GetLocation(const char* pos) -> Diagnostic::Location override;
+    auto GetLocation(const char* loc) -> Diagnostic::Location override;
 
    private:
     TokenizedBuffer* buffer_;
@@ -396,7 +396,7 @@ class TokenizedBuffer {
   struct PrintWidths {
     // Widens `this` to the maximum of `this` and `new_width` for each
     // dimension.
-    auto Widen(const PrintWidths& new_width) -> void;
+    auto Widen(const PrintWidths& widths) -> void;
 
     int index;
     int kind;

toolchain/lexer/tokenized_buffer_test.cpp

@@ -919,7 +919,7 @@ TEST_F(LexerTest, TypeLiterals) {
   auto token_i20 = buffer.tokens().begin() + 2;
   EXPECT_EQ(buffer.GetTypeLiteralSize(*token_i20), 20);
   auto token_i999999999999 = buffer.tokens().begin() + 3;
-  EXPECT_EQ(buffer.GetTypeLiteralSize(*token_i999999999999), 999999999999ull);
+  EXPECT_EQ(buffer.GetTypeLiteralSize(*token_i999999999999), 999999999999ULL);
   auto token_u1 = buffer.tokens().begin() + 6;
   EXPECT_EQ(buffer.GetTypeLiteralSize(*token_u1), 1);
   auto token_u64 = buffer.tokens().begin() + 7;

toolchain/parser/parse_test_helpers.h

@@ -289,8 +289,10 @@ auto MatchNode(Args... args) -> ExpectedNode {
     void UpdateExpectationsForArg(std::string text) {
       expected.text = std::move(text);
     }
-    void UpdateExpectationsForArg(HasErrorTag) { expected.has_error = true; }
-    void UpdateExpectationsForArg(AnyChildrenTag) {
+    void UpdateExpectationsForArg(HasErrorTag /*tag*/) {
+      expected.has_error = true;
+    }
+    void UpdateExpectationsForArg(AnyChildrenTag /*tag*/) {
       expected.skip_subtree = true;
     }
     void UpdateExpectationsForArg(ExpectedNode node) {

toolchain/parser/parser_impl.h

@@ -20,7 +20,7 @@ class ParseTree::Parser {
   // Parses the tokens into a parse tree, emitting any errors encountered.
   //
   // This is the entry point to the parser implementation.
-  static auto Parse(TokenizedBuffer& tokens, TokenDiagnosticEmitter& de)
+  static auto Parse(TokenizedBuffer& tokens, TokenDiagnosticEmitter& emitter)
       -> ParseTree;
 
  private: