Add ParsedAttrInfo::handleDeclAttribute

[john.brawn]

Summary

This makes it possible for plugin attributes to actually do something, and also removes a lot of boilerplate for simple attributes in SemaDeclAttr.cpp.

[john.brawn]

Resurrecting this old patch, plus take the opportunity to remove the need for some boilerplate code. This is the third of four patches to make it possible for clang plugins to define attributes.

[aaron.ballman]

clang/docs/ClangPlugins.rst

74 ↗

(On Diff #239299)

We should be documenting the fields of ParsedAttrInfo that the user would be interacting with. We should also consider whether we are introducing a new stability guarantee with that interface and document accordingly. I think we should not promise that this structure will be stable from release to release, but it will be stable within a given release (e.g., the type can change between Clang 11 -> Clang 12, but the type will not change between Clang 11 -> Clang 11.0.1).

clang/lib/Sema/SemaDeclAttr.cpp

6701–6702 ↗

(On Diff #239299)

I think this might make more logical sense as part of the default label in the switch statement.

[john.brawn]

clang/docs/ClangPlugins.rst

74 ↗

(On Diff #239299)

I'll add documentation of the relevant fields. As to API stability, looking at Tooling.rst it says only libclang has a stable API (or rather, it says libclang has a stable API, says libtooling doesn't, and says nothing about plugins).

clang/lib/Sema/SemaDeclAttr.cpp

6701–6702 ↗

(On Diff #239299)

Sounds reasonable, I'll do that.

[aaron.ballman]

Adding @rsmith for questions about stability guarantees.

clang/docs/ClangPlugins.rst

74 ↗

(On Diff #239299)

We may want to bump this out into a larger discussion and it shouldn't impact the ability to move forward with this patch while we discuss.

I think that for practical purposes we should be guaranteeing ABI stability for plugins during a patch releases (10.0 -> 10.0.1) or otherwise patch releases become a major compiler upgrade as opposed to an incremental one. I thought that we had already promised such stability for things like the AST serialization format for similar reasons, but maybe I'm mis-remembering. @rsmith -- do you have thoughts?

To be clear about the situation I'm concerned with -- it would be a shame to have a working attribute plugin in Clang 10.0 that either fails to load or silently behaves differently in Clang 10.0.1 because we generally want developers to be able to move to the latest patch release without undue burden.

[john.brawn]

Update based on review comments.

[aaron.ballman]

clang/docs/ClangPlugins.rst

89 ↗

(On Diff #246987)

If it must always be this one specific value, why do we make users set it manually instead of setting it automatically for them?

90 ↗

(On Diff #246987)

The documentation should mention what type this is -- the example above doesn't name the type, just uses braced initialization, so it may be unclear how to specify a scope, for instance.

92 ↗

(On Diff #246987)

This returns a value, but there's no mention of what true/false means, and the example doesn't make it clear how one would "handle" the attribute (e.g., how one creates the semantic attribute and attaches it to the declaration).

[aaron.ballman]

clang/docs/ClangPlugins.rst

105 ↗

(On Diff #246987)

It might also be nice to link to the example code from D31343 from the documentation to help get users started. e.g., "To see a working example of an attribute plugin, see clang/examples/Attribute/Attribute.cpp" with a link to the git repo source viewer for that file. WDYT?

[john.brawn]

Update based on review comments.

[john.brawn]

clang/docs/ClangPlugins.rst

89 ↗

(On Diff #246987)

I've adjusted ParsedAttrInfo to default to NoSemaHandlerAttr.

105 ↗

(On Diff #246987)

I'll add that as part of D31343.

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

6701–6707 ↗

(On Diff #247681)

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?

[john.brawn]

clang/lib/Sema/SemaDeclAttr.cpp

6701–6707 ↗

(On Diff #247681)

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.

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

6701–6707 ↗

(On Diff #247681)

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.

[john.brawn]

clang/lib/Sema/SemaDeclAttr.cpp

6701–6707 ↗

(On Diff #247681)

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.

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

6701–6707 ↗

(On Diff #247681)

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();

[john.brawn]

Use an enum for the return value.

[aaron.ballman]

LGTM!

include/clang/Sema/AttributeList.h

struct ParsedAttrInfo {
   /// issue a diagnostic if not
   virtual bool diagLangOpts(Sema &S, const AttributeList &Attr) const {
     return true;
   }
   /// Check if this attribute is allowed when compiling for the given target
   virtual bool existsInTarget(const TargetInfo &Target) const {
     return true;
   }
+  /// If this ParsedAttrInfo knows how to apply an attribute to a decl then do
+  /// so and return true. If not return false.
+  virtual bool handleDeclAttribute(Sema &S, Decl *D,
+                                   const AttributeList &Attr) {
+    return false;
+  }
 };
 
 /// AttributeList - Represents a syntactic attribute.
 ///
 /// For a GNU attribute, there are four forms of this construct:
 ///
 /// 1: __attribute__(( const )). ParmName/Args/NumArgs will all be unused.
 /// 2: __attribute__(( mode(byte) )). ParmName used, Args/NumArgs unused.

lib/Sema/SemaDeclAttr.cpp

S.Diag(Attr.getLoc(), Attr.isDeclspecAttribute()
                               : diag::warn_unknown_attribute_ignored)
         << Attr.getName();
     return;
   }
 
   if (handleCommonAttributeFeatures(S, scope, D, Attr))
     return;
 
+  if (Attr.getInfo()->handleDeclAttribute(S, D, Attr))
+    return;
+
   switch (Attr.getKind()) {
   default:
     if (!Attr.isStmtAttr()) {
       // Type attributes are handled elsewhere; silently move on.
       assert(Attr.isTypeAttr() && "Non-type attribute not handled");
       break;
     }
     S.Diag(Attr.getLoc(), diag::err_stmt_attribute_invalid_on_decl)