[PATCH] Add 'return_typestate' function attribute

[Aaron Ballman]

On Mon, Sep 2, 2013 at 8:12 PM, Christian Wailes <chriswailes at google.com> wrote:
>
>
>
> On Mon, Sep 2, 2013 at 3:54 PM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> On Mon, Sep 2, 2013 at 6:45 PM, Christian Wailes <chriswailes at google.com>
>> wrote:
>> > Notes below.  Updated patch for your refactoring of AttributeList coming
>> > soon.
>> >
>> > - Chris
>> >
>> >
>> > On Sun, Sep 1, 2013 at 7:50 AM, Aaron Ballman <aaron.ballman at gmail.com>
>> > wrote:
>> >>
>> >> No major complaints, but some comments are interspersed below.
>> >>
>> >> > Index: include/clang/Analysis/Analyses/Consumed.h
>> >> > ===================================================================
>> >> > --- include/clang/Analysis/Analyses/Consumed.h
>> >> > +++ include/clang/Analysis/Analyses/Consumed.h
>> >> > @@ -39,11 +39,27 @@
>> >> >
>> >> >      /// \brief Emit the warnings and notes left by the analysis.
>> >> >      virtual void emitDiagnostics() {}
>> >> > -
>> >> > +
>> >> > +    // FIXME: This can be removed when the attr propagation fix for
>> >> > templated
>> >> > +    //        classes lands.
>> >> > +    /// \brief 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) {}
>> >> > +
>> >> > +    /// \brief Warn about return typestate mismatches.
>> >> > +    /// \param Loc -- The SourceLocation of the return statement.
>> >> > +    virtual void warnReturnTypestateMismatch(SourceLocation Loc) {}
>> >> > +
>> >> >      /// \brief Warn about unnecessary-test errors.
>> >> >      /// \param VariableName -- The name of the variable that holds
>> >> > the
>> >> > unique
>> >> >      /// value.
>> >> >      ///
>> >> > +    /// \param VariableState -- The known state of the value.
>> >> > +    ///
>> >> >      /// \param Loc -- The SourceLocation of the unnecessary test.
>> >> >      virtual void warnUnnecessaryTest(StringRef VariableName,
>> >> >                                       StringRef VariableState,
>> >> > @@ -170,6 +186,8 @@
>> >> >      ConsumedBlockInfo BlockInfo;
>> >> >      ConsumedStateMap *CurrStates;
>> >> >
>> >> > +    ConsumedState ExpectedReturnState;
>> >> > +
>> >> >      bool hasConsumableAttributes(const CXXRecordDecl *RD);
>> >> >      bool splitState(const CFGBlock *CurrBlock,
>> >> >                      const ConsumedStmtVisitor &Visitor);
>> >> > @@ -181,6 +199,8 @@
>> >> >      ConsumedAnalyzer(ConsumedWarningsHandlerBase &WarningsHandler)
>> >> >          : WarningsHandler(WarningsHandler) {}
>> >> >
>> >> > +    ConsumedState getExpectedReturnState() { return
>> >> > ExpectedReturnState; }
>> >>
>> >> Method can be const
>> >>
>> >> > +
>> >> >      /// \brief Check a function's CFG for consumed violations.
>> >> >      ///
>> >> >      /// We traverse the blocks in the CFG, keeping track of the
>> >> > state
>> >> > of each
>> >> > Index: include/clang/Basic/Attr.td
>> >> > ===================================================================
>> >> > --- include/clang/Basic/Attr.td
>> >> > +++ include/clang/Basic/Attr.td
>> >> > @@ -953,6 +953,14 @@
>> >> >    let Subjects = [CXXMethod];
>> >> >  }
>> >> >
>> >> > +def ReturnTypestate : InheritableAttr {
>> >> > +  let Spellings = [GNU<"return_typestate">];
>> >> > +  let Subjects = [Function];
>> >> > +  let Args = [EnumArgument<"State", "ConsumedState",
>> >> > +                           ["unknown", "consumed", "unconsumed"],
>> >> > +                           ["Unknown", "Consumed", "Unconsumed"]>];
>> >> > +}
>> >> > +
>> >> >  // Type safety attributes for `void *' pointers and type tags.
>> >> >
>> >> >  def ArgumentWithTypeTag : InheritableAttr {
>> >> > Index: include/clang/Basic/DiagnosticSemaKinds.td
>> >> > ===================================================================
>> >> > --- include/clang/Basic/DiagnosticSemaKinds.td
>> >> > +++ include/clang/Basic/DiagnosticSemaKinds.td
>> >> > @@ -2190,8 +2190,16 @@
>> >> >    "invocation of method '%0' on a temporary object while it is in
>> >> > the "
>> >> >    "'consumed' state">, InGroup<Consumed>, DefaultIgnore;
>> >> >  def warn_attr_on_unconsumable_class : Warning<
>> >> > -  "consumed analysis attribute is attached to class '%0' which isn't
>> >> > marked "
>> >> > -  "as consumable">, InGroup<Consumed>, DefaultIgnore;
>> >> > +  "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_unknown_consumed_state : Warning<
>> >> > +  "unknown consumed analysis state">, InGroup<Consumed>,
>> >> > DefaultIgnore;
>> >>
>> >> It would be good to list the state.
>> >>
>> >> > +def warn_return_typestate_mismatch : Warning<
>> >> > +  "return value not in expected state">, InGroup<Consumed>,
>> >> > +  DefaultIgnore;
>> >>
>> >> It might make for a better diagnostics to tell the user what state the
>> >> return value is in, and what the expected state was.
>> >>
>> >
>> > I was planning on adding that information in a note as I couldn't think
>> > of a
>> > nice, concise way of expressing it in the warning.  This will also allow
>> > me
>> > to point to where the return typestate is declared.
>>
>> It could even be as simple as: inconsistent return value state;
>> expected %0, got %1
>>
>> Though I'm sure someone better with words can improve upon that.  ;-)
>
>
> Done.
>
>>
>>
>> >>
>> >> >
>> >> >  // ConsumedStrict warnings
>> >> >  def warn_use_in_unknown_state : Warning<
>> >> > Index: lib/Analysis/Consumed.cpp
>> >> > ===================================================================
>> >> > --- lib/Analysis/Consumed.cpp
>> >> > +++ lib/Analysis/Consumed.cpp
>> >> > @@ -31,6 +31,7 @@
>> >> >  #include "llvm/Support/Compiler.h"
>> >> >  #include "llvm/Support/raw_ostream.h"
>> >> >
>> >> > +// TODO: Add notes about the actual and expected state for
>> >> >  // TODO: Correctly identify unreachable blocks when chaining boolean
>> >> > operators.
>> >> >  // TODO: Warn about unreachable code.
>> >> >  // TODO: Switch to using a bitmap to track unreachable blocks.
>> >> > @@ -256,6 +257,8 @@
>> >> >    void forwardInfo(const Stmt *From, const Stmt *To);
>> >> >    void handleTestingFunctionCall(const CallExpr *Call, const VarDecl
>> >> > *Var);
>> >> >    bool isLikeMoveAssignment(const CXXMethodDecl *MethodDecl);
>> >> > +  void propagateReturnType(const Stmt *Call, const FunctionDecl
>> >> > *Fun,
>> >> > +                           QualType ReturnType);
>> >> >
>> >> >  public:
>> >> >
>> >> > @@ -272,6 +275,7 @@
>> >> >    void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr
>> >> > *Temp);
>> >> >    void VisitMemberExpr(const MemberExpr *MExpr);
>> >> >    void VisitParmVarDecl(const ParmVarDecl *Param);
>> >> > +  void VisitReturnStmt(const ReturnStmt *Ret);
>> >> >    void VisitUnaryOperator(const UnaryOperator *UOp);
>> >> >    void VisitVarDecl(const VarDecl *Var);
>> >> >
>> >> > @@ -373,6 +377,34 @@
>> >> >
>> >> > MethodDecl->getParamDecl(0)->getType()->isRValueReferenceType());
>> >> >  }
>> >> >
>> >> > +void ConsumedStmtVisitor::propagateReturnType(const Stmt *Call,
>> >> > +                                              const FunctionDecl
>> >> > *Fun,
>> >> > +                                              QualType ReturnType) {
>> >> > +  if (isConsumableType(ReturnType)) {
>> >> > +    if (Fun->hasAttr<ReturnTypestateAttr>()) {
>> >> > +      switch (Fun->getAttr<ReturnTypestateAttr>()->getState()) {
>> >> > +      case ReturnTypestateAttr::Unconsumed:
>> >> > +        PropagationMap.insert(PairType(Call,
>> >> > +
>> >> > PropagationInfo(consumed::CS_Unconsumed)));
>> >> > +        break;
>> >> > +
>> >> > +      case ReturnTypestateAttr::Consumed:
>> >> > +        PropagationMap.insert(PairType(Call,
>> >> > +
>> >> > PropagationInfo(consumed::CS_Consumed)));
>> >> > +        break;
>> >> > +
>> >> > +      case ReturnTypestateAttr::Unknown:
>> >> > +        PropagationMap.insert(PairType(Call,
>> >> > +
>> >> > PropagationInfo(consumed::CS_Unknown)));
>> >> > +        break;
>> >> > +      }
>> >> > +    } else {
>> >> > +      PropagationMap.insert(PairType(Call,
>> >> > +                            PropagationInfo(consumed::CS_Unknown)));
>> >> > +    }
>> >> > +  }
>> >> > +}
>> >>
>> >> There's a fair amount of duplication there.  The only variability is
>> >> what the PropagationInfo enumerant is, so I think it'd be more clear
>> >> to have a single call to insert instead of multiple.
>> >>
>> >
>> > Already done in a future patch.  Backported into the updated patch.
>> >
>> >>
>> >> > +
>> >> >  void ConsumedStmtVisitor::Visit(const Stmt *StmtNode) {
>> >> >
>> >> >    ConstStmtVisitor<ConsumedStmtVisitor>::Visit(StmtNode);
>> >> > @@ -469,6 +501,8 @@
>> >> >          StateMap->setState(PInfo.getVar(), consumed::CS_Unknown);
>> >> >        }
>> >> >      }
>> >> > +
>> >> > +    propagateReturnType(Call, FunDecl,
>> >> > FunDecl->getCallResultType());
>> >> >    }
>> >> >  }
>> >> >
>> >> > @@ -483,8 +517,7 @@
>> >> >    QualType ThisType =
>> >> > Constructor->getThisType(CurrContext)->getPointeeType();
>> >> >
>> >> >    if (isConsumableType(ThisType)) {
>> >> > -    if (Constructor->hasAttr<ConsumesAttr>() ||
>> >> > -        Constructor->isDefaultConstructor()) {
>> >> > +    if (Constructor->isDefaultConstructor()) {
>> >> >
>> >> >        PropagationMap.insert(PairType(Call,
>> >> >          PropagationInfo(consumed::CS_Consumed)));
>> >> > @@ -513,8 +546,7 @@
>> >> >          PropagationMap.insert(PairType(Call, Entry->second));
>> >> >
>> >> >      } else {
>> >> > -      PropagationMap.insert(PairType(Call,
>> >> > -        PropagationInfo(consumed::CS_Unconsumed)));
>> >> > +      propagateReturnType(Call, Constructor, ThisType);
>> >> >      }
>> >> >    }
>> >> >  }
>> >> > @@ -677,6 +709,23 @@
>> >> >      StateMap->setState(Param, consumed::CS_Unknown);
>> >> >  }
>> >> >
>> >> > +void ConsumedStmtVisitor::VisitReturnStmt(const ReturnStmt *Ret) {
>> >> > +  if (Analyzer.getExpectedReturnState()) {
>> >> > +    InfoEntry Entry = PropagationMap.find(Ret->getRetValue());
>> >> > +
>> >> > +    if (Entry != PropagationMap.end()) {
>> >> > +      assert(Entry->second.isState() || Entry->second.isVar());
>> >> > +
>> >> > +      ConsumedState RetState = Entry->second.isState() ?
>> >> > +        Entry->second.getState() :
>> >> > StateMap->getState(Entry->second.getVar());
>> >> > +
>> >> > +      if (RetState != Analyzer.getExpectedReturnState())
>> >> > +        Analyzer.WarningsHandler.warnReturnTypestateMismatch(
>> >> > +          Ret->getReturnLoc());
>> >> > +    }
>> >> > +  }
>> >> > +}
>> >> > +
>> >> >  void ConsumedStmtVisitor::VisitUnaryOperator(const UnaryOperator
>> >> > *UOp)
>> >> > {
>> >> >    InfoEntry Entry =
>> >> > PropagationMap.find(UOp->getSubExpr()->IgnoreParens());
>> >> >    if (Entry == PropagationMap.end()) return;
>> >> > @@ -997,6 +1046,53 @@
>> >> >
>> >> >    if (!D) return;
>> >> >
>> >> > +  // 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.
>> >> > +  QualType ReturnType;
>> >> > +  if (const CXXConstructorDecl *Constructor =
>> >> > dyn_cast<CXXConstructorDecl>(D)) {
>> >> > +    ASTContext &CurrContext = AC.getASTContext();
>> >> > +    ReturnType =
>> >> > Constructor->getThisType(CurrContext)->getPointeeType();
>> >> > +
>> >> > +  } else {
>> >> > +    ReturnType = cast<FunctionDecl>(D)->getCallResultType();
>> >> > +  }
>> >> > +
>> >> > +  // Determine the expected return value.
>> >> > +  if (D->hasAttr<ReturnTypestateAttr>()) {
>> >> > +
>> >> > +    ReturnTypestateAttr *RTSAttr =
>> >> > D->getAttr<ReturnTypestateAttr>();
>> >> > +
>> >> > +    const CXXRecordDecl *RD = ReturnType->getAsCXXRecordDecl();
>> >> > +    if (!RD || !RD->hasAttr<ConsumableAttr>()) {
>> >> > +        // FIXME: This branch can be removed with the code above.
>> >> > +        WarningsHandler.warnReturnTypestateForUnconsumableType(
>> >> > +          RTSAttr->getLocation(), ReturnType.getAsString());
>> >> > +        ExpectedReturnState = CS_None;
>> >> > +
>> >> > +    } else {
>> >> > +      switch (RTSAttr->getState()) {
>> >> > +      case ReturnTypestateAttr::Unknown:
>> >> > +        ExpectedReturnState = CS_Unknown;
>> >> > +        break;
>> >> > +
>> >> > +      case ReturnTypestateAttr::Unconsumed:
>> >> > +        ExpectedReturnState = CS_Unconsumed;
>> >> > +        break;
>> >> > +
>> >> > +      case ReturnTypestateAttr::Consumed:
>> >> > +        ExpectedReturnState = CS_Consumed;
>> >> > +        break;
>> >> > +      }
>> >> > +    }
>> >> > +
>> >> > +  } else if (isConsumableType(ReturnType)) {
>> >> > +    ExpectedReturnState = CS_Unknown;
>> >> > +
>> >> > +  } else {
>> >> > +    ExpectedReturnState = CS_None;
>> >> > +  }
>> >> > +
>> >> >    BlockInfo = ConsumedBlockInfo(AC.getCFG());
>> >> >
>> >> >    PostOrderCFGView *SortedGraph =
>> >> > AC.getAnalysis<PostOrderCFGView>();
>> >> > Index: lib/Sema/AnalysisBasedWarnings.cpp
>> >> > ===================================================================
>> >> > --- lib/Sema/AnalysisBasedWarnings.cpp
>> >> > +++ lib/Sema/AnalysisBasedWarnings.cpp
>> >> > @@ -1445,11 +1445,21 @@
>> >> >      }
>> >> >    }
>> >> >
>> >> > -  /// Warn about unnecessary-test errors.
>> >> > -  /// \param VariableName -- The name of the variable that holds the
>> >> > unique
>> >> > -  /// value.
>> >> > -  ///
>> >> > -  /// \param Loc -- The SourceLocation of the unnecessary test.
>> >> > +  void warnReturnTypestateForUnconsumableType(SourceLocation Loc,
>> >> > +                                          StringRef TypeName) {
>> >> > +    PartialDiagnosticAt Warning(Loc, S.PDiag(
>> >> > +      diag::warn_return_typestate_for_unconsumable_type) <<
>> >> > TypeName);
>> >>
>> >> You are missing an enabled test case for this warning.
>> >>
>> >
>> > The test for this is on line 42 of warn-consumed-parsing.cpp.  It is
>> > currently commented out because an issue with attributes being
>> > incorrectly
>> > propagated for template specializations is preventing us from checking
>> > for
>> > this problem when parsing the attributes.
>>
>> Yes, that's why I mentioned an enabled test.  But it seems like you
>> could write a test that doesn't use template specialization?
>>
>
> Can you elaborate on what you mean by an 'enabled test'?  And while we could
> test for non-template specialized cases in SemaDeclAttr.cpp now, I was told
> to comment out that code and check for the warning in the ConsumedAnalyzer
> by DeLesely.

Currently, the only test for
warn_return_typestate_for_unconsumable_type is in a #if 0 block; we
should have a test for that warning which isn't #if'ed out.  I'm not
worried about which part of the code does the analysis for the
warning, just that the warning is covered by at least one test case
that will run.

>
>>
>> >  There are some FIXMEs running
>> > around telling someone to un-comment the test and move some code around
>> > once
>> > the issue is resolved.  Specifically, this warning function will be
>> > removed
>> > and the warning will be issued from SemaDeclAttr directly.
>> >
>> >>
>> >> > +
>> >> > +    Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
>> >> > +  }
>> >> > +
>> >> > +  void warnReturnTypestateMismatch(SourceLocation Loc) {
>> >> > +    PartialDiagnosticAt Warning(Loc, S.PDiag(
>> >> > +      diag::warn_return_typestate_mismatch));
>> >> > +
>> >> > +    Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
>> >> > +  }
>> >> > +
>> >> >    void warnUnnecessaryTest(StringRef VariableName, StringRef
>> >> > VariableState,
>> >> >                             SourceLocation Loc) {
>> >> >
>> >> > @@ -1459,11 +1469,6 @@
>> >> >      Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
>> >> >    }
>> >> >
>> >> > -  /// Warn about use-while-consumed errors.
>> >> > -  /// \param MethodName -- The name of the method that was
>> >> > incorrectly
>> >> > -  /// invoked.
>> >> > -  ///
>> >> > -  /// \param Loc -- The SourceLocation of the method invocation.
>> >> >    void warnUseOfTempWhileConsumed(StringRef MethodName,
>> >> > SourceLocation
>> >> > Loc) {
>> >> >
>> >> >      PartialDiagnosticAt Warning(Loc, S.PDiag(
>> >> > @@ -1472,11 +1477,6 @@
>> >> >      Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
>> >> >    }
>> >> >
>> >> > -  /// Warn about use-in-unknown-state errors.
>> >> > -  /// \param MethodName -- The name of the method that was
>> >> > incorrectly
>> >> > -  /// invoked.
>> >> > -  ///
>> >> > -  /// \param Loc -- The SourceLocation of the method invocation.
>> >> >    void warnUseOfTempInUnknownState(StringRef MethodName,
>> >> > SourceLocation
>> >> > Loc) {
>> >> >
>> >> >      PartialDiagnosticAt Warning(Loc, S.PDiag(
>> >> > @@ -1485,14 +1485,6 @@
>> >> >      Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
>> >> >    }
>> >> >
>> >> > -  /// Warn about use-while-consumed errors.
>> >> > -  /// \param MethodName -- The name of the method that was
>> >> > incorrectly
>> >> > -  /// invoked.
>> >> > -  ///
>> >> > -  /// \param VariableName -- The name of the variable that holds the
>> >> > unique
>> >> > -  /// value.
>> >> > -  ///
>> >> > -  /// \param Loc -- The SourceLocation of the method invocation.
>> >> >    void warnUseWhileConsumed(StringRef MethodName, StringRef
>> >> > VariableName,
>> >> >                              SourceLocation Loc) {
>> >> >
>> >> > @@ -1502,14 +1494,6 @@
>> >> >      Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
>> >> >    }
>> >> >
>> >> > -  /// Warn about use-in-unknown-state errors.
>> >> > -  /// \param MethodName -- The name of the method that was
>> >> > incorrectly
>> >> > -  /// invoked.
>> >> > -  ///
>> >> > -  /// \param VariableName -- The name of the variable that holds the
>> >> > unique
>> >> > -  /// value.
>> >> > -  ///
>> >> > -  /// \param Loc -- The SourceLocation of the method invocation.
>> >> >    void warnUseInUnknownState(StringRef MethodName, StringRef
>> >> > VariableName,
>> >> >                               SourceLocation Loc) {
>> >>
>> >> Why are all of the comments being removed?
>> >>
>> >
>> > These comments are redundent, as they are also present on the virtual
>> > versions of these functions.  Rather than risk them becoming
>> > unsynchronized
>> > I'm removing the copies here and keeping the ones on the interface.
>>
>> Sounds good.
>>
>> >
>> >>
>> >> >
>> >> > Index: lib/Sema/SemaDeclAttr.cpp
>> >> > ===================================================================
>> >> > --- lib/Sema/SemaDeclAttr.cpp
>> >> > +++ lib/Sema/SemaDeclAttr.cpp
>> >> > @@ -1071,6 +1071,62 @@
>> >> >
>> >> > Attr.getAttributeSpellingListIndex()));
>> >> >  }
>> >> >
>> >> > +static void handleReturnTypestateAttr(Sema &S, Decl *D,
>> >> > +                                      const AttributeList &Attr) {
>> >> > +
>> >> > +  ReturnTypestateAttr::ConsumedState ReturnState;
>> >> > +
>> >> > +  if (Attr.getParameterName()) {
>> >> > +    StringRef Param = Attr.getParameterName()->getName();
>> >>
>> >> You'll have to rebase this since r189711 got rid of the notion of a
>> >> distinct parameter for attributes.  It shouldn't be too bad -- the
>> >> parameter is now the first argument in the list of args.
>> >>
>> >
>> > Already saw that and am working on it now.  This will also come in handy
>> > for
>> > a future patch I'm about to submit.
>> >
>> >>
>> >> > +
>> >> > +    if (Param == "unknown") {
>> >> > +      ReturnState = ReturnTypestateAttr::Unknown;
>> >> > +    } else if (Param == "consumed") {
>> >> > +      ReturnState = ReturnTypestateAttr::Consumed;
>> >> > +    } else if (Param == "unconsumed") {
>> >> > +      ReturnState = ReturnTypestateAttr::Unconsumed;
>> >> > +    } else {
>> >> > +      S.Diag(Attr.getLoc(), diag::warn_unknown_consumed_state);
>> >>
>> >> You are missing test cases for this.
>> >>
>> >> > +      return;
>> >> > +    }
>> >> > +
>> >> > +  } else {
>> >> > +    S.Diag(Attr.getLoc(),
>> >> > diag::err_attribute_wrong_number_arguments)
>> >> > +      << Attr.getName() << 1;
>> >>
>> >> You should use checkAttributeNumArgs instead.
>> >
>> >
>> > That wouldn't have worked with the previous version of AttributeList as
>> > if
>> > an attribute had a getNumArgs would still return 0.  With your patch I
>> > can
>> > now use checkAttributeNumArgs.
>> >
>> >>
>> >>
>> >> > +  }
>> >> > +
>> >> > +  if (!isa<FunctionDecl>(D)) {
>> >> > +    S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<
>> >> > +      Attr.getName() << ExpectedFunction;
>> >> > +    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 CXXConstructorDecl *Constructor =
>> >> > dyn_cast<CXXConstructorDecl>(D)) {
>> >> > +    //~ ReturnType =
>> >> > Constructor->getThisType(S.getASTContext())->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;
>> >> > +  //~ }
>> >>
>> >> What is with the ~ at the start of the comments?
>> >>
>> >
>> > These are from my IDE so it knows which comments it should remove via
>> > the
>> > keyboard shortcut.  I can remove them, or comment out this block with /*
>> > */
>> > comments.
>>
>> Thanks!
>>
>> >
>> >>
>> >> > +
>> >> > +  D->addAttr(::new (S.Context)
>> >> > +             ReturnTypestateAttr(Attr.getRange(), S.Context,
>> >> > ReturnState,
>> >> > +
>> >> > Attr.getAttributeSpellingListIndex()));
>> >> > +}
>> >> > +
>> >> >  static void handleExtVectorTypeAttr(Sema &S, Scope *scope, Decl *D,
>> >> >                                      const AttributeList &Attr) {
>> >> >    TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(D);
>> >> > @@ -5052,6 +5108,9 @@
>> >> >    case AttributeList::AT_TestsUnconsumed:
>> >> >      handleTestsUnconsumedAttr(S, D, Attr);
>> >> >      break;
>> >> > +  case AttributeList::AT_ReturnTypestate:
>> >> > +    handleReturnTypestateAttr(S, D, Attr);
>> >> > +    break;
>> >> >
>> >> >    // Type safety attributes.
>> >> >    case AttributeList::AT_ArgumentWithTypeTag:
>> >> > Index: test/SemaCXX/warn-consumed-analysis-strict.cpp
>> >> > ===================================================================
>> >> > --- test/SemaCXX/warn-consumed-analysis-strict.cpp
>> >> > +++ test/SemaCXX/warn-consumed-analysis-strict.cpp
>> >> > @@ -3,6 +3,7 @@
>> >> >  #define CALLABLE_WHEN_UNCONSUMED __attribute__
>> >> > ((callable_when_unconsumed))
>> >> >  #define CONSUMABLE               __attribute__ ((consumable))
>> >> >  #define CONSUMES                 __attribute__ ((consumes))
>> >> > +#define RETURN_TYPESTATE(State)  __attribute__
>> >> > ((return_typestate(State)))
>> >> >  #define TESTS_UNCONSUMED         __attribute__ ((tests_unconsumed))
>> >> >
>> >> >  #define TEST_VAR(Var) Var.isValid()
>> >> > @@ -15,9 +16,10 @@
>> >> >
>> >> >    public:
>> >> >    ConsumableClass();
>> >> > -  ConsumableClass(T val);
>> >> > -  ConsumableClass(ConsumableClass<T> &other);
>> >> > -  ConsumableClass(ConsumableClass<T> &&other);
>> >> > +  ConsumableClass(nullptr_t p) RETURN_TYPESTATE(consumed);
>> >> > +  ConsumableClass(T val) RETURN_TYPESTATE(unconsumed);
>> >> > +  ConsumableClass(ConsumableClass<T> &other)
>> >> > RETURN_TYPESTATE(unconsumed);
>> >> > +  ConsumableClass(ConsumableClass<T> &&other)
>> >> > RETURN_TYPESTATE(unconsumed);
>> >> >
>> >> >    ConsumableClass<T>& operator=(ConsumableClass<T>  &other);
>> >> >    ConsumableClass<T>& operator=(ConsumableClass<T> &&other);
>> >> > Index: test/SemaCXX/warn-consumed-analysis.cpp
>> >> > ===================================================================
>> >> > --- test/SemaCXX/warn-consumed-analysis.cpp
>> >> > +++ test/SemaCXX/warn-consumed-analysis.cpp
>> >> > @@ -3,6 +3,7 @@
>> >> >  #define CALLABLE_WHEN_UNCONSUMED __attribute__
>> >> > ((callable_when_unconsumed))
>> >> >  #define CONSUMABLE               __attribute__ ((consumable))
>> >> >  #define CONSUMES                 __attribute__ ((consumes))
>> >> > +#define RETURN_TYPESTATE(State)  __attribute__
>> >> > ((return_typestate(State)))
>> >> >  #define TESTS_UNCONSUMED         __attribute__ ((tests_unconsumed))
>> >> >
>> >> >  typedef decltype(nullptr) nullptr_t;
>> >> > @@ -13,10 +14,10 @@
>> >> >
>> >> >    public:
>> >> >    ConsumableClass();
>> >> > -  ConsumableClass(nullptr_t p) CONSUMES;
>> >> > -  ConsumableClass(T val);
>> >> > -  ConsumableClass(ConsumableClass<T> &other);
>> >> > -  ConsumableClass(ConsumableClass<T> &&other);
>> >> > +  ConsumableClass(nullptr_t p) RETURN_TYPESTATE(consumed);
>> >> > +  ConsumableClass(T val) RETURN_TYPESTATE(unconsumed);
>> >> > +  ConsumableClass(ConsumableClass<T> &other)
>> >> > RETURN_TYPESTATE(unconsumed);
>> >> > +  ConsumableClass(ConsumableClass<T> &&other)
>> >> > RETURN_TYPESTATE(unconsumed);
>> >> >
>> >> >    ConsumableClass<T>& operator=(ConsumableClass<T>  &other);
>> >> >    ConsumableClass<T>& operator=(ConsumableClass<T> &&other);
>> >> > @@ -48,6 +49,16 @@
>> >> >
>> >> >  void baf3(ConsumableClass<int> &&var);
>> >> >
>> >> > +ConsumableClass<int> returnsUnconsumed()
>> >> > RETURN_TYPESTATE(unconsumed);
>> >> > +ConsumableClass<int> returnsUnconsumed() {
>> >> > +  return ConsumableClass<int>(); // expected-warning {{return value
>> >> > not
>> >> > in expected state}}
>> >> > +}
>> >> > +
>> >> > +ConsumableClass<int> returnsConsumed() RETURN_TYPESTATE(consumed);
>> >> > +ConsumableClass<int> returnsConsumed() {
>> >> > +  return ConsumableClass<int>();
>> >> > +}
>> >> > +
>> >> >  void testInitialization() {
>> >> >    ConsumableClass<int> var0;
>> >> >    ConsumableClass<int> var1 = ConsumableClass<int>();
>> >> > @@ -253,6 +264,16 @@
>> >> >    *var; // expected-warning {{invocation of method 'operator*' on
>> >> > object 'var' while it is in the 'consumed' state}}
>> >> >  }
>> >> >
>> >> > +void testReturnStates() {
>> >> > +  ConsumableClass<int> var;
>> >> > +
>> >> > +  var = returnsUnconsumed();
>> >> > +  *var;
>> >> > +
>> >> > +  var = returnsConsumed();
>> >> > +  *var; // expected-warning {{invocation of method 'operator*' on
>> >> > object 'var' while it is in the 'consumed' state}}
>> >> > +}
>> >> > +
>> >> >  void testMoveAsignmentish() {
>> >> >    ConsumableClass<int>  var0;
>> >> >    ConsumableClass<long> var1(42);
>> >> > Index: test/SemaCXX/warn-consumed-parsing.cpp
>> >> > ===================================================================
>> >> > --- test/SemaCXX/warn-consumed-parsing.cpp
>> >> > +++ test/SemaCXX/warn-consumed-parsing.cpp
>> >> > @@ -1,9 +1,10 @@
>> >> >  // RUN: %clang_cc1 -fsyntax-only -verify -Wconsumed -std=c++11 %s
>> >> >
>> >> > -#define CONSUMABLE                __attribute__ ((consumable))
>> >> > -#define CONSUMES                  __attribute__ ((consumes))
>> >> > -#define TESTS_UNCONSUMED          __attribute__ ((tests_unconsumed))
>> >> > -#define CALLABLE_WHEN_UNCONSUMED  __attribute__
>> >> > ((callable_when_unconsumed))
>> >> > +#define CALLABLE_WHEN_UNCONSUMED __attribute__
>> >> > ((callable_when_unconsumed))
>> >> > +#define CONSUMABLE               __attribute__ ((consumable))
>> >> > +#define CONSUMES                 __attribute__ ((consumes))
>> >> > +#define RETURN_TYPESTATE(State)  __attribute__
>> >> > ((return_typestate(State)))
>> >> > +#define TESTS_UNCONSUMED         __attribute__ ((tests_unconsumed))
>> >> >
>> >> >  class AttrTester0 {
>> >> >    void Consumes()        __attribute__ ((consumes(42))); //
>> >> > expected-error {{attribute takes no arguments}}
>> >> > @@ -16,6 +17,7 @@
>> >> >  int var1 TESTS_UNCONSUMED; // expected-warning {{'tests_unconsumed'
>> >> > attribute only applies to methods}}
>> >> >  int var2 CALLABLE_WHEN_UNCONSUMED; // expected-warning
>> >> > {{'callable_when_unconsumed' attribute only applies to methods}}
>> >> >  int var3 CONSUMABLE; // expected-warning {{'consumable' attribute
>> >> > only
>> >> > applies to classes}}
>> >> > +int var4 RETURN_TYPESTATE(consumed); // expected-warning
>> >> > {{'return_typestate' attribute only applies to functions}}
>> >> >
>> >> >  void function0() CONSUMES; // expected-warning {{'consumes'
>> >> > attribute
>> >> > only applies to methods}}
>> >> >  void function1() TESTS_UNCONSUMED; // expected-warning
>> >> > {{'tests_unconsumed' attribute only applies to methods}}
>> >> > @@ -29,7 +31,13 @@
>> >> >  };
>> >> >
>> >> >  class AttrTester2 {
>> >> > -  void callableWhenUnconsumed() CALLABLE_WHEN_UNCONSUMED; //
>> >> > expected-warning {{consumed analysis attribute is attached to class
>> >> > 'AttrTester2' which isn't marked as consumable}}
>> >> > -  void consumes()               CONSUMES; // expected-warning
>> >> > {{consumed analysis attribute is attached to class 'AttrTester2'
>> >> > which isn't
>> >> > marked as consumable}}
>> >> > -  bool testsUnconsumed()        TESTS_UNCONSUMED; //
>> >> > expected-warning
>> >> > {{consumed analysis attribute is attached to class 'AttrTester2'
>> >> > which isn't
>> >> > marked as consumable}}
>> >> > +  void callableWhenUnconsumed() CALLABLE_WHEN_UNCONSUMED; //
>> >> > expected-warning {{consumed analysis attribute is attached to member
>> >> > of
>> >> > class 'AttrTester2' which isn't marked as consumable}}
>> >> > +  void consumes()               CONSUMES; // expected-warning
>> >> > {{consumed analysis attribute is attached to member of class
>> >> > 'AttrTester2'
>> >> > which isn't marked as consumable}}
>> >> > +  bool testsUnconsumed()        TESTS_UNCONSUMED; //
>> >> > expected-warning
>> >> > {{consumed analysis attribute is attached to member of class
>> >> > 'AttrTester2'
>> >> > which isn't marked as consumable}}
>> >> >  };
>> >> > +
>> >> > +// FIXME: This can be re-added once the attribute propagation for
>> >> > template
>> >> > +//        specializations is fixed.
>> >> > +#if 0
>> >> > +int returnStateFunction() RETURN_TYPESTATE(consumed); //
>> >> > expected-warning {{return state set for an unconsumable type 'int'}}
>> >> > +#endif
>> >> >
>> >>
>> >> ~Aaron
>> >
>> >
>>
>> ~Aaron
>

~Aaron