[clang-tidy] Recognize [[clang::reinitializes]] attribute in bugprone-use-after-move

[mboehme]

Summary

This allows member functions to be marked as reinitializing the object. After a
moved-from object has been reinitialized, the check will no longer consider it
to be in an indeterminate state.

The patch that adds the attribute itself is at https://reviews.llvm.org/D49911

[mboehme]

[clang-tidy] Recognize [[clang::reinitializes]] attribute in bugprone-use-after-move.

This allows member functions to be marked as reinitializing the object. After a
moved-from object has been reinitialized, the check will no longer consider it
to be in an indeterminate state.

I'll submit a patch for Clang in a moment that adds the attribute itself.

Reviewers:
aaron.ballman, ilya-biryukov

Subscribers:
cfe-commits

[mboehme]

Any comments?

https://reviews.llvm.org/D49911, on which this relies, is now ready to land.

[aaron.ballman]

Are you going to propose adding this attribute to libc++, or is this expected to only work with UDTs?

[mboehme]

In D49910#1187455, @aaron.ballman wrote:

Are you going to propose adding this attribute to libc++, or is this expected to only work with UDTs?

I don't have any experience contributing to libc++, but I think this would make sense.

The check currently hard-codes various member functions of classes in the "std" namespace that do reinitializations; I'm not sure though if those can be removed after the attribute has been added to libc++. We'd would also presumably have to add the attribute to libstdc++ -- does it accept Clang-only attributes? And what is the story for people using clang-tidy with MSVC projects? (I have to admit I'm very hazy on how that works...)

[aaron.ballman]

In D49910#1187492, @mboehme wrote:

In D49910#1187455, @aaron.ballman wrote:

Are you going to propose adding this attribute to libc++, or is this expected to only work with UDTs?

I don't have any experience contributing to libc++, but I think this would make sense.

The check currently hard-codes various member functions of classes in the "std" namespace that do reinitializations; I'm not sure though if those can be removed after the attribute has been added to libc++. We'd would also presumably have to add the attribute to libstdc++ -- does it accept Clang-only attributes? And what is the story for people using clang-tidy with MSVC projects? (I have to admit I'm very hazy on how that works...)

I ask the question because it's novel to add an attribute to Clang that's used only by clang-tidy, and I'm wondering who is expected to use that attribute to help with this check. Is it expected to be an attribute users use with their own custom datatypes and it's not needed for standards-based APIs (because we handle those some other way), or something else? As it stands, I sort of feel like this is a very heavy approach to solve an edge case -- is there a lot of evidence for re-using a moved-from object after reinitializing it?

[mboehme]

In D49910#1187809, @aaron.ballman wrote:

In D49910#1187492, @mboehme wrote:

In D49910#1187455, @aaron.ballman wrote:

Are you going to propose adding this attribute to libc++, or is this expected to only work with UDTs?

I don't have any experience contributing to libc++, but I think this would make sense.

The check currently hard-codes various member functions of classes in the "std" namespace that do reinitializations; I'm not sure though if those can be removed after the attribute has been added to libc++. We'd would also presumably have to add the attribute to libstdc++ -- does it accept Clang-only attributes? And what is the story for people using clang-tidy with MSVC projects? (I have to admit I'm very hazy on how that works...)

I ask the question because it's novel to add an attribute to Clang that's used only by clang-tidy, and I'm wondering who is expected to use that attribute to help with this check. Is it expected to be an attribute users use with their own custom datatypes and it's not needed for standards-based APIs (because we handle those some other way), or something else? As it stands, I sort of feel like this is a very heavy approach to solve an edge case -- is there a lot of evidence for re-using a moved-from object after reinitializing it?

Ah, now I understand what you're getting at.

Yes, I see this attribute mainly as something that people would add to their own user-defined types -- typically, containers and smart pointers.

I've regularly been getting internal feature requests for this. One typical pattern in which this comes up is the following:

MyContainer<T> container;
T t;
while (GetNextT(&t)) {

container.Add(t);
if (SomeCondition()) {
  PassToConsumer(std::move(container));
  container.Clear();
}

}

I.e. you're incrementally adding items to some data structure, and at some point you decide the data structure is now complete, so you hand it off to a consumer and clear it, ready to be filled with the next batch of items.

As clang-tidy doesn't understand that the "container.Clear()" reinitializes the container, it complains that the Clear() is a use-after-move.

Yes, it's unusual to add an attribute to Clang that is only (at least initially) intended for use by clang-tidy -- but I don't really see how to implement this other than with an attribute.

[mboehme]

Sorry, that code got formatted strangely. Here's the same code block in more legible form:

MyContainer<T> container;
T t;
while (GetNextT(&t)) {
  container.Add(t);
  if (SomeCondition()) {
    PassToConsumer(std::move(container));
    container.Clear();
  }
}

[aaron.ballman]

LGTM! An attribute really is the right way to go for this sort of thing, and it sounds like you've got a base of users looking for the functionality.

[mboehme]

Rebase to head.

[mboehme]

Thank you for the review!

clang-tools-extra/trunk/clang-tidy/bugprone/UseAfterMoveCheck.cpp

auto ReinitMatcher =
                    // deque, forward_list and list have assign(). If assign()
                    // is called on any of the other containers, this will be
                    // flagged by a compile error anyway.
                    callee(cxxMethodDecl(hasAnyName("clear", "assign")))),
                // reset() on standard smart pointers.
                cxxMemberCallExpr(
                    on(allOf(DeclRefMatcher, StandardSmartPointerTypeMatcher)),
                    callee(cxxMethodDecl(hasName("reset")))),
+               // Methods that have the [[clang::reinitializes]] attribute.
+               cxxMemberCallExpr(
+                   on(DeclRefMatcher),
+                   callee(cxxMethodDecl(hasAttr(clang::attr::Reinitializes)))),
                // Passing variable to a function as a non-const pointer.
                callExpr(forEachArgumentWithParam(
                    unaryOperator(hasOperatorName("&"),
                                  hasUnaryOperand(DeclRefMatcher)),
                    unless(parmVarDecl(hasType(pointsTo(isConstQualified())))))),
                // Passing variable to a function as a non-const lvalue reference
                // (unless that function is std::move()).
                callExpr(forEachArgumentWithParam(

clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-use-after-move.rst

- ``clear()`` or ``assign()`` is called on the variable and the variable is of
     one of the standard container types ``basic_string``, ``vector``, ``deque``,
     ``forward_list``, ``list``, ``set``, ``map``, ``multiset``, ``multimap``,
     ``unordered_set``, ``unordered_map``, ``unordered_multiset``,
     ``unordered_multimap``.
 
   - ``reset()`` is called on the variable and the variable is of type
     ``std::unique_ptr``, ``std::shared_ptr`` or ``std::weak_ptr``.
 
+  - A member function marked with the ``[[clang::reinitializes]]`` attribute is
+    called on the variable.
+
 If the variable in question is a struct and an individual member variable of
 that struct is written to, the check does not consider this to be a
 reinitialization -- even if, eventually, all member variables of the struct are
 written to. For example:
 
   .. code-block:: c++
 
     struct S {

clang-tools-extra/trunk/test/clang-tidy/bugprone-use-after-move.cpp

public:
   void foo() const;
   int getInt() const;
 
   operator bool() const;
 
   int i;
 };
 
+template <class T>
+class AnnotatedContainer {
+public:
+  AnnotatedContainer();
+
+  void foo() const;
+  [[clang::reinitializes]] void clear();
+};
+
 ////////////////////////////////////////////////////////////////////////////////
 // General tests.
 
 // Simple case.
 void simple() {
   A a;
   a.foo();
   A other_a = std::move(a);
void standardSmartPointerResetIsReinit() {
   {
     std::shared_ptr<A> ptr;
     std::move(ptr);
     ptr.reset(new A);
     *ptr;
   }
 }
 
+void reinitAnnotation() {
+  {
+    AnnotatedContainer<int> obj;
+    std::move(obj);
+    obj.foo();
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'obj' used after it was
+    // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here
+  }
+  {
+    AnnotatedContainer<int> obj;
+    std::move(obj);
+    obj.clear();
+    obj.foo();
+  }
+  {
+    // Calling clear() on a different object to the one that was moved is not
+    // considered a reinitialization.
+    AnnotatedContainer<int> obj1, obj2;
+    std::move(obj1);
+    obj2.clear();
+    obj1.foo();
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'obj1' used after it was
+    // CHECK-MESSAGES: [[@LINE-4]]:5: note: move occurred here
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Tests related to order of evaluation within expressions
 
 // Relative sequencing of move and use.
 void passByRvalueReference(int i, A &&a);
 void passByValue(int i, A a);
 void passByValue(A a, int i);
 A g(A, A &&);