[clang-tidy] Add check to detect dangling references in value handlers.

[sbenza]

Summary

Add check misc-dangling-handle to detect dangling references in value
handles like std::experimental::string_view.
It provides a configuration option to specify other handle types that
should also be checked.

Right now it detects:

• Construction from temporaries.
• Assignment from temporaries.
• Return statements from temporaries or locals.
• Insertion into containers from temporaries.

[Eugene.Zelenko]

Does it make D17772 obsolete?

[sbenza]

In D17811#366482, @Eugene.Zelenko wrote:

Does it make D17772 obsolete?

Yes. The other patch has already been abandoned.

[jbcoe]

I think there are some minor issues with the tests, easily fixed. I wonder if the matchers can catch things inside implementation-defined inline namespaces like std::_v1_::experimental::library_fundamentals_v1::string_view.

clang-tidy/misc/DanglingHandleCheck.cpp

57

Will this (and similar checkers) handle inline namespaces?

test/clang-tidy/misc-dangling-handle.cpp

73

What does NOLINT do here?

85

This (and other) check-messages line looks truncated.

[alexfh]

clang-tidy/misc/DanglingHandleCheck.cpp

24

A very similar code has been added recently to clang-tidy/utils/HeaderFileExtensionsUtils.*. Maybe make that code generic enough and reuse it?

82

nit: The comment can well fit on a single line without making it less readable. Also, please add a trailing period.

116

This use of internal classes doesn't look nice. Can you add a hasAnyName overload taking a set or an ArrayRef of names (or maybe a template version that'll take a range of something convertible to StringRef)?

162

nit: Please add a trailing period (and optionally an ellipsis at the start of the comment and at the end of the previous comment).

175

handleFromTemporaryValue already binds something to bad, which looks suspicious. Should we try to place all .binds() on the upper-most level, when it's possible, to make the whole picture easier to see?

test/clang-tidy/misc-dangling-handle.cpp

85

This is intentional. It makes sense to specify the whole message once and remove repeated static text from other CHECK-MESSAGES lines to make them shorter and the whole test more readable.

[sbenza]

Minor fixes

clang-tidy/misc/DanglingHandleCheck.cpp

24

It is not the same.
That one is filtering the characters using isAlphanumeric.
Type names can have non-alpha chars as part of template instantiations.

On the other hand, it is a copy of the one from FasterStringFindCheck.
We could move it to a central location.

57

Yes. hasName() and hasAnyName() have been fixed to look through inline namespaces.

116

Will do.

test/clang-tidy/misc-dangling-handle.cpp

73

Removed. Not needed here.

85

They are truncated on purpose.
We use the full message in the first one to check the message.
The rest are truncated to fit in 80 chars.

[jbcoe]

test/clang-tidy/misc-dangling-handle.cpp

87

Thanks.

[alexfh]

LG. Thank you for one more awesome check!

clang-tidy/misc/DanglingHandleCheck.cpp

26

That one is filtering the characters using isAlphanumeric.

Maybe it shouldn't ;)

Anyways, we have to converge all these, but it's not important to do it in this patch.

118

It's fine for a follow up, if it's more convenient to you.

[jbcoe]

Do you have commit access? I can apply this patch for you if not.

[alexfh]

In D17811#380124, @jbcoe wrote:

Do you have commit access? I can apply this patch for you if not.

He does have commit access.

[sbenza]

In D17811#380124, @jbcoe wrote:

Do you have commit access? I can apply this patch for you if not.

I do.
I am waiting on D18275 to fix the problem with using internal::HasNameMatcher directly.

[sbenza]

Using new public hasAnyName API.

[alexfh]

Still LG. Thank you!

[LegalizeAdulthood]

Can you add a synopsis of this new check to docs/ReleaseNotes.rst please?

[sbenza]

This is already being done in http://reviews.llvm.org/D18582

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
+  DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundings.cpp
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MisplacedWideningCastCheck.cpp

clang-tidy/misc/DanglingHandleCheck.h

