[Consumed] Treat by-value class arguments as consuming by default, like rvalue refs.

[comex]

Summary

Also, fix the order of if statements so that an explicit return_typestate annotation takes precedence over the default behavior for rvalue refs.

Note that for by-value arguments, the object being consumed is always a temporary. If you write:

void a(X); a((X &&)x);

...this first constructs a temporary X by moving from x, then calls a with a pointer to the temporary. Before and after this change, the move copies x's typestate to the temporary and marks x itself as consumed. But before this change, if the temporary started out unconsumed (because x was unconsumed before the move), it would still be unconsumed when its destructor was run after the call. Now it will be considered consumed at that point.

Also note that currently, only parameters explicitly marked return_typestate have their state-on-return checked on the callee side. Parameters which are implicitly consuming due to being rvalue references – or, after this patch, by-value – are checked only on the caller side. This discrepancy was very surprising to me as a user. I wrote a fix, but in my codebase it broke some code that was using std::forward. Therefore, I plan to hold off on submitting the fix until a followup patch, which will generalize the current std::move special case into an attribute that can be applied to any 'cast' function like std::move and std::forward.

[dblaikie]

"Also, fix the order of if statements so that an explicit return_typestate annotation takes precedence over the default behavior for rvalue refs."

I'd probably have split that out into a separate patch - in part to discuss the design implications of that. I have some doubts about supporting non-consumed after passing by rvalue ref, but don't feel too strongly & the alternative would be having a warning about the attribute being ignored, etc - when it has a fairly clear meaning if it is there, just not sure people 'should' be doing that.

test/SemaCXX/warn-consumed-analysis.cpp

62–64

unless these are testing something noteworthy about them being static functions I'd probably suggest making them non-members like the other test functions below, for consistency (otherwise the inconsistency tends to raise the question of "what is significant about this difference?")

[comex]

In D67743#1675533, @dblaikie wrote:

"Also, fix the order of if statements so that an explicit return_typestate annotation takes precedence over the default behavior for rvalue refs."

I'd probably have split that out into a separate patch - in part to discuss the design implications of that. I have some doubts about supporting non-consumed after passing by rvalue ref, but don't feel too strongly & the alternative would be having a warning about the attribute being ignored, etc - when it has a fairly clear meaning if it is there, just not sure people 'should' be doing that.

Fair enough. I agree there's not much reason to do that, but it also seems pretty harmless to respect the user's choice. Silently ignoring the attribute as before is obviously wrong, so the alternative would be adding a diagnostic for that case, but it doesn't seem worth it...

unless these are testing something noteworthy about them being static functions I'd probably suggest making them non-members like the other test functions below, for consistency (otherwise the inconsistency tends to raise the question of "what is significant about this difference?")

Okay, I'll change them to non-members before committing.

lib/Analysis/Consumed.cpp

if (ParamTypestateAttr *PTA = Param->getAttr<ParamTypestateAttr>()) {
           Arg->getExprLoc(),
           stateToString(ExpectedState), stateToString(ParamState));
     }
 
     if (!(Entry->second.isVar() || Entry->second.isTmp()))
       continue;
 
     // Adjust state on the caller side.
-    if (isRValueRef(ParamType))
-      setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
-    else if (ReturnTypestateAttr *RT = Param->getAttr<ReturnTypestateAttr>())
+    if (ReturnTypestateAttr *RT = Param->getAttr<ReturnTypestateAttr>())
       setStateForVarOrTmp(StateMap, PInfo, mapReturnTypestateAttrState(RT));
+    else if (isRValueRef(ParamType) || isConsumableType(ParamType))
+      setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
     else if (isPointerOrRef(ParamType) &&
              (!ParamType->getPointeeType().isConstQualified() ||
               isSetOnReadPtrType(ParamType)))
       setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Unknown);
   }
 
   if (!ObjArg)
     return false;

test/SemaCXX/warn-consumed-analysis.cpp

