r195533 - Add back experimental attribute objc_suppress_protocol_methods (slightly renamed).

[Aaron Ballman]

On Fri, Nov 22, 2013 at 8:01 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Author: kremenek
> Date: Fri Nov 22 19:01:34 2013
> New Revision: 195533
>
> URL: http://llvm.org/viewvc/llvm-project?rev=195533&view=rev
> Log:
> Add back experimental attribute objc_suppress_protocol_methods (slightly renamed).
>
> This is still an experimental attribute, but I wanted it in tree
> for review.  It may still get yanked.
>
> This attribute can only be applied to a class @interface, not
> a class extension or category.  It does not change the type
> system rules for Objective-C, but rather the implementation checking
> for Objective-C classes that explicitly conform to a protocol.
> During protocol conformance checking, clang recursively searches
> up the class hierarchy for the set of methods that compose
> a protocol.  This attribute will cause the compiler to not consider
> the methods contributed by a super class, its categories, and those
> from its ancestor classes.  Thus this attribute is used to force
> subclasses to redeclare (and hopefully re-implement) methods if
> they decide to explicitly conform to a protocol where some of those
> methods may be provided by a super class.
>
> This attribute intentionally leaves out properties, which are associated
> with state.  This attribute only considers methods (at least right now)
> that are non-property accessors.  These represent methods that "do something"
> as dictated by the protocol.  This may be further refined, and this
> should be considered a WIP until documentation gets written or this
> gets removed.
>
> Added:
>     cfe/trunk/test/SemaObjC/protocols-suppress-conformance.m
> Modified:
>     cfe/trunk/include/clang/AST/DeclObjC.h
>     cfe/trunk/include/clang/Basic/Attr.td
>     cfe/trunk/lib/AST/DeclObjC.cpp
>     cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>     cfe/trunk/lib/Sema/SemaDeclObjC.cpp
>
> Modified: cfe/trunk/include/clang/AST/DeclObjC.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclObjC.h?rev=195533&r1=195532&r2=195533&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/DeclObjC.h (original)
> +++ cfe/trunk/include/clang/AST/DeclObjC.h Fri Nov 22 19:01:34 2013
> @@ -1143,7 +1143,8 @@ public:
>    ObjCMethodDecl *lookupMethod(Selector Sel, bool isInstance,
>                                 bool shallowCategoryLookup = false,
>                                 bool followSuper = true,
> -                               const ObjCCategoryDecl *C = 0) const;
> +                               const ObjCCategoryDecl *C = 0,
> +                               const ObjCProtocolDecl *P = 0) const;
>
>    /// Lookup an instance method for a given selector.
>    ObjCMethodDecl *lookupInstanceMethod(Selector Sel) const {
>
> Modified: cfe/trunk/include/clang/Basic/Attr.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=195533&r1=195532&r2=195533&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/Attr.td (original)
> +++ cfe/trunk/include/clang/Basic/Attr.td Fri Nov 22 19:01:34 2013
> @@ -642,6 +642,12 @@ def ObjCRootClass : InheritableAttr {
>    let Subjects = [ObjCInterface];
>  }
>
> +def ObjCSuppressProtocol : InheritableAttr {
> +  let Spellings = [GNU<"objc_suppress_protocol_methods">];
> +  let Subjects = [ObjCInterface];
> +  let Args = [IdentifierArgument<"Protocol", 1>];

This isn't actually optional in practice.

> +}
> +
>  def Overloadable : Attr {
>    let Spellings = [GNU<"overloadable">];
>    let Subjects = [Function];
>
> Modified: cfe/trunk/lib/AST/DeclObjC.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclObjC.cpp?rev=195533&r1=195532&r2=195533&view=diff
> ==============================================================================
> --- cfe/trunk/lib/AST/DeclObjC.cpp (original)
> +++ cfe/trunk/lib/AST/DeclObjC.cpp Fri Nov 22 19:01:34 2013
> @@ -460,7 +460,8 @@ ObjCMethodDecl *ObjCInterfaceDecl::looku
>                                                  bool isInstance,
>                                                  bool shallowCategoryLookup,
>                                                  bool followSuper,
> -                                                const ObjCCategoryDecl *C) const
> +                                                const ObjCCategoryDecl *C,
> +                                                const ObjCProtocolDecl *P) const
>  {
>    // FIXME: Should make sure no callers ever do this.
>    if (!hasDefinition())
> @@ -473,6 +474,22 @@ ObjCMethodDecl *ObjCInterfaceDecl::looku
>      LoadExternalDefinition();
>
>    while (ClassDecl) {
> +    // If we are looking for a method that is part of protocol conformance,
> +    // check if the superclass has been marked to suppress conformance
> +    // of that protocol.
> +    if (P && ClassDecl->hasAttrs()) {
> +      const AttrVec &V = ClassDecl->getAttrs();
> +      const IdentifierInfo *PI = P->getIdentifier();
> +      for (AttrVec::const_iterator I = V.begin(), E = V.end(); I != E; ++I) {
> +        if (const ObjCSuppressProtocolAttr *A =
> +            dyn_cast<ObjCSuppressProtocolAttr>(*I)){

I think this loop could be improved by using a specific_attr_iterator.

> +          if (A->getProtocol() == PI) {
> +            return 0;
> +          }
> +        }
> +      }
> +    }
> +
>      if ((MethodDecl = ClassDecl->getMethod(Sel, isInstance)))
>        return MethodDecl;
>
>
> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=195533&r1=195532&r2=195533&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Fri Nov 22 19:01:34 2013
> @@ -2134,6 +2134,28 @@ static void handleObjCRootClassAttr(Sema
>                                 Attr.getAttributeSpellingListIndex()));
>  }
>
> +static void handleObjCSuppresProtocolAttr(Sema &S, Decl *D,
> +                                          const AttributeList &Attr) {
> +  if (!isa<ObjCInterfaceDecl>(D)) {
> +    S.Diag(Attr.getLoc(), diag::err_attribute_wrong_decl_type)
> +      << Attr.getName() << ExpectedObjectiveCInterface;
> +    return;
> +  }
> +
> +  IdentifierLoc *Parm = (Attr.getNumArgs() == 1 && Attr.isArgIdent(0))
> +                        ? Attr.getArgAsIdent(0) : 0;

No need to check for getNumargs once you rectify Attr.td

> +
> +  if (!Parm) {
> +    S.Diag(D->getLocStart(), diag::err_objc_attr_not_id) << Attr.getName() << 1;

You're not actually checking whether this is an id or not.

> +    return;
> +  }
> +
> +  D->addAttr(::new (S.Context)
> +             ObjCSuppressProtocolAttr(Attr.getRange(), S.Context, Parm->Ident,
> +                                      Attr.getAttributeSpellingListIndex()));
> +}
> +
> +
>  static void handleObjCRequiresPropertyDefsAttr(Sema &S, Decl *D,
>                                                 const AttributeList &Attr) {
>    if (!isa<ObjCInterfaceDecl>(D)) {
> @@ -4713,7 +4735,10 @@ static void ProcessDeclAttribute(Sema &S
>    case AttributeList::AT_ObjCRootClass:
>      handleObjCRootClassAttr(S, D, Attr);
>      break;
> -  case AttributeList::AT_ObjCRequiresPropertyDefs:
> +  case AttributeList::AT_ObjCSuppressProtocol:
> +    handleObjCSuppresProtocolAttr(S, D, Attr);
> +    break;
> +  case AttributeList::AT_ObjCRequiresPropertyDefs:
>      handleObjCRequiresPropertyDefsAttr (S, D, Attr);
>      break;
>    case AttributeList::AT_Unused:      handleUnusedAttr      (S, D, Attr); break;
>
> Modified: cfe/trunk/lib/Sema/SemaDeclObjC.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclObjC.cpp?rev=195533&r1=195532&r2=195533&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDeclObjC.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclObjC.cpp Fri Nov 22 19:01:34 2013
> @@ -1667,7 +1667,9 @@ void Sema::CheckProtocolMethodDefs(Sourc
>            (!Super || !Super->lookupMethod(method->getSelector(),
>                                            true /* instance */,
>                                            false /* shallowCategory */,
> -                                          true /* followsSuper */))) {
> +                                          true /* followsSuper */,
> +                                          NULL /* category */,
> +                                          PDecl /* protocol */))) {
>              // If a method is not implemented in the category implementation but
>              // has been declared in its primary class, superclass,
>              // or in one of their protocols, no need to issue the warning.
> @@ -1703,7 +1705,9 @@ void Sema::CheckProtocolMethodDefs(Sourc
>          (!Super || !Super->lookupMethod(method->getSelector(),
>                                          false /* class method */,
>                                          false /* shallowCategoryLookup */,
> -                                        true  /* followSuper */))) {
> +                                        true  /* followSuper */,
> +                                        NULL /* category */,
> +                                        PDecl /* protocol */))) {
>        // See above comment for instance method lookups.
>        if (C && IDecl->lookupMethod(method->getSelector(),
>                                     false /* class */,
>
> Added: cfe/trunk/test/SemaObjC/protocols-suppress-conformance.m
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/protocols-suppress-conformance.m?rev=195533&view=auto
> ==============================================================================
> --- cfe/trunk/test/SemaObjC/protocols-suppress-conformance.m (added)
> +++ cfe/trunk/test/SemaObjC/protocols-suppress-conformance.m Fri Nov 22 19:01:34 2013
> @@ -0,0 +1,79 @@
> +// RUN: %clang_cc1  -triple x86_64-apple-darwin11 -fsyntax-only -verify %s -Wno-objc-root-class
> +
> + at protocol Protocol
> +- (void) theBestOfTimes; // expected-note {{method 'theBestOfTimes' declared here}}
> + at property (readonly) id theWorstOfTimes; // expected-note {{property declared here}}
> + at end
> +
> +// In this example, the root class provides all the methods for
> +// a protocol, and the immediate subclass adopts the attribute.
> +//
> +// The further subclasses should not have access to the root class's
> +// methods for checking protocol conformance.
> +//
> +// ClassC states protocol conformance, but does not redeclare the method.
> +// For this case we get a warning.
> +//
> +// ClassD states protocol conformance, but does redeclare the method.
> +// For this case we do not get a warning.
> +//
> +
> + at interface ClassA <Protocol>
> +- (void) theBestOfTimes;
> +//@property (readonly) id theWorstOfTimes;
> + at end
> +
> +__attribute__((objc_suppress_protocol_methods(Protocol))) @interface ClassB : ClassA @end
> +
> + at interface ClassC : ClassB <Protocol> @end // expected-note {{required for direct or indirect protocol 'Protocol'}}
> +
> + at interface ClassD : ClassB <Protocol>
> +- (void) theBestOfTimes;
> + at property (readonly) id theWorstOfTimes;
> + at end
> +
> + at implementation ClassA // expected-warning {{auto property synthesis will not synthesize property declared in a protocol}}
> +- (void) theBestOfTimes {}
> + at end
> +
> + at implementation ClassC @end // expected-warning {{method 'theBestOfTimes' in protocol not implemented}}
> +
> + at implementation ClassD // no-warning
> +- (void) theBestOfTimes {}
> + at end
> +
> +// In this example, the class both conforms to the protocl and adopts
> +// the attribute.  This illustrates that the attribute does not
> +// interfere with the protocol conformance checking for the class
> +// itself.
> +__attribute__((objc_suppress_protocol_methods(Protocol)))
> + at interface AdoptsAndConforms <Protocol>
> +- (void) theBestOfTimes;
> + at property (readonly) id theWorstOfTimes;
> + at end
> +
> + at implementation AdoptsAndConforms // no-warning
> +- (void) theBestOfTimes {}
> + at end
> +
> +// This attribute cannot be added to a class extension or category.
> + at interface ClassE
> +-(void) theBestOfTimes;
> + at end
> +
> +__attribute__((objc_supress_protocol(Protocol)))
> + at interface ClassE () @end // expected-error {{attributes may not be specified on a category}}
> +
> +__attribute__((objc_supress_protocol(Protocol)))
> + at interface ClassE (MyCat) @end // expected-error {{attributes may not be specified on a category}}
> +
> +// The attribute requires one or more identifiers.
> +__attribute__((objc_suppress_protocol_methods()))
> + at interface ClassF @end // expected-error {{parameter of 'objc_suppress_protocol_methods' attribute must be a single name of an Objective-C protocol}}

This test will be improved once you remove the optional bit from Attr.td

> +
> +// The attribute requires one or more identifiers.
> +__attribute__((objc_suppress_protocol_methods(ProtoA, ProtoB))) // expected-error {{use of undeclared identifier 'ProtoB'}}

This test should be complaining about the second argument not that it
doesn't know what second arg is.

> + at interface ClassG @end
> +__attribute__((objc_suppress_protocol_methods(1+2)))
> + at interface ClassH @end // expected-error {{parameter of 'objc_suppress_protocol_methods' attribute must be a single name of an Objective-C protocol}}
> +
> \ No newline at end of file

Missing a newline here

~Aaron