Summary:Add clang::reinitializes attribute

[mboehme]

Summary

This is for use by clang-tidy's bugprone-use-after-move check -- see
corresponding clang-tidy patch at https://reviews.llvm.org/D49910.

[aaron.ballman]

Should this attribute have some semantic checking that ensures the non-static data members are accessed in the function that claims it reinitializes the object? Otherwise, it seems like this would not trigger any diagnostics:

class C {
  int a, b;
public:
  [[clang::reinitializes]] void clear() {}
};

include/clang/Basic/Attr.td

96

non-static, non-const member functions (with the comma)

2984

I think it makes sense to use CXX11 instead of Clang as this attribute doesn't make sense in C2x mode, but should there be a GNU spelling as well?

include/clang/Basic/AttrDocs.td

3465

While I kind of understand the restriction on a const member function, what about code that has mutable members being reset within a const method?

test/SemaCXX/attr-reinitializes.cpp

4

Please spell out the full diagnostic the first time it appears, and shorten the other uses, if you'd like.

10

Please also add tests that demonstrate the attribute is intended to accept no args.

[mboehme]

Various changes in response to reviewer comments.

[mboehme]

Should this attribute have some semantic checking that ensures the non-static data members are accessed in the function that claims it reinitializes the object?

I think this would be hard to do in a way that provides meaningful value.

We can't demand that the function should modify all member variables because not all functions that perform a logical "clear" or other reinitialization need to do this. For example, a typical implementation of std::string::clear() only modifies the string length but leaves the capacity and buffer unchanged.

The strongest requirement I think we can impose is that the function needs to modify at least one of the member variables -- possibly indirectly (i.e. through a call to another function). I don't think checking this would provide a real benefit though. We already disallow the reinitializes attribute for const member functions -- so if someone misuses the attribute, it'll be on a non-const member function that most likely modifies member variables, just not in a way that's guaranteed to reinitialize the object.

What we'd really want to check is whether the function is actually doing a logical reinitialization of the object. Instead of putting the 'reinitializes' attribute on the function, we'd want some way of expressing "this function has the precondition true and the postcondition empty()", and then we'd like to prove this at compile time or at least check it at runtime. That's obviously beyond the scope of what is possible in C++ today though.

include/clang/Basic/Attr.td

96

IIUC, Clang then translates the comma into an "and" -- so the actual diagnostic becomes "non-static and non-const member functions" (see the expected-error in the tests). Is this as intended?

2984

I have to admit I'm not sure. What are the usual considerations here? Does this need to be coordinated in any way with GNU, or can Clang simply introduce additional "GNU-style" spellings (i.e. with __attribute__) that are Clang-only?

I understand there's a difference between GCC spellings and GNU spellings -- but I'm not sure what the rules around the latter are. Let me know if you think this should have a GNU spelling, and I'll add it!

include/clang/Basic/AttrDocs.td

3465

I find it hard to envision a method that reinitializes an object by modifying only mutable member variables. Mutable members are, after all, intended to be used for purposes (such as caching and locking) that do not change the "logical state" of the object, whereas reinitialization is an operation that does change the logical state of the object.

If it really does turn out that there are legitimate use cases for applying the 'reinitializes' attribute to a const member function, we can always relax this requirement later on.

[aaron.ballman]

The attribute itself is looking reasonable aside from some minor nits, but this should not be committed until the clang-tidy functionality is approved (since it has no utility beyond clang-tidy).

include/clang/Basic/Attr.td

96

Ugh, that's a deficiency in ClangAttrEmitter.cpp. Good thing diagnostics aren't grammatically correct anyway, you can roll back to the previous form. Sorry about the churn!

2984

We generally want things with both spellings unless there's rationale justifying why only one spelling is better. I don't see any reason why this shouldn't have a GNU spelling as well.

I'd spell this as Clang<"reinitializes", 0> so it gets both the CXX11 and GNU spellings but not the C2x spelling.

include/clang/Basic/AttrDocs.td

3465

That makes sense to me, thank you for the explanation.

[mboehme]

Various changes in response to review comments.

[mboehme]

Thanks for the review!

In D49911#1186185, @aaron.ballman wrote:

The attribute itself is looking reasonable aside from some minor nits, but this should not be committed until the clang-tidy functionality is approved (since it has no utility beyond clang-tidy).

Will do.

include/clang/Basic/Attr.td

96

No worries!

2984

Thanks for the explanation -- done!

I've also added a test that the GNU spelling is recognized.

