diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index e6115f67656bc..7ff5562f8351b 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -42,6 +42,7 @@ #include "IntegerDivisionCheck.h" #include "InvalidEnumDefaultInitializationCheck.h" #include "LambdaFunctionNameCheck.h" +#include "LoopVariableCopiedThenModifiedCheck.h" #include "MacroParenthesesCheck.h" #include "MacroRepeatedSideEffectsCheck.h" #include "MisleadingSetterOfReferenceCheck.h" @@ -161,6 +162,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-incorrect-enable-if"); CheckFactories.registerCheck( "bugprone-incorrect-enable-shared-from-this"); + CheckFactories.registerCheck( + "bugprone-loop-variable-copied-then-modified"); CheckFactories.registerCheck( "bugprone-unintended-char-ostream-output"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index c8943e5b22ef8..3b2d02a89dccd 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -33,6 +33,7 @@ add_clang_library(clangTidyBugproneModule STATIC IncorrectEnableIfCheck.cpp IncorrectEnableSharedFromThisCheck.cpp InvalidEnumDefaultInitializationCheck.cpp + LoopVariableCopiedThenModifiedCheck.cpp UnintendedCharOstreamOutputCheck.cpp ReturnConstRefFromParameterCheck.cpp SuspiciousStringviewDataUsageCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.cpp new file mode 100644 index 0000000000000..7cc7ae35670da --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.cpp @@ -0,0 +1,95 @@ +//===----------------------------------------------------------------------===// +// +// 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 "LoopVariableCopiedThenModifiedCheck.h" +#include "../utils/Matchers.h" +#include "../utils/TypeTraits.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +namespace { +AST_MATCHER(VarDecl, isInMacro) { return Node.getBeginLoc().isMacroID(); } +} // namespace + +LoopVariableCopiedThenModifiedCheck::LoopVariableCopiedThenModifiedCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), IgnoreInexpensiveVariables(Options.get( + "IgnoreInexpensiveVariables", false)), + WarnOnlyOnAutoCopies(Options.get("WarnOnlyOnAutoCopies", false)) {} + +void LoopVariableCopiedThenModifiedCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IgnoreInexpensiveVariables", IgnoreInexpensiveVariables); + Options.store(Opts, "WarnOnlyOnAutoCopies", WarnOnlyOnAutoCopies); +} + +void LoopVariableCopiedThenModifiedCheck::registerMatchers( + MatchFinder *Finder) { + const auto HasReferenceOrPointerType = hasType(qualType( + unless(hasCanonicalType(anyOf(referenceType(), pointerType()))))); + const auto IteratorReturnsValueType = cxxOperatorCallExpr( + hasOverloadedOperatorName("*"), + callee( + cxxMethodDecl(returns(unless(hasCanonicalType(referenceType())))))); + const auto NotConstructedByCopy = cxxConstructExpr( + hasDeclaration(cxxConstructorDecl(unless(isCopyConstructor())))); + const auto ConstructedByConversion = + cxxMemberCallExpr(callee(cxxConversionDecl())); + const auto LoopVar = + varDecl(unless(isInMacro()), HasReferenceOrPointerType, + unless(hasInitializer(expr(hasDescendant(expr( + anyOf(materializeTemporaryExpr(), IteratorReturnsValueType, + NotConstructedByCopy, ConstructedByConversion))))))); + Finder->addMatcher(cxxForRangeStmt(hasLoopVariable(LoopVar.bind("loopVar"))) + .bind("forRange"), + this); +} + +void LoopVariableCopiedThenModifiedCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *LoopVar = Result.Nodes.getNodeAs("loopVar"); + std::optional Expensive = utils::type_traits::isExpensiveToCopy( + LoopVar->getType(), *Result.Context); + if ((!Expensive || !*Expensive) && IgnoreInexpensiveVariables) + return; + if (WarnOnlyOnAutoCopies) { + if (!isa(LoopVar->getType())) { + return; + } + } + const auto *ForRange = Result.Nodes.getNodeAs("forRange"); + + if (!ExprMutationAnalyzer(*ForRange->getBody(), *Result.Context) + .isMutated(LoopVar)) { + return; + } + + clang::SourceRange LoopVarSourceRange = + LoopVar->getTypeSourceInfo()->getTypeLoc().getSourceRange(); + clang::SourceLocation EndLoc = clang::Lexer::getLocForEndOfToken( + LoopVarSourceRange.getEnd(), 0, Result.Context->getSourceManager(), + Result.Context->getLangOpts()); + diag(LoopVar->getLocation(), + "loop variable '%0' is copied and then (possibly) modified; use an " + "explicit copy inside the body of the loop or make the variable a " + "reference") + << LoopVar->getName(); + diag(LoopVar->getLocation(), "consider making '%0' a reference", + DiagnosticIDs::Note) + << LoopVar->getName() + << FixItHint::CreateInsertion(LoopVarSourceRange.getBegin(), "const ") + << FixItHint::CreateInsertion(EndLoc, "&"); +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.h b/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.h new file mode 100644 index 0000000000000..dfc18f98d2f80 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.h @@ -0,0 +1,38 @@ +//===----------------------------------------------------------------------===// +// +// 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_LOOPVARIABLECOPIEDTHENMODIFIEDCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_LOOPVARIABLECOPIEDTHENMODIFIEDCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Finds loop variables that are copied and subsequently modified. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/loop-variable-copied-then-modified.html +class LoopVariableCopiedThenModifiedCheck : public ClangTidyCheck { +public: + LoopVariableCopiedThenModifiedCheck(StringRef Name, + ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + 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; + } + +private: + const bool IgnoreInexpensiveVariables; + const bool WarnOnlyOnAutoCopies; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_LOOPVARIABLECOPIEDTHENMODIFIEDCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 62e1987377989..29ec8fdb54d9d 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -161,16 +161,22 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ +- New :doc:`bugprone-derived-method-shadowing-base-method + ` check. + + Finds derived class methods that shadow a (non-virtual) base class method. + - New :doc:`bugprone-invalid-enum-default-initialization ` check. Detects default initialization (to 0) of variables with ``enum`` type where the enum has no enumerator with value of 0. -- New :doc:`bugprone-derived-method-shadowing-base-method - ` check. +- New :doc:`bugprone-loop-variable-copied-then-modified + ` check. - Finds derived class methods that shadow a (non-virtual) base class method. + Detects when a loop variable is copied and then subsequently (possibly) modified + and suggests replacing with a reference or an explicit copy. - New :doc:`cppcoreguidelines-pro-bounds-avoid-unchecked-container-access ` diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/loop-variable-copied-then-modified.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/loop-variable-copied-then-modified.rst new file mode 100644 index 0000000000000..d2a476d918e94 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/loop-variable-copied-then-modified.rst @@ -0,0 +1,55 @@ +.. title:: clang-tidy - bugprone-loop-variable-copied-then-modified + +bugprone-loop-variable-copied-then-modified +=========================================== + +Detects when a loop variable is copied and then subsequently (possibly) modified +and suggests replacing with a reference or an explicit copy. + +This pattern is considered bugprone because, frequently, programmers do not +realize that they are modifying a *copy* rather than an underlying value, +resulting in subtly erroneous code. + +For instance, the following code attempts to null out a value in a map, but only +succeeds in nulling out a value in a *copy* of the map: + +.. code-block:: c++ + + for (auto target : target_map) { + target.value = nullptr; + } + +The programmer is likely to have intended this code instead: + +.. code-block:: c++ + + for (auto& target : target_map) { + target.value = nullptr; + } + +This code can be fixed in one of two ways: + - In cases where the programmer did not intend to create a copy, they can + convert the loop variable to a reference or a ``const`` reference. A + fix-note message will provide a naive suggestion of how to achieve this, + which works in most cases. + - In cases where the intent is in fact to modify a copy, they may perform the + copy explicitly, inside the body of the loop, and perform whatever + operations they like on that copy. + +This is a conservative check: in cases where it cannot be determined at compile +time whether or not a particular function modifies the variable, it assumes a +modification has ocurred and warns accordingly. However, in such cases, the +warning can still be suppressed by doing one of the actions described above. + +Options +------- + +.. option:: IgnoreInexpensiveVariables + + When `true`, this check will only alert on types that are expensive to copy. + This will lead to fewer false positives, but will also overlook some + instances where there may be an actual bug. Default is `false`. + +.. option:: WarnOnlyOnAutoCopies + + When `true`, this check will only alert on `auto` types. Default is `false`. \ No newline at end of file diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index f94696d4ef9c7..80f3bb0f0f39f 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -110,6 +110,7 @@ Clang-Tidy Checks :doc:`bugprone-integer-division `, :doc:`bugprone-invalid-enum-default-initialization `, :doc:`bugprone-lambda-function-name `, + :doc:`bugprone-loop-variable-copied-then-modified `, "Yes" :doc:`bugprone-macro-parentheses `, "Yes" :doc:`bugprone-macro-repeated-side-effects `, :doc:`bugprone-misleading-setter-of-reference `, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified-generics.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified-generics.cpp new file mode 100644 index 0000000000000..a7bf7017c8aec --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified-generics.cpp @@ -0,0 +1,74 @@ +// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-loop-variable-copied-then-modified %t --fix-notes + +template +struct Iterator { + void operator++() {} + const T& operator*() { + static T* TT = new T(); + return *TT; + } + bool operator!=(const Iterator &) { return false; } +}; +template +struct View { + T begin() { return T(); } + T begin() const { return T(); } + T end() { return T(); } + T end() const { return T(); } +}; + +struct ConstructorConvertible { +}; + +struct S { + int value; + + S() : value(0) {}; + S(const S &); + S(const ConstructorConvertible&) {} + ~S(); + S &operator=(const S &); + void modify() { + value++; + } +}; + +struct Convertible { + operator S() const { + return S(); + } +}; + +struct PairLike { + int id; + S data; +}; + +template +struct Generic { + V value; + + Generic() : value{} {}; + Generic(const Generic &); + ~Generic(); + Generic &operator=(const Generic &); + void modify() { + value++; + } +}; + +void PositiveLoopVariableCopiedAndThenModfiedGeneric() { + for (Generic G : View>>()) { + // CHECK-MESSAGES: [[@LINE-1]]:16: warning: loop variable 'G' is copied and then (possibly) modified; use an explicit copy inside the body of the loop or make the variable a reference + // CHECK-MESSAGES: [[@LINE-2]]:16: note: consider making 'G' a reference + // CHECK-FIXES: for (const Generic& G : View>>()) { + G.modify(); + } +} + +void NegativeLoopVariableIsReferenceAndModifiedGeneric() { + for (const Generic& G : View>>()) { + // It's fine to copy-by-value G into some other G. + Generic G2 = G; + } +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified-ignore-inexpensive.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified-ignore-inexpensive.cpp new file mode 100644 index 0000000000000..13b47dcdd2b63 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified-ignore-inexpensive.cpp @@ -0,0 +1,54 @@ +// RUN: %check_clang_tidy %s bugprone-loop-variable-copied-then-modified %t --fix-notes \ +// RUN: -config="{CheckOptions: \ +// RUN: {bugprone-loop-variable-copied-then-modified.IgnoreInexpensiveVariables: true}}" \ +// RUN: -- -I%S +#include "Inputs/system-header-simulator/sim_initializer_list" +#include "Inputs/system-header-simulator/sim_vector" + +template +struct Iterator { + void operator++() {} + const T& operator*() { + static T* TT = new T(); + return *TT; + } + bool operator!=(const Iterator &) { return false; } +}; +template +struct View { + T begin() { return T(); } + T begin() const { return T(); } + T end() { return T(); } + T end() const { return T(); } +}; + +struct S { + int value; + + S() : value(0) {}; + S(const S &); + ~S(); + S &operator=(const S &); + void modify() { + value++; + } +}; + +void NegativeOnlyCopyingInts() { + std::vector foo; + foo.push_back(1); + foo.push_back(2); + foo.push_back(3); + for (int v : foo) { + v += 1; + } +} + +void PositiveLoopVariableCopiedAndThenModfied() { + for (S S1 : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: loop variable 'S1' is copied and then (possibly) modified; use an explicit copy inside the body of the loop or make the variable a reference + // CHECK-MESSAGES: [[@LINE-2]]:10: note: consider making 'S1' a reference + // CHECK-FIXES: for (const S& S1 : View>()) { + S1.modify(); + } +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified-ignore-non-auto-types.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified-ignore-non-auto-types.cpp new file mode 100644 index 0000000000000..c82a3453e9a8b --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified-ignore-non-auto-types.cpp @@ -0,0 +1,50 @@ +// RUN: %check_clang_tidy %s bugprone-loop-variable-copied-then-modified %t --fix-notes \ +// RUN: -config="{CheckOptions: \ +// RUN: {bugprone-loop-variable-copied-then-modified.WarnOnlyOnAutoCopies: true}}" \ +// RUN: -- -I%S +#include "Inputs/system-header-simulator/sim_initializer_list" +#include "Inputs/system-header-simulator/sim_vector" + +template +struct Iterator { + void operator++() {} + const T& operator*() { + static T* TT = new T(); + return *TT; + } + bool operator!=(const Iterator &) { return false; } +}; +template +struct View { + T begin() { return T(); } + T begin() const { return T(); } + T end() { return T(); } + T end() const { return T(); } +}; + +struct S { + int value; + + S() : value(0) {}; + S(const S &); + ~S(); + S &operator=(const S &); + void modify() { + value++; + } +}; + +void NegativeLoopVariableIsNotAuto() { + for (S S1 : View>()) { + S1.modify(); + } +} + +void PositiveLoopVariableIsAuto() { + for (auto S1 : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable 'S1' is copied and then (possibly) modified; use an explicit copy inside the body of the loop or make the variable a reference + // CHECK-MESSAGES: [[@LINE-2]]:13: note: consider making 'S1' a reference + // CHECK-FIXES: for (const auto& S1 : View>()) { + S1.modify(); + } +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified.cpp new file mode 100644 index 0000000000000..1fb6d6116c8b3 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified.cpp @@ -0,0 +1,152 @@ +// RUN: %check_clang_tidy %s bugprone-loop-variable-copied-then-modified %t --fix-notes + +template +struct Iterator { + void operator++() {} + const T& operator*() { + static T* TT = new T(); + return *TT; + } + bool operator!=(const Iterator &) { return false; } +}; +template +struct View { + T begin() { return T(); } + T begin() const { return T(); } + T end() { return T(); } + T end() const { return T(); } +}; + +struct ConstructorConvertible { +}; + +struct S { + int value; + + S() : value(0) {}; + S(const S &); + S(const ConstructorConvertible&) {} + ~S(); + S &operator=(const S &); + void modify() { + value++; + } +}; + +struct Convertible { + operator S() const { + return S(); + } +}; + +struct PairLike { + int id; + S data; +}; + +template +struct Generic { + V value; + + Generic() : value{} {}; + Generic(const Generic &); + ~Generic(); + Generic &operator=(const Generic &); + void modify() { + value++; + } +}; + +#define LOOP_AND_MODIFY(VAR_NAME, TYPE) \ + for (TYPE VAR_NAME : View>()) { \ + VAR_NAME.modify(); \ + } + +void NegativeLoopVariableNotCopied() { + for (const S& S1 : View>()) { + // It's fine to copy-by-value S1 into some other S. + S S2 = S1; + } +} + +void NegativeLoopVariableCopiedButNotModified() { + for (S S1 : View>()) { + } +} + +void PositiveLoopVariableCopiedAndThenModfied() { + for (S S1 : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: loop variable 'S1' is copied and then (possibly) modified; use an explicit copy inside the body of the loop or make the variable a reference + // CHECK-MESSAGES: [[@LINE-2]]:10: note: consider making 'S1' a reference + // CHECK-FIXES: for (const S& S1 : View>()) { + S1.modify(); + } +} + +void PositiveLoopVariableCopiedAndThenModifiedAuto() { + for (auto S1 : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable 'S1' is copied and then (possibly) modified; use an explicit copy inside the body of the loop or make the variable a reference + // CHECK-MESSAGES: [[@LINE-2]]:13: note: consider making 'S1' a reference + // CHECK-FIXES: for (const auto& S1 : View>()) { + S1.modify(); + } +} + +void PositiveLoopVariableIsTypedefType() { + typedef int TypedefInt; + for (TypedefInt ti : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:19: warning: loop variable 'ti' is copied and then (possibly) modified; use an explicit copy inside the body of the loop or make the variable a reference + // CHECK-MESSAGES: [[@LINE-2]]:19: note: consider making 'ti' a reference + // CHECK-FIXES: for (const TypedefInt& ti : View>()) { + ti += 1; + } +} + +void NegativeLoopVariableIsInsideMacro() { + LOOP_AND_MODIFY(S1, S); +} + +template +struct ValueReturningIterator { + void operator++() {} + T operator*() { return T(); } + bool operator!=(const ValueReturningIterator &) { return false; } + typedef const T &const_reference; +}; + +void NegativeValueIterator() { + // Check does not trigger for iterators that return elements by value. + for (S S1 : View>()) { + S1.modify(); + } +} + +void NegativeConstructedByConversion() { + Convertible C[0]; + for (S S1 : C) { + S1.modify(); + } +} + +void NegativeNotConstructedByCopy() { + // Designed to exercise the unless(NotConstructedByCopy) clause in the check's source code. + // Distinct from NegativeConstructedByConversion because it exercises a converting constructor rather than a conversion operator. + ConstructorConvertible C[0]; + for (S S1 : C) { + S1.modify(); + } +} + +void PositiveLoopVariableIsStructuredBinding() { + for (auto [id, data] : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable '' is copied and then (possibly) modified; use an explicit copy inside the body of the loop or make the variable a reference + // CHECK-MESSAGES: [[@LINE-2]]:13: note: consider making '' a reference + // CHECK-FIXES: for (const auto& [id, data] : View>()) { + data.modify(); + } +} + +void NegativeLoopVariableIsStructuredBinding() { + for (const auto& [id, data] : View>()) { + } +}