[clang-tidy] Add misc-mutating-copy check

[gbencze]

Summary

The check warns when (a member of) the copied object is assigned to in a
copy constructor or copy assignment operator. Based on
https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object

[mgehre]

Have you run your check over the LLVM/clang source base and seen true positives/false positives?

clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst

6 ↗

(On Diff #228606)

to and to

[gbencze]

In D70052#1740075, @mgehre wrote:

Have you run your check over the LLVM/clang source base and seen true positives/false positives?

I tested it on the Xerces and Bitcoin codebases and did not get any warnings.

[mgehre]

Did you consider to warn on copy constructors/assignment operators take a their arguments by non-const reference, and suggest the user to turn them into const references? This seems like a more useful (and easier) check to me.
The link above contains Ideally, the copy operator should have an idiomatic signature. For copy constructors, that is T(const T&); and for copy assignment operators, that is T& operator=(const T&);..

The only remaining case are then mutable members which are quite rare.

[gbencze]

In D70052#1740142, @mgehre wrote:

Did you consider to warn on copy constructors/assignment operators take a their arguments by non-const reference, and suggest the user to turn them into const references? This seems like a more useful (and easier) check to me.
The link above contains Ideally, the copy operator should have an idiomatic signature. For copy constructors, that is T(const T&); and for copy assignment operators, that is T& operator=(const T&);..

The only remaining case are then mutable members which are quite rare.

I haven't really considered it as the non-compliant example has the idiomatic signature, but it probably would be a good addition to the check. misc-unconventional-assign-operator already seems to do this for the assignment operator.

[Eugene.Zelenko]

If this is CERT rule, why check is not placed in relevant module?

clang-tools-extra/docs/ReleaseNotes.rst

149

Please synchronize with first statement in documentation.

[gbencze]

In D70052#1740235, @Eugene.Zelenko wrote:

If this is CERT rule, why check is not placed in relevant module?

To be honest I was hoping for some feedback on this as I wasn't sure what the best place for this check would be. Quite a few CERT rules seem to be implemented in other modules and have cert aliases.
Do you think this check should be moved there or should I just add an alias?

[gbencze]

Update documentation and release notes.

[Eugene.Zelenko]

In D70052#1741246, @gbencze wrote:

In D70052#1740235, @Eugene.Zelenko wrote:

If this is CERT rule, why check is not placed in relevant module?

To be honest I was hoping for some feedback on this as I wasn't sure what the best place for this check would be. Quite a few CERT rules seem to be implemented in other modules and have cert aliases.
Do you think this check should be moved there or should I just add an alias?

Misc is just heap of unsorted checks, so CERT is definitely better place.

[aaron.ballman]

In D70052#1741246, @gbencze wrote:

In D70052#1740235, @Eugene.Zelenko wrote:

If this is CERT rule, why check is not placed in relevant module?

To be honest I was hoping for some feedback on this as I wasn't sure what the best place for this check would be. Quite a few CERT rules seem to be implemented in other modules and have cert aliases.
Do you think this check should be moved there or should I just add an alias?

I'd probably implement this only in the cert module, but if we wanted to expose it outside of there as well, I'd suggest bugprone.

clang-tools-extra/test/clang-tidy/misc-mutating-copy.cpp

107 ↗

(On Diff #228758)

I think a case like this should also diagnose:

class S {
  int a;
public:
  void fine_func() const;
  void questionable_func();
  void bad_func() { a = 12; }

  S(S& other) : a(other.a) {
    other.fine_func(); // This is fine
    other.bad_func(); // This is a problem
    other.questionable_func(); // Not certain what to do here.
  }

  bool operator==(const S& other) { return a == other.a; }
};

[gbencze]

Moved check to CERT module.
Added warnings for calling mutating member functions.

[JonasToth]

There is a ExprMutAnalyzer that is able to find mutation of expressions in general (even though it is kinda experimental still). Maybe that should be utilized somehow? I think the current implementation does not cover when the address is taken and mutation happens through pointers/references in free standing functions, does it?

On the other hand it makes the check more complicated, slower. Additionally the most cases are catched with this version, i guess.

[aaron.ballman]

clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp

74–80

You can elide the curly braces here per our usual coding conventions.

clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst

12

Please add the newline to the end of the file.

clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp

2

This -std looks incorrect to me -- you can remove it entirely (we already default to later than C++11 anyway).

150

Please add the newline to the end of the file.

[gbencze]

Formatting changes (curly braces, newline at EOF)
Remove incorrect flag from test

[gbencze]

In D70052#1749730, @JonasToth wrote:

There is a ExprMutAnalyzer that is able to find mutation of expressions in general (even though it is kinda experimental still). Maybe that should be utilized somehow? I think the current implementation does not cover when the address is taken and mutation happens through pointers/references in free standing functions, does it?

On the other hand it makes the check more complicated, slower. Additionally the most cases are catched with this version, i guess.

You're right, the current version does not cover mutations through pointers and references. I'm not sure how common these would be, but ExprMutAnalyzer seems like a great option if we wanted to add support for those cases as well.

[gbencze]

Mark comments as Done.

[Charusso]

I think it is fine, but please let us wait with the final thoughts from @aaron.ballman.

clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp

22

What about just "assignment" and "member-call"?

64

I think hasDescendant() is more appropriate compared to the slower forEachDescendant().

[aaron.ballman]

In D70052#1750664, @gbencze wrote:

In D70052#1749730, @JonasToth wrote:

There is a ExprMutAnalyzer that is able to find mutation of expressions in general (even though it is kinda experimental still). Maybe that should be utilized somehow? I think the current implementation does not cover when the address is taken and mutation happens through pointers/references in free standing functions, does it?

On the other hand it makes the check more complicated, slower. Additionally the most cases are catched with this version, i guess.

You're right, the current version does not cover mutations through pointers and references. I'm not sure how common these would be, but ExprMutAnalyzer seems like a great option if we wanted to add support for those cases as well.

It also doesn't cover mutations through function calls. e.g.,

void some_func(S &); // Could mutate the passed object

struct S {
  int i, j;
  S(S &s) : i(s.i), j(s.j) { some_func(s); }
};

However, I think the clang-tidy check will suffice for catching a significant percentage of mutating copy bugs so it seems reasonable to keep this rather than insist on a static analyzer check.

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
 #include "../performance/MoveConstructorInitCheck.h"
 #include "../readability/UppercaseLiteralSuffixCheck.h"
 #include "CommandProcessorCheck.h"
 #include "DefaultOperatorNewAlignmentCheck.h"
 #include "DontModifyStdNamespaceCheck.h"
 #include "FloatLoopCounter.h"
 #include "LimitedRandomnessCheck.h"
+#include "MutatingCopyCheck.h"
 #include "PostfixOperatorCheck.h"
 #include "ProperlySeededRandomGeneratorCheck.h"
 #include "SetLongJmpCheck.h"
 #include "StaticObjectExceptionCheck.h"
 #include "StrToNumCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc50-cpp");
     CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
         "cert-msc51-cpp");
     // OOP
     CheckFactories.registerCheck<performance::MoveConstructorInitCheck>(
         "cert-oop11-cpp");
     CheckFactories.registerCheck<bugprone::UnhandledSelfAssignmentCheck>(
         "cert-oop54-cpp");
+    CheckFactories.registerCheck<MutatingCopyCheck>(
+        "cert-oop58-cpp");
 
     // C checkers
     // DCL
     CheckFactories.registerCheck<misc::StaticAssertCheck>("cert-dcl03-c");
     CheckFactories.registerCheck<readability::UppercaseLiteralSuffixCheck>(
         "cert-dcl16-c");
     // ENV
     CheckFactories.registerCheck<CommandProcessorCheck>("cert-env33-c");

clang-tools-extra/clang-tidy/cert/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCERTModule
   CERTTidyModule.cpp
   CommandProcessorCheck.cpp
   DefaultOperatorNewAlignmentCheck.cpp
   DontModifyStdNamespaceCheck.cpp
   FloatLoopCounter.cpp
   LimitedRandomnessCheck.cpp
+  MutatingCopyCheck.cpp
   PostfixOperatorCheck.cpp
   ProperlySeededRandomGeneratorCheck.cpp
   SetLongJmpCheck.cpp
   StaticObjectExceptionCheck.cpp
   StrToNumCheck.cpp
   ThrownExceptionTypeCheck.cpp
   VariadicFunctionDefCheck.cpp
 

clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.h

+//===--- MutatingCopyCheck.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_CERT_MUTATINGCOPYCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_MUTATINGCOPYCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Finds assignments to the copied object and its direct or indirect members
+/// in copy constructors and copy assignment operators.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-oop58-cpp.html
+class MutatingCopyCheck : public ClangTidyCheck {
+public:
+  MutatingCopyCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_MUTATINGCOPYCHECK_H

clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp

+//===--- MutatingCopyCheck.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 "MutatingCopyCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+static constexpr llvm::StringLiteral SourceDeclName = "ChangedPVD";
+static constexpr llvm::StringLiteral MutatingOperatorName = "MutatingOp";
+static constexpr llvm::StringLiteral MutatingCallName = "MutatingCall";
+
+void MutatingCopyCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  const auto MemberExprOrSourceObject = anyOf(
+      memberExpr(), declRefExpr(to(decl(equalsBoundNode(SourceDeclName)))));
+
+  const auto IsPartOfSource =
+      allOf(unless(hasDescendant(expr(unless(MemberExprOrSourceObject)))),
+            MemberExprOrSourceObject);
+
+  const auto IsSourceMutatingAssignment =
+      expr(anyOf(binaryOperator(isAssignmentOperator(), hasLHS(IsPartOfSource))
+                     .bind(MutatingOperatorName),
+                 cxxOperatorCallExpr(isAssignmentOperator(),
+                                     hasArgument(0, IsPartOfSource))
+                     .bind(MutatingOperatorName)));
+
+  const auto MemberExprOrSelf = anyOf(memberExpr(), cxxThisExpr());
+
+  const auto IsPartOfSelf = allOf(
+      unless(hasDescendant(expr(unless(MemberExprOrSelf)))), MemberExprOrSelf);
+
+  const auto IsSelfMutatingAssignment =
+      expr(anyOf(binaryOperator(isAssignmentOperator(), hasLHS(IsPartOfSelf)),
+                 cxxOperatorCallExpr(isAssignmentOperator(),
+                                     hasArgument(0, IsPartOfSelf))));
+
+  const auto IsSelfMutatingMemberFunction =
+      functionDecl(hasBody(hasDescendant(IsSelfMutatingAssignment)));
+
+  const auto IsSourceMutatingMemberCall =
+      cxxMemberCallExpr(on(IsPartOfSource),
+                        callee(IsSelfMutatingMemberFunction))
+          .bind(MutatingCallName);
+
+  const auto MutatesSource = allOf(
+      hasParameter(
+          0, parmVarDecl(hasType(lValueReferenceType())).bind(SourceDeclName)),
+      anyOf(forEachDescendant(IsSourceMutatingAssignment),
+            forEachDescendant(IsSourceMutatingMemberCall)));
+
+  Finder->addMatcher(cxxConstructorDecl(isCopyConstructor(), MutatesSource),
+                     this);
+
+  Finder->addMatcher(cxxMethodDecl(isCopyAssignmentOperator(), MutatesSource),
+                     this);
+}
+
+void MutatingCopyCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *MemberCall =
+          Result.Nodes.getNodeAs<CXXMemberCallExpr>(MutatingCallName))
+    diag(MemberCall->getBeginLoc(), "call mutates copied object");
+  else if (const auto *Assignment =
+               Result.Nodes.getNodeAs<Expr>(MutatingOperatorName))
+    diag(Assignment->getBeginLoc(), "mutating copied object");
+}
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`cert-mem57-cpp
   Checks if an object of type with extended alignment is allocated by using
   the default ``operator new``.
 
 - New alias :doc:`cert-pos44-c
   <clang-tidy/checks/cert-pos44-c>` to
   :doc:`bugprone-bad-signal-to-kill-thread
   <clang-tidy/checks/bugprone-bad-signal-to-kill-thread>` was added.
 
+- New :doc:`cert-oop58-cpp
+  <clang-tidy/checks/cert-oop58-cpp>` check.
+
+  Finds assignments to the copied object and its direct or indirect members
+  in copy constructors and copy assignment operators.
+
 - New :doc:`cppcoreguidelines-init-variables
   <clang-tidy/checks/cppcoreguidelines-init-variables>` check.
 
 - New :doc:`darwin-dispatch-once-nonstatic
   <clang-tidy/checks/darwin-dispatch-once-nonstatic>` check.
 
   Finds declarations of ``dispatch_once_t`` variables without static or global
   storage.
- New :doc:`llvm-prefer-register-over-unsigned
   <clang-tidy/checks/llvm-prefer-register-over-unsigned>` check.
 
   Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites
   them to use ``Register``
 
 - New :doc:`objc-missing-hash
   <clang-tidy/checks/objc-missing-hash>` check.
 
   Finds Objective-C implementations that implement ``-isEqual:`` without also
   appropriately implementing ``-hash``.
 
 - New :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance-no-automatic-move>` check.
 
   Finds local variables that cannot be automatically moved due to constness.
 
 - New :doc:`performance-trivially-destructible

clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst

+.. title:: clang-tidy - cert-mutating-copy
+
+cert-oop58-cpp
+==============
+
+Finds assignments to the copied object and its direct or indirect members
+in copy constructors and copy assignment operators.
+
+This check corresponds to the CERT C Coding Standard rule
+`OOP58-CPP. Copy operations must not mutate the source object
+<https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object>`_.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. toctree::
    cert-flp30-c
    cert-mem57-cpp
    cert-msc30-c (redirects to cert-msc50-cpp) <cert-msc30-c>
    cert-msc32-c (redirects to cert-msc51-cpp) <cert-msc32-c>
    cert-msc50-cpp
    cert-msc51-cpp
    cert-oop11-cpp (redirects to performance-move-constructor-init) <cert-oop11-cpp>
    cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) <cert-oop54-cpp>
+   cert-oop58-cpp
    cert-pos44-c (redirects to bugprone-bad-signal-to-kill-thread) <cert-pos44-c>
    clang-analyzer-core.CallAndMessage (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.CallAndMessage>
    clang-analyzer-core.DivideZero (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.DivideZero>
    clang-analyzer-core.DynamicTypePropagation
    clang-analyzer-core.NonNullParamChecker (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.NonNullParamChecker>
    clang-analyzer-core.NullDereference (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.NullDereference>
    clang-analyzer-core.StackAddressEscape (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.StackAddressEscape>
    clang-analyzer-core.UndefinedBinaryOperatorResult (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.UndefinedBinaryOperatorResult>

clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp

+// RUN: %check_clang_tidy %s cert-oop58-cpp %t
+
+// Example test cases from CERT rule
+// https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object
+namespace test_mutating_noncompliant_example {
+class A {
+  mutable int m;
+
+public:
+  A() : m(0) {}
+  explicit A(int m) : m(m) {}
+
+  A(const A &other) : m(other.m) {
+    other.m = 0;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+
+  A &operator=(const A &other) {
+    if (&other != this) {
+      m = other.m;
+      other.m = 0;
+      // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: mutating copied object
+    }
+    return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_noncompliant_example
+
+namespace test_mutating_compliant_example {
+class B {
+  int m;
+
+public:
+  B() : m(0) {}
+  explicit B(int m) : m(m) {}
+
+  B(const B &other) : m(other.m) {}
+  B(B &&other) : m(other.m) {
+    other.m = 0; //no-warning: mutation allowed in move constructor
+  }
+
+  B &operator=(const B &other) {
+    if (&other != this) {
+      m = other.m;
+    }
+    return *this;
+  }
+
+  B &operator=(B &&other) {
+    m = other.m;
+    other.m = 0; //no-warning: mutation allowed in move assignment operator
+    return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_compliant_example
+
+namespace test_mutating_pointer {
+class C {
+  C *ptr;
+  int value;
+
+  C();
+  C(C &other) {
+    other = {};
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+    other.ptr = nullptr;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+    other.value = 0;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+
+    // no-warning: mutating a pointee is allowed
+    other.ptr->value = 0;
+    *other.ptr = {};
+  }
+};
+} // namespace test_mutating_pointer
+
+namespace test_mutating_indirect_member {
+struct S {
+  int x;
+};
+
+class D {
+  S s;
+  D(D &other) {
+    other.s = {};
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+    other.s.x = 0;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+};
+} // namespace test_mutating_indirect_member
+
+namespace test_mutating_other_object {
+class E {
+  E();
+  E(E &other) {
+    E tmp;
+    // no-warning: mutating an object that is not the source is allowed
+    tmp = {};
+  }
+};
+} // namespace test_mutating_other_object
+
+namespace test_mutating_member_function {
+class F {
+  int a;
+
+public:
+  void bad_func() { a = 12; }
+  void fine_func() const;
+  void fine_func_2(int x) { x = 5; }
+  void questionable_func();
+
+  F(F &other) : a(other.a) {
+    this->bad_func(); // no-warning: mutating this is allowed
+
+    other.bad_func();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: call mutates copied object
+
+    other.fine_func();
+    other.fine_func_2(42);
+    other.questionable_func();
+  }
+};
+} // namespace test_mutating_member_function
+
+namespace test_mutating_function_on_nested_object {
+struct S {
+  int x;
+  void mutate(int y) {
+    x = y;
+  }
+};
+
+class G {
+  S s;
+  G(G &other) {
+    s.mutate(0); // no-warning: mutating this is allowed
+
+    other.s.mutate(0);
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: call mutates copied object
+  }
+};
+} // namespace test_mutating_function_on_nested_object