[clang-tools-extra] [clang-tidy] Add check bugprone-misleading-setter-of-reference (PR #132242)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tools-extra

Author: Balázs Kéri (balazske)

<details>
<summary>Changes</summary>

There can be concerns about the usefulness of this check but for some code it can be useful.

---
Full diff: https://github.com/llvm/llvm-project/pull/132242.diff


6 Files Affected:

- (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3) 
- (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1) 
- (added) clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp (+58) 
- (added) clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.h (+37) 
- (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst (+42) 
- (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp (+50) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index b780a85bdf3fe..64f4a524daf0d 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -41,6 +41,7 @@
 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
+#include "MisleadingSetterOfReferenceCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedPointerArithmeticInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
@@ -170,6 +171,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-macro-parentheses");
     CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
         "bugprone-macro-repeated-side-effects");
+    CheckFactories.registerCheck<MisleadingSetterOfReferenceCheck>(
+        "bugprone-misleading-setter-of-reference");
     CheckFactories.registerCheck<MisplacedOperatorInStrlenInAllocCheck>(
         "bugprone-misplaced-operator-in-strlen-in-alloc");
     CheckFactories.registerCheck<MisplacedPointerArithmeticInAllocCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index e310ea9c94543..d862794cde323 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -42,6 +42,7 @@ add_clang_library(clangTidyBugproneModule STATIC
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
+  MisleadingSetterOfReferenceCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MisplacedPointerArithmeticInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
new file mode 100644
index 0000000000000..043d15e7fead2
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
@@ -0,0 +1,58 @@
+//===--- MisleadingSetterOfReferenceCheck.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 "MisleadingSetterOfReferenceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void MisleadingSetterOfReferenceCheck::registerMatchers(MatchFinder *Finder) {
+  auto RefField =
+      fieldDecl(unless(isPublic()),
+                hasType(referenceType(pointee(equalsBoundNode("type")))))
+          .bind("member");
+  auto AssignLHS =
+      memberExpr(hasObjectExpression(cxxThisExpr()), member(RefField));
+  auto DerefOperand = expr(ignoringParenImpCasts(
+      declRefExpr(to(parmVarDecl(equalsBoundNode("parm"))))));
+  auto AssignRHS = expr(ignoringParenImpCasts(
+      unaryOperator(hasOperatorName("*"), hasUnaryOperand(DerefOperand))));
+
+  auto BinaryOpAssign = binaryOperator(hasOperatorName("="), hasLHS(AssignLHS),
+                                       hasRHS(AssignRHS));
+  auto CXXOperatorCallAssign = cxxOperatorCallExpr(
+      hasOverloadedOperatorName("="), hasLHS(AssignLHS), hasRHS(AssignRHS));
+
+  auto SetBody =
+      compoundStmt(statementCountIs(1),
+                   anyOf(has(BinaryOpAssign), has(CXXOperatorCallAssign)));
+  auto BadSetFunction =
+      cxxMethodDecl(parameterCountIs(1), isPublic(),
+                    hasAnyParameter(parmVarDecl(hasType(pointerType(pointee(
+                                                    qualType().bind("type")))))
+                                        .bind("parm")),
+                    hasBody(SetBody))
+          .bind("bad-set-function");
+  Finder->addMatcher(BadSetFunction, this);
+}
+
+void MisleadingSetterOfReferenceCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Found = Result.Nodes.getNodeAs<CXXMethodDecl>("bad-set-function");
+  const auto *Member = Result.Nodes.getNodeAs<FieldDecl>("member");
+
+  diag(Found->getBeginLoc(),
+       "function \'%0\' can be mistakenly used in order to change the "
+       "reference \'%1\' instead of the value of it")
+      << Found->getName() << Member->getName();
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.h b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.h
new file mode 100644
index 0000000000000..99e7a9435cfa9
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.h
@@ -0,0 +1,37 @@
+//===--- MisleadingSetterOfReferenceCheck.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_MISLEADINGSETTEROFREFERENCECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MISLEADINGSETTEROFREFERENCECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Emits a warning when a setter-like function that has a pointer parameter
+/// is used to set value of a field with reference type.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/throw-keyword-missing.html
+class MisleadingSetterOfReferenceCheck : public ClangTidyCheck {
+public:
+  MisleadingSetterOfReferenceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MISLEADINGSETTEROFREFERENCECHECK_H
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
new file mode 100644
index 0000000000000..69c0d5530fb61
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
@@ -0,0 +1,42 @@
+.. title:: clang-tidy - bugprone-misleading-setter-of-reference
+
+bugprone-misleading-setter-of-reference
+=======================================
+
+Finds setter-like functions that take a pointer parameter and set a (non-const)
+reference with the pointed value. The fact that a setter function takes a
+pointer might cause the belief that an internal reference (if it would be a
+pointer) is changed instead of the pointed-to (or referenced) value.
+
+Only member functions are detected which have a single parameter and contain a
+single (maybe overloaded) assignment operator call.
+
+Example:
+
+.. code-block:: c++
+
+  class Widget {
+    int& ref_;  // non-const reference member
+  public:
+    // non-copyable
+    Widget(const Widget&) = delete;
+    // non-movable
+    Widget(Widget&&) = delete;
+ 
+    Widget(int& value) : ref_(value) {}
+    
+    // Potentially dangerous setter that could lead to unintended behaviour
+    void setRef(int* ptr) {
+        ref_ = *ptr;  // This assigns to the referenced value, not changing what ref_ references
+    }
+  };
+
+  int main() {
+    int value1 = 42;
+    int value2 = 100;
+    Widget w(value1);
+    
+    // This might look like it changes what ref_ references to,
+    // but it actually modifies value1 to be 100
+    w.setRef(&value2);  // value1 is now 100, ref_ still references value1
+  }
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp
new file mode 100644
index 0000000000000..9f9a616824469
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp
@@ -0,0 +1,50 @@
+// RUN: %check_clang_tidy %s bugprone-misleading-setter-of-reference %t
+
+struct X {
+  X &operator=(const X &) { return *this; }
+  int &Mem;
+};
+
+class Test1 {
+  X &MemX;
+  int &MemI;
+protected:
+  long &MemL;
+public:
+  long &MemLPub;
+
+  Test1(X &MemX, int &MemI, long &MemL) : MemX(MemX), MemI(MemI), MemL(MemL), MemLPub(MemL) {}
+  void setI(int *NewValue) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setI' can be mistakenly used in order to change the reference 'MemI' instead of the value of it [bugprone-misleading-setter-of-reference]
+    MemI = *NewValue;
+  }
+  void setL(long *NewValue) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setL' can be mistakenly used in order to change the reference 'MemL' instead of the value of it [bugprone-misleading-setter-of-reference]
+    MemL = *NewValue;
+  }
+  void setX(X *NewValue) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setX' can be mistakenly used in order to change the reference 'MemX' instead of the value of it [bugprone-misleading-setter-of-reference]
+    MemX = *NewValue;
+  }
+  void set1(int *NewValue) {
+    MemX.Mem = *NewValue;
+  }
+  void set2(int *NewValue) {
+    MemL = static_cast<long>(*NewValue);
+  }
+  void set3(int *NewValue) {
+    MemI = *NewValue;
+    MemL = static_cast<long>(*NewValue);
+  }
+  void set4(long *NewValue, int) {
+    MemL = *NewValue;
+  }
+  void setLPub(long *NewValue) {
+    MemLPub = *NewValue;
+  }
+
+protected:
+  void set5(long *NewValue) {
+    MemL = *NewValue;
+  }
+};

``````````

</details>


https://github.com/llvm/llvm-project/pull/132242