[clang-tidy] Add bugprone-move-shared-pointer-contents check. #67467

pizzud · 2023-09-26T17:48:40Z

This check detects moves of the contents of a shared pointer rather than the pointer itself. Other code with a reference to the shared pointer is probably not expecting the move.

The set of shared pointer classes is configurable via options to allow individual projects to cover additional types.

5chmidti

I noticed a problem with your matcher, so I reviewed the rest of it while I was at it.

The problem is that you do not consider a type-dependent std::shared_ptr<T> and the following test case fails:

template <typename T>
void dependentType() {
  std::shared_ptr<T> p;
  T y = std::move(*p);
  // CHECK-FIXES: *std::move(p)
}
// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents]

In this case, your callee is no longer a functionDecl but an unresolvedLookupExpr and the * operator is a UnaryOperator instead of a cxxOperatorCallExpr.

Otherwise, looks like a good check to have.

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

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h

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

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h

clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst

github-actions · 2023-12-01T23:50:38Z

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

pizzud · 2023-12-01T23:58:51Z

I noticed a problem with your matcher, so I reviewed the rest of it while I was at it.

The problem is that you do not consider a type-dependent std::shared_ptr<T> and the following test case fails:
template <typename T>
void dependentType() {
  std::shared_ptr<T> p;
  T y = std::move(*p);
  // CHECK-FIXES: *std::move(p)
}
// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents]
In this case, your callee is no longer a functionDecl but an unresolvedLookupExpr and the * operator is a UnaryOperator instead of a cxxOperatorCallExpr.

Otherwise, looks like a good check to have.

I'm really struggling with this, and also with getting the git history right after three months oops. The UnaryOperator is matchable without much difficulty, but there appears to be no way to introspect the UnresolvedLookupExpr to ensure I'm matching calls to std::move.

5chmidti · 2023-12-04T14:41:14Z

but there appears to be no way to introspect the UnresolvedLookupExpr to ensure I'm matching calls to std::move

Try searching for unresolvedLookupExpr & UnresolvedLookupExpr in the clang-tidy directory. There is a high probability that another check has done something related/similar.

llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp

Lines 46 to 47 in 2b7191c

    
           callee(unresolvedLookupExpr(hasAnyDeclaration( 
        
               namedDecl(hasUnderlyingDecl(hasName("::std::move"))))))),

And there is OverloadExpr::decls, which UnresolvedLookupExpr inherits (https://clang.llvm.org/doxygen/classclang_1_1UnresolvedLookupExpr.html).

PiotrZSL

Few fixes needed, but looks you are on good road.

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

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h

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

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

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

clang-tools-extra/docs/clang-tidy/checks/list.rst

clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp

pizzud · 2023-12-04T23:30:24Z

Try searching for unresolvedLookupExpr & UnresolvedLookupExpr in the clang-tidy directory. There is a high probability that another check has done something related/similar.

llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp

Lines 46 to 47 in 2b7191c

callee(unresolvedLookupExpr(hasAnyDeclaration(

namedDecl(hasUnderlyingDecl(hasName("::std::move"))))))),

Thanks, this was super helpful and exactly what I was looking for!

I think the missing insight was that I could go from the lookup -> decls.

PiotrZSL

Few issues with unaryOperator left, after that will be fixed, it would look fine.

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h

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

pizzud · 2024-01-05T21:55:41Z

Not sure how to squash properly; I tried rebasing but all the commits on the main branch snuck in to the history and I'm quite new to git. If it would be preferred I can store this off as a patch and open up a new PR. Sorry for all the hassle with the merges!

EugeneZelenko · 2024-01-05T22:43:04Z

You need to do interactive rebase:

git fetch --prune # get latest main and remove local copies deleted branch 
git rebase origin/main # rebase from main
git rebase -i origin/main # interactive rebase, just use fixup command
git rebase --force origin <your branch name> # push updated branch

pizzud · 2024-01-08T19:49:00Z

A coworker was able to help me get this working by rebasing against upstream, so I think we're good to go at this point. Thanks for all the feedback!

clang-tools-extra/docs/clang-tidy/checks/list.rst

clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h

clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst

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

denzor200 · 2025-01-03T22:59:36Z

Wouldn't that be a runtime check, and thus not something clang-tidy can do without significant data flow analysis effort?

I'm not experienced enough in clang-tidy internals, but I believe it will not be a serious challenge to check for having assert(sp.use_count() == 1) one line before *sp being moved.

I suppose everybody agree that this snippet

assert(sp.use_count() == 1);
auto y = std::move(*sp);

..is better than that one:

// NOLINTNEXTLINE(clang-tidy-bugprone-move-shared-pointer-contents)
auto y = std::move(*sp);

