[Consumed] Refactor and improve diagnostics

[comex]

Summary

As suggested by FIXME comments, fix commented-out diagnostic in Sema and remove the equivalent check within the consumed analysis.

The diagnostic in question is the warning for using param_typestate and return_typestate with a type that is not consumable.

There were several FIXME comments about the same issue; the most detailed was before the commented-out check:

// FIXME: This check is currently being done in the analysis.  It can be
//        enabled here only after the parser propagates attributes at
//        template specialization definition, not declaration.

I was confused what this meant. After investigating, I think it actually refers to the fact that attributes are parsed only once for a template, and are not re-parsed when the template is instantiated. If I'm right, the issue actually has nothing to do with template specializations or with definitions versus declarations. I could be missing something, though, so please let me know if there's a case I'm not thinking of. I did add some template specializations as test cases (for both class and function templates).

This patch addresses the issue by moving the diagnostic to a function which is called from both parsing and template instantiation, similar to what's already done with some other attributes.

The analysis version of the check didn't always work, and only applied to return_typestate rather than param_typestate (even though both had commented-out Sema checks); therefore, the fixed code may produce warnings that didn't appear before.

Also, add a new diagnostic for when the set_typestate or test_typestate attribute is applied to a constructor or static method; previously Clang would ignore it or crash, respectively. (removed)

[comex]

Fixed a mistake where the diagnostic would print the wrong parameter type.

[dblaikie]

Looks like this might benefit from being split into independent changes - the work related to templates (I haven't looked closely, but I assume that's fairly indivisible) and the work related to other diagnostics seems fairly separable - and maybe there's other pieces too.

include/clang/Basic/DiagnosticSemaKinds.td

3234

"member function" would be more correct than "method" here (the diagnostics in this file using the word "method" are mostly for Objective C things)

Are there other diagnostics that are similar? (like maybe function "const" - which can't be on non-member, static member, or ctors - wouldn't that be the same here? Where's the failure path for a non-member (free) function? Could it be unified with the static member/ctor case you're adding?)

Ah, looks like in Attr.td these attributes could be flagged "NonStaticCXXMethod" rather than "CXXMethod" - perhaps that narrower classification wasn't available when this was originally implemented. (then, I think, the attribute parsing logic will do the job of warning about the misuse and ignoring the attribute entirely)

lib/Sema/SemaDeclAttr.cpp

1237–1240

Drop the else-after-return https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

[comex]

In D67740#1675564, @dblaikie wrote:

Looks like this might benefit from being split into independent changes - the work related to templates (I haven't looked closely, but I assume that's fairly indivisible) and the work related to other diagnostics seems fairly separable - and maybe there's other pieces too.

Okay, I'll take the set_typestate/test_typestate part out of this and submit it separately.

include/clang/Basic/DiagnosticSemaKinds.td

3234

That doesn't check for constructors, but sure, I'll add a "NonStaticNonConstructorCXXMethod" instead (in the separate submission).

lib/Sema/SemaDeclAttr.cpp

1237–1240

Done.

[comex]

Addressed feedback.

[comex]

Ping...

[dblaikie]

lib/Sema/SemaDeclAttr.cpp

1257

Do you need the "? 0 : 1" here? Or would the boolean value be sufficient to use directly?

lib/Sema/SemaTemplateInstantiateDecl.cpp

552–553

Collapse these two conditions into one 'if', perhaps?

include/clang/Analysis/Analyses/Consumed.h

virtual void warnParamReturnTypestateMismatch(SourceLocation Loc,
                                                   StringRef ExpectedState,
                                                   StringRef ObservedState) {}
 
     // FIXME: Add documentation.
     virtual void warnParamTypestateMismatch(SourceLocation LOC,
                                             StringRef ExpectedState,
                                             StringRef ObservedState) {}
 
-    // FIXME: This can be removed when the attr propagation fix for templated
-    //        classes lands.
-    /// Warn about return typestates set for unconsumable types.
-    ///
-    /// \param Loc -- The location of the attributes.
-    ///
-    /// \param TypeName -- The name of the unconsumable type.
-    virtual void warnReturnTypestateForUnconsumableType(SourceLocation Loc,
-                                                        StringRef TypeName) {}
-
     /// Warn about return typestate mismatches.
     ///
     /// \param Loc -- The SourceLocation of the return statement.
     ///
     /// \param ExpectedState -- The state the return value was expected to be
     /// in.
     ///
     /// \param ObservedState -- The state the return value was observed to be

