[clang-tidy] Fix a false positive in bugprone-assignment-in-if-condition

[njames93]

Summary

Fixed a false positive where a lambda expression in the condition which contained an assignement would trigger a warning.
Fixes #56729

[dodohand]

IMO you have just introduced a bug, not fixed one.
A lambda expression in an if statement condition clause is exactly the kind of thing that this checker was designed to flag.
The BARR group coding guideline, with which this is intended to comply in spirit, makes no exception for lambda expressions. (see 8.2c of https://barrgroup.com/sites/default/files/barr_c_coding_standard_2018.pdf)

It would seem to me that rather than this change as a default behavior, it would be better if @njames93 created an option flag for this checker which enabled this exclusion as a special case. This would preserve the intent of the original check, while also allowing the use case which njames93 has in mind.

Is it possible to retroactively reject/un-commit a contribution?

[njames93]

In D132786#3755390, @dodohand wrote:

IMO you have just introduced a bug, not fixed one.
A lambda expression in an if statement condition clause is exactly the kind of thing that this checker was designed to flag.
The BARR group coding guideline, with which this is intended to comply in spirit, makes no exception for lambda expressions. (see 8.2c of https://barrgroup.com/sites/default/files/barr_c_coding_standard_2018.pdf)

It would seem to me that rather than this change as a default behavior, it would be better if @njames93 created an option flag for this checker which enabled this exclusion as a special case. This would preserve the intent of the original check, while also allowing the use case which njames93 has in mind.

Is it possible to retroactively reject/un-commit a contribution?

Granted that the coding guidelines this was intended for are designed for writing embedded C code, there was never any need to consider lambdas. As such, this change just makes the check a little bit more useful on C++ codebases.
Going by the name of this, it gives the impression that we are trying to detect bugs where an equality comparison was intended, or handle the case when an assignment isn't executed due to things like short circuiting rules. Neither of these use cases are expected when the assignment appears in the body of a lambda
I'd be happy to support this as an option though.

clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.h

 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-assignment-in-if-condition.html
 class AssignmentInIfConditionCheck : public ClangTidyCheck {
 public:
   AssignmentInIfConditionCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void report(const Expr *E);
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ASSIGNMENTINIFCONDITIONCHECK_H

clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp

 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 void AssignmentInIfConditionCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt(hasCondition(forEachDescendant(
-                         binaryOperator(isAssignmentOperator())
-                             .bind("assignment_in_if_statement")))),
-                     this);
-  Finder->addMatcher(ifStmt(hasCondition(forEachDescendant(
-                         cxxOperatorCallExpr(isAssignmentOperator())
-                             .bind("assignment_in_if_statement")))),
-                     this);
+  Finder->addMatcher(translationUnitDecl(), this);
 }
 
 void AssignmentInIfConditionCheck::check(
-    const MatchFinder::MatchResult &Result) {
-  const auto *MatchedDecl =
-      Result.Nodes.getNodeAs<clang::Stmt>("assignment_in_if_statement");
-  if (!MatchedDecl) {
-    return;
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  class Visitor : public RecursiveASTVisitor<Visitor> {
+    AssignmentInIfConditionCheck &Check;
+
+  public:
+    explicit Visitor(AssignmentInIfConditionCheck &Check) : Check(Check) {}
+    bool VisitIfStmt(IfStmt *If) {
+      class ConditionVisitor : public RecursiveASTVisitor<ConditionVisitor> {
+        AssignmentInIfConditionCheck &Check;
+
+      public:
+        explicit ConditionVisitor(AssignmentInIfConditionCheck &Check)
+            : Check(Check) {}
+
+        // Dont traverse into any lambda expressions.
+        bool TraverseLambdaExpr(LambdaExpr *, DataRecursionQueue * = nullptr) {
+          return true;
         }
+
+        bool VisitBinaryOperator(BinaryOperator *BO) {
+          if (BO->isAssignmentOp())
+            Check.report(BO);
+          return true;
+        }
+
+        bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr *OCE) {
+          if (OCE->isAssignmentOp())
+            Check.report(OCE);
+          return true;
+        }
+      };
+
+      ConditionVisitor(Check).TraverseStmt(If->getCond());
+      return true;
+    }
+  };
+  Visitor(*this).TraverseAST(*Result.Context);
+}
+
+void AssignmentInIfConditionCheck::report(const Expr *MatchedDecl) {
   diag(MatchedDecl->getBeginLoc(),
        "an assignment within an 'if' condition is bug-prone");
   diag(MatchedDecl->getBeginLoc(),
        "if it should be an assignment, move it out of the 'if' condition",
        DiagnosticIDs::Note);
   diag(MatchedDecl->getBeginLoc(),
        "if it is meant to be an equality check, change '=' to '=='",
        DiagnosticIDs::Note);
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New alias :doc:`cert-msc54-cpp
   <clang-tidy/checks/cert/msc54-cpp>` to
   :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` was added.
 
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Fixed a false positive in :doc:`bugprone-assignment-in-if-condition
+  <clang-tidy/checks/bugprone/assignment-in-if-condition>` check when there
+  was an assignement in a lambda found in the condition of an ``if``.
+
 - Improved :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` check. Partial
   support for C++14 signal handler rules was added. Bug report generation was
   improved.
 
 - Fixed a false positive in :doc:`cppcoreguidelines-pro-type-member-init
   <clang-tidy/checks/cppcoreguidelines/pro-type-member-init>` when warnings
   would be emitted for uninitialized members of an anonymous union despite

clang-tools-extra/test/clang-tidy/checkers/bugprone/assignment-in-if-condition.cpp

 }
 
 void awesome_f4(int arg) {
   int f = 3;
   if ((f == arg) || ((arg + 6 < f) && ((f == v) || (f < 8)))) {
     f = 5;
   }
 }
+
+template <typename Func> bool exec(Func F) { return F(); }
+
+void lambda_if() {
+  int X;
+  if ([&X] {
+        X = 5;
+        return true;
+      }()) {
+  }
+
+  if (exec([&] {
+        X = 5;
+        return true;
+      })) {
+  }
+}