+//===--- DanglingHandleCheck.h - clang-tidy----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DANGLING_HANDLE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DANGLING_HANDLE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Detect dangling references in value handlers like
+/// std::experimental::string_view.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-dangling-handle.html
+class DanglingHandleCheck : public ClangTidyCheck {
+public:
+  DanglingHandleCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  void registerMatchersForVariables(ast_matchers::MatchFinder *Finder);
+  void registerMatchersForReturn(ast_matchers::MatchFinder *Finder);
+  ast_matchers::internal::Matcher<RecordDecl> isAHandle() const;
+
+  const std::vector<std::string> HandleClasses;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DANGLING_HANDLE_H

clang-tidy/misc/DanglingHandleCheck.cpp

+//===--- DanglingHandleCheck.cpp - clang-tidy------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "DanglingHandleCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+static const char HandleClassesDelimiter[] = ";";
+
+std::vector<std::string> parseClasses(StringRef Option) {
+  SmallVector<StringRef, 4> Classes;
+  Option.split(Classes, HandleClassesDelimiter);
+  std::vector<std::string> Result;
+  for (StringRef &Class : Classes) {
+    Class = Class.trim();
+    if (!Class.empty())
+      Result.push_back(Class);
+  }
+  return Result;
+}
+
+ast_matchers::internal::BindableMatcher<Stmt> handleFrom(
+    ast_matchers::internal::Matcher<RecordDecl> IsAHandle,
+    ast_matchers::internal::Matcher<Expr> Arg) {
+  return cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
+                          hasArgument(0, Arg));
+}
+
+ast_matchers::internal::Matcher<Stmt> handleFromTemporaryValue(
+    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()));
+
+  return handleFrom(IsAHandle, anyOf(cxxBindTemporaryExpr(), TemporaryTernary))
+      .bind("bad");
+}
+
+ast_matchers::internal::Matcher<RecordDecl> isASequence() {
+  return hasAnyName("::std::deque", "::std::forward_list", "::std::list",
+                    "::std::vector");
+}
+
+ast_matchers::internal::Matcher<RecordDecl> isASet() {
+  return hasAnyName("::std::set", "::std::multiset", "::std::unordered_set",
+                    "::std::unordered_multiset");
+}
+
+ast_matchers::internal::Matcher<RecordDecl> isAMap() {
+  return hasAnyName("::std::map", "::std::multimap", "::std::unordered_map",
+                    "::std::unordered_multimap");
+}
+
+ast_matchers::internal::Matcher<Stmt>
+makeContainerMatcher(ast_matchers::internal::Matcher<RecordDecl> IsAHandle) {
+  // This matcher could be expanded to detect:
+  //  - Constructors: eg. vector<string_view>(3, string("A"));
+  //  - emplace*(): This requires a different logic to determine that
+  //                the conversion will happen inside the container.
+  //  - map's insert: This requires detecting that the pair conversion triggers
+  //                  the bug. A little more complicated than what we have now.
+  return callExpr(
+      hasAnyArgument(handleFromTemporaryValue(IsAHandle)),
+      anyOf(
+          // For sequences:
+          // assign, push_back, resize
+          cxxMemberCallExpr(
+              callee(functionDecl(hasAnyName("assign", "push_back", "resize"))),
+              on(expr(hasType(recordDecl(isASequence()))))),
+          // For sequences and sets:
+          // insert
+          cxxMemberCallExpr(
+              callee(functionDecl(hasName("insert"))),
+              on(expr(hasType(recordDecl(anyOf(isASequence(), isASet())))))),
+          // For maps:
+          // operator[]
+          cxxOperatorCallExpr(callee(cxxMethodDecl(ofClass(isAMap()))),
+                              hasOverloadedOperatorName("[]"))));
+}
+
+}  // anonymous namespace
+
+DanglingHandleCheck::DanglingHandleCheck(StringRef Name,
+                                         ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      HandleClasses(parseClasses(Options.get(
+          "HandleClasses",
+          "std::basic_string_view;std::experimental::basic_string_view"))) {}
+
+void DanglingHandleCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "HandleClasses",
+                llvm::join(HandleClasses.begin(), HandleClasses.end(),
+                           HandleClassesDelimiter));
+}
+
+ast_matchers::internal::Matcher<RecordDecl>
+DanglingHandleCheck::isAHandle() const {
+  return cxxRecordDecl(internal::Matcher<NamedDecl>(
+                           new internal::HasNameMatcher(HandleClasses)))
+      .bind("handle");
+}
+
+void DanglingHandleCheck::registerMatchersForVariables(MatchFinder* Finder) {
+  const auto IsAHandle = isAHandle();
+  const auto ConvertedHandle = handleFromTemporaryValue(IsAHandle);
+
+  // Find 'Handle foo(ReturnsAValue());'
+  Finder->addMatcher(
+      varDecl(hasType(cxxRecordDecl(IsAHandle)),
+              hasInitializer(exprWithCleanups(has(ConvertedHandle)))),
+      this);
+
+  // Find 'Handle foo = ReturnsAValue();'
+  Finder->addMatcher(varDecl(hasType(cxxRecordDecl(IsAHandle)),
+                             unless(parmVarDecl()),
+                             hasInitializer(exprWithCleanups(
+                                 has(handleFrom(IsAHandle, ConvertedHandle))))),
+                     this);
+  // Find 'foo = ReturnsAValue();  // foo is Handle'
+  Finder->addMatcher(
+      cxxOperatorCallExpr(callee(cxxMethodDecl(ofClass(IsAHandle))),
+                          hasOverloadedOperatorName("="),
+                          hasArgument(1, ConvertedHandle)),
+      this);
+
+  // Container insertions that will dangle.
+  Finder->addMatcher(makeContainerMatcher(IsAHandle), this);
+}
+
+void DanglingHandleCheck::registerMatchersForReturn(MatchFinder* Finder) {
+  const auto IsAHandle = isAHandle();
+
+  // Return a local.
+  Finder->addMatcher(
+      returnStmt(
+          // The AST contains two constructor calls:
+          //   1. Value to Handle conversion.
+          //   2. Handle copy construction.
+          // We have to match both.
+          has(handleFrom(
+              IsAHandle,
+              handleFrom(IsAHandle, declRefExpr(to(varDecl(
+                                        // Is function scope
+                                        hasAutomaticStorageDuration(),
+                                        // and it is a local array or Value
+                                        anyOf(hasType(arrayType()),
+                                              hasType(recordDecl(
+                                                  unless(IsAHandle)))))))))),
+          // Temporary fix for false positives inside lambdas.
+          unless(hasAncestor(lambdaExpr())))
+          .bind("bad"),
+      this);
+
+  // Return a temporary.
+  Finder->addMatcher(
+      returnStmt(has(exprWithCleanups(has(handleFrom(
+                     IsAHandle, handleFromTemporaryValue(IsAHandle))))))
+          .bind("bad"),
+      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")->getLocStart(),
+       "%0 outlives its value")
+      << Handle->getQualifiedNameAsString();
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "AssignOperatorSignatureCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
+#include "DanglingHandleCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
     CheckFactories.registerCheck<AssignOperatorSignatureCheck>(
         "misc-assign-operator-signature");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
+    CheckFactories.registerCheck<DanglingHandleCheck>(
+        "misc-dangling-handle");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "misc-inaccurate-erase");
     CheckFactories.registerCheck<IncorrectRoundings>(
         "misc-incorrect-roundings");
     CheckFactories.registerCheck<InefficientAlgorithmCheck>(
         "misc-inefficient-algorithm");

