Repository navigation
[clang-tidy] Only flag uses of moved field in bugprone-use-after-move - #236086
Open
yashaggarwal85d wants to merge 1 commit into
Open
yashaggarwal85d wants to merge 1 commit into
yashaggarwal85d wants to merge 1 commit into
Conversation
|
Hello @yashaggarwal85d 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Yash Aggarwal (yashaggarwal85d) ChangesWhen the result of Now, if the move is directly followed by a field access, later uses of other fields are not reported. Uses of the same field, method calls and uses of the whole object are still reported. Accessing another field through a second This is only done for Fixes #236015 These changes are assisted by AI. I reviewed and tested the changes myself Full diff: https://github.com/llvm/llvm-project/pull/236086.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b13367e25dcb2..ad669155765d9 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -67,7 +67,7 @@ class UseAfterMoveFinder {
UseAfterMoveFinder(ASTContext *TheContext,
llvm::ArrayRef<StringRef> InvalidationFunctions,
llvm::ArrayRef<StringRef> ReinitializationFunctions,
- const CXXRecordDecl *MovedAs);
+ const CXXRecordDecl *MovedAs, const FieldDecl *MovedField);
// Within the given code block, finds the first use of 'MovedVariable' that
// occurs after 'MovingCall' (the expression that performs the move). If a
@@ -93,6 +93,7 @@ class UseAfterMoveFinder {
llvm::ArrayRef<StringRef> InvalidationFunctions;
llvm::ArrayRef<StringRef> ReinitializationFunctions;
const CXXRecordDecl *MovedAs;
+ const FieldDecl *MovedField;
std::unique_ptr<ExprSequence> Sequence;
std::unique_ptr<StmtToBlockMap> BlockMap;
llvm::SmallPtrSet<const CFGBlock *, 8> Visited;
@@ -205,9 +206,10 @@ static StatementMatcher inDecltypeOrTemplateArg() {
UseAfterMoveFinder::UseAfterMoveFinder(
ASTContext *TheContext, llvm::ArrayRef<StringRef> InvalidationFunctions,
llvm::ArrayRef<StringRef> ReinitializationFunctions,
- const CXXRecordDecl *MovedAs)
+ const CXXRecordDecl *MovedAs, const FieldDecl *MovedField)
: Context(TheContext), InvalidationFunctions(InvalidationFunctions),
- ReinitializationFunctions(ReinitializationFunctions), MovedAs(MovedAs) {}
+ ReinitializationFunctions(ReinitializationFunctions), MovedAs(MovedAs),
+ MovedField(MovedField) {}
std::optional<UseAfterMove>
UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
@@ -424,6 +426,10 @@ void UseAfterMoveFinder::getDeclRefs(
!MovedAs->hasMemberName(Member->getMemberDecl()->getIdentifier())) {
continue;
}
+ // Only one field was moved, e.g. `std::move(a).b`.
+ if (Member && MovedField && isa<FieldDecl>(Member->getMemberDecl()) &&
+ Member->getMemberDecl() != MovedField)
+ continue;
if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block &&
(Operator || !isSpecifiedAfterMove(DeclRef->getDecl())))
// Ignore uses of a standard smart pointer or classes annotated as
@@ -433,13 +439,19 @@ void UseAfterMoveFinder::getDeclRefs(
}
};
+ // `std::move(a).b` only uses the field `b` of `a`.
+ const auto MovedFieldAccess = callExpr(
+ callee(functionDecl(hasAnyName("::std::move", "::std::forward"))),
+ hasParentIgnoringParenImpCasts(
+ memberExpr(unless(isArrow()), member(fieldDecl()))
+ .bind("member-expr")));
const auto DeclRefMatcher =
declRefExpr(hasDeclaration(equalsNode(MovedVariable)),
unless(inDecltypeOrTemplateArg()),
unless(hasParentIgnoringParenImpCasts(
memberExpr(hasDeclaration(cxxDestructorDecl())))),
- optionally(hasParentIgnoringParenImpCasts(
- memberExpr().bind("member-expr"))))
+ optionally(hasParentIgnoringParenImpCasts(anyOf(
+ memberExpr().bind("member-expr"), MovedFieldAccess))))
.bind("declref");
AddDeclRefs(match(traverse(TK_AsIs, findAll(DeclRefMatcher)), *S->getStmt(),
@@ -574,6 +586,8 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
anyOf(hasParent(DerivedToBaseCast),
hasArgument(
0, traverse(TK_AsIs, expr(hasParent(DerivedToBaseCast)))))),
+ optionally(hasParentIgnoringParenImpCasts(memberExpr(
+ unless(isArrow()), member(fieldDecl().bind("moved-field"))))),
anyOf(hasAncestor(compoundStmt(
hasParent(lambdaExpr().bind("containing-lambda")))),
hasAncestor(functionDecl(
@@ -621,6 +635,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
const auto *MoveDecl = Result.Nodes.getNodeAs<FunctionDecl>("move-decl");
const auto *ParentCast =
Result.Nodes.getNodeAs<ImplicitCastExpr>("optional-cast");
+ const auto *MovedField = Result.Nodes.getNodeAs<FieldDecl>("moved-field");
if (!MovingCall || !MovingCall->getExprLoc().isValid())
MovingCall = CallMove;
@@ -653,13 +668,18 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
const CXXRecordDecl *MovedAs =
ParentCast ? ParentCast->getType()->getAsCXXRecordDecl() : nullptr;
+ const MoveType Type = determineMoveType(MoveDecl);
+ // Other invalidation functions don't return the object itself, so a member
+ // access on their result says nothing about which part was invalidated.
+ if (Type == MoveType::Invalidation)
+ MovedField = nullptr;
for (Stmt *CodeBlock : CodeBlocks) {
UseAfterMoveFinder Finder(Result.Context, InvalidationFunctions,
- ReinitializationFunctions, MovedAs);
+ ReinitializationFunctions, MovedAs, MovedField);
if (auto Use = Finder.find(CodeBlock, MovingCall, Arg))
- emitDiagnostic(MovingCall, Arg, *Use, this, Result.Context,
- determineMoveType(MoveDecl), MoveDecl);
+ emitDiagnostic(MovingCall, Arg, *Use, this, Result.Context, Type,
+ MoveDecl);
}
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 310b084c04eb0..ab8b76d15146c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -236,6 +236,11 @@ infrastructure are described first, followed by tool-specific sections.
positives on `bsl::optional` and `bdlb::NullableValue` constructed from a
value or returned by `bsl::make_optional`.
+- Improved {doc}`bugprone-use-after-move
+ <clang-tidy/checks/bugprone/use-after-move>` check by no longer reporting
+ uses of other fields after only one field was moved or forwarded, as in
+ `std::forward<T>(t).b`.
+
- Improved {doc}`cppcoreguidelines-missing-std-forward
<clang-tidy/checks/cppcoreguidelines/missing-std-forward>` check by diagnosing
unforwarded `auto&&` parameters in C++20 abbreviated function templates.
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 1dd67941bd84c..ab251c7f2ecea 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
@@ -1998,3 +1998,75 @@ void callPartialForwardTemplate(Derived &&d) {
partialForwardTemplate<Derived>(std::forward<Derived>(d));
}
} // namespace GH63202
+
+namespace GH236015 {
+
+struct Pair {
+ std::string a;
+ std::string b;
+ void method();
+};
+
+void take(std::string, std::string);
+
+template <typename T>
+void forwardMember(T &&p) {
+ take(p.a, std::forward<T>(p).b);
+}
+
+void callForwardMember(Pair &p) {
+ forwardMember(p);
+ forwardMember(std::move(p));
+}
+
+void moveMemberOtherField(Pair p) {
+ std::string s = std::move(p).b;
+ p.a.size();
+}
+
+void moveMemberParens(Pair p) {
+ std::string s = (std::move(p)).b;
+ p.a.size();
+}
+
+void moveMemberSameField(Pair p) {
+ std::string s = std::move(p).b;
+ p.b.size();
+ // CHECK-NOTES: [[@LINE-1]]:3: warning: 'p' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:32: note: move occurred here
+}
+
+void moveMemberWholeObject(Pair p) {
+ std::string s = std::move(p).b;
+ Pair other = p;
+ // CHECK-NOTES: [[@LINE-1]]:16: warning: 'p' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:32: note: move occurred here
+}
+
+void moveMemberThenMethod(Pair p) {
+ std::string s = std::move(p).b;
+ p.method();
+ // CHECK-NOTES: [[@LINE-1]]:3: warning: 'p' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:32: note: move occurred here
+}
+
+void moveFieldsOneByOne(Pair p) {
+ std::string a = std::move(p).a;
+ std::string b = std::move(p).b;
+}
+
+void moveSameFieldTwice(Pair p) {
+ std::string a = std::move(p).a;
+ std::string b = std::move(p).a;
+ // CHECK-NOTES: [[@LINE-1]]:29: warning: 'p' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:32: note: move occurred here
+}
+
+void forwardMemberSameField(Pair &&p) {
+ std::string s = std::forward<Pair>(p).b;
+ p.b.size();
+ // CHECK-NOTES: [[@LINE-1]]:3: warning: 'p' used after it was forwarded
+ // CHECK-NOTES: [[@LINE-3]]:41: note: forward occurred here
+}
+
+} // namespace GH236015
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the result of
std::move(x)/std::forward<T>(x)is only used to access a field, likestd::forward<T>(x).b, only that field is moved. The check still treated it as a move of the wholex, so a laterx.agot flagged.Now, if the move is directly followed by a field access, later uses of other fields are not reported. Uses of the same field, method calls and uses of the whole object are still reported. Accessing another field through a second
std::move(x).aalso counts as a use of just that field, so moving fields one by one is fine.This is only done for
std::moveandstd::forward, not for customInvalidationFunctions, since those don't return the object itself.Fixes #236015
These changes are assisted by AI. I reviewed and tested the changes myself