[PATCH] clang-tidy check for incorrect move constructor initializers

[aaron.ballman]

Summary

This patch adds a new clang-tidy to identify situations where a user is accidentally calling a copy constructor instead of a move constructor as part of a constructor initializer list. The code is likely to silently "work", but with surprising results. Consider:

struct B {

B() {}
B(const B&) {}
B(B &&) {}

};

struct D : B {

D() : B() {}
D(const D &RHS) : B(RHS) {} // Correct
D(D &&RHS) : B(RHS) {} // Oops, calls the copy constructor

};

One thing I am not certain of in this patch is how to test it. I have some rudimentary tests, but am unable to test the "note:" diagnostics from FileCheck (attempting to add any cause the "warning:" diagnostics to not be found). I suspect this is why clang-tidy.sh exists, but unfortunately, that means the tests will not be run on Windows (which is the platform I am developing on). Suggestions on how to improve the tests are welcome, but for now, I'm not testing the note diagnostics.

Another problem I'm not certain how to solve is how to phrase the note diagnostics with the constructors are implicitly-defined instead of written. Currently, the notes attach to the record declaration. Would it make sense to have extra text in that case which tells the user the constructors are implicitly defined? Or is there a better way to surface the notes?

Thanks!

~Aaron

[aaron.ballman]

Ping?

FWIW, this patch almost caught a bug in LLVM. ;-) DependenceAnalysis.h has a class: FullDependence which would suffer from this problem if the Dependence base class did not accidentally suppress creation of the move constructor by defaulting only the copy constructor. (Separate patch forthcoming.) I think that may be a reasonable option for this patch to test for, but wasn't quite certain. What do others think?

~Aaron

[alexfh]

In D11784#220654, @aaron.ballman wrote:

Ping?

Sorry for the delay. I'm on vacation currently. Will be back tomorrow.

• Alex

[alexfh]

A high level question: why is this check specific to base class initialization? Isn't the same issue possible when initializing members, for example? What would change if in your example D would have a member of type B instead of deriving from it?

One thing I am not certain of in this patch is how to test it. I have some rudimentary tests, but am unable to test the "note:" diagnostics from
FileCheck (attempting to add any cause the "warning:" diagnostics to not be found).

Can you give an example of what you do and what results do you get?

I suspect this is why clang-tidy.sh exists, but unfortunately, that means the tests will not be run on Windows (which is the platform I am
developing on). Suggestions on how to improve the tests are welcome, but for now, I'm not testing the note diagnostics.

Converting the script to python seems like the most universal approach. Should be trivial with the sh python package, but I'm not sure whether we can rely on it being installed.

[alexfh]

In D11784#224386, @alexfh wrote:

Converting the script to python seems like the most universal approach. Should be trivial with the sh python package, but I'm not sure whether we can rely on it being installed.

Actually, python sh doesn't work on windows. I'll write a "pure" python replacement then.

[aaron.ballman]

In D11784#224386, @alexfh wrote:

A high level question: why is this check specific to base class initialization? Isn't the same issue possible when initializing members, for example? What would change if in your example D would have a member of type B instead of deriving from it?

Hmmm, I suppose this would apply equally to member initializers as well.

I was envisioning this as a strange dichotomy issue more than a performance issue. The caller goes to the trouble of move constructing, and you only half-move construct by calling the base class copy constructor which can potentially break class hierarchy invariants.

However, I think that's overthinking the situation and I should warn on member initialization that follows the same pattern.

One thing I am not certain of in this patch is how to test it. I have some rudimentary tests, but am unable to test the "note:" diagnostics from
FileCheck (attempting to add any cause the "warning:" diagnostics to not be found).

Can you give an example of what you do and what results do you get?

I put "CHECK: :[[@LINE+1]]:3: note: copy constructor being called" into the source file, and tests no longer pass because it cannot find the matching "warning: " diagnostic. If I then remove the warning diagnostic, the tests pass again. So it seems I can test one or the other, but not both. Specifically (with note and warning):

E:\llvm\2015>clang-tidy ..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp -checks=-*,misc-move-constructor-base -- -std=c++14 | FileCheck ..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp
2 warnings generated.
..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp:27:12: error: expected string not found in input
// CHECK: :[[@LINE+1]]:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-base]

^

