[clang-tidy] add new check bugprone-reset-ambiguous-call

Author: vbvictor

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

@@ -64,6 +64,7 @@
 #include "SignedCharMisuseCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
+#include "SmartptrResetAmbiguousCallCheck.h"
 #include "SpuriouslyWakeUpFunctionsCheck.h"
 #include "StandaloneEmptyCheck.h"
 #include "StringConstructorCheck.h"
@@ -207,6 +208,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "bugprone-sizeof-expression");
+    CheckFactories.registerCheck<SmartptrResetAmbiguousCallCheck>(
+        "bugprone-smartptr-reset-ambiguous-call");
     CheckFactories.registerCheck<SpuriouslyWakeUpFunctionsCheck>(
         "bugprone-spuriously-wake-up-functions");
     CheckFactories.registerCheck<StandaloneEmptyCheck>(

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

@@ -65,6 +65,7 @@ add_clang_library(clangTidyBugproneModule STATIC
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   SmartPtrArrayMismatchCheck.cpp
+  SmartptrResetAmbiguousCallCheck.cpp
   SpuriouslyWakeUpFunctionsCheck.cpp
   StandaloneEmptyCheck.cpp
   StringConstructorCheck.cpp

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

@@ -0,0 +1,146 @@
+//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h"
+#include "../utils/OptionsUtils.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) {
+  for (const auto *Arg : Node.arguments()) {
+    if (!InnerMatcher.matches(*Arg, Finder, Builder))
+      return false;
+  }
+
+  return true;
+}
+
+AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) {
+  for (const auto *Param : Node.parameters()) {
+    if (!Param->hasDefaultArg())
+      return false;
+  }
+
+  return true;
+}
+
+const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr";
+} // namespace
+
+SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      SmartPointers(utils::options::parseStringList(
+          Options.get("SmartPointers", DefaultSmartPointers))) {}
+
+void SmartptrResetAmbiguousCallCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "SmartPointers",
+                utils::options::serializeStringList(SmartPointers));
+}
+
+void SmartptrResetAmbiguousCallCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IsSmartptr = hasAnyName(SmartPointers);
+
+  const auto ResetMethod =
+      cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters());
+
+  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 SmartptrResetAmbiguousCallCheck::check(
+    const MatchFinder::MatchResult &Result) {
+
+  if (const auto *SmartptrResetCall =
+          Result.Nodes.getNodeAs<CXXMemberCallExpr>("smartptrResetCall")) {
+    const auto *Member = cast<MemberExpr>(SmartptrResetCall->getCallee());
+
+    diag(SmartptrResetCall->getBeginLoc(),
+         "be explicit when calling 'reset()' on a smart pointer with a "
+         "pointee that has a 'reset()' method");
+
+    diag(SmartptrResetCall->getBeginLoc(), "assign the pointer to 'nullptr'",
+         DiagnosticIDs::Note)
+        << FixItHint::CreateReplacement(
+               SourceRange(Member->getOperatorLoc(),
+                           Member->getOperatorLoc().getLocWithOffset(0)),
+               " =")
+        << FixItHint::CreateReplacement(
+               SourceRange(Member->getMemberLoc(),
+                           SmartptrResetCall->getEndLoc()),
+               " nullptr");
+    return;
+  }
+
+  if (const auto *ObjectResetCall =
+          Result.Nodes.getNodeAs<CXXMemberCallExpr>("objectResetCall")) {
+    const auto *Arrow = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("OpCall");
+
+    const CharSourceRange SmartptrSourceRange =
+        Lexer::getAsCharRange(Arrow->getArg(0)->getSourceRange(),
+                              *Result.SourceManager, getLangOpts());
+
+    diag(ObjectResetCall->getBeginLoc(),
+         "be explicit when calling 'reset()' on a pointee of a smart pointer");
+
+    diag(ObjectResetCall->getBeginLoc(),
+         "use dereference to call 'reset' method of the pointee",
+         DiagnosticIDs::Note)
+        << FixItHint::CreateInsertion(SmartptrSourceRange.getBegin(), "(*")
+        << FixItHint::CreateInsertion(SmartptrSourceRange.getEnd(), ")")
+        << FixItHint::CreateReplacement(
+               CharSourceRange::getCharRange(
+                   Arrow->getOperatorLoc(),
+                   Arrow->getOperatorLoc().getLocWithOffset(2)),
+               ".");
+  }
+}
+
+} // namespace clang::tidy::bugprone

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

