[clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[zinovy.nis]

Summary

Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups.

Bug: https://bugs.llvm.org/show_bug.cgi?id=48008.

[njames93]

Think this has an incorrect name, seems to have something to do with bugprone-redundant-branch-condition

[Eugene.Zelenko]

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

87

Please fix warning.

[zinovy.nis]

In D91037#2381606, @njames93 wrote:

Think this has an incorrect name, seems to have something to do with bugprone-redundant-branch-condition

Oops, thanks to autofill. Fixed.

[zinovy.nis]

auto -> const auto.

[aaron.ballman]

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

85–87

Under what circumstances does getCond() return nullptr?

90

You can call ExprWithCleanupsCond->getSubExpr() to do this more cleanly -- but similar question here as above -- what circumstances lead to a null sub expression?

134–135

Same feedback here as above.

[zinovy.nis]

*iterator -> getSubExpr()

[zinovy.nis]

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

85–87

getCond() is not null, but it can be ExprWithCleanupsCond leading the original dyn_cast to crash. That what this bug is about.

90

Thanks, fixed to use getSubExpr().

[aaron.ballman]

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

85–87

That's what I figured, but you changed dyn_cast<> to be dyn_cast_or_null<> and that seems incorrect -- getCond() doesn't return null so the dyn_cast<> was correct.

[zinovy.nis]

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

85–87

dyn_cast asserts unless getCond() returns BinaryExpression, right?
But getCond() can return (and it does so in the test case) ExprWithCleanups which is not a subclass of BinaryExpression.
That's why I use _or_null version of dyn_cast.

[aaron.ballman]

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

85–87

dyn_cast asserts unless getCond() returns BinaryExpression, right?

Nope.

cast<> asserts if the cast cannot be completed or if the cast input is null.
dyn_cast<> returns null if the cast cannot be completed and asserts if the cast input is null.
There are _or_null variants of each which will accept a null input without asserting.

[zinovy.nis]

dyn_cast_or_null -> dyn_cast

[zinovy.nis]

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

85–87

Yes, you are right, I definitely need a vacation :-)
I confused dyn_cast with cast
I've uploaded a fix.

[aaron.ballman]

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

136

The old code used to assert that CondOp was a BinaryOperator but the new code means that CondOp can be null -- should you add an assert in to ensure we have a valid CondOp still or will that assert trigger on some constructs?

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1077

Can remove the newline here.

[njames93]

Taking a step back, rather than worrying about if its an ExprWithCleanups. Shouldn't we just get the condition removing all implicit nodes.

const Expr* Cond = InnerIf->getCond()->ignoreImplicit();

This has to be simpler and will likely future proof this against any other implicit nodes potentially added in future that can appear between the Condition and the binary operator.

[zinovy.nis]

Simplified to use IgnoreImplicit().

[zinovy.nis]

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

136

Reverted to cast.

[zinovy.nis]

In D91037#2387377, @njames93 wrote:

Taking a step back, rather than worrying about if its an ExprWithCleanups. Shouldn't we just get the condition removing all implicit nodes.

const Expr* Cond = InnerIf->getCond()->ignoreImplicit();

This has to be simpler and will likely future proof this against any other implicit nodes potentially added in future that can appear between the Condition and the binary operator.

Nice! Thanks. Changed to use IgnoreImplicit().

[njames93]

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1092

This line isn't really doing anything. CHECK-FIXES just runs FileCheck on the whole file after clang tidy has applied any fixes. You need to explicitly say what you are expecting it to be.

[njames93]

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

45

Just noticed, as this is ignoring Parenthesis and implicit nodes/casts, maybe we should also be ignoring those when getting the condition in the check
InnerIf->getCond()->IgnoreParenImpCasts()
I reckon if that change isnt made this could fail on

if (IsSet){
  if ((OtherCond && IsSet))
    ;
}

[aaron.ballman]

In D91037#2387377, @njames93 wrote:

Taking a step back, rather than worrying about if its an ExprWithCleanups. Shouldn't we just get the condition removing all implicit nodes.

const Expr* Cond = InnerIf->getCond()->ignoreImplicit();

This has to be simpler and will likely future proof this against any other implicit nodes potentially added in future that can appear between the Condition and the binary operator.

Good call!

[zinovy.nis]

Use IgnoreParenImpCasts

[zinovy.nis]

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

45

You are right, thanks! I fixed it.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1092

There's not a whole lot of context for FileCheck to determine if it's been correctly applied or not (same below) -- for instance, won't this pass even if no changes are applied because FileCheck is still going to find isSet in the file?

[zinovy.nis]

Fixed fix-it section.

[zinovy.nis]

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1092

Thanks. Fixed.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1092

Maybe it's just early in the morning for me, but... I was expecting the transformation to be:

if (RetT::Test(isSet).Ok() && isSet) {
  if (RetT::Test(isSet).Ok() && isSet) {
  }
}

turns into

if (RetT::Test(isSet).Ok() && isSet) {
}

Why does it remove the && isSet instead? That seems like it's changing the logic here from if (true && false) to if (true).

[zinovy.nis]

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1092

IMO it's correct.
isSet cannot change its value between ifs while RetT::Test(isSet).Ok() can.
So we don't need to re-evaluate isSet and need to re-evaluate RetT::Test(isSet).Ok() only.

That seems like it's changing the logic here from if (true && false) to if (true).

As I understand only the second if is transformed.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1092

Does this only trigger as a redundant branch condition if the definition of RetT::Test() is available? Because Test() takes a bool& so it sure seems like isSet could be modified between the branches if the definition isn't found because it's being passed as a non-const reference to Test().

[zinovy.nis]

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1092

if the definition of RetT::Test() is available?

Removing the body from RetT::Test changes nothing.

