[private][clang-tidy] New bugprone-loop-variable-mutations check

[lebedev.ri]

Summary

Not quite ready for the review.
Lacks tests, docs.

Needs PR38890 to be resolved first, to get rid of the ExprMutationAnalyzer false-positives.

Also, currently mishandles cases like

std::pair<int, int> a {0, 0};
for (a.first = 0; a.first < 10; a.first++)
  a.second = 0;

but maybe this is not ExprMutationAnalyzer's problem..

Submitting as a differential just as a backup :D

[lebedev.ri]

Rebased

clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "LambdaFunctionNameCheck.h"
+#include "LoopVariableMutationsCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "ParentVirtualCallCheck.h"
 #include "SizeofContainerCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "bugprone-inaccurate-erase");
     CheckFactories.registerCheck<IncorrectRoundingsCheck>(
         "bugprone-incorrect-roundings");
     CheckFactories.registerCheck<IntegerDivisionCheck>(
         "bugprone-integer-division");
     CheckFactories.registerCheck<LambdaFunctionNameCheck>(
         "bugprone-lambda-function-name");
+    CheckFactories.registerCheck<LoopVariableMutationsCheck>(
+        "bugprone-loop-variable-mutations");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
         "bugprone-macro-parentheses");
     CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
         "bugprone-macro-repeated-side-effects");
     CheckFactories.registerCheck<MisplacedOperatorInStrlenInAllocCheck>(
         "bugprone-misplaced-operator-in-strlen-in-alloc");
     CheckFactories.registerCheck<MisplacedWideningCastCheck>(
         "bugprone-misplaced-widening-cast");

clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundingsCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
+  LoopVariableMutationsCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp
   MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
   ParentVirtualCallCheck.cpp
   SizeofContainerCheck.cpp

clang-tidy/bugprone/LoopVariableMutationsCheck.h