include/clang/Basic/DiagnosticSemaKinds.td

def warn_use_in_invalid_state : Warning<
   "invalid invocation of method '%0' on object '%1' while it is in the '%2' "
   "state">, InGroup<Consumed>, DefaultIgnore;
 def warn_use_of_temp_in_invalid_state : Warning<
   "invalid invocation of method '%0' on a temporary object while it is in the "
   "'%1' state">, InGroup<Consumed>, DefaultIgnore;
 def warn_attr_on_unconsumable_class : Warning<
   "consumed analysis attribute is attached to member of class '%0' which isn't "
   "marked as consumable">, InGroup<Consumed>, DefaultIgnore;
-def warn_return_typestate_for_unconsumable_type : Warning<
-  "return state set for an unconsumable type '%0'">, InGroup<Consumed>,
-  DefaultIgnore;
+def warn_consumable_attr_on_static : Warning<
+  "consumed analysis attribute is attached to a "
+  "%select{static method|constructor}0 of class '%1'">,
+  InGroup<Consumed>, DefaultIgnore;
+def warn_consumable_attr_for_unconsumable_type : Warning<
+  "attribute %0 invalid for %select{parameter|return value}1 of type %2: "
+  "expected a consumable class type as a value, reference, or rvalue reference">,
+  InGroup<Consumed>, DefaultIgnore;
 def warn_return_typestate_mismatch : Warning<
   "return value not in expected state; expected '%0', observed '%1'">,
   InGroup<Consumed>, DefaultIgnore;
 def warn_loop_state_mismatch : Warning<
   "state of variable '%0' must match at the entry and exit of loop">,
   InGroup<Consumed>, DefaultIgnore;
 def warn_param_return_typestate_mismatch : Warning<
   "parameter '%0' not in expected state when the function returns: expected "

include/clang/Sema/Sema.h

 
   /// addAMDGPUWavePersEUAttr - Adds an amdgpu_waves_per_eu attribute to a
   /// particular declaration.
   void addAMDGPUWavesPerEUAttr(Decl *D, const AttributeCommonInfo &CI,
                                Expr *Min, Expr *Max);
 
   bool checkNSReturnsRetainedReturnType(SourceLocation loc, QualType type);
 
+  bool CheckParamOrReturnTypestateAttr(const Attr *A, const Decl *D,
+                                       const Decl *Old);
+
   //===--------------------------------------------------------------------===//
   // C++ Coroutines TS
   //
   bool ActOnCoroutineBodyStart(Scope *S, SourceLocation KwLoc,
                                StringRef Keyword);
   ExprResult ActOnCoawaitExpr(Scope *S, SourceLocation KwLoc, Expr *E);
   ExprResult ActOnCoyieldExpr(Scope *S, SourceLocation KwLoc, Expr *E);
   StmtResult ActOnCoreturnStmt(Scope *S, SourceLocation KwLoc, Expr *E);

