[clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements

[njames93]

Summary

fixes readability-braces-around-statements broken for if constexpr and bugprone-branch-clone false positive with template functions and constexpr by disabling the relevant checks on if constexpr statements while inside an instantiated template. This is due to how the else branch of an if constexpr statement is folded away to a null statement if the condition evaluates to false

[njames93]

Forgot to clang-format...

[lebedev.ri]

Missing tests; please upload patches with full context (-U99999); just referencing bug# is non-informative - best to say "[clang-tidy] check-name: fix 'constexpr if' handling (PR#)"; possibly you want to split this up into per-check patches

[njames93]

In D71980#1798517, @lebedev.ri wrote:

Missing tests; please upload patches with full context (-U99999); just referencing bug# is non-informative - best to say "[clang-tidy] check-name: fix 'constexpr if' handling (PR#)"; possibly you want to split this up into per-check patches

I was thinking as these are all manifestations of the same bug they could go as one patch. As for the test cases, how do you write an effective test when you are dealing with template instantiations given that the code will effectively be checked multiple times for the definition and each instantiation

[JonasToth]

Thank you for working on this!

In D71980#1798680, @njames93 wrote:

In D71980#1798517, @lebedev.ri wrote:

Missing tests; please upload patches with full context (-U99999); just referencing bug# is non-informative - best to say "[clang-tidy] check-name: fix 'constexpr if' handling (PR#)"; possibly you want to split this up into per-check patches

I was thinking as these are all manifestations of the same bug they could go as one patch. As for the test cases, how do you write an effective test when you are dealing with template instantiations given that the code will effectively be checked multiple times for the definition and each instantiation

The bug-reports had cases that reproduced the bugs, you could use those (or minimal versions of them).
When you write the templates, you just need to ensure that they are instantiated at some point.

template <typename T>
T my_sqrt(T val) {
  // This is fake, just to make a templated condition that does not rely on the standard library or anything else (tests should not have dependencies in LLVM and we do not allow the STL in tests)
  if constexpr (sizeof(T) == 8) {
      return 42.0;
  }
  else {
    return 42.0f;
  }
}

void instantiate() {
  my_sqrt<double>(10.);
  my_sqrt<float>(10.f);
}

These should trigger the problem. A test with if constexpr () ... else if constexpr () ... would be required as well.

To test each of these checks you can use multiple RUN: lines in a single test-file.
You can see clang-tools-extra/test/clang-tidy/checkers/misc-non-private-member-variables-in-classes.cpp for a test that takes heavy advantage from that.
When writing the test please add them to the same directory (test/clang-tidy/checkers/<some-check-name>) and follow the same naming conventions as those files.

[njames93]

I can't seem to reproduce the bug for misleading-indentation so I have removed that check. Test cases have been added

[xazax.hun]

Thanks for working on this, please see one of my concerns inline.

If you are trying to fix this problem, alternatively, you could also fix it in Clang.

Richard outlined the best possible way to solve this class of errors by introducing a new type of AST node in: https://reviews.llvm.org/D46234

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

62–63

Why do we care if we are inside a template instantiation?

Couldn't we trigger the bug with something like:

void shouldPass() {
  if constexpr (constexprFun(1) == 0) {
    handle(0);
  } else if constexpr (constexprFun(1)  == 1) {
    handle(1);
  } else {
    handle(2);
  }
}

?

[njames93]

In D71980#1799437, @xazax.hun wrote:

Thanks for working on this, please see one of my concerns inline.

If you are trying to fix this problem, alternatively, you could also fix it in Clang.

Richard outlined the best possible way to solve this class of errors by introducing a new type of AST node in: https://reviews.llvm.org/D46234

I'm going to look into adding a DiscardedStmt, see how that goes. would probably be best just creating a new Stmt rather than trying to inherit from NullStmt.

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

62–63

We are disabling the bug check when we are in a template instantiation. Reason being the template instantiation folds away the false branches to null statements which is the cause of the false positives. Also if there is a bug in a templated if constexpr, it will be caught when the template is defined.

[xazax.hun]

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

62–63

Yeah, be people are free to write if constexpr outside of templates right? So instantiations are not the only places where branches can be folded away.

[njames93]

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

62–63

The issue is that only template instantiations cause the false branch to get folded away, observe here to see how its handled in templated code that isn't yet instantiated, instantiated template code and normal untemplated code

[xazax.hun]

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

62–63

Oh, I see. This is really weird. I do not really see why should the if constexpr behave differently outside of instantiations. Thanks for the clarification!

[xazax.hun]

Please add new lines to the end of the test files. Also please add a test similar to the godbolt link you gave me to show that if consexpr (false) outside of templates work as expected. Otherwise LGTM!

[aaron.ballman]

LGTM with newlines in the test files.

[njames93]

I have a version with a new DiscaredStmt working, but it's currently lacking test cases, docs and it was pretty much made as a copy paste job from NullStmt so I'm not going to hurry up about trying to polish that one. The new lines and test cases patch I'll get done probably 2nd January now the booziness is beginning

[njames93]

Is this good to land and if so anyone able to commit?

[JonasToth]

In D71980#1810086, @njames93 wrote:

Is this good to land and if so anyone able to commit?

I marked this patch for porting into the release.

[hans]

Cherry-picked to 10.x in 1f98c2b586e4ebce68d75856699059663a052cb7

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

 }
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 void BranchCloneCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
-      ifStmt(stmt().bind("if"),
+      ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())),
+             stmt().bind("if"),
              hasParent(stmt(unless(ifStmt(hasElse(equalsBoundNode("if")))))),
              hasElse(stmt().bind("else"))),
       this);
   Finder->addMatcher(switchStmt().bind("switch"), this);
   Finder->addMatcher(conditionalOperator().bind("condOp"), this);
 }
 
 void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {

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

: ClangTidyCheck(Name, Context),
       ShortStatementLines(Options.get("ShortStatementLines", 0U)) {}
 
 void BracesAroundStatementsCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ShortStatementLines", ShortStatementLines);
 }
 
 void BracesAroundStatementsCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt().bind("if"), this);
