https://github.com/pizzud created https://github.com/llvm/llvm-project/pull/67467
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. >From 6d5d35e1273f595e8a0382053d5183cbce7a9d8a Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Tue, 26 Sep 2023 10:45:42 -0700 Subject: [PATCH] [clang-tidy] Add bugprone-move-shared-pointer-contents check. 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. --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt | 2 + .../MoveSharedPointerContentsCheck.cpp | 60 ++++++++++++++++ .../bugprone/MoveSharedPointerContentsCheck.h | 37 ++++++++++ clang-tools-extra/docs/ReleaseNotes.rst | 6 ++ .../bugprone/move-shared-pointer-contents.rst | 17 +++++ .../docs/clang-tidy/checks/list.rst | 2 + .../bugprone/move-shared-pointer-contents.cpp | 68 +++++++++++++++++++ 8 files changed, 195 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index a67a91eedd10482..7f4a504f9930f17 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -39,6 +39,7 @@ #include "MisplacedPointerArithmeticInAllocCheck.h" #include "MisplacedWideningCastCheck.h" #include "MoveForwardingReferenceCheck.h" +#include "MoveSharedPointerContentsCheck.h" #include "MultiLevelImplicitPointerConversionCheck.h" #include "MultipleNewInOneExpressionCheck.h" #include "MultipleStatementMacroCheck.h" @@ -125,6 +126,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-inaccurate-erase"); CheckFactories.registerCheck<IncorrectEnableIfCheck>( "bugprone-incorrect-enable-if"); + CheckFactories.registerCheck<MoveSharedPointerContentsCheck>( + "bugprone-move-shared-pointer-contents"); CheckFactories.registerCheck<SwitchMissingDefaultCaseCheck>( "bugprone-switch-missing-default-case"); CheckFactories.registerCheck<IncDecInConditionsCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 3c768021feb1502..c017f0c0cc52021 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -23,6 +23,7 @@ add_clang_library(clangTidyBugproneModule ImplicitWideningOfMultiplicationResultCheck.cpp InaccurateEraseCheck.cpp IncorrectEnableIfCheck.cpp + MoveSharedPointerContentsCheck.cpp SwitchMissingDefaultCaseCheck.cpp IncDecInConditionsCheck.cpp IncorrectRoundingsCheck.cpp @@ -35,6 +36,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 000000000000000..b4a393b7f2f2000 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -0,0 +1,60 @@ +//===--- 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 "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 { + +MoveSharedPointerContentsCheck::MoveSharedPointerContentsCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + SharedPointerClasses(utils::options::parseStringList( + Options.get("SharedPointerClasses", "std::shared_ptr"))) {} + +MoveSharedPointerContentsCheck::~MoveSharedPointerContentsCheck() = default; + +bool MoveSharedPointerContentsCheck::isLanguageVersionSupported( + const LangOptions &LangOptions) const { + return LangOptions.CPlusPlus11; +} + +void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + callExpr( + callee(functionDecl(hasName("std::move"))), + hasArgument(0, cxxOperatorCallExpr(hasOverloadedOperatorName("*"), + callee(cxxMethodDecl(ofClass( + matchers::matchesAnyListedName( + SharedPointerClasses))))) + .bind("op"))) + .bind("call"), + this); +} + +void MoveSharedPointerContentsCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call"); + const auto *Op = Result.Nodes.getNodeAs<Expr>("op"); + + if (Op) { + diag(Call->getBeginLoc(), + "don't move the contents out of a shared pointer, as other accessors " + "expect them to remain in a determinate state") + << FixItHint::CreateInsertion(Call->getBeginLoc(), "*") + << FixItHint::CreateRemoval(Op->getBeginLoc()); + } +} + +} // 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 000000000000000..a9ae8d335941e8d --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -0,0 +1,37 @@ +//===--- 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 { + +/// FIXME: Write a short description. +/// +/// 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); + ~MoveSharedPointerContentsCheck() override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool + isLanguageVersionSupported(const LangOptions &LangOptions) const override; + +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 19c977977f9044c..cbd472d86c42273 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -138,6 +138,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. 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 000000000000000..4536663a1b74704 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst @@ -0,0 +1,17 @@ +.. 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)``. 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. By default only `std::shared_ptr` is included. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 75a86431d264412..bfe8b15ccc7ea3a 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -105,6 +105,7 @@ Clang-Tidy Checks :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc>`, "Yes" :doc:`bugprone-misplaced-widening-cast <bugprone/misplaced-widening-cast>`, :doc:`bugprone-move-forwarding-reference <bugprone/move-forwarding-reference>`, "Yes" + :doc:`bugprone-move-shared-pointer-contents <bugprone/move-shared-pointer-contents>`, "Yes" :doc:`bugprone-multi-level-implicit-pointer-conversion <bugprone/multi-level-implicit-pointer-conversion>`, :doc:`bugprone-multiple-new-in-one-expression <bugprone/multiple-new-in-one-expression>`, :doc:`bugprone-multiple-statement-macro <bugprone/multiple-statement-macro>`, @@ -116,6 +117,7 @@ Clang-Tidy Checks :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-shared-pointer-contents-move <bugprone/shared-pointer-contents-move>`, "Yes" :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 000000000000000..0837708a7171c5b --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp @@ -0,0 +1,68 @@ +// 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>; +} // namespace my + +void correct() { + std::shared_ptr<int> p; + int x = *std::move(p); +} + +void simpleFinding() { + std::shared_ptr<int> p; + int 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] + +void aliasedType() { + using int_ptr = std::shared_ptr<int>; + int_ptr p; + int x = 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] + +void configWorks() { + my::OtherSharedPtr<int> p; + int x = 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] + +void multiStars() { + std::shared_ptr<int> p; + int x = 2 * std::move(*p) * 3; + // CHECK-FIXES: *std::move(p) +} +// CHECK-MESSAGES: :[[@LINE-3]]: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] _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits