https://github.com/balazske updated https://github.com/llvm/llvm-project/pull/132242
From e3064b600ea726ab7b3dea054e9f11e1ce028297 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com> Date: Wed, 19 Mar 2025 16:09:04 +0100 Subject: [PATCH 1/3] [clang-tidy] Add check bugprone-misleading-setter-of-reference --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt | 1 + .../MisleadingSetterOfReferenceCheck.cpp | 58 +++++++++++++++++++ .../MisleadingSetterOfReferenceCheck.h | 37 ++++++++++++ .../misleading-setter-of-reference.rst | 42 ++++++++++++++ .../misleading-setter-of-reference.cpp | 50 ++++++++++++++++ 6 files changed, 191 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp 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; + } +}; From 2a7b326318f4dcab3647337a398977f69a1fe662 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com> Date: Thu, 20 Mar 2025 17:27:55 +0100 Subject: [PATCH 2/3] updates from review and improvements --- .../MisleadingSetterOfReferenceCheck.cpp | 30 ++++---- .../misleading-setter-of-reference.rst | 33 +++++---- .../misleading-setter-of-reference.cpp | 71 ++++++++++++++++++- 3 files changed, 104 insertions(+), 30 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp index 043d15e7fead2..212c24440c594 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp @@ -16,14 +16,14 @@ namespace clang::tidy::bugprone { void MisleadingSetterOfReferenceCheck::registerMatchers(MatchFinder *Finder) { auto RefField = - fieldDecl(unless(isPublic()), - hasType(referenceType(pointee(equalsBoundNode("type"))))) + fieldDecl(unless(isPublic()), hasType(hasCanonicalType(referenceType( + pointee(equalsBoundNode("type")))))) .bind("member"); - auto AssignLHS = - memberExpr(hasObjectExpression(cxxThisExpr()), member(RefField)); - auto DerefOperand = expr(ignoringParenImpCasts( + auto AssignLHS = memberExpr( + hasObjectExpression(ignoringParenCasts(cxxThisExpr())), member(RefField)); + auto DerefOperand = expr(ignoringParenCasts( declRefExpr(to(parmVarDecl(equalsBoundNode("parm")))))); - auto AssignRHS = expr(ignoringParenImpCasts( + auto AssignRHS = expr(ignoringParenCasts( unaryOperator(hasOperatorName("*"), hasUnaryOperand(DerefOperand)))); auto BinaryOpAssign = binaryOperator(hasOperatorName("="), hasLHS(AssignLHS), @@ -35,11 +35,14 @@ void MisleadingSetterOfReferenceCheck::registerMatchers(MatchFinder *Finder) { 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)) + cxxMethodDecl( + parameterCountIs(1), isPublic(), + hasParameter( + 0, + parmVarDecl(hasType(hasCanonicalType(pointerType(pointee(qualType( + hasCanonicalType(qualType().bind("type")))))))) + .bind("parm")), + hasBody(SetBody)) .bind("bad-set-function"); Finder->addMatcher(BadSetFunction, this); } @@ -50,8 +53,9 @@ void MisleadingSetterOfReferenceCheck::check( 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") + "function '%0' can be mistakenly used in order to change the " + "reference '%1' instead of the value of it; consider not using a " + "pointer as argument") << Found->getName() << Member->getName(); } 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 index 69c0d5530fb61..90537cf64f4cf 100644 --- 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 @@ -3,13 +3,17 @@ 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. +Finds setter-like member functions that take a pointer parameter and set a +(non-const) reference member of the same class with the pointed value. + +The factthat 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. +single (maybe overloaded) assignment operator call. The changed member variable +must be private (or protected) for the checker to detect the fault (a public +member can be changed anyway without a set function). Example: @@ -18,15 +22,10 @@ Example: 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) { + Widget(int &value) : ref_(value) {} + + // Warning: 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 } }; @@ -40,3 +39,9 @@ Example: // but it actually modifies value1 to be 100 w.setRef(&value2); // value1 is now 100, ref_ still references value1 } + +Possible fixes: + - Change the parameter of the "set" function to non-pointer or const reference + type. + - Change the type of the member variable to a pointer and in the "set" + function assign a value to the pointer (without dereference). 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 index 9f9a616824469..59d67f1d68561 100644 --- 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 @@ -2,7 +2,9 @@ struct X { X &operator=(const X &) { return *this; } +private: int &Mem; + friend class Test1; }; class Test1 { @@ -15,15 +17,15 @@ class Test1 { 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] + // 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 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] + // 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 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] + // 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 MemX = *NewValue; } void set1(int *NewValue) { @@ -48,3 +50,66 @@ class Test1 { MemL = *NewValue; } }; + +class Base { +protected: + int &MemI; +}; + +class Derived : public Base { +public: + 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 + MemI = *NewValue; + } +}; + +using UIntRef = unsigned int &; +using UIntPtr = unsigned int *; +using UInt = unsigned int; + +class AliasTest { + UIntRef Value1; + UInt &Value2; + unsigned int &Value3; +public: + void setValue1(UIntPtr NewValue) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setValue1' can be mistakenly used in order to change the reference 'Value1' instead of the value of it + Value1 = *NewValue; + } + void setValue2(unsigned int *NewValue) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setValue2' can be mistakenly used in order to change the reference 'Value2' instead of the value of it + Value2 = *NewValue; + } + void setValue3(UInt *NewValue) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setValue3' can be mistakenly used in order to change the reference 'Value3' instead of the value of it + Value3 = *NewValue; + } +}; + +template <typename T> +class TemplateTest { + T &Mem; +public: + void setValue(T *NewValue) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setValue' can be mistakenly used in order to change the reference 'Mem' instead of the value of it + Mem = *NewValue; + } +}; + +void f_TTChar(TemplateTest<char> *); +void f_TTChar(TemplateTest<float> *); + +template <typename T> +class AddMember { +protected: + T &Value; +}; + +class TemplateBaseTest : public AddMember<int> { +public: + void setValue(int *NewValue) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setValue' can be mistakenly used in order to change the reference 'Value' instead of the value of it + Value = *NewValue; + } +}; From 8f2c2d0bd502c72352fc5c784b585692443eaa15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com> Date: Thu, 3 Apr 2025 10:47:46 +0200 Subject: [PATCH 3/3] small documentation fix --- .../checks/bugprone/misleading-setter-of-reference.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 90537cf64f4cf..b3a750d452541 100644 --- 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 @@ -6,7 +6,7 @@ bugprone-misleading-setter-of-reference Finds setter-like member functions that take a pointer parameter and set a (non-const) reference member of the same class with the pointed value. -The factthat a setter function takes a pointer might cause the belief that an +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. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits