[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:3184
+            continue;
+          // We reject AT_LifetimeBound and AT_AnyX86NoCfCheck, even though they
+          // are type attributes, because we historically haven't allowed these
----------------
mboehme wrote:
> aaron.ballman wrote:
> > I think this is a FIXME situation -- our policy is to diagnose attributes that are being ignored, and it seems pretty important to (someday, not part of this patch) diagnose these. Especially the lifetime bound attribute given that it's intended to help harden code.
> Maybe I should clarify what's happening here: We _do_ produce diagnostics (errors) for AT_LifetimeBound and AT_AnyX86NoCfCheck, both before and after this patch. (For these attributes, the code falls through to the `Diag(PA.getLoc(), diag::err_attribute_not_type_attr)` call below.)
> 
> Before this patch, we would reject (produce errors for) _all_ C++11 attributes here. Now, we only reject non-type attributes, and in addition, we also reject AT_LifetimeBound and AT_AnyX86NoCfCheck (even though they are type attributes) because we historically haven't allowed them to be used in this way. There are tests for this behavior, so it seemed important to preserve it.
Oh derp, that's my mistake, I read the `!=` logic wrong. Thank you for the clarification!


================
Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {
----------------
mboehme wrote:
> aaron.ballman wrote:
> > mboehme wrote:
> > > aaron.ballman wrote:
> > > > Can you also add some test coverage for applying the attribute to a declaration instead of a type or not giving it any arguments? Also, should test arguments which are not a constant expression.
> > > I've added tests as you suggested, though I put most of them in Sema/annotate-type.c, as they're not specific to C++.
> > > 
> > > Thanks for prompting me to do this -- the tests caused me to notice and fix a number of bugs.
> > > 
> > > I haven't managed to diagnose the case when the attribute appears at the beginning of the declaration. It seems to me that, at the point where I've added the check, this case is indistinguishable from an attribute that appears on the type. In both cases, the `TAL` is `TAL_DeclSpec`, and the attribute is contained in `DeclSpec::getAttributes()`. This is because `Parser::ParseSimpleDeclaration` forwards the declaration attributes to the `DeclSpec`:
> > > 
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > 
> > > I believe if I wanted to prohibit this case, I would need to add a check to `Parser::ParseStatementOrDeclaration` and prohibit any occurrences of `annotate_type` there. However, this seems the wrong place to do this because it doesn't contain any specific processing for other attributes.
> > > 
> > > I've noticed that Clang also doesn't prohibit other type attributes (even ones with C++ 11 syntax) from being applied to declarations. For example: https://godbolt.org/z/Yj1zWY7nn
> > > 
> > > How do you think I should proceed here? I think the underlying issue is that Clang doesn't always distinguish cleanly between declaration attributes and type attributes, and fixing this might be nontrivial.
> > > How do you think I should proceed here? I think the underlying issue is that Clang doesn't always distinguish cleanly between declaration attributes and type attributes, and fixing this might be nontrivial.
> > 
> > This is a general issue with attribute processing. I would imagine that SemaDeclAttr.cpp should be able to diagnose that case when the attribute only applies to types and not declarations, but it'd take some investigation for me to be sure.
> > 
> > Because this issue isn't new to your situation, I'd recommend filing an issue about the general problem and then we can solve that later.
> I've done some more investigation myself, and I think I've come up with a solution; actually, two potential solutions. I have draft patches for both of these; I wanted to run these by you here first, so I haven't opened a bug yet.
> 
> I'd appreciate your input on how you'd prefer me to proceed with this. I do think it makes sense to do this work now because otherwise, people will start putting `annotate_type` in places where it doesn't belong, and then we'll have to keep supporting that in the future.
> 
> **My preferred solution**
> 
> https://reviews.llvm.org/D124081
> 
> This adds a function `DiagnoseCXX11NonDeclAttrs()` and calls it in all places where we should reject attributes that can't be put on declarations. (As part of this work, I noticed that `gsl::suppress` should be a `DeclOrStmtAttr`, not just a `StmtAttr`.)
> 
> Advantages:
> - Not very invasive.
> Disadvantages:
> - Need to make sure we call `DiagnoseCXX11NonDeclAttrs()` everywhere that non-declaration attributes should be rejected. (Not sure if I've identified all of those places yet?)
> 
> **Alternative solution**
> 
> https://reviews.llvm.org/D124083
> 
> This adds an `appertainsTo` parameter to `ParseCXX11Attributes()` and other "parse attributes" functions that call it. This parameter specifies whether the attributes in the place where they're currently being parsed appertain to a declaration, statement or type. If `ParseCXX11Attributes()` encounters an attribute that is not compatible with `appertainsTo`, it outputs a diagnostic.
> 
> Advantages:
> - Every call to `ParseCXX11Attributes()` _has_ to specify `appertainsTo` -- so there's no risk of missing some case where we have to output diagnostics
> Disadvantages:
> - This change is _much_ more invasive.
> - It's not always clear what value to specify for `appertainsTo`. The poster child for this is `Parser::ParseStatementOrDeclaration()`: As the name says, we don't yet know whether we're parsing a statement or a declaration. As a result, we need to be able to specify `AppertainsToUnknown` and would need to add additional logic deeper in the call stack to produce diagnostics once we know for sure whether we're dealing with a statement or declaration. (This isn't yet implemented.) This makes the solution less elegant that it initially seems.
> - There's a tension between this new functionality and the existing `ParsedAttr::diagnoseAppertainsTo()`. However, we unfortunately can't extend the existing `ParsedAttr::diagnoseAppertainsTo()` because it is called from `ProcessDeclAttribute()` and (despite the name) that function is also processes some legitimate type attributes.
> I do think it makes sense to do this work now because otherwise, people will start putting annotate_type in places where it doesn't belong, and then we'll have to keep supporting that in the future.

Thank you for working on this now, I agree that'd be good to avoid.

My concern with both of those approaches is that they impact *parsing* rather than *semantics* and that seems wrong. As far as the parser is concerned, attributes really only exist as one particular form of syntax or another, but it shouldn't care about whether an attribute is allowed to appertain to a particular thing or not. e.g., an attribute list can either appear somewhere in the syntax or it can't, but what particular attributes are specified do not matter at parsing time. Parsing determine what the attribute is associated with (a declaration, a type, a statement, etc). After we've finished parsing the attributes in the list, we convert the attributes into their semantic form and that's the time at which we care about whether the attribute may be written on the thing it appertains to.

I think we handle all of the parsing correctly today already, but the issue is on the semantic side of things. An attribute at the start of a line can only be one of two things: a statement attribute or a declaration attribute, it can never be a type attribute. So what I think should happen is that `ProcessDeclAttribute()` needs to get smarter when it's given an attribute that can only appertain to a type. I don't think we should have to touch the parser at all, especially because you can specify multiple attributes in one list, as in:
```
void func() {
  [[decl_attr, type_attr]] int foo;
}
```
and we'd want to diagnose `type_attr` but still allow `decl_attr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111548