[clang-tidy] Extend bugprone-use-after-move to allow custom invalidation functions

[higher-performance]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: None (higher-performance)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/170346.diff

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp (+44-20)
• (modified) clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h (+5-2)
• (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp (+40-2)

diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index efb5ec64689cf..404fc4de8ef3c 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -19,6 +19,7 @@
 
 #include "../utils/ExprSequence.h"
 #include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
 #include <optional>
 
 using namespace clang::ast_matchers;
@@ -48,7 +49,8 @@ struct UseAfterMove {
 /// various internal helper functions).
 class UseAfterMoveFinder {
 public:
-  UseAfterMoveFinder(ASTContext *TheContext);
+  UseAfterMoveFinder(ASTContext *TheContext,
+                     llvm::ArrayRef<StringRef> InvalidationFunctions);
 
   // Within the given code block, finds the first use of 'MovedVariable' that
   // occurs after 'MovingCall' (the expression that performs the move). If a
@@ -71,11 +73,17 @@ class UseAfterMoveFinder {
                   llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs);
 
   ASTContext *Context;
+  llvm::ArrayRef<StringRef> InvalidationFunctions;
   std::unique_ptr<ExprSequence> Sequence;
   std::unique_ptr<StmtToBlockMap> BlockMap;
   llvm::SmallPtrSet<const CFGBlock *, 8> Visited;
 };
 
+static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) {
+  return anyOf(hasAnyName("::std::move", "::std::forward"),
+               matchers::matchesAnyListedName(InvalidationFunctions));
+}
+
 } // namespace
 
 // Matches nodes that are
@@ -92,8 +100,9 @@ static StatementMatcher inDecltypeOrTemplateArg() {
                hasAncestor(expr(hasUnevaluatedContext())));
 }
 
-UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
-    : Context(TheContext) {}
+UseAfterMoveFinder::UseAfterMoveFinder(
+    ASTContext *TheContext, llvm::ArrayRef<StringRef> InvalidationFunctions)
+    : Context(TheContext), InvalidationFunctions(InvalidationFunctions) {}
 
 std::optional<UseAfterMove>
 UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
@@ -359,7 +368,7 @@ void UseAfterMoveFinder::getReinits(
                             unless(parmVarDecl(hasType(
                                 references(qualType(isConstQualified())))))),
                         unless(callee(functionDecl(
-                            hasAnyName("::std::move", "::std::forward")))))))
+                            getNameMatcher(InvalidationFunctions)))))))
           .bind("reinit");
 
   Stmts->clear();
@@ -389,8 +398,9 @@ void UseAfterMoveFinder::getReinits(
 }
 
 enum class MoveType {
-  Move,    // std::move
-  Forward, // std::forward
+  Forward,      // std::forward
+  Move,         // std::move
+  Invalidation, // other
 };
 
 static MoveType determineMoveType(const FunctionDecl *FuncDecl) {
@@ -399,7 +409,7 @@ static MoveType determineMoveType(const FunctionDecl *FuncDecl) {
   if (FuncDecl->getName() == "forward")
     return MoveType::Forward;
 
-  llvm_unreachable("Invalid move type");
+  return MoveType::Invalidation;
 }
 
 static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
@@ -408,29 +418,40 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
   const SourceLocation UseLoc = Use.DeclRef->getExprLoc();
   const SourceLocation MoveLoc = MovingCall->getExprLoc();
 
-  const bool IsMove = (Type == MoveType::Move);
+  const int Kind = static_cast<int>(Type);
 
-  Check->diag(UseLoc, "'%0' used after it was %select{forwarded|moved}1")
-      << MoveArg->getDecl()->getName() << IsMove;
-  Check->diag(MoveLoc, "%select{forward|move}0 occurred here",
+  Check->diag(UseLoc,
+              "'%0' used after it was %select{forwarded|moved|invalidated}1")
+      << MoveArg->getDecl()->getName() << Kind;
+  Check->diag(MoveLoc, "%select{forward|move|invalidation}0 occurred here",
               DiagnosticIDs::Note)
-      << IsMove;
+      << Kind;
   if (Use.EvaluationOrderUndefined) {
     Check->diag(
         UseLoc,
-        "the use and %select{forward|move}0 are unsequenced, i.e. "
+        "the use and %select{forward|move|invalidation}0 are unsequenced, i.e. "
         "there is no guarantee about the order in which they are evaluated",
         DiagnosticIDs::Note)
-        << IsMove;
+        << Kind;
   } else if (Use.UseHappensInLaterLoopIteration) {
     Check->diag(UseLoc,
                 "the use happens in a later loop iteration than the "
-                "%select{forward|move}0",
+                "%select{forward|move|invalidation}0",
                 DiagnosticIDs::Note)
-        << IsMove;
+        << Kind;
   }
 }
 
+UseAfterMoveCheck::UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      InvalidationFunctions(utils::options::parseStringList(
+          Options.get("InvalidationFunctions", ""))) {}
+
+void UseAfterMoveCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "InvalidationFunctions",
+                utils::options::serializeStringList(InvalidationFunctions));
+}
+
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
   // try_emplace is a common maybe-moving function that returns a
   // bool to tell callers whether it moved. Ignore std::move inside
