[clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check

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>(

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

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

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

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.
 

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

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>`,