<stdin>:5:2: note: scanning from here
B(const B&) {}
^
<stdin>:5:2: note: with expression "@LINE+1" equal to "28"
B(const B&) {}
^
<stdin>:10:94: note: possible intended match here
E:\llvm\2015\..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp:70:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-base]

             ^

with just note, no warning:

E:\llvm\2015>clang-tidy ..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp -checks=-*,misc-move-constructor-base -- -std=c++14 | FileCheck ..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp
2 warnings generated.

~Aaron

I suspect this is why clang-tidy.sh exists, but unfortunately, that means the tests will not be run on Windows (which is the platform I am
developing on). Suggestions on how to improve the tests are welcome, but for now, I'm not testing the note diagnostics.

Converting the script to python seems like the most universal approach. Should be trivial with the sh python package, but I'm not sure whether we can rely on it being installed.

[aaron.ballman]

In D11784#220654, @aaron.ballman wrote:

Ping?

FWIW, this patch almost caught a bug in LLVM. ;-) DependenceAnalysis.h has a class: FullDependence which would suffer from this problem if the Dependence base class did not accidentally suppress creation of the move constructor by defaulting only the copy constructor. (Separate patch forthcoming.) I think that may be a reasonable option for this patch to test for, but wasn't quite certain. What do others think?

Do you think that this patch should have an option for the case where the initialization cannot use a move constructor because the default one is deleted?

~Aaron

[alexfh]

In D11784#224421, @aaron.ballman wrote:

In D11784#224386, @alexfh wrote:

One thing I am not certain of in this patch is how to test it. I have some rudimentary tests, but am unable to test the "note:" diagnostics from
FileCheck (attempting to add any cause the "warning:" diagnostics to not be found).

Can you give an example of what you do and what results do you get?

I put "CHECK: :[[@LINE+1]]:3: note: copy constructor being called" into the source file, and tests no longer pass because it cannot find the matching "warning: " diagnostic. If I then remove the warning diagnostic, the tests pass again. So it seems I can test one or the other, but not both. Specifically (with note and warning):

Might it be that you got the line offsets in @LINE incorrectly? A test like this should work, if both the warning and the note are on the same line:

// CHECK: :[[@LINE+2]]:...: warning: ....
// CHECK: :[[@LINE+1]]:...: note: ....
some_line_that_generates_a_warning_with_a_note();

If your note is generated on a different line than the warning (e.g. class declaration vs. the incorrect use of a variable), then you may have to use @LINE+x for the warning and the line number verbatim for the note check.

[alexfh]

In D11784#224424, @aaron.ballman wrote:

In D11784#220654, @aaron.ballman wrote:

Do you think that this patch should have an option for the case where the initialization cannot use a move constructor because the default one is deleted?

Does it make sense to warn if the move constructor can't be used anyway? I'm not sure.

[aaron.ballman]

In D11784#224430, @alexfh wrote:

In D11784#224421, @aaron.ballman wrote:

In D11784#224386, @alexfh wrote:

One thing I am not certain of in this patch is how to test it. I have some rudimentary tests, but am unable to test the "note:" diagnostics from
FileCheck (attempting to add any cause the "warning:" diagnostics to not be found).

Can you give an example of what you do and what results do you get?

I put "CHECK: :[[@LINE+1]]:3: note: copy constructor being called" into the source file, and tests no longer pass because it cannot find the matching "warning: " diagnostic. If I then remove the warning diagnostic, the tests pass again. So it seems I can test one or the other, but not both. Specifically (with note and warning):

Might it be that you got the line offsets in @LINE incorrectly? A test like this should work, if both the warning and the note are on the same line:

// CHECK: :[[@LINE+2]]:...: warning: ....
// CHECK: :[[@LINE+1]]:...: note: ....
some_line_that_generates_a_warning_with_a_note();

The warning is on line 28, the note is on line 20.

If your note is generated on a different line than the warning (e.g. class declaration vs. the incorrect use of a variable), then you may have to use @LINE+x for the warning and the line number verbatim for the note check.

Ah, interesting -- I was avoiding that because of how fragile it is. Also, it seems to require me grouping the CHECK lines together. e.g.) this fails:

struct B {

B() {}
B(const B&) {}
B(B &&) {}   // CHECK: 19:3: note: copy constructor being called

};

struct D : B {

D() : B() {}
D(const D &RHS) : B(RHS) {}
// CHECK: :[[@LINE+1]]:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-base]
D(D &&RHS) : B(RHS) {}

};