@@ -438,11 +459,14 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
   // the bool.
   auto TryEmplaceMatcher =
       cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));
+  auto Arg = declRefExpr().bind("arg");
+  auto IsMemberCallee = callee(functionDecl(unless(isStaticStorageClass())));
   auto CallMoveMatcher =
-      callExpr(argumentCountIs(1),
-               callee(functionDecl(hasAnyName("::std::move", "::std::forward"))
+      callExpr(callee(functionDecl(getNameMatcher(InvalidationFunctions))
                           .bind("move-decl")),
-               hasArgument(0, declRefExpr().bind("arg")),
+               anyOf(cxxMemberCallExpr(IsMemberCallee, on(Arg)),
+                     callExpr(unless(cxxMemberCallExpr(IsMemberCallee)),
+                              hasArgument(0, Arg))),
                unless(inDecltypeOrTemplateArg()),
                unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"),
                anyOf(hasAncestor(compoundStmt(
@@ -521,7 +545,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
   }
 
   for (Stmt *CodeBlock : CodeBlocks) {
-    UseAfterMoveFinder Finder(Result.Context);
+    UseAfterMoveFinder Finder(Result.Context, InvalidationFunctions);
     if (auto Use = Finder.find(CodeBlock, MovingCall, Arg))
       emitDiagnostic(MovingCall, Arg, *Use, this, Result.Context,
                      determineMoveType(MoveDecl));
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h
index d38b29e09fa8b..1bbf5c00785ff 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h
@@ -20,13 +20,16 @@ namespace clang::tidy::bugprone {
 /// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html
 class UseAfterMoveCheck : public ClangTidyCheck {
 public:
-  UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus11;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::vector<StringRef> InvalidationFunctions;
 };
 
 } // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
index 87dfec4f68061..57920f3bd7657 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
@@ -1,5 +1,5 @@
-// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CXX11 %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
-// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CXX11 %s bugprone-use-after-move %t -- -config="{CheckOptions: {bugprone-use-after-move.InvalidationFunctions: 'Database::StaticCloseConnection;Database::CloseConnection;FriendCloseConnection'}}" -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- -config="{CheckOptions: {bugprone-use-after-move.InvalidationFunctions: 'Database::StaticCloseConnection;Database::CloseConnection;FriendCloseConnection'}}" -- -fno-delayed-template-parsing
 
 typedef decltype(nullptr) nullptr_t;
 
@@ -1645,3 +1645,41 @@ void create() {
 }
 
 } // namespace issue82023
+
+namespace custom_invalidation
+{
+
+struct Database {
+  void CloseConnection();
+  static void StaticCloseConnection(Database&);
+  friend void FriendCloseConnection(Database&);
+  void Query();
+};
+
+void Run() {
+  Database db1;
+  db1.CloseConnection();
+  db1.Query();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db1' used after it was invalidated
+  // CHECK-NOTES: [[@LINE-3]]:7: note: invalidation occurred here
+
+  Database db2;
+  Database::StaticCloseConnection(db2);
+  db2.Query();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db2' used after it was invalidated
+  // CHECK-NOTES: [[@LINE-3]]:3: note: invalidation occurred here
+
+  Database db3;
+  Database().StaticCloseConnection(db3);
+  db3.Query();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db3' used after it was invalidated
+  // CHECK-NOTES: [[@LINE-3]]:3: note: invalidation occurred here
+
+  Database db4;
+  FriendCloseConnection(db4);
+  db4.Query();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db4' used after it was invalidated
+  // CHECK-NOTES: [[@LINE-3]]:3: note: invalidation occurred here
+}
+
+}  // namespace custom_invalidation

[github-actions]

✅ With the latest revision this PR passed the C/C++ code linter.

[vbvictor]

Please add release notes and documentation for new option

[zeyi2]

But would be nice to wait for other reviewers :)

[vbvictor]

Could we add test with FriendCloseConnection(db5, /*disconnect timeout*/ 5);, I guess it should flag as invalidation too.

Also please add:

template <typename T>
void TemplateRun() {
   DB<T> db;
   // tests
}

[higher-performance]

That template wouldn't work, but it's not really supposed to either, given we don't have resolved FunctionDecls at that point to begin with. That's not part of my change, it's the underlying design and applies to the existing ::std::move cases as well. All I'm doing is adding to the list of names to check. If you feel it should be doing something, I think that should be a separate PR?

[higher-performance]

Added the timeout case.