[mboehme]

Rebase to head.

include/clang/Basic/Attr.td

def NonParmVar : SubsetSubject<Var,
                                "variables">;
 def NonLocalVar : SubsetSubject<Var,
                                 [{!S->hasLocalStorage()}],
                                 "variables with non-local storage">;
 def NonBitField : SubsetSubject<Field,
                                 [{!S->isBitField()}],
                                 "non-bit-field non-static data members">;
 
+def NonStaticNonConstCXXMethod
+    : SubsetSubject<CXXMethod,
+                    [{!S->isStatic() && !S->isConst()}],
+                    "non-static non-const member functions">;
+
 def ObjCInstanceMethod : SubsetSubject<ObjCMethod,
                                        [{S->isInstanceMethod()}],
                                        "Objective-C instance methods">;
 
 def ObjCInterfaceDeclInitMethod : SubsetSubject<ObjCMethod,
                                [{S->getMethodFamily() == OMF_init &&
                                  (isa<ObjCInterfaceDecl>(S->getDeclContext()) ||
                                   (isa<ObjCCategoryDecl>(S->getDeclContext()) &&
def OMPDeclareTargetDecl : Attr {
   }];
 }
 
 def InternalLinkage : InheritableAttr {
   let Spellings = [Clang<"internal_linkage">];
   let Subjects = SubjectList<[Var, Function, CXXRecord]>;
   let Documentation = [InternalLinkageDocs];
 }
+
+def Reinitializes : InheritableAttr {
+  let Spellings = [Clang<"reinitializes", 0>];
+  let Subjects = SubjectList<[NonStaticNonConstCXXMethod], ErrorDiag>;
+  let Documentation = [ReinitializesDocs];
+}

include/clang/Basic/AttrDocs.td

def ArtificialDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 The ``artificial`` attribute can be applied to an inline function. If such a
 function is inlined, the attribute indicates that debuggers should associate
 the resulting instructions with the call site, rather than with the
 corresponding line within the inlined callee.
   }];
 }
+
+def ReinitializesDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The ``reinitializes`` attribute can be applied to a non-static, non-const C++
+member function to indicate that this member function reinitializes the entire
+object to a known state, independent of the previous state of the object.
+
+This attribute can be interpreted by static analyzers that warn about uses of an
+object that has been left in an indeterminate state by a move operation. If a
+member function marked with the ``reinitializes`` attribute is called on a
+moved-from object, the analyzer can conclude that the object is no longer in an
+indeterminate state.
+
+A typical example where this attribute would be used is on functions that clear
+a container class:
+
+.. code-block:: c++
+
+  template <class T>
+  class Container {
+  public:
+    ...
+    [[clang::reinitializes]] void Clear();
+    ...
+  };
+  }];
+}

lib/Sema/SemaDeclAttr.cpp

case ParsedAttr::AT_RenderScriptKernel:
     break;
   // XRay attributes.
   case ParsedAttr::AT_XRayInstrument:
     handleSimpleAttribute<XRayInstrumentAttr>(S, D, AL);
     break;
   case ParsedAttr::AT_XRayLogArgs:
     handleXRayLogArgsAttr(S, D, AL);
     break;
+
+  // Move semantics attribute.
+  case ParsedAttr::AT_Reinitializes:
+    handleSimpleAttribute<ReinitializesAttr>(S, D, AL);
+    break;
   }
 }
 
 /// ProcessDeclAttributeList - Apply all the decl attributes in the specified
 /// attribute list to the specified decl, ignoring any type attributes.
 void Sema::ProcessDeclAttributeList(Scope *S, Decl *D,
                                     const ParsedAttributesView &AttrList,
                                     bool IncludeCXX11Attributes) {

test/SemaCXX/attr-reinitializes.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+
+[[clang::reinitializes]] int a; // expected-error {{'reinitializes' attribute only applies to non-static non-const member functions}}
+
+[[clang::reinitializes]] void f(); // expected-error {{only applies to}}
+
+struct A {
+  [[clang::reinitializes]] void foo();
+  __attribute__((reinitializes)) void gnu_foo();
+  [[clang::reinitializes]] void bar() const; // expected-error {{only applies to}}
+  [[clang::reinitializes]] static void baz(); // expected-error {{only applies to}}
+  [[clang::reinitializes]] int a; // expected-error {{only applies to}}
+
+  [[clang::reinitializes("arg")]] void qux(); // expected-error {{'reinitializes' attribute takes no arguments}}
+};