[clang-tidy] Don't treat invalid branches as identical

[ishaangandhi]

Summary

The clang-tidy check bugprone-branch-clone has a false positive if some symbols are undefined. This patch silences the warning when the two sides of a branch are invalid.

See //github.com/llvm/llvm-project/issues/56057 for the original issue.

[ishaangandhi]

Rebased

[ishaangandhi]

Move test to new file with -fix-errors flag

[ishaangandhi]

Check for error messages in test case

[ishaangandhi]

Include error: in error line

[Eugene.Zelenko]

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

26

auto could be used because type is spelled in same statement.

27

Ditto.

clang-tools-extra/docs/ReleaseNotes.rst

236

Documentation path was changed recently. Please also keep alphabetical order inside section.

[njames93]

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

26

No need for dyn_cast here, we already know it's an Expr so cast would be sufficient.

28

Surely we can bail if only one of the sides contains an errors.

[ishaangandhi]

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

28

Makes no difference, since when only one of the side has errors, the two profile hashes will be different.

clang-tools-extra/docs/ReleaseNotes.rst

236

Hm? Its still at this path on mastesr: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/docs/ReleaseNotes.rst

[ishaangandhi]

clang-tools-extra/docs/ReleaseNotes.rst

236

Also, it doesn't look alphabetized to me? Two points up is "performance-unnecessary-..." then its "bugprone-use-after...".

[ishaangandhi]

Use auto instead of Expr and llvm::cast instead of llvm::dyn_cast.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

236

Sorry, my mistake.

236

It'll be good idea to fix order of other entries too.

[ishaangandhi]

clang-tools-extra/docs/ReleaseNotes.rst

236

Happy to! Would you like that in this diff or a new one?

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

236

This is trivial change and could be made in same commit.

[LegalizeAdulthood]

clang-tools-extra/docs/ReleaseNotes.rst

237

This link is wrong, needs to use the bugprone/ directory.

Make the target docs-clang-tools-html to verify that all your links are correct.

[ishaangandhi]

Alphabetize release notes

[ishaangandhi]

clang-tools-extra/docs/ReleaseNotes.rst

236

Done

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

236

Sorry for being unclear, but I meant alphabetical order for check names, not beginning of sentences.

[ishaangandhi]

Change doc path

[ishaangandhi]

clang-tools-extra/docs/ReleaseNotes.rst

236

This is then unclear to me. How do you alphabetize by check name?

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

236

altera-struct-pack-align, bugprone-infinite-loop, bugprone-use-after-move, etc.

[ishaangandhi]

Alphabetize some more

[LegalizeAdulthood]

clang-tools-extra/docs/ReleaseNotes.rst

208–211

I'm confused. The link text says performance-unnecessary-value-param, but the link is to readability-suspicious-call-argument and neither appears to be an alias for the other.

It appears this mistake was already there and you just alphabetized the list. I'm going to fix it in main and alphabetize the list so you can rebase and just add your changes in isolation.

[LegalizeAdulthood]

OK, I've pushed the link fix and the sorting fix to main so if you rebase again, it should just show your changes to the release notes.

[LegalizeAdulthood]

Rebase against main to get updated docs

[LegalizeAdulthood]

LGTM

[ishaangandhi]

@LegalizeAdulthood Thanks! I don't have commit rights to the repository, can you commit it on my behalf?

[ishaangandhi]

Does anybody on this thread have land permissions? If not, would anyone know who to tag?

[njames93]

clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp

2

is -fix-errors necessary here, given we aren't verifying any fixits?

[LegalizeAdulthood]

I can submit after you address additional comments by Nathan James.

[ishaangandhi]

Remove -fix-errors

[ishaangandhi]

I remember now, @njames93 :

Without the -fix-errors, the test fails as follows:

Command Output (stdout):

--

Running ['clang-tidy', '/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp', '-fix', '--checks=-*,bugprone-branch-clone', '-config={}', '--', '-std=c++11', '-nostdinc++']...

clang-tidy /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp -fix --checks=-*,bugprone-branch-clone -config={} -- -std=c++11 -nostdinc++ failed:

3 errors generated.

Error while processing /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp.