vbvictor · 2025-06-03T07:51:45Z

@pizzud, you wish to continue work on this PR? 21th release of LLVM would be in late July, we have time to prepare and merge this check before the release. Also, It's okay to not cover all cases in the first iteration, some improvements can be added in later patches.

Windows build is failing most likely to absent of "-fno-delayed-template-parsing" flag in test command, look for it in other tests for reference.

pizzud · 2025-06-13T18:43:46Z

@denzor2000 I agree it's better and technically possible within clang-tidy, but I am not sure it's simple to do. I'd like to get this in as-is.

@vbvictor
Thanks for the MSVC tip! I am still interested in this, and I will spend some time to get the git history back in shape and publish my contents.

vbvictor · 2025-06-13T19:03:04Z

I agree that modelling assert(sp.use_count() == 1); is not easy in clang-tidy itself. In one of my checks I needed to check that two declStmt are placed one after another in compoundStmt and I ended up manually traversing all stmt within compoundStmt - not a very pleasant solution.
clang static analyzer has built-in functionality for modelling assert() and other macros, but clang-tidy doesn't

clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst

vbvictor · 2025-06-13T19:07:59Z

Please ping me when you're done working and want to test CI build
I need to approve every CI build/format run... not very convenient.

…pointer-contents.rst Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>

…s and provides limited additional coverage

pizzud · 2025-06-13T20:10:35Z

Give it a go?

vbvictor · 2025-06-14T07:43:24Z

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