docs/clang-tidy/checks/list.rst

.. toctree::
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-argument-comment
    misc-assert-side-effect
    misc-assign-operator-signature
    misc-bool-pointer-implicit-conversion
+   misc-dangling-handle
    misc-definitions-in-headers
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-widening-cast
    misc-move-constructor-init

docs/clang-tidy/checks/misc-dangling-handle.rst

+.. title:: clang-tidy - misc-dangling-handle
+
+misc-dangling-handle
+====================
+
+Detect dangling references in value handlers like
+`std::experimental::string_view`.
+These dangling references can come from constructing handles from temporary
+values, where the temporary is destroyed soon after the handle is created.
+
+By default only `std::experimental::basic_string_view` is considered.
+This list can be modified by passing a ; separated list of class names using
+the HandleClasses option.
+
+Examples:
+
+.. code-block:: c++
+
+  string_view View = string();  // View will dangle.
+  string A;
+  View = A + "A";  // still dangle.
+
+  vector<string_view> V;
+  V.push_back(string());  // V[0] is dangling.
+  V.resize(3, string());  // V[1] and V[2] will also dangle.
+
+  string_view f() {
+    // All these return values will dangle.
+    return string();
+    string S;
+    return S;
+    char Array[10]{};
+    return Array;
+  }

test/clang-tidy/misc-dangling-handle.cpp

