[clang-tidy] Fix DanglingHandleCheck to work in C++17 and later mode

[loskutov]

Summary

Due to guaranteed copy elision, not only do some nodes get removed from the AST,
but also some existing nodes change the source locations they correspond to.
Hence, the check works slightly differently in pre-C++17 and C++17-and-later modes
in terms of what gets highlighted.

[loskutov]

git clang-format

[PiotrZSL]

Overall looks to be fine, but I didn't get too deep into AST matchers.
I assume that tests cover them.
Please wait like 2 weeks before pushing this, so someone else could be able to verify this if they need.

[loskutov]

In D158371#4619348, @PiotrZSL wrote:

Please wait like 2 weeks before pushing this, so someone else could be able to verify this if they need.

It's been 2 weeks, so I guess nothing prevents from getting this change merged. However, I don't have push access to the repo, so could you commit the change?

[PiotrZSL]

In D158371#4640664, @loskutov wrote:

However, I don't have push access to the repo, so could you commit the change?

Name / Email (to be used as author) ?

[loskutov]

In D158371#4642758, @PiotrZSL wrote:

Name / Email (to be used as author) ?

Ignat Loskutov <[e-mail]>

[PiotrZSL]

Commited in f2e5000937235aa35a9ee4423045b265c2c79e85

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

return expr(
                               on(Arg))));
 }
 
 ast_matchers::internal::Matcher<Stmt> handleFromTemporaryValue(
     const ast_matchers::internal::Matcher<RecordDecl> &IsAHandle) {
   // If a ternary operator returns a temporary value, then both branches hold a
   // temporary value. If one of them is not a temporary then it must be copied
   // into one to satisfy the type of the operator.
-  const auto TemporaryTernary =
-      conditionalOperator(hasTrueExpression(cxxBindTemporaryExpr()),
-                          hasFalseExpression(cxxBindTemporaryExpr()));
+  const auto TemporaryTernary = conditionalOperator(
+      hasTrueExpression(ignoringParenImpCasts(cxxBindTemporaryExpr())),
+      hasFalseExpression(ignoringParenImpCasts(cxxBindTemporaryExpr())));
 
   return handleFrom(IsAHandle, anyOf(cxxBindTemporaryExpr(), TemporaryTernary));
 }
 
 ast_matchers::internal::Matcher<RecordDecl> isASequence() {
   return hasAnyName("::std::deque", "::std::forward_list", "::std::list",
                     "::std::vector");
 }

 void DanglingHandleCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "HandleClasses",
                 utils::options::serializeStringList(HandleClasses));
 }
 
 void DanglingHandleCheck::registerMatchersForVariables(MatchFinder *Finder) {
   const auto ConvertedHandle = handleFromTemporaryValue(IsAHandle);
 
-  // Find 'Handle foo(ReturnsAValue());'
+  // Find 'Handle foo(ReturnsAValue());', 'Handle foo = ReturnsAValue();'
   Finder->addMatcher(
       varDecl(hasType(hasUnqualifiedDesugaredType(
                   recordType(hasDeclaration(cxxRecordDecl(IsAHandle))))),
+              unless(parmVarDecl()),
               hasInitializer(
-                  exprWithCleanups(has(ignoringParenImpCasts(ConvertedHandle)))
+                  exprWithCleanups(ignoringElidableConstructorCall(has(
+                                       ignoringParenImpCasts(ConvertedHandle))))
                       .bind("bad_stmt"))),
       this);
 
-  // Find 'Handle foo = ReturnsAValue();'
-  Finder->addMatcher(
-      traverse(TK_AsIs,
-               varDecl(hasType(hasUnqualifiedDesugaredType(recordType(
-                           hasDeclaration(cxxRecordDecl(IsAHandle))))),
-                       unless(parmVarDecl()),
-                       hasInitializer(exprWithCleanups(
-                                          has(ignoringParenImpCasts(handleFrom(
-                                              IsAHandle, ConvertedHandle))))
-                                          .bind("bad_stmt")))),
-      this);
   // Find 'foo = ReturnsAValue();  // foo is Handle'
   Finder->addMatcher(
       traverse(TK_AsIs,
                cxxOperatorCallExpr(callee(cxxMethodDecl(ofClass(IsAHandle))),
                                    hasOverloadedOperatorName("="),
                                    hasArgument(1, ConvertedHandle))
                    .bind("bad_stmt")),
       this);
 
   // Container insertions that will dangle.
   Finder->addMatcher(
       traverse(TK_AsIs, makeContainerMatcher(IsAHandle).bind("bad_stmt")),
       this);
 }
 
 void DanglingHandleCheck::registerMatchersForReturn(MatchFinder *Finder) {
   // Return a local.
   Finder->addMatcher(
-      traverse(
-          TK_AsIs,
+      traverse(TK_AsIs,
                returnStmt(
                    // The AST contains two constructor calls:
                    //   1. Value to Handle conversion.
-              //   2. Handle copy construction.
+                   //   2. Handle copy construction (elided in C++17+).
                    // We have to match both.
-              has(ignoringImplicit(handleFrom(
+                   has(ignoringImplicit(ignoringElidableConstructorCall(
+                       ignoringImplicit(handleFrom(
                            IsAHandle,
-                  handleFrom(IsAHandle,
-                             declRefExpr(to(varDecl(
-                                 // Is function scope ...
-                                 hasAutomaticStorageDuration(),
-                                 // ... and it is a local array or Value.
-                                 anyOf(hasType(arrayType()),
-                                       hasType(hasUnqualifiedDesugaredType(
-                                           recordType(hasDeclaration(recordDecl(
-                                               unless(IsAHandle)))))))))))))),
+                                             unless(IsAHandle))))))))))))))),
                    // Temporary fix for false positives inside lambdas.
                    unless(hasAncestor(lambdaExpr())))
                    .bind("bad_stmt")),
       this);
 
   // Return a temporary.
   Finder->addMatcher(
-      traverse(
-          TK_AsIs,
-          returnStmt(has(exprWithCleanups(has(ignoringParenImpCasts(handleFrom(
-                         IsAHandle, handleFromTemporaryValue(IsAHandle)))))))
+      traverse(TK_AsIs,
+               returnStmt(has(exprWithCleanups(ignoringElidableConstructorCall(
+                              has(ignoringParenImpCasts(
+                                  handleFromTemporaryValue(IsAHandle)))))))
                    .bind("bad_stmt")),
       this);
 }
 
 void DanglingHandleCheck::registerMatchers(MatchFinder *Finder) {
   registerMatchersForVariables(Finder);
   registerMatchersForReturn(Finder);
 }
 
 void DanglingHandleCheck::check(const MatchFinder::MatchResult &Result) {
   auto *Handle = Result.Nodes.getNodeAs<CXXRecordDecl>("handle");
   diag(Result.Nodes.getNodeAs<Stmt>("bad_stmt")->getBeginLoc(),
        "%0 outlives its value")
       << Handle->getQualifiedNameAsString();
 }
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp

