https://github.com/balazske created https://github.com/llvm/llvm-project/pull/132242
There can be concerns about the usefulness of this check but for some code it can be useful. 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] [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; + } +}; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits