Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+129 −30
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 50 additions & 26 deletions clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include "../utils/ExprSequence.h"
#include "../utils/Matchers.h"
#include "../utils/OptionsUtils.h"
#include <optional>

using namespace clang::ast_matchers;
Expand Down Expand Up @@ -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
Expand All @@ -71,13 +73,19 @@ 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;
};

} // namespace

static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) {
return anyOf(hasAnyName("::std::move", "::std::forward"),
matchers::matchesAnyListedName(InvalidationFunctions));
}

// Matches nodes that are
// - Part of a decltype argument or class template argument (we check this by
// seeing if they are children of a TypeLoc), or
Expand All @@ -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,
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -388,18 +397,21 @@ void UseAfterMoveFinder::getReinits(
}
}

enum class MoveType {
Move, // std::move
Forward, // std::forward
enum MoveType {
Forward = 0, // std::forward
Move = 1, // std::move
Invalidation = 2, // other
};

static MoveType determineMoveType(const FunctionDecl *FuncDecl) {
if (FuncDecl->getName() == "move")
return MoveType::Move;
if (FuncDecl->getName() == "forward")
return MoveType::Forward;
if (FuncDecl->isInStdNamespace()) {
if (FuncDecl->getName() == "move")
return MoveType::Move;
if (FuncDecl->getName() == "forward")
return MoveType::Forward;
}

llvm_unreachable("Invalid move type");
return MoveType::Invalidation;
}

static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
Expand All @@ -408,41 +420,53 @@ 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);

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() << Type;
Check->diag(MoveLoc, "%select{forward|move|invalidation}0 occurred here",
DiagnosticIDs::Note)
<< IsMove;
<< Type;
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;
<< Type;
} 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;
<< Type;
}
}

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
// try_emplace to avoid false positives as we don't track uses of
// 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))),
Comment on lines +467 to +469
Copy link
Contributor

@5chmidti 5chmidti Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the invalidated object o to detect, you are matching

o.destroy();
destroy(o);
Class::destroy(o);

but not

shredder.destroy(o);

But I see why it's not supported: is this or the first arg the things that is getting invalidated.

One solution would be to have a separate option list for either case, but that might confuse people. I think the cases you do match are the ones that are more likely to happen, so it might be fine. Just wanted to point this out if others think we should support the one that is currently not matched.

Copy link
Contributor Author

@higher-performance higher-performance Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I deliberately didn't try to generalize in this PR, since as you mentioned I expect that to be much less common, and I don't have a use case for it yet. There are other cases to consider too (like emplace), but if people want it later we can always extend it further.

unless(inDecltypeOrTemplateArg()),
unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"),
anyOf(hasAncestor(compoundStmt(
Expand Down Expand Up @@ -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));
Expand Down
7 changes: 5 additions & 2 deletions clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,10 @@ Changes in existing checks
suffix when the reason starts with the character `>` in the `CustomFunctions`
option.

- Improved :doc:`bugprone-use-after-move
<clang-tidy/checks/bugprone/use-after-move>` check by adding
`InvalidationFunctions` option to support custom invalidation functions.

- Improved :doc:`cppcoreguidelines-avoid-non-const-global-variables
<clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables>` check
by adding a new option `AllowThreadLocal` that suppresses warnings on
Expand Down
10 changes: 10 additions & 0 deletions clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,3 +253,13 @@ For example, if an additional member variable is added to ``S``, it is easy to
forget to add the reinitialization for this additional member. Instead, it is
safer to assign to the entire struct in one go, and this will also avoid the
use-after-move warning.

Options
-------

.. option:: InvalidationFunctions

A semicolon-separated list of names of functions that cause their initial
Copy link
Contributor

@5chmidti 5chmidti Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think initial->first is better in this context

Copy link
Contributor Author

@higher-performance higher-performance Dec 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deliberately avoided writing first because that felt super confusing when indices start at 0 and we might be sometimes before the first explicit argument... do you feel this is more confusing?

arguments to be invalidated (e.g., closing a handle).
For member functions, the initial argument is considered to be the implicit
object argument (`this`). Default value is an empty string.
62 changes: 60 additions & 2 deletions clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
// 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 -- \
// RUN: -config='{CheckOptions: { \
// RUN: bugprone-use-after-move.InvalidationFunctions: "::Database<>::StaticCloseConnection;Database<>::CloseConnection;FriendCloseConnection" \
// RUN: }}' -- \
// RUN: -fno-delayed-template-parsing
// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- \
// RUN: -config='{CheckOptions: { \
// RUN: bugprone-use-after-move.InvalidationFunctions: "::Database<>::StaticCloseConnection;Database<>::CloseConnection;FriendCloseConnection" \
// RUN: }}' -- \
// RUN: -fno-delayed-template-parsing

typedef decltype(nullptr) nullptr_t;

Expand Down Expand Up @@ -1645,3 +1653,53 @@ void create() {
}

} // namespace issue82023

namespace custom_invalidation
{

template<class T = int>
struct Database {
template<class...>
void CloseConnection(T = T()) {}
template<class...>
static void StaticCloseConnection(Database&, T = T()) {}
template<class...>
friend void FriendCloseConnection(Database&, T = T()) {}
void Query();
};

void Run() {
using DB = Database<>;

DB 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

DB db2;
DB::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

DB db3;
DB().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

DB 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

DB db5;
FriendCloseConnection(db5, /*disconnect timeout*/ 5);
db5.Query();
// CHECK-NOTES: [[@LINE-1]]:3: warning: 'db5' used after it was invalidated
// CHECK-NOTES: [[@LINE-3]]:3: note: invalidation occurred here
}
Copy link
Contributor

@vbvictor vbvictor Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

@higher-performance higher-performance Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@higher-performance higher-performance Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the timeout case.


} // namespace custom_invalidation