-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-dangling-handle %t -- \
+// RUN: %check_clang_tidy -std=c++11,c++14 -check-suffix=,CXX14 %s bugprone-dangling-handle %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN:             {bugprone-dangling-handle.HandleClasses: \
+// RUN:               'std::basic_string_view; ::llvm::StringRef;'}}"
+
+// RUN: %check_clang_tidy -std=c++17-or-later -check-suffix=,CXX17 %s bugprone-dangling-handle %t -- \
 // RUN:   -config="{CheckOptions: \
 // RUN:             {bugprone-dangling-handle.HandleClasses: \
 // RUN:               'std::basic_string_view; ::llvm::StringRef;'}}"
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 namespace std {
 
 template <typename T>
 class vector {
  public:
   using const_iterator = const T*;
   using iterator = T*;

 };
 
 }  // namespace llvm
 
 std::string ReturnsAString();
 
 void Positives() {
   std::string_view view1 = std::string();
-  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
+  // CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
+  // CHECK-MESSAGES-CXX17: [[@LINE-2]]:28: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
 
   std::string_view view_2 = ReturnsAString();
-  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+  // CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
+  // CHECK-MESSAGES-CXX17: [[@LINE-2]]:29: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
 
   view1 = std::string();
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives
 
   const std::string& str_ref = "";
   std::string_view view3 = true ? "A" : str_ref;
-  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+  // CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+  // CHECK-MESSAGES-CXX17: [[@LINE-2]]:28: warning: std::basic_string_view outlives
   view3 = true ? "A" : str_ref;
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives
 
   std::string_view view4(ReturnsAString());
-  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+  // CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+  // CHECK-MESSAGES-CXX17: [[@LINE-2]]:26: warning: std::basic_string_view outlives
 }
 
 void OtherTypes() {
   llvm::StringRef ref = std::string();
-  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: llvm::StringRef outlives its value
+  // CHECK-MESSAGES-CXX14: [[@LINE-1]]:19: warning: llvm::StringRef outlives its value
+  // CHECK-MESSAGES-CXX17: [[@LINE-2]]:25: warning: llvm::StringRef outlives its value
 }
 
 const char static_array[] = "A";
 std::string_view ReturnStatements(int i, std::string value_arg,
                                   const std::string &ref_arg) {
   const char array[] = "A";
   const char* ptr = "A";
   std::string s;