[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

…ared-ptr

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

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

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

PiotrZSL · 2024-01-14T08:11:35Z

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

+                   0, unaryOperator(hasOperatorName("*"),
+                                    hasDescendant(cxxDependentScopeMemberExpr(
+                                        hasMemberName("get"))),
+                                    hasDescendant(declRefExpr(to(


issue could be if you would have call expression in call expression, like:
x = std::move(getSomething(y).field)
Would be good to check if there are no other calls as parent.

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

PiotrZSL · 2024-01-20T09:56:29Z

@pizzud Just reminder that Clang-tidy 18 branch out is in ~3 days

PiotrZSL · 2024-03-06T16:42:36Z

@pizzud Do you plan to finish this check, just asking as idea behind it is really good.

pizzud · 2024-03-06T16:48:18Z

Yep. I've been slammed at work with other stuff and also ran into a knotty problem I haven't managed to find a solution for: when shared_ptr::operator* and/or get() are defined in a parent of shared_ptr, the returned class from the matchers is the parent, not the class in the config. This was revealed when I switched to the other check's testdata implementation, and it's a really good thing it was because MSVC and either libc++ or libstdc++ (I forget which) implement shared_ptr this way. I've been meaning to ask the internal clang-tidy experts if they have any ideas, but haven't managed to free up enough time. =/ Accessing the class with the method definition is easy, but we have a subclass and the available methods in CXXRecordDecl only let you go up the inheritance tree, not down. I was hoping to get back to this later this week.

…

On Wed, Mar 6, 2024 at 8:42 AM Piotr Zegar ***@***.***> wrote: @pizzud <https://github.com/pizzud> Do you plan to finish this check, just asking as idea behind it is really good. — Reply to this email directly, view it on GitHub <#67467 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A5LI24JQ3UELX3YBUN57JGLYW5BQZAVCNFSM6AAAAAA5IDKJ7KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBRGMYDSNZRGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

PiotrZSL · 2024-03-06T17:46:29Z

@pizzud don't worry much about this one, this can be easily solved by using hasType instead of ofClass.
Next branch out is in ~5 months, so there is some time.

…han 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.

llvmbot · 2024-04-01T20:20:35Z

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

Author: None (pizzud)

Changes

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.

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

8 Files Affected:

(modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3)
(modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1)
(added) clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp (+145)
(added) clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h (+42)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+8-3)
(added) clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst (+19)
(modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+2-1)
(added) clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp (+138)

diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 435cb1e3fbcff3..4e4092b90d4cf5 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -41,6 +41,7 @@
 #include "MisplacedPointerArithmeticInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveForwardingReferenceCheck.h"
+#include "MoveSharedPointerContentsCheck.h"
 #include "MultiLevelImplicitPointerConversionCheck.h"
 #include "MultipleNewInOneExpressionCheck.h"
 #include "MultipleStatementMacroCheck.h"
@@ -155,6 +156,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-misplaced-widening-cast");
     CheckFactories.registerCheck<MoveForwardingReferenceCheck>(
         "bugprone-move-forwarding-reference");
+    CheckFactories.registerCheck<MoveSharedPointerContentsCheck>(
+        "bugprone-move-shared-pointer-contents");
     CheckFactories.registerCheck<MultiLevelImplicitPointerConversionCheck>(
         "bugprone-multi-level-implicit-pointer-conversion");
     CheckFactories.registerCheck<MultipleNewInOneExpressionCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 70e7fbc7ec0c14..62bba10ad94dc6 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -37,6 +37,7 @@ add_clang_library(clangTidyBugproneModule
   MisplacedPointerArithmeticInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp
   MoveForwardingReferenceCheck.cpp
+  MoveSharedPointerContentsCheck.cpp
   MultiLevelImplicitPointerConversionCheck.cpp
   MultipleNewInOneExpressionCheck.cpp
   MultipleStatementMacroCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp
new file mode 100644
index 00000000000000..9ff357b58f9778
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp
@@ -0,0 +1,145 @@
+//===--- MoveSharedPointerContentsCheck.cpp - clang-tidy ------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <signal.h>
+#include <unistd.h>
+
+#include "MoveSharedPointerContentsCheck.h"
+#include "../ClangTidyCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+namespace {
+
+// Reports whether the QualType matches the inner matcher, which is expected to be
+// matchesAnyListedName. The QualType is expected to either point to a RecordDecl
+// (for concrete types) or an ElaboratedType (for dependent ones).
+AST_MATCHER_P(QualType, isSharedPointer,
+              clang::ast_matchers::internal::Matcher<NamedDecl>, InnerMatcher) {
+  if (const auto *RD = Node.getTypePtr()->getAsCXXRecordDecl(); RD != nullptr) {
+    return InnerMatcher.matches(*RD, Finder, Builder);
+  } else if (const auto *ED = Node.getTypePtr()->getAs<ElaboratedType>();
+             ED != nullptr) {
+    if (const auto *TS = ED->getNamedType()
+                             .getTypePtr()
+                             ->getAs<TemplateSpecializationType>();
+        TS != nullptr) {
+      return InnerMatcher.matches(*TS->getTemplateName().getAsTemplateDecl(),
+                                  Finder, Builder);
+    }
+  }
+
+  return false;
+}
+
+} // namespace
+
+MoveSharedPointerContentsCheck::MoveSharedPointerContentsCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      SharedPointerClasses(utils::options::parseStringList(
+          Options.get("SharedPointerClasses", "::std::shared_ptr"))) {}
+
+void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) {
+  auto isStdMove = callee(functionDecl(hasName("::std::move")));
+
+  auto resolvedType = callExpr(anyOf(
+      // Resolved type, direct move.
+      callExpr(
+          isStdMove,
+          hasArgument(
+              0,
+              cxxOperatorCallExpr(
+                  hasOverloadedOperatorName("*"),
+                  hasDescendant(declRefExpr(hasType(qualType(isSharedPointer(
+                      matchers::matchesAnyListedName(SharedPointerClasses)))))),
+                  callee(cxxMethodDecl()))))
+          .bind("call"),
+      // Resolved type, move out of get().
+      callExpr(
+          isStdMove,
+          hasArgument(
+              0, unaryOperator(
+                     hasOperatorName("*"),
+                     hasUnaryOperand(allOf(
+                         hasDescendant(declRefExpr(hasType(qualType(
+                             isSharedPointer(matchers::matchesAnyListedName(
+                                 SharedPointerClasses)))))),
+                         cxxMemberCallExpr(
+                             callee(cxxMethodDecl(hasName("get")))))))))
+          .bind("get_call")));
+
+  Finder->addMatcher(resolvedType, this);
+
+  auto isStdMoveUnresolved = callee(unresolvedLookupExpr(
+      hasAnyDeclaration(namedDecl(hasUnderlyingDecl(hasName("::std::move"))))));
+
+  auto unresolvedType = callExpr(anyOf(
+      // Unresolved type, direct move.
+      callExpr(
+          isStdMoveUnresolved,
+          hasArgument(0, unaryOperator(
+                             hasOperatorName("*"),
+                             hasUnaryOperand(declRefExpr(hasType(qualType(
+                                 isSharedPointer(matchers::matchesAnyListedName(
+                                     SharedPointerClasses)))))))))
+          .bind("unresolved_call"),
+
+      // Annoyingly, the declRefExpr in the unresolved-move-of-get() case
+      // is of <dependent type> rather than shared_ptr<T>, so we have to
+      // just fetch the variable. This does leave a gap where a temporary
+      // shared_ptr wouldn't be caught, but moving out of a temporary
+      // shared pointer is a truly wild thing to do so it should be okay.
+      callExpr(isStdMoveUnresolved,
+               hasArgument(
+                   0, unaryOperator(
+                          hasOperatorName("*"),
+                          hasDescendant(cxxDependentScopeMemberExpr(
+                              hasMemberName("get"))),
+                          hasDescendant(declRefExpr(to(varDecl(hasType(qualType(
+                              isSharedPointer(matchers::matchesAnyListedName(
+                                  SharedPointerClasses)))))))))))
+          .bind("unresolved_get_call")));
+
+  Finder->addMatcher(unresolvedType, this);
+}
+
+void MoveSharedPointerContentsCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  clang::SourceLocation Loc;
+  if (const auto *UnresolvedCall =
+          Result.Nodes.getNodeAs<CallExpr>("unresolved_call");
+      UnresolvedCall != nullptr) {
+    Loc = UnresolvedCall->getBeginLoc();
+  } else if (const auto *UnresolvedGetCall =
+                 Result.Nodes.getNodeAs<CallExpr>("unresolved_get_call");
+             UnresolvedGetCall != nullptr) {
+    Loc = UnresolvedGetCall->getBeginLoc();
+  } else if (const auto *GetCall = Result.Nodes.getNodeAs<CallExpr>("get_call");
+             GetCall != nullptr) {
+    Loc = GetCall->getBeginLoc();
+  } else if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
+             Call != nullptr) {
+    Loc = Call->getBeginLoc();
+  } else {
+    return;
+  }
+
+  if (Loc.isValid()) {
+    diag(Loc,
+         "don't move the contents out of a shared pointer, as other accessors "
+         "expect them to remain in a determinate state");
+  }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h
new file mode 100644
index 00000000000000..d2a42c49ddf679
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h
@@ -0,0 +1,42 @@
+//===--- MoveSharedPointerContentsCheck.h - clang-tidy ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MOVESHAREDPOINTERCONTENTSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MOVESHAREDPOINTERCONTENTSCHECK_H
+
+#include <vector>
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Detects calls to move the contents out of a ``std::shared_ptr`` rather than
+/// moving the pointer itself.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/move-shared-pointer-contents.html
+class MoveSharedPointerContentsCheck : public ClangTidyCheck {
+public:
+  MoveSharedPointerContentsCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool
+  isLanguageVersionSupported(const LangOptions &LangOptions) const override {
+    return LangOptions.CPlusPlus11;
+  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+
+private:
+  const std::vector<StringRef> SharedPointerClasses;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MOVESHAREDPOINTERCONTENTSCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index b4d87e0ed2a67a..1238ef8336239c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -162,6 +162,12 @@ New checks
   Detects incorrect usages of ``std::enable_if`` that don't name the nested
   ``type`` type.
 
+- New :doc:`bugprone-move-shared-pointer-contents
+  <clang-tidy/checks/bugprone/move-shared-pointer-contents>` check.
+
+  Detects calls to move the contents out of a ``std::shared_ptr`` rather than
+  moving the pointer itself.
+  
 - New :doc:`bugprone-multi-level-implicit-pointer-conversion
   <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` check.
 
@@ -368,8 +374,7 @@ Changes in existing checks
   <clang-tidy/checks/misc/const-correctness>` check to avoid false positive when
   using pointer to member function. Additionally, the check no longer emits
   a diagnostic when a variable that is not type-dependent is an operand of a
-  type-dependent binary operator. Improved performance of the check through
-  optimizations.
+  type-dependent binary operator.
 
 - Improved :doc:`misc-include-cleaner
   <clang-tidy/checks/misc/include-cleaner>` check by adding option
@@ -383,7 +388,7 @@ Changes in existing checks
 
 - Improved :doc:`misc-unused-using-decls
   <clang-tidy/checks/misc/unused-using-decls>` check to avoid false positive when
-  using in elaborated type and only check cpp files.
+  using in elaborated type.
 
 - Improved :doc:`modernize-avoid-bind
   <clang-tidy/checks/modernize/avoid-bind>` check to
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst
new file mode 100644
index 00000000000000..ea4c1fb6df708c
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst
@@ -0,0 +1,19 @@
+.. title:: clang-tidy - bugprone-move-shared-pointer-contents
+
+bugprone-move-shared-pointer-contents
+=====================================
+
+
+Detects calls to move the contents out of a ``std::shared_ptr`` rather
+than moving the pointer itself. In other words, calling
+``std::move(*p)`` or ``std::move(*p.get())``. Other reference holders
+may not be expecting the move and suddenly getting empty or otherwise
+indeterminate states can cause issues.
+
+Options
+-------
+.. option :: SharedPointerClasses
+
+   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`.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 2f86121ad87299..b7689889dd2838 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -117,7 +117,8 @@ Clang-Tidy Checks
    :doc:`bugprone-parent-virtual-call <bugprone/parent-virtual-call>`, "Yes"
    :doc:`bugprone-posix-return <bugprone/posix-return>`, "Yes"
    :doc:`bugprone-redundant-branch-condition <bugprone/redundant-branch-condition>`, "Yes"
-   :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes"
+   :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes",
+   :doc:`bugprone-shared-pointer-contents-move <bugprone/shared-pointer-contents-move>`,
    :doc:`bugprone-shared-ptr-array-mismatch <bugprone/shared-ptr-array-mismatch>`, "Yes"
    :doc:`bugprone-signal-handler <bugprone/signal-handler>`,
    :doc:`bugprone-signed-char-misuse <bugprone/signed-char-misuse>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp
new file mode 100644
index 00000000000000..8a741581c30929
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s bugprone-move-shared-pointer-contents %t -- -config="{CheckOptions: {bugprone-move-shared-pointer-contents.SharedPointerClasses: '::std::shared_ptr;my::OtherSharedPtr;'}}"
+
+// Some dummy definitions we'll need.
+
+namespace std {
+
+using size_t = int;
+
+template <typename> struct remove_reference;
+template <typename _Tp> struct remove_reference { typedef _Tp type; };
+template <typename _Tp> struct remove_reference<_Tp &> { typedef _Tp type; };
+template <typename _Tp> struct remove_reference<_Tp &&> { typedef _Tp type; };
+
+template <typename _Tp>
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+  return static_cast<typename std::remove_reference<_Tp>::type &&>(__t);
+}
+
+template <typename T>
+struct shared_ptr {
+  shared_ptr();
+  T *get() const;
+  explicit operator bool() const;
+  void reset(T *ptr);
+  T &operator*() const;
+  T *operator->() const;
+};
+
+}  // namespace std
+
+namespace my {
+template <typename T>
+using OtherSharedPtr = std::shared_ptr<T>;
+// Not part of the config.
+template <typename T>
+using YetAnotherSharedPtr = T*;
+}  // namespace my
+
+struct Nontrivial {
+  int x;
+  Nontrivial() : x(2) {}
+  Nontrivial(Nontrivial& other) { x = other.x; }
+  Nontrivial(Nontrivial&& other) { x = std::move(other.x); }
+  Nontrivial& operator=(Nontrivial& other) { x = other.x; }
+  Nontrivial& operator=(Nontrivial&& other) { x = std::move(other.x); }
+};
+
+// Test cases begin here.
+
+void correct() {
+  std::shared_ptr<Nontrivial> p;
+  Nontrivial x = *std::move(p);
+}
+
+void simpleFinding() {
+  std::shared_ptr<Nontrivial> p;
+  Nontrivial y = std::move(*p);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: 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]
+
+void aliasedType() {
+  using nontrivial_ptr = std::shared_ptr<Nontrivial>;
+  nontrivial_ptr p;
+  Nontrivial x = std::move(*p);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: 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]
+
+void configWorks() {
+  my::OtherSharedPtr<Nontrivial> p;
+  Nontrivial x = std::move(*p);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: 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]
+
+void sharedEsquePointerNotInConfig() {
+  my::YetAnotherSharedPtr<Nontrivial> p;
+  Nontrivial x = std::move(*p);
+}
+
+
+void multiStars() {
+  std::shared_ptr<Nontrivial> p;
+  int x = 2 * std::move(*p).x * 3;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:15: 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]
+
+template <typename T>
+void unresolved() {
+  std::shared_ptr<T> p;
+  int x = 2 * std::move(*p).x * 3;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:15: 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]
+
+template <typename T>
+void unresolvedAsAReference() {
+  std::shared_ptr<T> p;
+  std::shared_ptr<T>& q = p;
+  int x = 2 * std::move(*q).x * 3;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:15: 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]
+
+template <typename T>
+void unresolvedAlias() {
+  my::OtherSharedPtr<T> p;
+  Nontrivial x = std::move(*p);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: 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]
+
+void dereferencedGet() {
+  std::shared_ptr<Nontrivial> p;
+  Nontrivial x = std::move(*p.get());
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: 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]
+
+template <typename T>
+void unresolvedDereferencedGet() {
+  std::shared_ptr<T> p;
+  T x = std::move(*p.get());
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:9: 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]
+
+template <typename T>
+void rawPointer() {
+  T* p;
+  T x = std::move(*p);
+}
+
+struct HasASharedPtrField {
+  std::shared_ptr<Nontrivial> x;
+};
+
+HasASharedPtrField returnsStruct() {
+  HasASharedPtrField h;
+  return h;
+}
+  
+void subfield() {
+  int x = std::move(returnsStruct().x->x);
+}

PiotrZSL

@pizzud
To be honest I see this check being already done in 99%.
Please resolve some last nits, rebase code and we could merge it in this month.

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

clang-tools-extra/docs/ReleaseNotes.rst

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

…han 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.

…nto shared-ptr

PiotrZSL · 2024-07-22T18:54:47Z

Looks like there are still some merge conflicts, duplicated lines and so on.

pizzud · 2024-07-26T23:16:06Z

Should be resolved now. Thanks for your patience.

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

Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>

PiotrZSL

Matchers need simplification, mainly that hasDescendant and declRefExpr need to go

PiotrZSL · 2024-08-08T09:10:19Z

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

+              0,
+              cxxOperatorCallExpr(
+                  hasOverloadedOperatorName("*"),
+                  hasDescendant(declRefExpr(hasType(qualType(isSharedPointer(


this has hasDescendant may not work properly here, as this callExpr would match:

std::move(*getSomething(objec1, shared_ptr_obj)

Instead you could check type of object used for operator *:

cxxOperatorCallExpr(callee(ofClass(matchers::matchesAnyListedName(SharedPointerClasses)))

Doing that gives me a QualType for the underlying type, not the user-visible type. As an example, this matters when operator* or get() are declared in a parent rather than the visible class, which is the case for at least one major implementation of std::shared_ptr (I think it's libstdc++ but haven't looked in several months) and is visible in the unit tests because the dummy implementation in use has a std::__shared_ptr that defines them and std::shared_ptr inherits from it. Using DeclRefExpr is an intentional attempt to get at the user-visible type of the variable. The hasDescendant can at least be hasArgument(0).

The good news is that I added your example to the unit tests and it doesn't actually trigger.

PiotrZSL · 2024-08-08T09:10:46Z

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

+              0, unaryOperator(
+                     hasOperatorName("*"),
+                     hasUnaryOperand(allOf(
+                         hasDescendant(declRefExpr(hasType(qualType(


same issue here as above

Same response, but in this case hasArgument(0) hits a static_assert about right polymorphism.

PiotrZSL · 2024-08-08T09:12:26Z

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

+  if (const auto *UnresolvedCall =
+          Result.Nodes.getNodeAs<CallExpr>("unresolved_call");
+      UnresolvedCall != nullptr) {
+    Loc = UnresolvedCall->getBeginLoc();
+  } else if (const auto *UnresolvedGetCall =
+                 Result.Nodes.getNodeAs<CallExpr>("unresolved_get_call");
+             UnresolvedGetCall != nullptr) {
+    Loc = UnresolvedGetCall->getBeginLoc();
+  } else if (const auto *GetCall = Result.Nodes.getNodeAs<CallExpr>("get_call");
+             GetCall != nullptr) {
+    Loc = GetCall->getBeginLoc();
+  } else if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
+             Call != nullptr) {
+    Loc = Call->getBeginLoc();
+  } else {
+    return;
+  }


instead of using different names, just bind those to same name and then you will be able just call getBeginLoc once

denzor200 · 2024-12-29T19:56:05Z

Suppose this check should be silent for a shared_ptr with use_count() == 1. Is it possible to change implementation in order to following this way?

pizzud · 2025-01-03T17:22:50Z

Suppose this check should be silent for a shared_ptr with use_count() == 1. Is it possible to change implementation in order to following this way?

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

Separately, I finally noticed the failing unit tests on Windows and will start investigating, though I don't have a Windows box so it might be tricky.

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.

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 remote-tracking branch 'refs/remotes/origin/shared-ptr' into sh…

fa270b3

…ared-ptr

PiotrZSL reviewed Jan 14, 2024

View reviewed changes

llvmbot added the clang-tools-extra label Apr 1, 2024

PiotrZSL requested changes Jun 10, 2024

View reviewed changes

pizzud added 3 commits June 21, 2024 14:12

Merge branch 'shared-ptr' of https://github.com/pizzud/llvm-project i…

d0934e4

…nto shared-ptr

fix merge conflicts

0eadd0d

pizzud and others added 5 commits July 26, 2024 15:22

merge

131b99d

cleanup duplicate entry

52d9353

cleanup a dupe and restore release notes

83ffdd8

avoid merge conflict

14028ab

Merge branch 'main' into shared-ptr

ecf2164

EugeneZelenko reviewed Jul 26, 2024

View reviewed changes

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

Address formatting suggestion in checker doc.

4e516c0

Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>

PiotrZSL requested changes Aug 8, 2024

View reviewed changes

reflow check() implementation and add another unit test

70fdd1d

pizzud added 2 commits January 3, 2025 09:23

Merge branch 'main' into shared-ptr

f8a0d2b

Merge branch 'llvm:main' into shared-ptr

2761347

[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!

Uh oh!

Uh oh!

Uh oh!

PiotrZSL Jan 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PiotrZSL commented Jan 20, 2024

Uh oh!

PiotrZSL commented Mar 6, 2024

Uh oh!

pizzud commented Mar 6, 2024 via email

Uh oh!

PiotrZSL commented Mar 6, 2024

Uh oh!

llvmbot commented Apr 1, 2024

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 1, 2023 •

edited

Loading