@@ -0,0 +1,37 @@
+//===--- SmartptrResetAmbiguousCallCheck.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_SMARTPTRRESETAMBIGUOUSCALLCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SMARTPTRRESETAMBIGUOUSCALLCHECK_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/smartptr-reset-ambiguous-call.html
+class SmartptrResetAmbiguousCallCheck : public ClangTidyCheck {
+public:
+  SmartptrResetAmbiguousCallCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+
+private:
+  const std::vector<StringRef> SmartPointers;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SMARTPTRRESETAMBIGUOUSCALLCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -91,6 +91,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-smartptr-reset-ambiguous-call
+  <clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call>` check.
+
+  Finds potentially erroneous calls to ``reset`` method on smart pointers when
+  the pointee type also has a ``reset`` method.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst

@@ -0,0 +1,47 @@
+.. title:: clang-tidy - bugprone-smartptr-reset-ambiguous-call
+
+bugprone-smartptr-reset-ambiguous-call
+======================================
+
+Finds potentially erroneous calls to ``reset`` method on smart pointers when
+the pointee type also has a ``reset`` method. Having a ``reset`` method in
+both classes 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 a 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
+
+The default smart pointers that are considered are ``std::unique_ptr``,
+``std::shared_ptr``. To specify other smart pointers or other classes use the
+:option:`SmartPointers` option.
+
+Options
+-------
+
+.. option:: SmartPointers
+
+    Semicolon-separated list of class names of custom smart pointers.
+    Default value is `::std::unique_ptr;::std::shared_ptr`.

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

@@ -132,6 +132,7 @@ Clang-Tidy Checks
    :doc:`bugprone-signed-char-misuse <bugprone/signed-char-misuse>`,
    :doc:`bugprone-sizeof-container <bugprone/sizeof-container>`,
    :doc:`bugprone-sizeof-expression <bugprone/sizeof-expression>`,
+   :doc:`bugprone-smartptr-reset-ambiguous-call <bugprone/smartptr-reset-ambiguous-call>`, "Yes"
    :doc:`bugprone-spuriously-wake-up-functions <bugprone/spuriously-wake-up-functions>`,
    :doc:`bugprone-standalone-empty <bugprone/standalone-empty>`, "Yes"
    :doc:`bugprone-string-constructor <bugprone/string-constructor>`, "Yes"

clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call-custom-pointers.cpp

@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s bugprone-smartptr-reset-ambiguous-call %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  {bugprone-smartptr-reset-ambiguous-call.SmartPointers: "::std::unique_ptr;boost::shared_ptr"}}' \
+// RUN: --fix-notes --
+
+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
+
+namespace boost {
+
+template <typename T>
+struct shared_ptr {
+  T& operator*() const;
+  T* operator->() const;
+  void reset();
+};
+
+} // namespace boost
+
+struct Resettable {
+  void reset();
+  void doSomething();
+};
+
+void Positive() {
+  std::unique_ptr<Resettable> u;
+  u.reset();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: be explicit when calling 'reset()' on a smart pointer with a pointee that has a 'reset()' method
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: note: assign the pointer to 'nullptr'
+  // CHECK-FIXES: u = nullptr;
+  u->reset();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: be explicit when calling 'reset()' on a pointee of a smart pointer
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: note: use dereference to call 'reset' method of the pointee
+  // CHECK-FIXES: (*u).reset();
+
+  boost::shared_ptr<Resettable> s;
+  s.reset();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: be explicit when calling 'reset()' on a smart pointer with a pointee that has a 'reset()' method
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: note: assign the pointer to 'nullptr'
+  // CHECK-FIXES: s = nullptr;
+  s->reset();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: be explicit when calling 'reset()' on a pointee of a smart pointer
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: note: use dereference to call 'reset' method of the pointee
+  // CHECK-FIXES: (*s).reset();
+}
+
+void Negative() {
+  std::shared_ptr<Resettable> s_ptr;
+  s_ptr.reset();
+  s_ptr->reset();
+  s_ptr->doSomething();
+
+  std::unique_ptr<Resettable> u_ptr;
+  u_ptr.reset(nullptr);
+  u_ptr->doSomething();
+}