https://github.com/vbvictor created https://github.com/llvm/llvm-project/pull/121291
Add new clang-tidy check that finds potentially erroneous calls to ``reset()`` method on smart pointers when the pointee type also has a ``reset()`` method. It's easy to make typo and delete object because the difference between ``.`` and ``->`` is really small. Sometimes IDE's autocomplete will change ``->`` to ``.`` automatically. For example, developer wrote ``ptr->res`` but after _Tab_ it became ``ptr.reset()``. Small example: ```cpp struct Resettable { void reset() { /* Own reset logic */ } }; auto ptr = std::make_unique<Resettable>(); ptr->reset(); // Calls underlying reset method ptr.reset(); // Makes the pointer null // After Fix-its (*ptr).reset(); // Clearly calls underlying reset method ptr = nullptr; // Clearly makes the pointer null ``` >From 37dce6a7ed0cca2e9819c24f4d176c43e3c9f2ac Mon Sep 17 00:00:00 2001 From: Victor Baranov <bar.victor.2...@gmail.com> Date: Sun, 29 Dec 2024 15:32:22 +0300 Subject: [PATCH] [clang-tidy] Add bugprone-reset-call check --- .../bugprone/BugproneTidyModule.cpp | 2 + .../clang-tidy/bugprone/CMakeLists.txt | 1 + .../clang-tidy/bugprone/ResetCallCheck.cpp | 133 +++++++++++ .../clang-tidy/bugprone/ResetCallCheck.h | 34 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../clang-tidy/checks/bugprone/reset-call.rst | 33 +++ .../docs/clang-tidy/checks/list.rst | 1 + .../checkers/bugprone/reset-call.cpp | 215 ++++++++++++++++++ 8 files changed, 425 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 33ac65e715ce81..645958e47e22a5 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -57,6 +57,7 @@ #include "PosixReturnCheck.h" #include "RedundantBranchConditionCheck.h" #include "ReservedIdentifierCheck.h" +#include "ResetCallCheck.h" #include "ReturnConstRefFromParameterCheck.h" #include "SharedPtrArrayMismatchCheck.h" #include "SignalHandlerCheck.h" @@ -144,6 +145,7 @@ class BugproneModule : public ClangTidyModule { "bugprone-inaccurate-erase"); CheckFactories.registerCheck<IncorrectEnableIfCheck>( "bugprone-incorrect-enable-if"); + CheckFactories.registerCheck<ResetCallCheck>("bugprone-reset-call"); CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>( "bugprone-return-const-ref-from-parameter"); CheckFactories.registerCheck<SwitchMissingDefaultCaseCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 13adad7c3dadbd..17ab5b27ec5550 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -52,6 +52,7 @@ add_clang_library(clangTidyBugproneModule STATIC PosixReturnCheck.cpp RedundantBranchConditionCheck.cpp ReservedIdentifierCheck.cpp + ResetCallCheck.cpp ReturnConstRefFromParameterCheck.cpp SharedPtrArrayMismatchCheck.cpp SignalHandlerCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp new file mode 100644 index 00000000000000..305ac8d51adf3e --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp @@ -0,0 +1,133 @@ +//===--- ResetCallCheck.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 "ResetCallCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +namespace { + +AST_MATCHER_P(CallExpr, everyArgumentMatches, + ast_matchers::internal::Matcher<Expr>, InnerMatcher) { + if (Node.getNumArgs() == 0) + return true; + + for (const auto *Arg : Node.arguments()) { + if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultArgs) { + if (Node.param_empty()) + return true; + + for (const auto *Param : Node.parameters()) { + if (!Param->hasDefaultArg()) + return false; + } + return true; +} + +} // namespace + +void ResetCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName("::std::unique_ptr", "::std::shared_ptr"); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultArgs()); + + const auto TypeWithReset = + anyOf(cxxRecordDecl(hasMethod(ResetMethod)), + classTemplateSpecializationDecl( + hasSpecializedTemplate(classTemplateDecl(has(ResetMethod))))); + + const auto SmartptrWithBugproneReset = classTemplateSpecializationDecl( + IsSmartptr, + hasTemplateArgument( + 0, templateArgument(refersToType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(TypeWithReset))))))); + + // Find a.reset() calls + Finder->addMatcher( + cxxMemberCallExpr(callee(memberExpr(member(hasName("reset")))), + everyArgumentMatches(cxxDefaultArgExpr()), + on(expr(hasType(SmartptrWithBugproneReset)))) + .bind("smartptrResetCall"), + this); + + // Find a->reset() calls + Finder->addMatcher( + cxxMemberCallExpr( + callee(memberExpr( + member(ResetMethod), + hasObjectExpression( + cxxOperatorCallExpr( + hasOverloadedOperatorName("->"), + hasArgument( + 0, expr(hasType( + classTemplateSpecializationDecl(IsSmartptr))))) + .bind("OpCall")))), + everyArgumentMatches(cxxDefaultArgExpr())) + .bind("objectResetCall"), + this); +} + +void ResetCallCheck::check(const MatchFinder::MatchResult &Result) { + const auto *SmartptrResetCall = + Result.Nodes.getNodeAs<CXXMemberCallExpr>("smartptrResetCall"); + const auto *ObjectResetCall = + Result.Nodes.getNodeAs<CXXMemberCallExpr>("objectResetCall"); + + if (SmartptrResetCall) { + const auto *Member = cast<MemberExpr>(SmartptrResetCall->getCallee()); + + auto DiagBuilder = diag(SmartptrResetCall->getBeginLoc(), + "bugprone 'reset()' call on smart pointer"); + + DiagBuilder << FixItHint::CreateReplacement( + SourceRange(Member->getOperatorLoc(), + Member->getOperatorLoc().getLocWithOffset(0)), + " ="); + + DiagBuilder << FixItHint::CreateReplacement( + SourceRange(Member->getMemberLoc(), SmartptrResetCall->getEndLoc()), + " nullptr"); + } + + if (ObjectResetCall) { + const auto *Arrow = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("OpCall"); + + const auto SmartptrSourceRange = + Lexer::getAsCharRange(Arrow->getArg(0)->getSourceRange(), + *Result.SourceManager, getLangOpts()); + + auto DiagBuilder = diag(ObjectResetCall->getBeginLoc(), + "bugprone 'reset()' call on smart pointer"); + + DiagBuilder << FixItHint::CreateInsertion(SmartptrSourceRange.getBegin(), + "(*"); + DiagBuilder << FixItHint::CreateInsertion(SmartptrSourceRange.getEnd(), + ")"); + + DiagBuilder << FixItHint::CreateReplacement( + CharSourceRange::getCharRange( + Arrow->getOperatorLoc(), + Arrow->getOperatorLoc().getLocWithOffset(2)), + "."); + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h b/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h new file mode 100644 index 00000000000000..18f6ee4f6bed22 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h @@ -0,0 +1,34 @@ +//===--- ResetCallCheck.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_RESETCALLCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_RESETCALLCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Finds potentially erroneous calls to 'reset()' method on smart pointers when +/// the pointee type also has a 'reset()' method +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/reset-call.html +class ResetCallCheck : public ClangTidyCheck { +public: + ResetCallCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_RESETCALLCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index fa3a8e577a33ad..d125afef0bb039 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -148,6 +148,12 @@ New checks Finds nondeterministic usages of pointers in unordered containers. +- New :doc:`bugprone-reset-call + <clang-tidy/checks/bugprone/reset-call>` check. + + Finds potentially erroneous calls to ``reset()`` method on smart pointers when + the pointee type also has a ``reset()`` method. + - New :doc:`bugprone-tagged-union-member-count <clang-tidy/checks/bugprone/tagged-union-member-count>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst new file mode 100644 index 00000000000000..87cf955b26b648 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst @@ -0,0 +1,33 @@ +.. title:: clang-tidy - bugprone-reset-call + +bugprone-reset-call +=================== + +Finds calls to ``reset()`` method on smart pointers where the pointee type +also has a ``reset()`` method, which makes it easy to accidentally +make the pointer null when intending to reset the underlying object. + +.. code-block:: c++ + + struct Resettable { + void reset() { /* Own reset logic */ } + }; + + auto ptr = std::make_unique<Resettable>(); + + ptr->reset(); // Calls underlying reset method + ptr.reset(); // Makes the pointer null + +Both calls are valid C++ code, but the second one might not be +what the developer intended, as it destroys the pointed-to object +rather than resetting its state. +It's easy to make such typo because the difference between ``.`` and ``->`` is really small. + +The recommended approach is to make the intent explicit by using either member access or direct assignment: + +.. code-block:: c++ + + std::unique_ptr<Resettable> ptr = std::make_unique<Resettable>(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null \ No newline at end of file diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 4d8853a0f6d86c..9ca866da74666d 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -124,6 +124,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-reset-call <bugprone/reset-call>`, "Yes" :doc:`bugprone-return-const-ref-from-parameter <bugprone/return-const-ref-from-parameter>`, :doc:`bugprone-shared-ptr-array-mismatch <bugprone/shared-ptr-array-mismatch>`, "Yes" :doc:`bugprone-signal-handler <bugprone/signal-handler>`, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp new file mode 100644 index 00000000000000..d98f4b463840c7 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp @@ -0,0 +1,215 @@ +// RUN: %check_clang_tidy %s bugprone-reset-call %t + +namespace std { + +template <typename T> +struct unique_ptr { + T& operator*() const; + T* operator->() const; + void reset(T* p = nullptr); +}; + +template <typename T> +struct shared_ptr { + T& operator*() const; + T* operator->() const; + void reset(); + void reset(T*); +}; + +} // namespace std + +struct Resettable { + void reset(); + void doSomething(); +}; + +struct ResettableWithParam { + void reset(int a); + void doSomething(); +}; + +struct ResettableWithDefaultParams { + void reset(int a = 0, double b = 0.0); + void doSomething(); +}; + +struct NonResettable { + void doSomething(); +}; + +void Positive() { + std::unique_ptr<Resettable> u; + u.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: u = nullptr; + u->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*u).reset(); + + std::shared_ptr<Resettable> s; + s.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: s = nullptr; + s->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*s).reset(); + + std::unique_ptr<std::unique_ptr<int>> uu_ptr; + uu_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: uu_ptr = nullptr; + uu_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*uu_ptr).reset(); + + std::unique_ptr<std::shared_ptr<int>> su_ptr; + su_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: su_ptr = nullptr; + su_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*su_ptr).reset(); + + std::unique_ptr<ResettableWithDefaultParams> rd; + rd.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: rd = nullptr; + rd->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*rd).reset(); + + std::unique_ptr<std::shared_ptr<std::unique_ptr<Resettable>>> nested_ptr; + nested_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: nested_ptr = nullptr; + nested_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*nested_ptr).reset(); + (*nested_ptr).reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*nested_ptr) = nullptr; + (*nested_ptr)->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*(*nested_ptr)).reset(); + (**nested_ptr).reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (**nested_ptr) = nullptr; + (**nested_ptr)->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*(**nested_ptr)).reset(); +} + +void Negative() { + std::unique_ptr<Resettable> u_ptr; + u_ptr.reset(nullptr); + u_ptr->doSomething(); + + std::shared_ptr<Resettable> s_ptr; + s_ptr.reset(nullptr); + s_ptr->doSomething(); + + Resettable* raw_ptr; + raw_ptr->reset(); + raw_ptr->doSomething(); + + Resettable resettable; + resettable.reset(); + resettable.doSomething(); + + std::unique_ptr<ResettableWithParam> u_ptr_param; + u_ptr_param.reset(); + u_ptr_param.reset(nullptr); + u_ptr_param->reset(0); + + std::unique_ptr<NonResettable> u_ptr_no_reset; + u_ptr_no_reset.reset(); + + std::shared_ptr<ResettableWithParam> s_ptr_param; + s_ptr_param.reset(); + s_ptr_param->reset(0); + s_ptr_param->doSomething(); + + std::shared_ptr<NonResettable> s_ptr_no_reset; + s_ptr_no_reset.reset(); + + std::unique_ptr<ResettableWithDefaultParams> u_ptr_default_params; + u_ptr_default_params.reset(nullptr); + u_ptr_default_params->reset(1); + u_ptr_default_params->reset(1, 2.0); +} + +template <typename T> +void TemplatePositiveTest() { + std::unique_ptr<T> u_ptr; + + u_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: u_ptr = nullptr; + u_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*u_ptr).reset(); + + std::shared_ptr<T> s_ptr; + s_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: s_ptr = nullptr; + s_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*s_ptr).reset(); +} + +template <typename T> +void TemplatNegativeTestTypeWithReset() { + std::unique_ptr<T> u_ptr; + u_ptr.reset(); + u_ptr->reset(0); + + std::shared_ptr<T> s_ptr; + s_ptr.reset(); + s_ptr->reset(0); +} + +template <typename T> +void TemplatNegativeTestTypeWithoutReset() { + std::unique_ptr<T> u_ptr; + u_ptr.reset(); + + std::unique_ptr<T> s_ptr; + s_ptr.reset(); +} + +void instantiate() { + TemplatePositiveTest<Resettable>(); + TemplatePositiveTest<std::unique_ptr<int>>(); + TemplatePositiveTest<std::shared_ptr<int>>(); + TemplatNegativeTestTypeWithReset<ResettableWithParam>(); + TemplatNegativeTestTypeWithoutReset<NonResettable>(); +} + +struct S { + void foo() { + u_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: u_ptr = nullptr; + u_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*u_ptr).reset(); + u_ptr.reset(nullptr); + u_ptr->doSomething(); + + s_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: s_ptr = nullptr; + s_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: bugprone 'reset()' call on smart pointer + // CHECK-FIXES: (*s_ptr).reset(); + s_ptr.reset(nullptr); + + ptr.reset(); + } + + std::unique_ptr<Resettable> u_ptr; + std::unique_ptr<std::shared_ptr<int>> s_ptr; + std::unique_ptr<NonResettable> ptr; +}; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits