[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6701-6707
     if (!AL.isStmtAttr()) {
       // Type attributes are handled elsewhere; silently move on.
       assert(AL.isTypeAttr() && "Non-type attribute not handled");
       break;
     }
     S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)
         << AL << D->getLocation();
----------------
john.brawn wrote:
> aaron.ballman wrote:
> > john.brawn wrote:
> > > aaron.ballman wrote:
> > > > If we're in the situation where we parse a plugin-based attribute (so `AL.getKind()` falls into the `default` label) and its `handleDeclAttributes()` (incorrectly) returns false rather than true, it seems like we would not want to trigger either of these tests, correct? I'm thinking about a case where the plugin knows about that attribute but cannot apply it because of some other issues (maybe the subject is wrong, or args are incorrect, etc) and returns `false` here. We wouldn't want to assert or claim that this is an invalid statement attribute applied to a decl in that case.
> > > > 
> > > > Rather than requiring the user to return true from `handleDeclAttribute()`, it seems that what we really want is something more like:
> > > > ```
> > > > if (AL.getInfo().hasHandlerFunc()) {
> > > >   AL.getInfo().handleDeclAttribute(S, D, AL);
> > > >   break;
> > > > }
> > > > if (!AL.isStmtAttr()) {
> > > >   ...
> > > > }
> > > > S.Diag(...);
> > > > ```
> > > > where an unknown attribute does not have a handler, but simple and plugin attributes do have a handler. This way the user doesn't have the chance to accidentally get confused about what it means to handle an attribute. WDYT?
> > > That sounds a little overly complex, as instead of defining one thing you now have to define two things so you have the possibility of defining one but not the other which won't do anything useful.
> > I was hoping we could do it in such a way that the plugin user defines `handleDeclAttribute()` and the `hasHandlerFunc()` logic is automatic. This way, the user just has to define their function and "everything just works".
> > 
> > My big concern here is that we have a function with a useless return value (from the plugin author's perspective) but they're likely to get the behavior wrong through simple misunderstanding. Unless the plugin author is careful with their testing, this will lead to failed assertions when we go to check for whether it's type attribute or not.
> I don't think there's any straightforward way that hasHanderFunc could be implemented automatically. The obvious choice of
> 
> ```
>   return &AL.handleDeclAttribute == &ParsedAttrInfo::handleDeclAttribute;
> ```
> doesn't work as &X.Y is only allowed if X.Y is an lvalue, which isn't the case for a member function.
> 
> We could do something by use of a template:
> 
> ```
> class ParsedAttrInfo {
> public:
>   virtual bool hasHandlerFunc() = 0;
>   virtual void handleDeclAttribute() { }
> };
> 
> template<class C> class ParsedAttrInfoTemplate : public ParsedAttrInfo {
> public:
>   virtual bool hasHandlerFunc() {
>     return &C::handleDeclAttribute != &ParsedAttrInfo::handleDeclAttribute;
>   }
> };
> 
> class ExampleAttribute : public ParsedAttrInfoTemplate<ExampleAttribute> {
> public:
>   virtual void handleDeclAttribute() { }
> };
> 
> void call_fn(ParsedAttrInfo *p) {
>   if (p->hasHandlerFunc()) {
>     p->handleDeclAttribute();
>   }
> }
> ```
> This works, but now we have to be careful to inherit from ParsedAttrInfoTemplate in the right way.
Thank you for investigating that as a possible solution -- I agree that it doesn't seem too feasible. However, my concern still stands that this particular bit of code is too fragile and can lead to mysterious behavior for plugin authors pretty easily.

What about a tri-state boolean return value (aka, an enum)? The default `ParsedAttrInfo::handleDeclAttribute` function could return "attribute not known", and the other two states could be "attribute applied" or "attribute not applied". The logic here would then become something like:
```
if (AL.getInfo().handleDeclAttribute(S, D, AL) != AttributeNotKnown)
  break;
if (!AL.isStmtAttr()) {
  // Type attributes are handled elsewhere; silently move on.
  assert(AL.isTypeAttr() && "Non-type attribute not handled");
  break;
}
S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)
    << AL << D->getLocation();
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D31342/new/

https://reviews.llvm.org/D31342