+    }
+  }
+
+  if (Call == nullptr && !Call->getBeginLoc().isValid()) {


Should this be Call != nullptr or simply Call &&?

Neither. It should be || instead of &&. The somewhat scary (but unsurprising part since I'm not sure how you would actually get here) is that all the unit tests passed either way.

vbvictor · 2025-06-14T07:43:45Z

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

+
+  if (Call == nullptr && !Call->getBeginLoc().isValid()) {
+    return;
+  }


Avoid using braces for single if-stmt

vbvictor · 2025-06-14T07:46:03Z

clang-tools-extra/docs/ReleaseNotes.rst

@@ -148,6 +148,12 @@ New checks
  Finds potentially erroneous calls to ``reset`` method on smart pointers when
  the pointee type also has a ``reset`` method.

+- New :doc:`bugprone-move-shared-pointer-contents


Please place in alphabetical order by check name

vbvictor

Could you please run clang-tidy over your file and fix the errors, clangd can detect them, too.

vbvictor · 2025-06-21T18:18:32Z

clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst

+   A semicolon-separated list of class names that should be treated as
+   shared pointers. Classes are resolved through aliases, so any alias
+   to the defined classes will be considered. Default is
+   `::std::shared_ptr;::boost::shared_pointer`.


Boost has ::boost::shared_ptr, not ::boost::shared_pointer

Oops, done. Fixed in the actual checker as well.

vbvictor · 2025-06-21T18:49:22Z

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

+#include "../ClangTidyCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"


Unused include?

Suggested change

#include "clang/AST/ASTContext.h"

vbvictor · 2025-06-21T18:54:34Z

I played with matchers a bit, one hasDescendant may be eliminated by:

diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp
index 4d5c25bf9f27..ab5e13fbb9cc 100644
--- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp
@@ -70,12 +70,11 @@ void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) {
           hasArgument(
               0, unaryOperator(
                      hasOperatorName("*"),
-                     hasUnaryOperand(allOf(
-                         hasDescendant(declRefExpr(hasType(qualType(
-                             isSharedPointer(matchers::matchesAnyListedName(
-                                 SharedPointerClasses)))))),
-                         cxxMemberCallExpr(
-                             callee(cxxMethodDecl(hasName("get")))))))))
+                     hasUnaryOperand(cxxMemberCallExpr(
+                         callee(cxxMethodDecl(hasName("get"))),
+                         on(hasType(qualType(isSharedPointer(
+                             matchers::matchesAnyListedName(
+                                 SharedPointerClasses))))))))))
           .bind("get_call")));

All the tests seem to pass on ubuntu, couldn't test on windows

pizzud

I think I got the clang-tidy issues sorted out.

pizzud · 2025-06-30T20:32:31Z

clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst

+   A semicolon-separated list of class names that should be treated as
+   shared pointers. Classes are resolved through aliases, so any alias
+   to the defined classes will be considered. Default is
+   `::std::shared_ptr;::boost::shared_pointer`.


Oops, done. Fixed in the actual checker as well.

pizzud · 2025-06-30T20:32:47Z

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

+#include "../ClangTidyCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"


vbvictor

Could you also try to eliminate second hasDescendant(declRefExpr()) in similar way if possible. I'd try to look at it this week too.
I suppose it's the only issue to be resolved before merge.

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

pizzud

I'll give the matcher simplification a shot and see how it goes; I have a very short working week this week so I'm unlikely to make much progress.

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

llvmbot added the clang-tidy label Sep 26, 2023

pizzud mentioned this pull request Sep 26, 2023

[clang-tidy] Add performance-move-smart-pointer-contents check. #66139

Closed

5chmidti reviewed Sep 26, 2023

View reviewed changes

EugeneZelenko requested review from PiotrZSL and carlosgalvezp September 27, 2023 05:14

EugeneZelenko reviewed Sep 27, 2023

View reviewed changes

PiotrZSL requested changes Dec 4, 2023

View reviewed changes

PiotrZSL reviewed Dec 5, 2023

View reviewed changes

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h Show resolved Hide resolved

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp Outdated Show resolved Hide resolved

EugeneZelenko reviewed Jan 3, 2024

View reviewed changes

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp Outdated Show resolved Hide resolved

pizzud force-pushed the shared-ptr branch 2 times, most recently from 97323d0 to ab88688 Compare January 8, 2024 19:44

pizzud force-pushed the shared-ptr branch from ab88688 to 8e1216f Compare January 8, 2024 20:00

PiotrZSL requested changes Jan 9, 2024

View reviewed changes

pizzud requested review from antiagainst, kuhar, hanhanW, nicolasvasilache, Groverkss, Superty, dcaballe, banach-space, JDevlieghere and Endilll as code owners January 11, 2024 20:26

Merge branch 'main' into shared-ptr

eeb16e7

Merge branch 'llvm:main' into shared-ptr

8e30a35

EugeneZelenko reviewed Jun 13, 2025

View reviewed changes

clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst Show resolved Hide resolved

pizzud and others added 3 commits June 13, 2025 13:07

Update clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-…

38fab16

…pointer-contents.rst Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>

fix whitespace and remove test case that mysteriously fails on Window…

1283932

…s and provides limited additional coverage

Add flag for Windows fixes

1b96eca

pizzud added 2 commits June 13, 2025 13:20

cleanup includes/whitespace

e7c0677

whitespace fix

cb4b4ba

vbvictor reviewed Jun 14, 2025

View reviewed changes

pizzud and others added 4 commits June 16, 2025 11:30

Merge branch 'llvm:main' into shared-ptr

8fc9c22

whitespace and formatting fixes

9ea914f

Merge branch 'llvm:main' into shared-ptr

4216b46

whitespace and formatting fixes

d8c9558

vbvictor reviewed Jun 21, 2025

View reviewed changes

pizzud and others added 2 commits June 30, 2025 13:31

Merge branch 'llvm:main' into shared-ptr

4938dd8

Fix lint errors and Boost shared_ptr name

cbe532b

pizzud commented Jun 30, 2025

View reviewed changes

vbvictor reviewed Jun 30, 2025

View reviewed changes

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp Outdated Show resolved Hide resolved

pizzud and others added 2 commits June 30, 2025 15:31

Merge branch 'llvm:main' into shared-ptr

8ea2ea1

usings and whitespace

5f7c21e

pizzud commented Jun 30, 2025

View reviewed changes

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp Outdated Show resolved Hide resolved

pizzud and others added 2 commits July 1, 2025 15:57

Merge branch 'llvm:main' into shared-ptr

18a76d3

fix using since clang-tidy should allow this

b9bfdef

[clang-tidy] Add bugprone-move-shared-pointer-contents check. #67467

Are you sure you want to change the base?

[clang-tidy] Add bugprone-move-shared-pointer-contents check. #67467

Uh oh!

Conversation

pizzud commented Sep 26, 2023

Uh oh!

5chmidti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pizzud commented Dec 1, 2023

Uh oh!

5chmidti commented Dec 4, 2023

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pizzud commented Dec 4, 2023

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pizzud commented Jan 5, 2024

Uh oh!

EugeneZelenko commented Jan 5, 2024

Uh oh!

pizzud commented Jan 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

denzor200 commented Jan 3, 2025

Uh oh!

vbvictor commented Jun 3, 2025

Uh oh!

pizzud commented Jun 13, 2025

Uh oh!

vbvictor commented Jun 13, 2025

Uh oh!

Uh oh!

vbvictor commented Jun 13, 2025

Uh oh!

pizzud commented Jun 13, 2025

Uh oh!

vbvictor Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

github-actions bot commented Dec 1, 2023 •

edited

Loading

vbvictor Jun 14, 2025 •

edited

Loading

vbvictor commented Jun 21, 2025 •

edited

Loading