+//===--- LoopVariableMutationsCheck.h - clang-tidy---------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_LOOPVARIABLEMUTATIONSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_LOOPVARIABLEMUTATIONSCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-loop-variable-mutations.html
+class LoopVariableMutationsCheck : public ClangTidyCheck {
+public:
+  LoopVariableMutationsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_LOOPVARIABLEMUTATIONSCHECK_H

clang-tidy/bugprone/LoopVariableMutationsCheck.cpp

+//===--- LoopVariableMutationsCheck.cpp - clang-tidy-----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "LoopVariableMutationsCheck.h"
+#include "../utils/ExprMutationAnalyzer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <initializer_list>
+#include <utility>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+namespace {
+
+struct LoopVariableMutations {
+  const Stmt *MutationInCond = nullptr;
+  static constexpr auto MutationInCondDsc =
+      llvm::StringLiteral("mutation in loop condition occurs here");
+  const Stmt *MutationInBody = nullptr;
+  static constexpr auto MutationInBodyDsc =
+      llvm::StringLiteral("mutation in loop body occurs here");
+  const Stmt *MutationInIncr = nullptr;
+  static constexpr auto MutationInIncrDsc =
+      llvm::StringLiteral("mutation in loop iteration expression occurs here");
+
+  bool hasMoreThanOneMutation() const {
+    // Check that more than one of these is not a nullptr.
+    const int MutationCount = int(bool(MutationInCond)) +
+                              int(bool(MutationInBody)) +
+                              int(bool(MutationInIncr));
+    return MutationCount > 1;
+  }
+
+  static constexpr auto InAllThree =
+      llvm::StringLiteral("variable is mutated in the loop condition, loop "
+                          "body and loop iteration expression");
+  static constexpr auto InCondAndBody = llvm::StringLiteral(
+      "variable is mutated in the loop condition and loop body");
+  static constexpr auto InBodyAndIncr = llvm::StringLiteral(
+      "variable is mutated in the loop body and loop iteration expression");
+  static constexpr auto InCondAndIncr =
+      llvm::StringLiteral("variable is mutated in the loop condition and loop "
+                          "iteration expression");
+
+  llvm::StringLiteral describe() const {
+    if (MutationInCond && MutationInBody && MutationInIncr)
+      return InAllThree;
+    else if (MutationInBody && MutationInIncr)
+      return InBodyAndIncr;
+    else if (MutationInCond && MutationInBody)
+      return InCondAndBody;
+    else if (MutationInCond && MutationInIncr)
+      return InCondAndIncr;
+    llvm_unreachable("are those all the combinations?");
+  }
+
+  LoopVariableMutations(const ForStmt &Loop, const VarDecl &Variable,
+                        ASTContext &Context) {
+    for (const auto &PossibleMutation :
+         std::initializer_list<std::pair<const Stmt *const, const Stmt *&>>{
+             {Loop.getCond(), MutationInCond},
+             {Loop.getBody(), MutationInBody},
+             {Loop.getInc(), MutationInIncr}}) {
+      if (PossibleMutation.first) {
+        PossibleMutation.second =
+            utils::ExprMutationAnalyzer(PossibleMutation.first, &Context)
+                .findDeclMutation(&Variable);
+      }
+    }
+  }
+};
+
+constexpr llvm::StringLiteral LoopVariableMutations::MutationInCondDsc;
+constexpr llvm::StringLiteral LoopVariableMutations::MutationInBodyDsc;
+constexpr llvm::StringLiteral LoopVariableMutations::MutationInIncrDsc;
+
+} // namespace
+
+void LoopVariableMutationsCheck::registerMatchers(MatchFinder *Finder) {
+  auto matchVarDeclRefExpr = forEachDescendant(
+      declRefExpr(hasDeclaration(varDecl().bind("variable"))));
+  Finder->addMatcher(forStmt(eachOf(hasIncrement(matchVarDeclRefExpr),
+                                    hasCondition(matchVarDeclRefExpr)))
+                         .bind("loop"),
+                     this);
+}
+
+void LoopVariableMutationsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Loop = Result.Nodes.getNodeAs<ForStmt>("loop");
+  const auto *Variable = Result.Nodes.getNodeAs<VarDecl>("variable");
+  assert(Loop && Variable && "Should have both of these.");
+
+  // Analyze the mutations of the variable in the loop.
+  LoopVariableMutations A(*Loop, *Variable, *Result.Context);
+
+  if (!A.hasMoreThanOneMutation())
+    return; // Great, only mutated in one (or zero!) place.
+
+  diag(Variable->getLocation(), A.describe(), DiagnosticIDs::Warning);
+
+  diag(Loop->getForLoc(), "loop in question", DiagnosticIDs::Note);
+
+  for (const auto &Mutation : std::initializer_list<
+           std::pair<const Stmt *const &, const llvm::StringLiteral>>{
+           {A.MutationInCond, A.MutationInCondDsc},
+           {A.MutationInBody, A.MutationInBodyDsc},
+           {A.MutationInIncr, A.MutationInIncrDsc}}) {
+    if (Mutation.first)
+      diag(Mutation.first->getBeginLoc(), Mutation.second, DiagnosticIDs::Note);
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tidy/utils/ExprMutationAnalyzer.h

 //===---------- ExprMutationAnalyzer.h - clang-tidy -----------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPRMUTATIONANALYZER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPRMUTATIONANALYZER_H
 
+#include <cassert>
 #include <type_traits>
 
 #include "clang/AST/AST.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "llvm/ADT/DenseMap.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 
 /// Analyzes whether any mutative operations are applied to an expression within
 /// a given statement.
 class ExprMutationAnalyzer {
 public:
   ExprMutationAnalyzer(const Stmt *Stm, ASTContext *Context)
-      : Stm(Stm), Context(Context) {}
+      : Stm(Stm), Context(Context) {
+    assert(Stm && "Statement must not be null");
+    assert(Context && "Context must not be null");
+  }
 
   bool isMutated(const Decl *Dec) { return findDeclMutation(Dec) != nullptr; }
   bool isMutated(const Expr *Exp) { return findMutation(Exp) != nullptr; }
   const Stmt *findMutation(const Expr *Exp);
+  const Stmt *findDeclMutation(const Decl *Dec);
 
 private:
   bool isUnevaluated(const Expr *Exp);
 
   const Stmt *findExprMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
   const Stmt *findDeclMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
-  const Stmt *findDeclMutation(const Decl *Dec);
 
   const Stmt *findDirectMutation(const Expr *Exp);
   const Stmt *findMemberMutation(const Expr *Exp);
   const Stmt *findArrayElementMutation(const Expr *Exp);
   const Stmt *findCastMutation(const Expr *Exp);
   const Stmt *findRangeLoopMutation(const Expr *Exp);
   const Stmt *findReferenceMutation(const Expr *Exp);
 

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`bugprone-loop-variable-mutations
+  <clang-tidy/checks/bugprone-loop-variable-mutations>` check.
+
+  FIXME: add release notes.
+
 - New :doc:`abseil-duration-division
   <clang-tidy/checks/abseil-duration-division>` check.
 
   Checks for uses of ``absl::Duration`` division that is done in a
   floating-point context, and recommends the use of a function that
   returns a floating-point value.
 
 - New :doc:`abseil-faster-strsplit-delimiter

docs/clang-tidy/checks/bugprone-loop-variable-mutations.rst

+.. title:: clang-tidy - bugprone-loop-variable-mutations
+
+bugprone-loop-variable-mutations
+================================
+
+FIXME: Describe what patterns does the check detect and why. Give examples.

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    abseil-duration-division
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls
-   abseil-string-find-startswith
    abseil-str-cat-append
+   abseil-string-find-startswith
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat
    android-cloexec-dup
    android-cloexec-epoll-create
    android-cloexec-epoll-create1
    android-cloexec-fopen
    android-cloexec-inotify-init
.. toctree::
    bugprone-exception-escape
    bugprone-fold-init-type
    bugprone-forward-declaration-namespace
    bugprone-forwarding-reference-overload
    bugprone-inaccurate-erase
    bugprone-incorrect-roundings
    bugprone-integer-division
    bugprone-lambda-function-name
+   bugprone-loop-variable-mutations
    bugprone-macro-parentheses
    bugprone-macro-repeated-side-effects
    bugprone-misplaced-operator-in-strlen-in-alloc
    bugprone-misplaced-widening-cast
    bugprone-move-forwarding-reference
    bugprone-multiple-statement-macro
    bugprone-parent-virtual-call
    bugprone-sizeof-container

test/clang-tidy/bugprone-loop-variable-mutations.cpp

+// RUN: %check_clang_tidy %s bugprone-loop-variable-mutations %t
+
+// FIXME: Add something that triggers the check here.
+void f();
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'f' is insufficiently awesome [bugprone-loop-variable-mutations]
+
+// FIXME: Verify the applied fix.
+//   * Make the CHECK patterns specific enough and try to make verified lines
+//     unique to avoid incorrect matches.
+//   * Use {{}} for regular expressions.
+// CHECK-FIXES: {{^}}void awesome_f();{{$}}
+
+// FIXME: Add something that doesn't trigger the check here.
+void awesome_f2();