Page MenuHomePhabricator

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

Authored by njames93 on Dec 29 2019, 2:35 PM.

Details

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

Diff Detail

Event Timeline

njames93 created this revision.Dec 29 2019, 2:35 PM
njames93 updated this revision to Diff 235525.Dec 29 2019, 2:40 PM

Forgot to clang-format...

Eugene.Zelenko retitled this revision from Fix ClangTidy Bug - 44229, 33203 and 32204 to [clang-tidy] Fix bug - 44229, 33203 and 32204.Dec 29 2019, 4:13 PM
Eugene.Zelenko edited reviewers, added: alexfh, hokein, aaron.ballman; removed: Restricted Project, cfe-commits.
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added a subscriber: cfe-commits.
Herald added a project: Restricted Project. · View Herald TranscriptDec 29 2019, 4:13 PM
Herald added a subscriber: xazax.hun. · View Herald Transcript

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

Jim added a subscriber: Jim.Dec 30 2019, 2:28 AM
njames93 updated this revision to Diff 235570.Dec 30 2019, 2:52 AM
njames93 retitled this revision from [clang-tidy] Fix bug - 44229, 33203 and 32204 to [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone, ReadabilityBracesAroundStatements and ReadabilityMisleadingIndentation.

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

Thank you for working on this!

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 updated this revision to Diff 235645.Dec 30 2019, 2:35 PM
njames93 retitled this revision from [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone, ReadabilityBracesAroundStatements and ReadabilityMisleadingIndentation to [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements.
njames93 edited the summary of this revision. (Show Details)

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

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 marked an inline comment as done.Dec 30 2019, 6:12 PM

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 added inline comments.Dec 30 2019, 8:03 PM
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 added inline comments.Dec 30 2019, 8:50 PM
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 added inline comments.Dec 30 2019, 9:57 PM
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 accepted this revision.Dec 30 2019, 10:00 PM

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!

This revision is now accepted and ready to land.Dec 30 2019, 10:00 PM
aaron.ballman accepted this revision.Dec 31 2019, 6:44 AM

LGTM with newlines in the test files.

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 updated this revision to Diff 235755.Jan 1 2020, 3:39 AM

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

This revision was automatically updated to reflect the committed changes.

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

I marked this patch for porting into the release.