2. Turning RetT Test(bool &_isSet) -> RetT Test(bool _isSet) also changes nothing.

[aaron.ballman]

LGTM!

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1092

Given the following four ways of declaring Test():

static RetT Test(bool &_isSet); // #1
static RetT Test(bool _isSet); // #2
static RetT Test(const bool &_isSet); // #3
static RetT Test(bool &_isSet) { return 0; } // #4

I would expect #2 and #3 to behave the same way -- the isSet argument could never be modified by calling Test() and so the second Test(isSet) && isSet is partially redundant and the second if statement can drop the && isSet. (Without dropping the Test(isSet) because the call could still modify some global somewhere, etc.)

I would expect #1 to not modify the second if statement at all because there's no way of knowing whether Test(isSet) && isSet is the same between the first if statement and the second one (because the second call to Test(isSet) may modify isSet in a way the caller can observe).

Ideally, #4 would be a case where we could remove the entire second if statement because we can identify that not only does isSet not get modified despite being passed by non-const reference, we can see that the Test() function doesn't modify any state at all. However, this seems like it would require data flow analysis and so I think it makes sense to treat #4 the same as #1.

That said, I just realized the check isn't being very careful with reference parameters in the first place: https://godbolt.org/z/P1aP3W, so your changes aren't introducing a new problem, they just happened to highlight an existing issue.

[zinovy.nis]

In D91037#2391915, @aaron.ballman wrote:

LGTM!

Thanks! I'll see what can be done to deal with it in a separate commit.

[zinovy.nis]

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

1092

Please look at https://reviews.llvm.org/D91495 - there's a fix for it.

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

 
 void RedundantBranchConditionCheck::registerMatchers(MatchFinder *Finder) {
   const auto ImmutableVar =
       varDecl(anyOf(parmVarDecl(), hasLocalStorage()), hasType(isInteger()),
               unless(hasType(isVolatileQualified())))
           .bind(CondVarStr);
   Finder->addMatcher(
       ifStmt(
           hasCondition(ignoringParenImpCasts(anyOf(
               declRefExpr(hasDeclaration(ImmutableVar)),
               binaryOperator(hasOperatorName("&&"),
                              hasEitherOperand(ignoringParenImpCasts(declRefExpr(
                                  hasDeclaration(ImmutableVar)))))))),
           hasThen(hasDescendant(
               ifStmt(hasCondition(ignoringParenImpCasts(
                          anyOf(declRefExpr(hasDeclaration(
                                    varDecl(equalsBoundNode(CondVarStr)))),
void RedundantBranchConditionCheck::check(const MatchFinder::MatchResult &Result) {
 
   if (isChangedBefore(OuterIf->getThen(), InnerIf, CondVar, Result.Context))
     return;
 
   auto Diag = diag(InnerIf->getBeginLoc(), "redundant condition %0") << CondVar;
 
   // For standalone condition variables and for "or" binary operations we simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast<BinaryOperator>(InnerIf->getCond());
+  const auto *BinOpCond =
+      dyn_cast<BinaryOperator>(InnerIf->getCond()->IgnoreParenImpCasts());
+
   if (isa<DeclRefExpr>(InnerIf->getCond()->IgnoreParenImpCasts()) ||
       (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
     SourceLocation IfBegin = InnerIf->getBeginLoc();
     const Stmt *Body = InnerIf->getThen();
     const Expr *OtherSide = nullptr;
     if (BinOpCond) {
       const auto *LeftDRE =
           dyn_cast<DeclRefExpr>(BinOpCond->getLHS()->IgnoreParenImpCasts());
       if (LeftDRE && LeftDRE->getDecl() == CondVar)
         OtherSide = BinOpCond->getRHS();
       else
if (isa<DeclRefExpr>(InnerIf->getCond()->IgnoreParenImpCasts()) ||
     // For comound statements also remove the right brace at the end.
     if (isa<CompoundStmt>(Body))
       Diag << FixItHint::CreateRemoval(
           CharSourceRange::getTokenRange(Body->getEndLoc(), Body->getEndLoc()));
 
     // For "and" binary operations we remove the "and" operation with the
     // condition variable from the inner if.
   } else {
-    const auto *CondOp = cast<BinaryOperator>(InnerIf->getCond());
+    const auto *CondOp =
+        cast<BinaryOperator>(InnerIf->getCond()->IgnoreParenImpCasts());
     const auto *LeftDRE =
         dyn_cast<DeclRefExpr>(CondOp->getLHS()->IgnoreParenImpCasts());
     if (LeftDRE && LeftDRE->getDecl() == CondVar) {
       SourceLocation BeforeRHS =
           CondOp->getRHS()->getBeginLoc().getLocWithOffset(-1);
       Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
           CondOp->getLHS()->getBeginLoc(), BeforeRHS));
     } else {
       SourceLocation AfterLHS =

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

if (doSomething(), onFire) {
       // FIXME: Handle comma operator
       // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
       // CHCK-FIXES-NOT: doSomething();
       scream();
     }
   }
 }
 
+// ExprWithCleanups doesn't crash
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+    RetT(const int _code) : code_(_code) {}
+    bool Ok() const { return code_ == 0; }
+    static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+    int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+    if (RetT::Test(isSet).Ok() && isSet) {
+      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+      // CHECK-FIXES: {{isSet}}
+    }
+  }
+  if (isSet) {
+    if ((RetT::Test(isSet).Ok() && isSet)) {
+      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+      // CHECK-FIXES: {{isSet}}
+    }
+  }
+  return 0;
+}
+
 //===--- Special Negatives ------------------------------------------------===//
 
 // Aliasing
 
 void negative_mutated_by_ptr() {
   bool onFire = isBurning();
   bool *firePtr = &onFire;
   if (onFire) {