public:
   void consume() SET_TYPESTATE(consumed);
   void unconsume() SET_TYPESTATE(unconsumed);
 };
 
 class CONSUMABLE(unconsumed) DestructorTester {
 public:
   DestructorTester();
   DestructorTester(int);
+  DestructorTester(nullptr_t) RETURN_TYPESTATE(unconsumed);
+  DestructorTester(DestructorTester &&);
   
   void operator*() CALLABLE_WHEN("unconsumed");
   
   ~DestructorTester() CALLABLE_WHEN("consumed");
+
+  static void byVal(DestructorTester);
+  static void byValMarkUnconsumed(DestructorTester RETURN_TYPESTATE(unconsumed));
 };
 
 void baf0(const ConsumableClass<int>  var);
 void baf1(const ConsumableClass<int> &var);
 void baf2(const ConsumableClass<int> *var);
 
 void baf3(ConsumableClass<int>   var);
 void baf4(ConsumableClass<int>  &var);
void testDestruction() {
   *D2; // expected-warning {{invalid invocation of method 'operator*' on object 'D2' while it is in the 'consumed' state}}
   
   D0.~DestructorTester(); // expected-warning {{invalid invocation of method '~DestructorTester' on object 'D0' while it is in the 'unconsumed' state}}
   
   return; // expected-warning {{invalid invocation of method '~DestructorTester' on object 'D0' while it is in the 'unconsumed' state}} \
              expected-warning {{invalid invocation of method '~DestructorTester' on object 'D1' while it is in the 'unconsumed' state}}
 }
 
+void testDestructionByVal() {
+  {
+    // both the var and the temporary are consumed:
+    DestructorTester D0(nullptr);
+    DestructorTester::byVal((DestructorTester &&)D0);
+  }
+  {
+    // the var is consumed but the temporary isn't:
+    DestructorTester D1(nullptr);
+    DestructorTester::byValMarkUnconsumed((DestructorTester &&)D1); // expected-warning {{invalid invocation of method '~DestructorTester' on a temporary object while it is in the 'unconsumed' state}}
+  }
+}
+
 void testTempValue() {
   *ConsumableClass<int>(); // expected-warning {{invalid invocation of method 'operator*' on a temporary object while it is in the 'consumed' state}}
 }
 
 void testSimpleRValueRefs() {
   ConsumableClass<int> var0;
   ConsumableClass<int> var1(42);
   
void testParamReturnTypestateCallee(bool cond, ConsumableClass<int> &Param RETURN_TYPESTATE(unconsumed)) { // expected-warning {{parameter 'Param' not in expected state when the function returns: expected 'unconsumed', observed 'consumed'}}
   if (cond) {
     Param.consume();
     return; // expected-warning {{parameter 'Param' not in expected state when the function returns: expected 'unconsumed', observed 'consumed'}}
   }
   
   Param.consume();
 }
 
+void testRvalueRefParamReturnTypestateCallee(ConsumableClass<int> &&Param RETURN_TYPESTATE(unconsumed)) {
+  Param.unconsume();
+}
+
 void testParamReturnTypestateCaller() {
   ConsumableClass<int> var;
   
   testParamReturnTypestateCallee(true, var);
+  testRvalueRefParamReturnTypestateCallee((ConsumableClass<int> &&)var);
   
   *var;
 }
 
 void testParamTypestateCallee(ConsumableClass<int>  Param0 PARAM_TYPESTATE(consumed),
                               ConsumableClass<int> &Param1 PARAM_TYPESTATE(consumed)) {
   
   *Param0; // expected-warning {{invalid invocation of method 'operator*' on object 'Param0' while it is in the 'consumed' state}}
void testCallingConventions() {
   baf0(var);  
   *var;
   
   baf1(var);  
   *var;
   
   baf2(&var);  
   *var;
-  
+  baf3(var);
+  *var;
+  
   baf4(var);  
   *var; // expected-warning {{invalid invocation of method 'operator*' on object 'var' while it is in the 'unknown' state}}
   
   var = ConsumableClass<int>(42);
   baf5(&var);  
   *var; // expected-warning {{invalid invocation of method 'operator*' on object 'var' while it is in the 'unknown' state}}
   
   var = ConsumableClass<int>(42);