But this succeeds:

struct B {

B() {}
B(const B&) {}
B(B &&) {}

};

struct D : B {

D() : B() {}
D(const D &RHS) : B(RHS) {}
// CHECK: :[[@LINE+2]]:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-base]
// CHECK: 19:3: note: copy constructor being called
D(D &&RHS) : B(RHS) {}

};

In any event, there's now a solution that lets me test the notes, so that's great. Thank you!

~Aaron

[alexfh]

In D11784#224433, @aaron.ballman wrote:

In D11784#224430, @alexfh wrote:

In D11784#224421, @aaron.ballman wrote:

In D11784#224386, @alexfh wrote:

Ah, interesting -- I was avoiding that because of how fragile it is. Also, it seems to require me grouping the CHECK lines together. e.g.) this fails:

FileCheck is rather primitive. It's behavior may confuse at first, but once you get accustomed to it, everything becomes clear.

[aaron.ballman]

In D11784#224437, @alexfh wrote:

In D11784#224433, @aaron.ballman wrote:

In D11784#224430, @alexfh wrote:

In D11784#224421, @aaron.ballman wrote:

In D11784#224386, @alexfh wrote:

Ah, interesting -- I was avoiding that because of how fragile it is. Also, it seems to require me grouping the CHECK lines together. e.g.) this fails:

FileCheck is rather primitive. It's behavior may confuse at first, but once you get accustomed to it, everything becomes clear.

I've definitely been spoiled by -verify. ;-)

~Aaron

[aaron.ballman]

Updated the patch to handle member initializers as well as base class initializers. This changes the name of things from "base" to "init" as well.

[alexfh]

Sorry for the long delay.

clang-tidy/misc/MoveConstructorInitCheck.cpp

41

clang-format?

47

This seems to be rather important to do from the beginning. Otherwise the check may be too noisy. BTW, did you run it over LLVM and Clang sources? Would be useful for some smoke testing.

test/clang-tidy/misc-move-constructor-init.cpp

65

I'd suggest using FileCheck -implicit-check-not='{{warning|error}}:' instead of stuffing the code with // CHECK-NOT: warning:. It will make the test more consistent with the other tests that use the clang_tidy_test.sh script.

[aaron.ballman]

clang-tidy/misc/MoveConstructorInitCheck.cpp

41