/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp:4:7: error: use of undeclared identifier 'unknown_expression_1' [clang-diagnostic-error]

  if (unknown_expression_1) {        //

      ^

/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp:5:15: error: use of undeclared identifier 'unknown_expression_2' [clang-diagnostic-error]

    function1(unknown_expression_2); //

              ^

/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp:7:15: error: use of undeclared identifier 'unknown_expression_3' [clang-diagnostic-error]

    function2(unknown_expression_3); //

              ^

Found compiler errors, but -fix-errors was not specified.

I won't update the diff, since I assume you will want to see the buildkite failure for yourself.

Once you see it, can you either confirm -fix-errors was correct originally, or instruct me on how to fix this test failure?

[njames93]

In D128402#3621002, @ishaangandhi wrote:

Once you see it, can you either confirm -fix-errors was correct originally, or instruct me on how to fix this test failure?

-expect-clang-tidy-error is the technically correct flag to use, but I'm easy either way.

[LegalizeAdulthood]

I can submit after you address additional comments by Nathan James.

[LegalizeAdulthood]

Please provide the name and email address you wish to use on the commit and I will submit.

[ishaangandhi]

Sure!

Ishaan Gandhi
ishaangandhi AT gmail DOT com

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

 #include "llvm/Support/Casting.h"
 
 using namespace clang;
 using namespace clang::ast_matchers;
 
 /// Returns true when the statements are Type I clones of each other.
 static bool areStatementsIdentical(const Stmt *LHS, const Stmt *RHS,
                                    const ASTContext &Context) {
+  if (isa<Expr>(LHS) && isa<Expr>(RHS)) {
+    // If we have errors in expressions, we will be unable
+    // to accurately profile and compute hashes for each
+    // of the left and right statements.
+    const Expr *LHSExpr = llvm::dyn_cast<Expr>(LHS);
+    const Expr *RHSExpr = llvm::dyn_cast<Expr>(RHS);
+    if (LHSExpr->containsErrors() && RHSExpr->containsErrors()) {
+      return false;
+    }
+  }
+
   llvm::FoldingSetNodeID DataLHS, DataRHS;
   LHS->Profile(DataLHS, Context, false);
   RHS->Profile(DataRHS, Context, false);
   return (DataLHS == DataRHS);
 }
 
 namespace {
 /// A branch in a switch may consist of several statements; while a branch in

clang-tools-extra/docs/ReleaseNotes.rst

- Fixed a false positive in :doc:`modernize-deprecated-headers
   files, since that header file might be used from C source files. By passing
   the ``CheckHeaderFile=true`` option if header files of the project only
   included by C++ source files.
 
 - Improved :doc:`performance-inefficient-vector-operation
   <clang-tidy/checks/performance/inefficient-vector-operation>` to work when
   the vector is a member of a structure.
 
 - Fixed a crash in :doc:`readability-const-return-type
   <clang-tidy/checks/readability/const-return-type>` when a pure virtual function
   overrided has a const return type. Removed the fix for a virtual function.
 
 - Fixed incorrect suggestions for :doc:`readability-container-size-empty
   <clang-tidy/checks/readability/container-size-empty>` when smart pointers are involved.
 
 - Fixed a false positive in :doc:`readability-non-const-parameter
   <clang-tidy/checks/readability/non-const-parameter>` when the parameter is
   referenced by an lvalue.
 
 - Expanded :doc:`readability-simplify-boolean-expr
   <clang-tidy/checks/readability/simplify-boolean-expr>` to simplify expressions
   using DeMorgan's Theorem.
 
 - Fixed a crash in :doc:`performance-unnecessary-value-param
   <clang-tidy/checks/readability/suspicious-call-argument>` when the specialization
   template has an unnecessary value parameter. Removed the fix for a template.
 
 - Fixed bugs in :doc:`bugprone-use-after-move
   <clang-tidy/checks/bugprone/use-after-move>`:
 
   - Treat a move in a lambda capture as happening in the function that defines
     the lambda, not within the body of the lambda (as we were previously doing
     erroneously).
 
   - Don't emit an erroneous warning on self-moves.
 
+- Fixed a false positive in :doc:`bugprone-branch-clone
+  <clang-tidy/checks/bugprone-branch-clone>` when the branches
+  involve unknown expressions.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp

+// RUN: %check_clang_tidy -fix-errors %s bugprone-branch-clone %t
+
+int test_unknown_expression() {
+  if (unknown_expression_1) {
+    function1(unknown_expression_1);
+  }
+  else {
+    function2(unknown_expression_2);
+  }
+}
+