+  Finder->addMatcher(
+      ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())))
+          .bind("if"),
+      this);
   Finder->addMatcher(whileStmt().bind("while"), this);
   Finder->addMatcher(doStmt().bind("do"), this);
   Finder->addMatcher(forStmt().bind("for"), this);
   Finder->addMatcher(cxxForRangeStmt().bind("for-range"), this);
 }
 
 void BracesAroundStatementsCheck::check(
     const MatchFinder::MatchResult &Result) {

clang-tools-extra/test/clang-tidy/checkers/bugprone-branch-clone-if-constexpr-template.cpp

+// RUN: %check_clang_tidy %s bugprone-branch-clone %t -- -- -std=c++17
+
+void handle(int);
+
+template <unsigned Index>
+void shouldFail() {
+  if constexpr (Index == 0) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: repeated branch in conditional chain [bugprone-branch-clone]
+    handle(0);
+  } else if constexpr (Index == 1) {
+    handle(1);
+  } else {
+    handle(0);
+  }
+}
+
+template <unsigned Index>
+void shouldPass() {
+  if constexpr (Index == 0) {
+    handle(0);
+  } else if constexpr (Index == 1) {
+    handle(1);
+  } else {
+    handle(2);
+  }
+}
+
+void shouldFailNonTemplate() {
+  constexpr unsigned Index = 1;
+  if constexpr (Index == 0) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: repeated branch in conditional chain [bugprone-branch-clone]
+    handle(0);
+  } else if constexpr (Index == 1) {
+    handle(1);
+  } else {
+    handle(0);
+  }
+}
+
+void shouldPassNonTemplate() {
+  constexpr unsigned Index = 1;
+  if constexpr (Index == 0) {
+    handle(0);
+  } else if constexpr (Index == 1) {
+    handle(1);
+  } else {
+    handle(2);
+  }
+}
+
+void run() {
+    shouldFail<0>();
+    shouldFail<1>();
+    shouldFail<2>();
+    shouldPass<0>();
+    shouldPass<1>();
+    shouldPass<2>();
+}

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements-constexpr-if-templates.cpp

+// RUN: %check_clang_tidy %s readability-braces-around-statements %t -- -- -std=c++17
+
+void handle(bool);
+
+template <bool branch>
+void shouldFail() {
+  if constexpr (branch)
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: statement should be inside braces [readability-braces-around-statements]
+    handle(true);
+  else
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: statement should be inside braces [readability-braces-around-statements]
+    handle(false);
+}
+
+template <bool branch>
+void shouldPass() {
+  if constexpr (branch) {
+    handle(true);
+  } else {
+    handle(false);
+  }
+}
+
+void shouldFailNonTemplate() {
+  constexpr bool branch = false;
+  if constexpr (branch)
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: statement should be inside braces [readability-braces-around-statements]
+    handle(true);
+  else
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: statement should be inside braces [readability-braces-around-statements]
+    handle(false);
+}
+
+void shouldPass() {
+  constexpr bool branch = false;
+  if constexpr (branch) {
+    handle(true);
+  } else {
+    handle(false);
+  }
+}
+
+void run() {
+    shouldFail<true>();
+    shouldFail<false>();
+    shouldPass<true>();
+    shouldPass<false>();
+}