+// RUN: %check_clang_tidy %s misc-dangling-handle %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN:             [{key: misc-dangling-handle.HandleClasses, \
+// RUN:               value: 'std::basic_string_view; ::llvm::StringRef;'}]}" \
+// RUN:   -- -std=c++11
+
+namespace std {
+
+template <typename T>
+class vector {
+ public:
+  using const_iterator = const T*;
+  using iterator = T*;
+  using size_type = int;
+
+  void assign(size_type count, const T& value);
+  iterator insert(const_iterator pos, const T& value);
+  iterator insert(const_iterator pos, T&& value);
+  iterator insert(const_iterator pos, size_type count, const T& value);
+  void push_back(const T&);
+  void push_back(T&&);
+  void resize(size_type count, const T& value);
+};
+
+template <typename, typename>
+class pair {};
+
+template <typename T>
+class set {
+ public:
+  using const_iterator = const T*;
+  using iterator = T*;
+
+  std::pair<iterator, bool> insert(const T& value);
+  std::pair<iterator, bool> insert(T&& value);
+  iterator insert(const_iterator hint, const T& value);
+  iterator insert(const_iterator hint, T&& value);
+};
+
+template <typename Key, typename Value>
+class map {
+ public:
+  using value_type = pair<Key, Value>;
+  value_type& operator[](const Key& key);
+  value_type& operator[](Key&& key);
+};
+
+class basic_string {
+ public:
+  basic_string();
+  basic_string(const char*);  // NOLINT
+  ~basic_string();
+};
+
+typedef basic_string string;
+
+class basic_string_view {
+ public:
+  basic_string_view(const char*);
+  basic_string_view(const basic_string&);
+};
+
+typedef basic_string_view string_view;
+
+}  // namespace std
+
+namespace llvm {
+
+class StringRef {
+ public:
+  StringRef();
+  StringRef(const char*);  // NOLINT
+  StringRef(const std::string&);  // NOLINT
+};
+
+}  // namespace llvm
+
+std::string ReturnsAString();
+
+void Positives() {
+  std::string_view view1 = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: std::basic_string_view outlives its value [misc-dangling-handle]
+
+  std::string_view view_2 = ReturnsAString();
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: std::basic_string_view outlives
+
+  view1 = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: std::basic_string_view outlives
+
+  const std::string& str_ref = "";
+  std::string_view view3 = true ? "A" : str_ref;
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: std::basic_string_view outlives
+  view3 = true ? "A" : str_ref;
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: std::basic_string_view outlives
+
+  std::string_view view4(ReturnsAString());
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+}
+
+void OtherTypes() {
+  llvm::StringRef ref = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]: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;
+  static std::string ss;
+  switch (i) {
+    // Bad cases
+    case 0:
+      return array;  // refers to local
+      // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+    case 1:
+      return s;  // refers to local
+      // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+    case 2:
+      return std::string();  // refers to temporary
+      // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+    case 3:
+      return value_arg;  // refers to by-value arg
+      // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+
+    // Ok cases
+    case 100:
+      return ss;  // refers to static
+    case 101:
+      return static_array;  // refers to static
+    case 102:
+      return ptr;  // pointer is ok
+    case 103:
+      return ref_arg;  // refers to by-ref arg
+  }
+
+  struct S {
+    std::string_view view() { return value; }
+    std::string value;
+  };
+
+  (void)[&]()->std::string_view {
+    // This should not warn. The string is bound by reference.
+    return s;
+  };
+  (void)[=]() -> std::string_view {
+    // This should not warn. The reference is valid as long as the lambda.
+    return s;
+  };
+  (void)[=]() -> std::string_view {
+    // FIXME: This one should warn. We are returning a reference to a local
+    // lambda variable.
+    std::string local;
+    return local;
+  };
+  return "";
+}
+
+void Containers() {
+  std::vector<std::string_view> v;
+  v.assign(3, std::string());
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: std::basic_string_view outlives
+  v.insert(nullptr, std::string());
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: std::basic_string_view outlives
+  v.insert(nullptr, 3, std::string());
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: std::basic_string_view outlives
+  v.push_back(std::string());
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: std::basic_string_view outlives
+  v.resize(3, std::string());
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: std::basic_string_view outlives
+
+  std::set<std::string_view> s;
+  s.insert(std::string());
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: std::basic_string_view outlives
+  s.insert(nullptr, std::string());
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: std::basic_string_view outlives
+
+  std::map<std::string_view, int> m;
+  m[std::string()];
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: std::basic_string_view outlives
+}
+
+void TakesAStringView(std::string_view);
+
+void Negatives(std::string_view default_arg = ReturnsAString()) {
+  std::string str;
+  std::string_view view = str;
+
+  TakesAStringView(std::string());
+}