lib/Analysis/Consumed.cpp

 void ConsumedAnalyzer::determineExpectedReturnState(AnalysisDeclContext &AC,
                                                     const FunctionDecl *D) {
   QualType ReturnType;
   if (const auto *Constructor = dyn_cast<CXXConstructorDecl>(D)) {
     ReturnType = Constructor->getThisType()->getPointeeType();
   } else
     ReturnType = D->getCallResultType();
 
-  if (const ReturnTypestateAttr *RTSAttr = D->getAttr<ReturnTypestateAttr>()) {
-    const CXXRecordDecl *RD = ReturnType->getAsCXXRecordDecl();
-    if (!RD || !RD->hasAttr<ConsumableAttr>()) {
-      // FIXME: This should be removed when template instantiation propagates
-      //        attributes at template specialization definition, not
-      //        declaration. When it is removed the test needs to be enabled
-      //        in SemaDeclAttr.cpp.
-      WarningsHandler.warnReturnTypestateForUnconsumableType(
-          RTSAttr->getLocation(), ReturnType.getAsString());
-      ExpectedReturnState = CS_None;
-    } else
-      ExpectedReturnState = mapReturnTypestateAttrState(RTSAttr);
-  } else if (isConsumableType(ReturnType)) {
+  if (const ReturnTypestateAttr *RTSAttr = D->getAttr<ReturnTypestateAttr>())
+  else if (isConsumableType(ReturnType)) {
     if (isAutoCastType(ReturnType))   // We can auto-cast the state to the
       ExpectedReturnState = CS_None;  // expected state.
     else
       ExpectedReturnState = mapConsumableAttrState(ReturnType);
   }
   else
     ExpectedReturnState = CS_None;
 }

lib/Sema/AnalysisBasedWarnings.cpp

void warnParamTypestateMismatch(SourceLocation Loc, StringRef ExpectedState,
                                   StringRef ObservedState) override {
 
     PartialDiagnosticAt Warning(Loc, S.PDiag(
       diag::warn_param_typestate_mismatch) << ExpectedState << ObservedState);
 
     Warnings.emplace_back(std::move(Warning), OptionalNotes());
   }
 
-  void warnReturnTypestateForUnconsumableType(SourceLocation Loc,
-                                              StringRef TypeName) override {
-    PartialDiagnosticAt Warning(Loc, S.PDiag(
-      diag::warn_return_typestate_for_unconsumable_type) << TypeName);
-
-    Warnings.emplace_back(std::move(Warning), OptionalNotes());
-  }
-
   void warnReturnTypestateMismatch(SourceLocation Loc, StringRef ExpectedState,
                                    StringRef ObservedState) override {
 
     PartialDiagnosticAt Warning(Loc, S.PDiag(
       diag::warn_return_typestate_mismatch) << ExpectedState << ObservedState);
 
     Warnings.emplace_back(std::move(Warning), OptionalNotes());
   }

lib/Sema/SemaDeclAttr.cpp

if (AL.isArgIdent(0)) {
     return;
   }
 
   D->addAttr(::new (S.Context) ConsumableAttr(S.Context, AL, DefaultState));
 }
 
 static bool checkForConsumableClass(Sema &S, const CXXMethodDecl *MD,
                                     const ParsedAttr &AL) {
-  QualType ThisType = MD->getThisType()->getPointeeType();
+  const CXXRecordDecl *RD = MD->getParent();
 
-  if (const CXXRecordDecl *RD = ThisType->getAsCXXRecordDecl()) {
-    if (!RD->hasAttr<ConsumableAttr>()) {
-      S.Diag(AL.getLoc(), diag::warn_attr_on_unconsumable_class) <<
-        RD->getNameAsString();
 
-      return false;
-    }
+
+  if (MD->isStatic() || isa<CXXConstructorDecl>(MD)) {
+      S.Diag(AL.getLoc(), diag::warn_consumable_attr_on_static) <<
+        (MD->isStatic() ? 0 : 1) <<
+        RD->getNameAsString();
+    return false;
   }
 
   return true;
 }
 
 static void handleCallableWhenAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (!checkAttributeAtLeastNumArgs(S, AL, 1))
     return;
if (!ParamTypestateAttr::ConvertStrToConsumedState(StateString,
       return;
     }
   } else {
     S.Diag(AL.getLoc(), diag::err_attribute_argument_type)
         << AL << AANT_ArgumentIdentifier;
     return;
   }
 
-  // FIXME: This check is currently being done in the analysis.  It can be
-  //        enabled here only after the parser propagates attributes at
-  //        template specialization definition, not declaration.
-  //QualType ReturnType = cast<ParmVarDecl>(D)->getType();
-  //const CXXRecordDecl *RD = ReturnType->getAsCXXRecordDecl();
-  //
-  //if (!RD || !RD->hasAttr<ConsumableAttr>()) {
-  //    S.Diag(AL.getLoc(), diag::warn_return_state_for_unconsumable_type) <<
-  //      ReturnType.getAsString();
-  //    return;
-  //}
-
-  D->addAttr(::new (S.Context) ParamTypestateAttr(S.Context, AL, ParamState));
+  Attr *A = ::new (S.Context) ParamTypestateAttr(S.Context, AL, ParamState);
+  if (S.CheckParamOrReturnTypestateAttr(A, D, nullptr))
+    D->addAttr(A);
 }
 
 static void handleReturnTypestateAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   ReturnTypestateAttr::ConsumedState ReturnState;
 
   if (AL.isArgIdent(0)) {
     IdentifierLoc *IL = AL.getArgAsIdent(0);
     if (!ReturnTypestateAttr::ConvertStrToConsumedState(IL->Ident->getName(),
                                                         ReturnState)) {
       S.Diag(IL->Loc, diag::warn_attribute_type_not_supported) << AL
                                                                << IL->Ident;
       return;
     }
   } else {
     S.Diag(AL.getLoc(), diag::err_attribute_argument_type)
         << AL << AANT_ArgumentIdentifier;
     return;
   }
 