I thought the current formatting was an improvement over what clang-format produces (for those of us with debuggers that aren't as good at subexpression highlighting). I'm fine either way, though.

47

In order to do that, I would need access to more parts of Sema. The check, as it currently stands, is fairly reasonable from what I can tell. The false positive rate appears to be low. I ran it over Clang and LLVM and it did point out one debatably-true-positive (which we've since resolved) with no false-positives. In testing other code bases, the diagnostic was not chatty, but perhaps they did not make heavy use of move semantics.

test/clang-tidy/misc-move-constructor-init.cpp

65

Can do (though I am explicitly not using clang_tidy_test.sh because I am working on Windows and all those tests are currently disabled due to REQUIRES: shell :-()

[aaron.ballman]

Addressed review comments. I re-ran the updated patch against LLVM and Clang, and there were some more false positives that I would like to address if possible. It seems my previous run against the source base was before expanding the scope of the patch to include more than just base class initialization (sorry for the earlier misinformation).

1. SourceMgr.h:58 is an example where the checker issues a diagnostic (for IncludeLoc), but given the triviality of the type, I don't see a reason to diagnose. However, this requires support from Sema, so I think a FIXME may be the best I can do. Note, adding a call to std::move() in these instances is not wrong, it's just not particularly useful.
2. we should not be warning on anything an implicit constructor does. For instance LiveQueryResult is triggering this because of SlotIndex. This should be fixed with this patch.

Running over Clang and LLVM, there are 7 distinct false positives (repeated due to being in header files) and they all relate to triviality. The total false positive count was 832 of which two warnings (SourceMgr.h and Preprocessor.h) accounted for probably close to 90% of the diagnostics. This time around there were no true positives.

[alexfh]

In D11784#227939, @aaron.ballman wrote:

Addressed review comments. I re-ran the updated patch against LLVM and Clang, and there were some more false positives that I would like to address if possible. It seems my previous run against the source base was before expanding the scope of the patch to include more than just base class initialization (sorry for the earlier misinformation).

1. SourceMgr.h:58 is an example where the checker issues a diagnostic (for IncludeLoc), but given the triviality of the type, I don't see a reason to diagnose. However, this requires support from Sema, so I think a FIXME may be the best I can do. Note, adding a call to std::move() in these instances is not wrong, it's just not particularly useful.

Determining whether a type is trivially-copyable can be done on the AST level without Sema (see QualType::isTriviallyCopyableType).

2. we should not be warning on anything an implicit constructor does. For instance LiveQueryResult is triggering this because of SlotIndex. This should be fixed with this patch.

Running over Clang and LLVM, there are 7 distinct false positives (repeated due to being in header files) and they all relate to triviality. The total false positive count was 832 of which two warnings (SourceMgr.h and Preprocessor.h) accounted for probably close to 90% of the diagnostics. This time around there were no true positives.

Can you list the list of unique locations?

test/clang-tidy/misc-move-constructor-init.cpp

66

Please try to use the python script from http://reviews.llvm.org/D12180.

[aaron.ballman]

In D11784#228764, @alexfh wrote:

In D11784#227939, @aaron.ballman wrote:

Addressed review comments. I re-ran the updated patch against LLVM and Clang, and there were some more false positives that I would like to address if possible. It seems my previous run against the source base was before expanding the scope of the patch to include more than just base class initialization (sorry for the earlier misinformation).

1. SourceMgr.h:58 is an example where the checker issues a diagnostic (for IncludeLoc), but given the triviality of the type, I don't see a reason to diagnose. However, this requires support from Sema, so I think a FIXME may be the best I can do. Note, adding a call to std::move() in these instances is not wrong, it's just not particularly useful.

Determining whether a type is trivially-copyable can be done on the AST level without Sema (see QualType::isTriviallyCopyableType).

That should get us closer, but doesn't quite cut it. I think I may be able to combine this with CXXRecordDecl::isTriviallyCopyable() to get us close enough to cut down on the number of false positives. Thank you for pointing this out, it is implemented in this patch.

2. we should not be warning on anything an implicit constructor does. For instance LiveQueryResult is triggering this because of SlotIndex. This should be fixed with this patch.

Running over Clang and LLVM, there are 7 distinct false positives (repeated due to being in header files) and they all relate to triviality. The total false positive count was 832 of which two warnings (SourceMgr.h and Preprocessor.h) accounted for probably close to 90% of the diagnostics. This time around there were no true positives.

Can you list the list of unique locations?

With the triviality implementation, I now get zero false positives (and zero true positives) in the Clang and LLVM source base.

test/clang-tidy/misc-move-constructor-init.cpp

66

Unfortunately, that script does not work for me on Windows yet. I am happy to switch the test over to using the new script, even if it's a follow-up commit, whenever the new script has landed (I don't think it should block this patch).

[alexfh]

With the triviality implementation, I now get zero false positives (and zero true positives) in the Clang and LLVM source base.

Awesome! Thanks for working on this!

LGTM

[aaron.ballman]

Closed with commit r245571.

clang-tidy/misc/CMakeLists.txt

   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
+  MoveConstructorInitCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
+#include "MoveConstructorInitCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"

         "misc-macro-parentheses");
     CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
         "misc-macro-repeated-side-effects");
