Author: Balázs Kéri Date: 2025-05-17T10:26:13+02:00 New Revision: d84b97ebb3bcdae2b815d1ecfaaee7b5ec8b63f9
URL: https://github.com/llvm/llvm-project/commit/d84b97ebb3bcdae2b815d1ecfaaee7b5ec8b63f9 DIFF: https://github.com/llvm/llvm-project/commit/d84b97ebb3bcdae2b815d1ecfaaee7b5ec8b63f9.diff LOG: [clang-tidy] Add check bugprone-misleading-setter-of-reference (#132242) Added: clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.h clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp Modified: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/list.rst Removed: ################################################################################ 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..4aba5831e6772 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp @@ -0,0 +1,61 @@ +//===--- 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(hasType(hasCanonicalType(referenceType( + pointee(equalsBoundNode("type")))))) + .bind("member"); + auto AssignLHS = memberExpr( + hasObjectExpression(ignoringParenCasts(cxxThisExpr())), member(RefField)); + auto DerefOperand = expr(ignoringParenCasts( + declRefExpr(to(parmVarDecl(equalsBoundNode("parm")))))); + auto AssignRHS = expr(ignoringParenCasts( + 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), + hasParameter( + 0, + parmVarDecl(hasType(hasCanonicalType(pointerType(pointee(qualType( + hasCanonicalType(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; consider not using a " + "pointer as argument") + << 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/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 579fca54924d5..9b29ab12e1bfa 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -124,6 +124,12 @@ New checks pointer and store it as class members without handle the copy and move constructors and the assignments. +- New :doc:`bugprone-misleading-setter-of-reference + <clang-tidy/checks/bugprone/misleading-setter-of-reference>` check. + + Finds setter-like member functions that take a pointer parameter and set a + reference member of the same class with the pointed value. + - New :doc:`bugprone-unintended-char-ostream-output <clang-tidy/checks/bugprone/unintended-char-ostream-output>` check. 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..43da9ae0ad199 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst @@ -0,0 +1,46 @@ +.. title:: clang-tidy - bugprone-misleading-setter-of-reference + +bugprone-misleading-setter-of-reference +======================================= + +Finds setter-like member functions that take a pointer parameter and set a +reference member of the same class with the pointed value. + +The check detects member functions that take a single pointer parameter, +and contain a single expression statement that dereferences the parameter and +assigns the result to a data member with a reference type. + +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. + +Example: + +.. code-block:: c++ + + class MyClass { + int &InternalRef; // non-const reference member + public: + MyClass(int &Value) : InternalRef(Value) {} + + // Warning: This setter could lead to unintended behaviour. + void setRef(int *Value) { + InternalRef = *Value; // This assigns to the referenced value, not changing what InternalRef references. + } + }; + + int main() { + int Value1 = 42; + int Value2 = 100; + MyClass X(Value1); + + // This might look like it changes what InternalRef references to, + // but it actually modifies Value1 to be 100. + X.setRef(&Value2); + } + +Possible fixes: + - Change the parameter type of the "set" function to non-pointer type (for + example, a const reference). + - 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/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 18f1467285fab..1ec476eef3420 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -109,6 +109,7 @@ Clang-Tidy Checks :doc:`bugprone-lambda-function-name <bugprone/lambda-function-name>`, :doc:`bugprone-macro-parentheses <bugprone/macro-parentheses>`, "Yes" :doc:`bugprone-macro-repeated-side-effects <bugprone/macro-repeated-side-effects>`, + :doc:`bugprone-misleading-setter-of-reference <bugprone/misleading-setter-of-reference>`, :doc:`bugprone-misplaced-operator-in-strlen-in-alloc <bugprone/misplaced-operator-in-strlen-in-alloc>`, "Yes" :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc>`, "Yes" :doc:`bugprone-misplaced-widening-cast <bugprone/misplaced-widening-cast>`, 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..695e250528536 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp @@ -0,0 +1,121 @@ +// RUN: %check_clang_tidy %s bugprone-misleading-setter-of-reference %t + +struct X { + X &operator=(const X &) { return *this; } +private: + int &Mem; + friend class Test1; +}; + +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 + 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 + 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 + 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) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setLPub' can be mistakenly used in order to change the reference 'MemLPub' instead of the value of it + MemLPub = *NewValue; + } + +private: + void set5(long *NewValue) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'set5' can be mistakenly used in order to change the reference 'MemL' instead of the value of it + 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: + TemplateTest(T &V) : Mem{V} {} + 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_TemplateTest(char *Value) { + char CharValue; + TemplateTest<char> TTChar{CharValue}; + TTChar.setValue(Value); +} + +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; + } +}; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
