https://github.com/pizzud updated https://github.com/llvm/llvm-project/pull/67467
>From 04a3e8d8cbd6943f44a81fddb0524902202a1a78 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. --- .../MoveSharedPointerContentsCheck.cpp | 157 ++++++++++++++++++ .../bugprone/MoveSharedPointerContentsCheck.h | 45 +++++ .../bugprone/move-shared-pointer-contents.rst | 19 +++ .../docs/clang-tidy/checks/list.rst | 4 + 4 files changed, 225 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 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..461fe91267e632 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -0,0 +1,157 @@ +//===--- 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"))) {} + +void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { + auto isStdMove = callee(functionDecl(hasName("::std::move"))); + + // Resolved type, direct move. + Finder->addMatcher( + callExpr(isStdMove, hasArgument(0, cxxOperatorCallExpr( + hasOverloadedOperatorName("*"), + callee(cxxMethodDecl(ofClass( + matchers::matchesAnyListedName( + SharedPointerClasses))))))) + .bind("call"), + this); + + // Resolved type, move out of get(). + Finder->addMatcher( + callExpr( + isStdMove, + hasArgument( + 0, unaryOperator( + hasOperatorName("*"), + hasUnaryOperand(cxxMemberCallExpr(callee(cxxMethodDecl( + hasName("get"), ofClass(matchers::matchesAnyListedName( + SharedPointerClasses))))))))) + .bind("get_call"), + this); + + auto isStdMoveUnresolved = callee(unresolvedLookupExpr( + hasAnyDeclaration(namedDecl(hasUnderlyingDecl(hasName("::std::move")))))); + + // Unresolved type, direct move. + Finder->addMatcher( + callExpr( + isStdMoveUnresolved, + hasArgument(0, unaryOperator(hasOperatorName("*"), + hasUnaryOperand(declRefExpr(hasType( + qualType().bind("unresolved_p"))))))) + .bind("unresolved_call"), + this); + // 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. + + // Unresolved type, move out of get(). + Finder->addMatcher( + callExpr(isStdMoveUnresolved, + hasArgument( + 0, unaryOperator(hasOperatorName("*"), + hasDescendant(cxxDependentScopeMemberExpr( + hasMemberName("get"))), + hasDescendant(declRefExpr(to( + varDecl().bind("unresolved_get_p"))))))) + .bind("unresolved_get_call"), + this); +} + +bool MoveSharedPointerContentsCheck::isSharedPointerClass( + const VarDecl *VD) const { + if (VD == nullptr) + return false; + + const QualType QT = VD->getType(); + return isSharedPointerClass(&QT); +} + +bool MoveSharedPointerContentsCheck::isSharedPointerClass( + const QualType *QT) const { + if (QT == nullptr) + return false; + + // We want the qualified name without template parameters, + // const/volatile, or reference/pointer qualifiers so we can look + // it up in SharedPointerClasses. This is a bit messy, but gets us + // to the underlying type without template parameters (eg + // std::shared_ptr) or const/volatile qualifiers even in the face of + // typedefs. + + bool found = false; + const auto *Template = llvm::dyn_cast<TemplateSpecializationType>( + QT->getSplitDesugaredType().Ty); + if (Template != nullptr) { + const std::string TypeName = Template->getTemplateName() + .getAsTemplateDecl() + ->getQualifiedNameAsString(); + for (const llvm::StringRef SharedPointer : SharedPointerClasses) { + // SharedPointer entries may or may not have leading ::, but TypeName + // definitely won't. + if (SharedPointer == TypeName || SharedPointer.substr(2) == TypeName) { + found = true; + break; + } + } + } + + return found; +} + +void MoveSharedPointerContentsCheck::check( + const MatchFinder::MatchResult &Result) { + const bool Unresolved = + isSharedPointerClass(Result.Nodes.getNodeAs<QualType>("unresolved_p")); + const bool UnresolvedGet = + isSharedPointerClass(Result.Nodes.getNodeAs<VarDecl>("unresolved_get_p")); + + clang::SourceLocation Loc; + if (const auto *UnresolvedCall = + Result.Nodes.getNodeAs<CallExpr>("unresolved_call"); + UnresolvedCall != nullptr && Unresolved) { + Loc = UnresolvedCall->getBeginLoc(); + } else if (const auto *UnresolvedGetCall = + Result.Nodes.getNodeAs<CallExpr>("unresolved_get_call"); + UnresolvedGetCall != nullptr && UnresolvedGet) { + 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..c3a6ca83b24ec7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -0,0 +1,45 @@ +//===--- 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: + // Returns whether the type or variable is one of the SharedPointerClasses. + bool isSharedPointerClass(const VarDecl *VD) const; + bool isSharedPointerClass(const QualType *QT) const; + 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/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..a2996b9f9f18fd 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -118,6 +118,10 @@ 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" +<<<<<<< HEAD +======= + :doc:`bugprone-shared-pointer-contents-move <bugprone/shared-pointer-contents-move>`, +>>>>>>> b81b29d180ac ([clang-tidy] Add bugprone-move-shared-pointer-contents check.) :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>`, _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