+    CheckFactories.registerCheck<MoveConstructorInitCheck>(
+        "misc-move-constructor-init");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<StaticAssertCheck>(

clang-tidy/misc/MoveConstructorInitCheck.h

+//===--- MoveConstructorInitCheck.h - clang-tidy-----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTRUCTORINITCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTRUCTORINITCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief The check flags user-defined move constructors that have a
+/// ctor-initializer initializing a member or base class through a copy
+/// constructor instead of a move constructor.
+class MoveConstructorInitCheck : public ClangTidyCheck {
+public:
+  MoveConstructorInitCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTRUCTORINITCHECK_H

clang-tidy/misc/MoveConstructorInitCheck.cpp

+//===--- MoveConstructorInitCheck.cpp - clang-tidy-------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "MoveConstructorInitCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void MoveConstructorInitCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+    constructorDecl(unless(isImplicit()), allOf(
+      isMoveConstructor(),
+      hasAnyConstructorInitializer(
+        ctorInitializer(withInitializer(constructExpr(hasDeclaration(
+          constructorDecl(isCopyConstructor()).bind("ctor")
+        )))).bind("init")
+      )
+    )), this);
+}
+
+void MoveConstructorInitCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *CopyCtor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
+  const auto *Initializer = Result.Nodes.getNodeAs<CXXCtorInitializer>("init");
+
+  // Do not diagnose if the expression used to perform the initialization is a
+  // trivially-copyable type.
+  QualType QT = Initializer->getInit()->getType();
+  if (QT.isTriviallyCopyableType(*Result.Context))
+    return;
+  
+  const auto *RD = QT->getAsCXXRecordDecl();
+  if (RD && RD->isTriviallyCopyable())
+    return;
+
+  // Diagnose when the class type has a move constructor available, but the
+  // ctor-initializer uses the copy constructor instead.
+  const CXXConstructorDecl *Candidate = nullptr;
+  for (const auto *Ctor : CopyCtor->getParent()->ctors()) {
+    if (Ctor->isMoveConstructor() && Ctor->getAccess() <= AS_protected &&
+        !Ctor->isDeleted()) {
+      // The type has a move constructor that is at least accessible to the
+      // initializer.
+      //
+      // FIXME: Determine whether the move constructor is a viable candidate
+      // for the ctor-initializer, perhaps provide a fixit that suggests
+      // using std::move().
+      Candidate = Ctor;
+      break;
+    }
+  }
+
+  if (Candidate) {
+    // There's a move constructor candidate that the caller probably intended
+    // to call instead.
+    diag(Initializer->getSourceLocation(),
+         "move constructor initializes %0 by calling a copy constructor")
+        << (Initializer->isBaseInitializer() ? "base class" : "class member");
+    diag(CopyCtor->getLocation(), "copy constructor being called",
+         DiagnosticIDs::Note);
+    diag(Candidate->getLocation(), "candidate move constructor here",
+         DiagnosticIDs::Note);
+  }
+}
+
+} // namespace tidy
+} // namespace clang
+

test/clang-tidy/misc-move-constructor-init.cpp

+// RUN: clang-tidy %s -checks=-*,misc-move-constructor-init -- -std=c++14 | FileCheck %s -implicit-check-not="{{warning|error}}:"
+
+template <class T> struct remove_reference      {typedef T type;};
+template <class T> struct remove_reference<T&>  {typedef T type;};
+template <class T> struct remove_reference<T&&> {typedef T type;};
+
+template <typename T>
+typename remove_reference<T>::type&& move(T&& arg) {
+  return static_cast<typename remove_reference<T>::type&&>(arg);
+}
+
+struct C {
+  C() = default;
+  C(const C&) = default;
+};
+
+struct B {
+  B() {}
+  B(const B&) {}
+  B(B &&) {}
+};
+
+struct D : B {
+  D() : B() {}
+  D(const D &RHS) : B(RHS) {}
+  // CHECK: :[[@LINE+3]]:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-init]
+  // CHECK: 19:3: note: copy constructor being called
+  // CHECK: 20:3: note: candidate move constructor here
+  D(D &&RHS) : B(RHS) {}
+};
+
+struct E : B {
+  E() : B() {}
+  E(const E &RHS) : B(RHS) {}
+  E(E &&RHS) : B(move(RHS)) {} // ok
+};
+
+struct F {
+  C M;
+
+  F(F &&) : M(C()) {} // ok
+};
+
+struct G {
+  G() = default;
+  G(const G&) = default;
+  G(G&&) = delete;
+};
+
+struct H : G {
+  H() = default;
+  H(const H&) = default;
+  H(H &&RHS) : G(RHS) {} // ok
+};
+
+struct I {
+  I(const I &) = default; // suppresses move constructor creation
+};
+
+struct J : I {
+  J(J &&RHS) : I(RHS) {} // ok
+};
+
+struct K {}; // Has implicit copy and move constructors, is trivially copyable
+struct L : K {
+  L(L &&RHS) : K(RHS) {} // ok
+};
+
+struct M {
+  B Mem;
+  // CHECK: :[[@LINE+1]]:16: warning: move constructor initializes class member by calling a copy constructor [misc-move-constructor-init]
+  M(M &&RHS) : Mem(RHS.Mem) {}
+};
+
+struct N {
+  B Mem;
+  N(N &&RHS) : Mem(move(RHS.Mem)) {}
+};