-  // FIXME: This check is currently being done in the analysis.  It can be
-  //        enabled here only after the parser propagates attributes at
-  //        template specialization definition, not declaration.
-  //QualType ReturnType;
-  //
-  //if (const ParmVarDecl *Param = dyn_cast<ParmVarDecl>(D)) {
-  //  ReturnType = Param->getType();
-  //
-  //} else if (const CXXConstructorDecl *Constructor =
-  //             dyn_cast<CXXConstructorDecl>(D)) {
-  //  ReturnType = Constructor->getThisType()->getPointeeType();
-  //
-  //} else {
-  //
-  //  ReturnType = cast<FunctionDecl>(D)->getCallResultType();
-  //}
-  //
-  //const CXXRecordDecl *RD = ReturnType->getAsCXXRecordDecl();
-  //
-  //if (!RD || !RD->hasAttr<ConsumableAttr>()) {
-  //    S.Diag(Attr.getLoc(), diag::warn_return_state_for_unconsumable_type) <<
-  //      ReturnType.getAsString();
-  //    return;
-  //}
+  Attr *A = ::new (S.Context) ReturnTypestateAttr(S.Context, AL, ReturnState);
+  if (S.CheckParamOrReturnTypestateAttr(A, D, nullptr))
+    D->addAttr(A);
+}
+
+// Check a `param_typestate` or `return_typestate` attribute: emit a warning
+// and return false if the parameter or return type is not consumable.
+
+bool Sema::CheckParamOrReturnTypestateAttr(const Attr *A, const Decl *D,
+                                           const Decl *Old) {
+  auto GetType = [&](const Decl *X) -> QualType {
+    // `return_typestate` can be applied to parameters (referring to the
+    // parameter), constructors (referring to the new object), or functions
+    // (referring to the return value).  `param_typestate` can only be applied
+    // to parameters.
+    if (auto *PX = dyn_cast<ParmVarDecl>(X))
+      return PX->getType();
+    else if (auto *CX = dyn_cast<CXXConstructorDecl>(X))
+      return CX->getThisType()->getPointeeType();
+    else
+      return cast<FunctionDecl>(X)->getCallResultType();
+  };
+  QualType Type = GetType(D);
+  if (Type->isDependentType())
+    return true; // will re-check on instantiation
+  if (Old && !GetType(Old)->isDependentType())
+    return true; // we already checked this
+  QualType BaseType = Type;
+  if (BaseType->isReferenceType())
+    BaseType = BaseType->getPointeeType();
+  if (const CXXRecordDecl *RD = BaseType->getAsCXXRecordDecl())
+    if (RD->hasAttr<ConsumableAttr>())
+      return true; // OK
 
-  D->addAttr(::new (S.Context) ReturnTypestateAttr(S.Context, AL, ReturnState));
+  Diag(A->getLoc(), diag::warn_consumable_attr_for_unconsumable_type) <<
+    A << (isa<ParmVarDecl>(D) ? 0 : 1) << Type;
+  return false;
 }
 
 static void handleSetTypestateAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (!checkForConsumableClass(S, cast<CXXMethodDecl>(D), AL))
     return;
 
   SetTypestateAttr::ConsumedState NewState;
   if (AL.isArgIdent(0)) {
     IdentifierLoc *Ident = AL.getArgAsIdent(0);

lib/Sema/SemaTemplateInstantiateDecl.cpp

for (const auto *TmplAttr : Tmpl->attrs()) {
     }
 
     if (auto *A = dyn_cast<OwnerAttr>(TmplAttr)) {
       if (!New->hasAttr<OwnerAttr>())
         New->addAttr(A->clone(Context));
       continue;
     }
 
+    if (isa<ParamTypestateAttr>(TmplAttr) || isa<ReturnTypestateAttr>(TmplAttr)) {
+      if (!CheckParamOrReturnTypestateAttr(TmplAttr, New, Tmpl))
+        continue; // don't add
+    }
+
     assert(!TmplAttr->isPackExpansion());
     if (TmplAttr->isLateParsed() && LateAttrs) {
       // Late parsed attributes must be instantiated and attached after the
       // enclosing class has been instantiated.  See Sema::InstantiateClass.
       LocalInstantiationScope *Saved = nullptr;
       if (CurrentInstantiationScope)
         Saved = CurrentInstantiationScope->cloneScopes(OuterMostScope);
       LateAttrs->push_back(LateInstantiatedAttribute(TmplAttr, Saved, New));

test/SemaCXX/warn-consumed-parsing.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -Wconsumed -std=c++11 %s
 
 #define CALLABLE_WHEN(...)      __attribute__ ((callable_when(__VA_ARGS__)))
 #define CONSUMABLE(state)       __attribute__ ((consumable(state)))
 #define SET_TYPESTATE(state)    __attribute__ ((set_typestate(state)))
 #define RETURN_TYPESTATE(state) __attribute__ ((return_typestate(state)))
 #define TEST_TYPESTATE(state)   __attribute__ ((test_typestate(state)))
 
-// FIXME: This test is here because the warning is issued by the Consumed
-//        analysis, not SemaDeclAttr.  The analysis won't run after an error
-//        has been issued.  Once the attribute propagation for template
-//        instantiation has been fixed, this can be moved somewhere else and the
-//        definition can be removed.
-int returnTypestateForUnconsumable() RETURN_TYPESTATE(consumed); // expected-warning {{return state set for an unconsumable type 'int'}}
-int returnTypestateForUnconsumable() {
-  return 0;
-}
-
 class AttrTester0 {
   void consumes()       __attribute__ ((set_typestate())); // expected-error {{'set_typestate' attribute takes one argument}}
   bool testUnconsumed() __attribute__ ((test_typestate())); // expected-error {{'test_typestate' attribute takes one argument}}
   void callableWhen()   __attribute__ ((callable_when())); // expected-error {{'callable_when' attribute takes at least 1 argument}}
 };
 
 int var0 SET_TYPESTATE(consumed); // expected-warning {{'set_typestate' attribute only applies to functions}}
 int var1 TEST_TYPESTATE(consumed); // expected-warning {{'test_typestate' attribute only applies to}}

 
 
 class CONSUMABLE(unconsumed)
       __attribute__((consumable_auto_cast_state))
       __attribute__((consumable_set_state_on_read))
       Status {
 };
 
+int returnTypestateForUnconsumable() RETURN_TYPESTATE(consumed); // expected-warning {{attribute 'return_typestate' invalid for return value of type 'int': expected a consumable class type as a value, reference, or rvalue reference}}
+int returnTypestateForUnconsumable() {
+  return 0;
+}
 
+struct CONSUMABLE(unknown) UselessAttrs {
+  static void x() SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a static method of class 'UselessAttrs'}}
+  UselessAttrs() SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a constructor of class 'UselessAttrs'}}
+  UselessAttrs(int) CALLABLE_WHEN(consumed); // expected-warning {{consumed analysis attribute is attached to a constructor of class 'UselessAttrs'}}
+  void operator+(UselessAttrs) SET_TYPESTATE(consumed); // OK
+  static void *operator new(unsigned long) SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a static method of class 'UselessAttrs'}}
+  template <typename T>
+  void a([[clang::param_typestate(consumed)]] const int &) {} // expected-warning {{attribute 'param_typestate' invalid for parameter of type 'const int &': expected a consumable class type as a value, reference, or rvalue reference}}
+  void b([[clang::return_typestate(consumed)]] UselessAttrs *) {} // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'UselessAttrs *': expected a consumable class type as a value, reference, or rvalue reference}}
+  template <typename T>
+  void c([[clang::return_typestate(consumed)]] T) {} // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'int': expected a consumable class type as a value, reference, or rvalue reference}}
+  template <> // test a template specialization
+  void c<long>([[clang::return_typestate(consumed)]] long); // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'long': expected a consumable class type as a value, reference, or rvalue reference}}
+  void instantiate() {
+    a<int>(42);
+    c(42);
+  }
+};
+void operator-(UselessAttrs, UselessAttrs) SET_TYPESTATE(consumed); // expected-warning {{'set_typestate' attribute only applies to functions}}
+
+template <typename T, typename U>
+struct CONSUMABLE(unknown) ClassTemplateSpecialization;
+template <typename T>
+struct ClassTemplateSpecialization<T, int> {
+  [[clang::return_typestate(consumed)]] ClassTemplateSpecialization(); // expected-warning {{attribute 'return_typestate' invalid for return value of type 'ClassTemplateSpecialization<long, int>': expected a consumable class type as a value, reference, or rvalue reference}}
+  void a([[clang::return_typestate(consumed)]] T) {} // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'long': expected a consumable class type as a value, reference, or rvalue reference}}
+};
+void testTS() { ClassTemplateSpecialization<long, int>().a(1); } // expected-note {{in instantiation of template class 'ClassTemplateSpecialization<long, int>' requested here}}