[clang-tidy] simplify-bool-expr ignores template instantiations

[njames93]

Summary

Ignore template instantiations in the matchers, Addresses readability-simplify-boolean-expr false-positive for bool from template.

[lebedev.ri]

Thank you for looking into it!

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

425

This is changing the behavior so that now it will diagnose in header files, no? Why is the correct change to replace this with unless(isInTemplateInstantiation()) instead of adding the new matcher?

[njames93]

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

425

It's changing behaviour that arguably shouldn't have been in the first place. But perhaps that change should go on a new patch or update the description of this one

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

425

I'll admit that the original code seems a bit suspect to me. I sort of wonder if it was being used to suppress diagnosing macros unless they're considered to be under the user's control. e.g., macros in headers may not be plausible to change but macros in source files are.

If changes should be made here, I don't have strong opinions on whether it requires a separate patch or can be done in this one, but I'd like to better understand why the original code was incorrect (if it is in fact incorrect).

[njames93]

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

425

So having a look in the archives shows this condition was on the first draft for this check. But I couldn't see any discussion about it.

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

425

Drat, that matches my digging too. I think the current patch should leave this requirement in place so we can get the bug fix in while we figure out what to do with this bit (assuming the bug fix doesn't rely on this change somehow). If you want to remove the main file requirement in another patch, I'd suggest adding @LegalizeAdulthood as a reviewer to see if they remember why this condition was in place and (I believe) wasn't tested.

[njames93]

Added back isExpansionInMainFile check

Should put a follow up to query removing this check.

[steveire]

This isn't needed if https://reviews.llvm.org/D80961 is merged.

[LegalizeAdulthood]

Yeah, the restriction to the header file is a bug. It was a misconception on my part when I originally wrote the matcher.

See https://bugs.llvm.org/show_bug.cgi?id=26332 which should also be fixed by the change here.

I had attempted to make progress on fixing that bug (26332) but some retooling of the testing framework is needed to validate
that the header has a fixit applied to it. I made some progress by refactoring the python script, but as usual it got bogged
down in clang review hell and I haven't had time to make any more forward progress on it.

[aaron.ballman]

In D81336#2090708, @LegalizeAdulthood wrote:

Yeah, the restriction to the header file is a bug. It was a misconception on my part when I originally wrote the matcher.

Excellent, thank you for weighing in! I'm fine with the original version of the patch going in with that modification if you'd like to go that route.

In D81336#2078669, @steveire wrote:

This isn't needed if https://reviews.llvm.org/D80961 is merged.

Alternatively, I would be fine waiting until that review is merged and making only the main file expansion fixes.

[lebedev.ri]

In D81336#2091833, @aaron.ballman wrote:

In D81336#2090708, @LegalizeAdulthood wrote:

Yeah, the restriction to the header file is a bug. It was a misconception on my part when I originally wrote the matcher.

Excellent, thank you for weighing in! I'm fine with the original version of the patch going in with that modification if you'd like to go that route.

In D81336#2078669, @steveire wrote:

This isn't needed if https://reviews.llvm.org/D80961 is merged.

Alternatively, I would be fine waiting until that review is merged and making only the main file expansion fixes.

It may be better to not wait for larger refactoring but handle known false-positive immediately.

[njames93]

Removed isExpansionInMainFile check as per original authors comment

[lebedev.ri]

LG, unless @aaron.ballman has concerns.
Thanks.

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

default:
       break;
   }
 }
 
 void SimplifyBooleanExprCheck::matchBoolCondition(MatchFinder *Finder,
                                                   bool Value,
                                                   StringRef BooleanId) {
   Finder->addMatcher(
-      ifStmt(isExpansionInMainFile(),
+      ifStmt(unless(isInTemplateInstantiation()),
              hasCondition(cxxBoolLiteral(equals(Value)).bind(BooleanId)))
           .bind(IfStmtId),
       this);
 }
 
 void SimplifyBooleanExprCheck::matchTernaryResult(MatchFinder *Finder,
                                                   bool Value,
                                                   StringRef TernaryId) {
   Finder->addMatcher(
-      conditionalOperator(isExpansionInMainFile(),
+      conditionalOperator(unless(isInTemplateInstantiation()),
                           hasTrueExpression(cxxBoolLiteral(equals(Value))),
                           hasFalseExpression(cxxBoolLiteral(equals(!Value))))
           .bind(TernaryId),
       this);
 }
 
 void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
   if (ChainedConditionalReturn)
-    Finder->addMatcher(ifStmt(isExpansionInMainFile(),
+    Finder->addMatcher(ifStmt(unless(isInTemplateInstantiation()),
                               hasThen(returnsBool(Value, ThenLiteralId)),
                               hasElse(returnsBool(!Value)))
                            .bind(Id),
                        this);
   else
-    Finder->addMatcher(ifStmt(isExpansionInMainFile(),
+    Finder->addMatcher(ifStmt(unless(isInTemplateInstantiation()),
                               unless(hasParent(ifStmt())),
                               hasThen(returnsBool(Value, ThenLiteralId)),
                               hasElse(returnsBool(!Value)))
                            .bind(Id),
                        this);
 }
 
 void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
   auto Then = anyOf(SimpleThen, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleThen)));
   auto SimpleElse =
       binaryOperator(hasOperatorName("="), hasLHS(anyOf(VarRef, MemRef)),
                      hasRHS(cxxBoolLiteral(equals(!Value))));
   auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleElse)));
   if (ChainedConditionalAssignment)
-    Finder->addMatcher(ifStmt(hasThen(Then), hasElse(Else)).bind(Id), this);
+    Finder->addMatcher(ifStmt(unless(isInTemplateInstantiation()),
+                              hasThen(Then), hasElse(Else))
+                           .bind(Id),
+                       this);
   else
-    Finder->addMatcher(
-        ifStmt(unless(hasParent(ifStmt())), hasThen(Then), hasElse(Else))
+    Finder->addMatcher(ifStmt(unless(isInTemplateInstantiation()),
+                              unless(hasParent(ifStmt())), hasThen(Then),
+                              hasElse(Else))
                            .bind(Id),
                        this);
 }
 
 void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder,
                                                           bool Value,
                                                           StringRef Id) {
   Finder->addMatcher(
       compoundStmt(
+          unless(isInTemplateInstantiation()),
           hasAnySubstatement(
               ifStmt(hasThen(returnsBool(Value)), unless(hasElse(stmt())))),
           hasAnySubstatement(returnStmt(has(ignoringParenImpCasts(
                                             cxxBoolLiteral(equals(!Value)))))
                                  .bind(CompoundReturnId)))
           .bind(Id),
       this);
 }

clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp

 bool expr_with_cleanups(A &S) {
   if (S != (A)S)
     return false;
 
   return true;
 }
 // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
 // CHECK-FIXES: S == (A)S;{{$}}
+
+template <bool B>
+void ignoreInstantiations() {
+  if (B) {
+    return;
+  } else {
+    return;
+  }
+}
+
+void instantiate() {
+  // Just make sure the check isn't fooled by template instantiations.
+  ignoreInstantiations<true>();
+  ignoreInstantiations<false>();
+}