mboehme added inline comments.
================
Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {
----------------
mboehme wrote:
> mboehme wrote:
> > rsmith wrote:
> > > mboehme wrote:
> > > > rsmith wrote:
> > > > > mboehme wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > mboehme wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > mboehme wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > 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`.
> > > > > > > > > > > > > I think we handle all of the parsing correctly today
> > > > > > > > > > > > > already
> > > > > > > > > > > >
> > > > > > > > > > > > I'm not sure about this. For example, take a look at
> > > > > > > > > > > > `Parser::ParseSimpleDeclaration`, which does this at
> > > > > > > > > > > > the end:
> > > > > > > > > > > >
> > > > > > > > > > > > ```
> > > > > > > > > > > > DS.takeAttributesFrom(Attrs);
> > > > > > > > > > > > ```
> > > > > > > > > > > >
> > > > > > > > > > > > Here, `DS` is a `ParsingDeclSpec`, and `Attrs` are the
> > > > > > > > > > > > attributes that were seen on the declaration (I
> > > > > > > > > > > > believe).
> > > > > > > > > > > >
> > > > > > > > > > > > If my understanding of the code is correct (which it
> > > > > > > > > > > > very well may not be), this is putting the attributes
> > > > > > > > > > > > that were seen on the declaration in the same place
> > > > > > > > > > > > (`DeclSpec::Attrs`) that will later receive the
> > > > > > > > > > > > attributes seen on the decl-specifier-seq. Once we've
> > > > > > > > > > > > put them there, it is no longer possible to tell
> > > > > > > > > > > > whether those attributes were seen on the declaration
> > > > > > > > > > > > (where type attributes are not allowed) or on the
> > > > > > > > > > > > decl-specifier-seq (where type attributes _are_
> > > > > > > > > > > > allowed).
> > > > > > > > > > > >
> > > > > > > > > > > > (This is why https://reviews.llvm.org/D124081 adds a
> > > > > > > > > > > > `DiagnoseCXX11NonDeclAttrs(Attrs)` just before the
> > > > > > > > > > > > `DS.takeAttributesFrom(Attrs)` call -- because this is
> > > > > > > > > > > > the last point at which we haven't lumped declaration
> > > > > > > > > > > > and decl-specifier-seq attributes together.)
> > > > > > > > > > > >
> > > > > > > > > > > > `ProcessDeclAttribute()`, when called through
> > > > > > > > > > > > `Sema::ProcessDeclAttributes()` (which I think is the
> > > > > > > > > > > > relevant call stack), only sees attributes after they
> > > > > > > > > > > > have been put in `DeclSpec::Attrs`. (It therefore sees,
> > > > > > > > > > > > and ignores, all of the type attributes that were put
> > > > > > > > > > > > on the decl-specifier-seq.) So I think it isn't
> > > > > > > > > > > > possible to tell there whether we put a type attribute
> > > > > > > > > > > > on a declaration.
> > > > > > > > > > > >
> > > > > > > > > > > > It might be possible to fix this by giving `DeclSpec`
> > > > > > > > > > > > two lists of attributes, one containing the attributes
> > > > > > > > > > > > for the declaration, and another containing the
> > > > > > > > > > > > attributes for the decl-specifier-seq itself. However,
> > > > > > > > > > > > this is a pretty invasive change, as everything that
> > > > > > > > > > > > consumes attributes from the `DeclSpec` potentially
> > > > > > > > > > > > needs to be changed (I tried and shied away from it).
> > > > > > > > > > > >
> > > > > > > > > > > > Am I misunderstanding something about the code? If not,
> > > > > > > > > > > > what are your thoughts on how best to proceed?
> > > > > > > > > > > > Am I misunderstanding something about the code? If not,
> > > > > > > > > > > > what are your thoughts on how best to proceed?
> > > > > > > > > > >
> > > > > > > > > > > I think your understanding is correct and I was
> > > > > > > > > > > imprecise. I meant that we generally parse the `[[]]` in
> > > > > > > > > > > all the expected locations in the parser. However, I
> > > > > > > > > > > think you're correct that the bits which determine what
> > > > > > > > > > > chunks in a declaration may need to be updated. That code
> > > > > > > > > > > largely exists because GNU-style attributes "slide"
> > > > > > > > > > > sometimes to other chunks, but that's not the case for
> > > > > > > > > > > [[]]-style attributes. It sounds like we are likely
> > > > > > > > > > > lacking expressivity for the `[[]]` type attributes in
> > > > > > > > > > > the hookup between what we parsed and when we form the
> > > > > > > > > > > semantic attribute because we need to convert the parsed
> > > > > > > > > > > type into a real type.
> > > > > > > > > > > It sounds like we are likely lacking expressivity for the
> > > > > > > > > > > [[]] type attributes in the hookup between what we parsed
> > > > > > > > > > > and when we form the semantic attribute because we need
> > > > > > > > > > > to convert the parsed type into a real type.
> > > > > > > > > >
> > > > > > > > > > OK, good, sounds like we're on the same page!
> > > > > > > > > >
> > > > > > > > > > I'll look at what we can do to retain the information of
> > > > > > > > > > whether an attribute was added to the declaration or the
> > > > > > > > > > decl-specifier-seq, so that Sema can then issue the correct
> > > > > > > > > > diagnostics. I'll look at changing `DeclSpec` so that it
> > > > > > > > > > retains not one, but two lists of attributes, for the
> > > > > > > > > > declaration and the decl-specifier-seq respectively. There
> > > > > > > > > > are likely to be some devils in the details -- `DeclSpec`
> > > > > > > > > > allows direct write access to its `Attrs` member variable
> > > > > > > > > > through the non-const version of `getAttributes()`, and
> > > > > > > > > > there are quite a few places in the code that call this.
> > > > > > > > > > Does this seem like the right direction to you in principle
> > > > > > > > > > though?
> > > > > > > > > >
> > > > > > > > > > Apart from all of this, are there any outstanding issues
> > > > > > > > > > that you see with this change? It does of course make sense
> > > > > > > > > > to prohibit type attributes on declarations before I land
> > > > > > > > > > this change, but I wanted to check whether there's anything
> > > > > > > > > > else that would need to happen here.
> > > > > > > > > > Does this seem like the right direction to you in principle
> > > > > > > > > > though?
> > > > > > > > >
> > > > > > > > > Maybe. In terms of the standards concepts, a
> > > > > > > > > `decl-specifier-seq` can have attributes associated with it
> > > > > > > > > (https://eel.is/c++draft/dcl.dcl#dcl.spec.general-1.sentence-2)
> > > > > > > > > and so from that perspective, a `DeclSpec` seems like it
> > > > > > > > > should have only one list of attributes (those for the entire
> > > > > > > > > sequence of declaration specifiers). However, GNU-style
> > > > > > > > > attributes "slide" around to other things, so it's possible
> > > > > > > > > that the behavior in `ParseSimpleDeclaration()` is correct
> > > > > > > > > for that particular syntax but incorrect for `[[]]` style
> > > > > > > > > attributes. So, depending on which devilish details are
> > > > > > > > > learned while working on this, it may turn out that the best
> > > > > > > > > approach is a secondary list, or it may turn out that we want
> > > > > > > > > to add a new `takeAll` variant that takes only the correct
> > > > > > > > > attributes while leaving others. (It may be useful to summon
> > > > > > > > > @rsmith for opinions on the right approach to take here.)
> > > > > > > > >
> > > > > > > > > > Apart from all of this, are there any outstanding issues
> > > > > > > > > > that you see with this change? It does of course make sense
> > > > > > > > > > to prohibit type attributes on declarations before I land
> > > > > > > > > > this change, but I wanted to check whether there's anything
> > > > > > > > > > else that would need to happen here.
> > > > > > > > >
> > > > > > > > > Nothing else is jumping out at me as missing or needing
> > > > > > > > > additional work aside from some formatting nits. I'm going to
> > > > > > > > > add another reviewer just to make sure I've not missed
> > > > > > > > > something, but I think this is about ready to go.
> > > > > > > > Thanks for the input!
> > > > > > > >
> > > > > > > > I've looked at this more closely, and I've concluded that
> > > > > > > > adding a second list of attributes to the `DeclSpec` isn't
> > > > > > > > really the right way to go. For one thing, as you say, the
> > > > > > > > attributes we're discussing don't really belong on the
> > > > > > > > decl-specifier-seq.
> > > > > > > >
> > > > > > > > But I _have_ come up with an alternative solution that I like
> > > > > > > > better than the alternatives I had considered so far and that,
> > > > > > > > as you suggest, emits the diagnostics from Sema, not the
> > > > > > > > Parser. The solution is not just more concise than the other
> > > > > > > > options I had explored but also just feels like the right way
> > > > > > > > of doing things.
> > > > > > > >
> > > > > > > > Taking a look at the standard, attributes in the location in
> > > > > > > > question (at the beginning of a simple-declaration) have the
> > > > > > > > same semantics as attributes that appear after a declarator-id,
> > > > > > > > and we already have a location to store the latter, namely
> > > > > > > > `Declarator::Attr`. It therefore seems the right thing to do
> > > > > > > > also put attributes from the simple-declaration in
> > > > > > > > `Declarator::Attr`. We can then take advantage of the fact that
> > > > > > > > there is existing code already in place to deal with attributes
> > > > > > > > in this location.
> > > > > > > >
> > > > > > > > The other part of the solution is to make
> > > > > > > > `ProcessDeclAttribute` warn about C++11 attributes that don’t
> > > > > > > > “slide” to the DeclSpec (except, of course, if
> > > > > > > > `ProcessDeclAttribute` is actually being called for attributes
> > > > > > > > on a `DeclSpec` or `Declarator`).
> > > > > > > >
> > > > > > > > Here’s a draft change that implements this:
> > > > > > > >
> > > > > > > > https://reviews.llvm.org/D124919
> > > > > > > >
> > > > > > > > (The change description contains more details on the attribute
> > > > > > > > semantics from the standard that I alluded to above.)
> > > > > > > >
> > > > > > > > The change still contains a few TODOs, but all of the hard bits
> > > > > > > > are done. Specifically, all of the tests pass too. I feel
> > > > > > > > pretty good about this change, but before I work on it further
> > > > > > > > to get it ready for review, I wanted to get your reaction: Do
> > > > > > > > you agree that this is the right general direction?
> > > > > > > > The change still contains a few TODOs, but all of the hard bits
> > > > > > > > are done. Specifically, all of the tests pass too. I feel
> > > > > > > > pretty good about this change, but before I work on it further
> > > > > > > > to get it ready for review, I wanted to get your reaction: Do
> > > > > > > > you agree that this is the right general direction?
> > > > > > >
> > > > > > > Thank you for your continued work on this, I *really* appreciate
> > > > > > > it! I think your newest approach is the one I'm happiest with,
> > > > > > > though there are still parts of it I hope we can find ways to
> > > > > > > improve. My biggest concern is with having to sprinkle
> > > > > > > `ExtractDefiniteDeclAttrs()` in the "correct" places, as that's
> > > > > > > likely to be fragile. If there was a way we only had to do that
> > > > > > > once instead of 8 times, I'd be much happier. My worry is mostly
> > > > > > > with someone forgetting to call it when they should or thinking
> > > > > > > they need to call it when modeling new code on existing code.
> > > > > > >
> > > > > > > Aside from that, I think the approach you've got is quite
> > > > > > > reasonable, though I'd be curious if @rsmith or @rjmccall have
> > > > > > > additional thoughts or concerns that I might not be seeing yet.
> > > > > > > My biggest concern is with having to sprinkle
> > > > > > > `ExtractDefiniteDeclAttrs()` in the "correct" places, as that's
> > > > > > > likely to be fragile. If there was a way we only had to do that
> > > > > > > once instead of 8 times, I'd be much happier. My worry is mostly
> > > > > > > with someone forgetting to call it when they should or thinking
> > > > > > > they need to call it when modeling new code on existing code.
> > > > > >
> > > > > > Agree. I would also prefer it if we could only do this in once
> > > > > > place -- but I can't really come up with a good way to do this.
> > > > > >
> > > > > > The underlying reason for why we're doing this in multiple places
> > > > > > is that there are multiple places in the C++ grammar where an
> > > > > > attribute-specifier-seq can occur that appertains to a declaration
> > > > > > (e.g. in a simple-declaration, parameter-declaration,
> > > > > > exception-declaration, and so on). These are parsed in different
> > > > > > places in the code, and so we also need to add
> > > > > > `ExtractDefiniteDeclAttrs()` in those different places. If a future
> > > > > > change to the language adds another type of declaration that can
> > > > > > contain an attribute-specifier-seq, we'll have to add a new
> > > > > > `ExtractDefiniteDeclAttrs()` there as well -- but that's really
> > > > > > just a part of saying that we need to write code that parses that
> > > > > > entire type of declaration correctly.
> > > > > >
> > > > > > Unfortunately, I don't really see what we could do to make sure
> > > > > > people don't get this wrong. The motivation behind my previous
> > > > > > attempt in https://reviews.llvm.org/D124083 was to try and make
> > > > > > errors impossible by making callers of `ParseCXX11Attributes` (and
> > > > > > related functions) specify what entity (type, statement, or
> > > > > > declaration) the attributes appertain to in the current position.
> > > > > > Unfortunately, this can't be done consistently, as we don't
> > > > > > actually always know what entity the attributes should appertain to
> > > > > > (e.g. in `Parser::ParseStatementOrDeclaration`), and the approach
> > > > > > has other downsides, as we've discussed before.
> > > > > >
> > > > > > > Aside from that, I think the approach you've got is quite
> > > > > > > reasonable, though I'd be curious if @rsmith or @rjmccall have
> > > > > > > additional thoughts or concerns that I might not be seeing yet.
> > > > > >
> > > > > > I'd like to hear their input too! Besides your mentioning them
> > > > > > here, is there anything else we need to do to make sure they see
> > > > > > this? (Should I reach out to @rsmith internally?)
> > > > > >
> > > > > > By the way, https://reviews.llvm.org/D124919 currently depends on
> > > > > > this change (https://reviews.llvm.org/D111548), so that we can show
> > > > > > the effect that it has on the tests for `annotate_type`, but if we
> > > > > > agree that https://reviews.llvm.org/D124919 is what we want to
> > > > > > proceed with, I would plan to submit it first so that we have the
> > > > > > "protections" it provides in place before adding the new attribute.
> > > > > I think the right approach here is that `ParseDeclaration`,
> > > > > `ParseExternalDeclaration`, and friends should only be given an
> > > > > attribute list that includes declaration attributes (that is, C++11
> > > > > attribute syntax attributes), and that list should not be muddled
> > > > > into the list of decl specifier attributes.
> > > > >
> > > > > As far as I can see, that is in fact already the case with only two
> > > > > exceptions;
> > > > >
> > > > > 1) `ParseExportDeclaration` parses MS attributes before recursing
> > > > > into `ParseExternalDeclaration`. That looks like it's simply a bug as
> > > > > far as I can tell, and that call can be removed. MS attributes will
> > > > > be parsed as part of the decl specifier sequence as needed and don't
> > > > > need to be parsed as declaration attributes.
> > > > > 2) `ParseStatementOrDeclarationAfterAttributes` is called after
> > > > > parsing an initial sequence of GNU attributes that might be part of
> > > > > the decl-specifier-seq, or might precede a label. In this case, we
> > > > > should simply have two attribute lists: the C++11-syntax declaration
> > > > > attributes, and the GNU decl-specifier-seq attributes that we've
> > > > > effectively tentatively parsed past.
> > > > >
> > > > > All other code paths into `ParseExternalDeclaration` /
> > > > > `ParseDeclaration` / `ParseSimpleDeclaration` parse only C++11
> > > > > attribute syntax before getting there.
> > > > >
> > > > > With those two exceptions changed, we should be able to keep the
> > > > > declaration attributes entirely separate from the decl-specifier-seq
> > > > > attributes. We can then carefully re-implement the "sliding" of
> > > > > declaration attributes onto the type in the cases where we want to do
> > > > > so for compatibility.
> > > > > I think the right approach here is that ParseDeclaration,
> > > > > ParseExternalDeclaration, and friends should only be given an
> > > > > attribute list that includes declaration attributes (that is, C++11
> > > > > attribute syntax attributes), and that list should not be muddled
> > > > > into the list of decl specifier attributes.
> > > >
> > > > I think this makes sense, but I'm not sure how we would do this in the
> > > > specific case of `ParseStatementOrDeclarationAfterAttributes` -- see
> > > > discussion below.
> > > >
> > > > Also, I wanted to make sure we're on the same page about a specific
> > > > point: For backwards compatibility, we need to continue to "slide" some
> > > > C++11 attributes to the `DeclSpec`, namely those type attributes for
> > > > which we have allowed this so far. Do you agree?
> > > >
> > > > > 1. ParseExportDeclaration parses MS attributes before recursing into
> > > > > ParseExternalDeclaration. That looks like it's simply a bug as far as
> > > > > I can tell, and that call can be removed. MS attributes will be
> > > > > parsed as part of the decl specifier sequence as needed and don't
> > > > > need to be parsed as declaration attributes
> > > >
> > > > That makes sense -- and indeed none of the other callers of
> > > > `ParseExternalDeclaration` parse MS attributes before calling it.
> > > >
> > > > I've tried removing the two calls to `MaybeParseMicrosoftAttributes` in
> > > > `ParseExportDeclaration`, and all of the tests still pass.
> > > >
> > > > I'm not sure though if this may produce new errors on some invalid code
> > > > that we used to allow, namely those cases in `ParseExternalDeclaration`
> > > > that call through to `ParseDeclaration` rather than
> > > > `ParseDeclOrFunctionDefInternal`. From looking at the code, one
> > > > erroneous code sample that I expected the current code to allow is
> > > > `export __declspec(deprecated) namespace foo {}`, but it appears that
> > > > we don't (https://godbolt.org/z/b4sE8E8GG). So maybe there's no concern
> > > > here?
> > > >
> > > > If we don't think there are any issues with this, I would prepare a
> > > > small patch that removes these two calls to
> > > > `MaybeParseMicrosoftAttributes`. (I think this can and should be a
> > > > separate patch as it's otherwise independent of the wider issue we're
> > > > discussing here.)
> > > >
> > > > > 2. ParseStatementOrDeclarationAfterAttributes is called after parsing
> > > > > an initial sequence of GNU attributes that might be part of the
> > > > > decl-specifier-seq, or might precede a label. In this case, we should
> > > > > simply have two attribute lists: the C++11-syntax declaration
> > > > > attributes, and the GNU decl-specifier-seq attributes that we've
> > > > > effectively tentatively parsed past.
> > > >
> > > > To make sure I understand: What you're saying is that
> > > > `ParseStatementOrDeclarationAfterAttributes` should take two
> > > > `ParsedAttributes` arguments, one for the C++11 attributes and one for
> > > > the GNU attributes?
> > > >
> > > > What I'm not quite clear on is how
> > > > `ParseStatementOrDeclarationAfterAttributes` should handle these two
> > > > attribute lists further. There are two places in this function that
> > > > call through to `ParseDeclaration` and I think you're saying that you'd
> > > > like `ParseDeclaration` to only take a list of C++11 attributes. But
> > > > what should `ParseStatementOrDeclarationAfterAttributes` then do with
> > > > the GNU attributes -- it can't simply drop them on the floor?
> > > >
> > > > Or does this mean that `ParseDeclaration` also needs to take two
> > > > `ParsedAttributes` arguments (and similarly for any other `Parse*`
> > > > functions it passes the attributes on to)? This is certainly doable,
> > > > but I'm not sure that we get much benefit from it. As noted at the
> > > > beginning of this comment, some "legacy" C++11 type attributes will
> > > > also need to "slide" to the `DeclSpec` for backwards compatibility, so
> > > > we can't simply put all the C++11 attributes on the declaration (which
> > > > would be the attraction of having separate lists). At that point, it
> > > > doesn't seem too harmful to me to have a single `ParsedAttributes`
> > > > argument containing all of the attributes (C++11 and GNU) that were
> > > > specified at the beginning of the declaration.
> > > >
> > > > > With those two exceptions changed, we should be able to keep the
> > > > > declaration attributes entirely separate from the decl-specifier-seq
> > > > > attributes. We can then carefully re-implement the "sliding" of
> > > > > declaration attributes onto the type in the cases where we want to do
> > > > > so for compatibility.
> > > >
> > > > This is essentially what https://reviews.llvm.org/D124919 does. To
> > > > emphasize this (and simplify the code slightly at the same time), I've
> > > > replaced the previous `ExtractDefiniteDeclAttrs()` with a function
> > > > called `SlideAttrsToDeclSpec()` that moves the "sliding" attributes
> > > > from the original `ParsedAttributes` directly to the `DeclSpec`. Is
> > > > this along the lines of what you were thinking of?
> > > > For backwards compatibility, we need to continue to "slide" some C++11
> > > > attributes to the `DeclSpec`, namely those type attributes for which we
> > > > have allowed this so far. Do you agree?
> > >
> > > I think we should allow this in the short term, but aim to remove it
> > > after we've been warning on it for a release or two. I'd even be OK with
> > > the warning being an error by default when it lands.
> > >
> > > > From looking at the code, one erroneous code sample that I expected the
> > > > current code to allow is `export __declspec(deprecated) namespace foo
> > > > {}`, but it appears that we don't (https://godbolt.org/z/b4sE8E8GG). So
> > > > maybe there's no concern here?
> > >
> > > MS attributes are `[`...`]` attributes, so the testcase would be `export
> > > [blah] namespace foo {}`, which we do currently erroneously accept.
> > > Still, I think that's highly unlikely to be a concern. Our support for
> > > C++20 modules is new, experimental, and incomplete; there's no need to
> > > worry about backwards-incompatibility here.
> > >
> > > > To make sure I understand: What you're saying is that
> > > > `ParseStatementOrDeclarationAfterAttributes` should take two
> > > > `ParsedAttributes` arguments, one for the C++11 attributes and one for
> > > > the GNU attributes?
> > >
> > > I would phrase that as: one for declaration attributes, and one for
> > > decl-specifier attributes (put another way: one parameter for declaration
> > > attributes, and one parameter for the portion of a decl-specifier-seq
> > > that we parsed while looking for a label, which happens to be only GNU
> > > attributes). But yes.
> > >
> > > > What I'm not quite clear on is how
> > > > `ParseStatementOrDeclarationAfterAttributes` should handle these two
> > > > attribute lists further. There are two places in this function that
> > > > call through to `ParseDeclaration` and I think you're saying that you'd
> > > > like `ParseDeclaration` to only take a list of C++11 attributes. But
> > > > what should `ParseStatementOrDeclarationAfterAttributes` then do with
> > > > the GNU attributes -- it can't simply drop them on the floor?
> > > >
> > > > Or does this mean that `ParseDeclaration` also needs to take two
> > > > `ParsedAttributes` arguments (and similarly for any other `Parse*`
> > > > functions it passes the attributes on to)? This is certainly doable,
> > > > but I'm not sure that we get much benefit from it. As noted at the
> > > > beginning of this comment, some "legacy" C++11 type attributes will
> > > > also need to "slide" to the `DeclSpec` for backwards compatibility, so
> > > > we can't simply put all the C++11 attributes on the declaration (which
> > > > would be the attraction of having separate lists).
> > >
> > > Yes, we should propagate the two lists into `ParseDeclaration` and
> > > `ParseSimpleDeclaration`. That should be the end of it:
> > > `ParseSimpleDeclaration` forms a `ParsingDeclSpec`, which can be handed
> > > the decl-specifier attributes. The other cases that `ParseDeclaration`
> > > can reach should all prohibit decl-specifier attributes.
> > >
> > > > At that point, it doesn't seem too harmful to me to have a single
> > > > `ParsedAttributes` argument containing all of the attributes (C++11 and
> > > > GNU) that were specified at the beginning of the declaration.
> > >
> > > I think it will be best for ongoing maintenance if we model the grammar
> > > in the parser as it is -- C++11 declaration attributes are a separate
> > > list that's not part of the decl-specifier-seq, and I expect we will have
> > > emergent bugs and confusion if we try to combine them. Attribute handling
> > > in declaration parsing is already made confusing because we don't clearly
> > > distinguish between these two lists, leading to bugs like parsing MS
> > > attributes in `ParseExportDeclaration`.
> > >
> > > > > With those two exceptions changed, we should be able to keep the
> > > > > declaration attributes entirely separate from the decl-specifier-seq
> > > > > attributes. We can then carefully re-implement the "sliding" of
> > > > > declaration attributes onto the type in the cases where we want to do
> > > > > so for compatibility.
> > > >
> > > > This is essentially what https://reviews.llvm.org/D124919 does. To
> > > > emphasize this (and simplify the code slightly at the same time), I've
> > > > replaced the previous `ExtractDefiniteDeclAttrs()` with a function
> > > > called `SlideAttrsToDeclSpec()` that moves the "sliding" attributes
> > > > from the original `ParsedAttributes` directly to the `DeclSpec`. Is
> > > > this along the lines of what you were thinking of?
> > >
> > > Approximately. I also think we should avoid moving attributes around
> > > between attribute lists entirely, and instead preserve the distinct
> > > attribute lists until we reach the point where we can apply them to a
> > > declaration in `Sema`. I would note that the approach in that patch seems
> > > like it'll reorder attributes, and we have attributes where the order
> > > matters (eg, `[[clang::enable_if(a, "")]] void f [[clang::enable_if(b,
> > > "")]]()`). Also, moving attributes around makes it hard to properly
> > > handle source locations / ranges for attributes.
> > >
> > > In particular: I would make the `ParsingDeclarator` have a pointer to the
> > > declaration attributes in addition to its list of attributes on the
> > > declarator-id. Then, when we apply those attributes to the declaration,
> > > pull them from both places (declaration attributes then declarator-id
> > > attributes, to preserve source order, skipping "slided" type attributes
> > > in the former list). And when we form the type for the declarator, also
> > > apply any attributes from the declaration's attribute list that should be
> > > "slided" on the type.
> > (Oops -- only just realized that I wrote up this comment in draft form last
> > week but never sent it.)
> >
> > Thank you for the really in-depth reply!
> >
> > > > For backwards compatibility, we need to continue to "slide" some C++11
> > > > attributes to the `DeclSpec`, namely those type attributes for which we
> > > > have allowed this so far. Do you agree?
> > >
> > > I think we should allow this in the short term, but aim to remove it
> > > after we've been warning on it for a release or two. I'd even be OK with
> > > the warning being an error by default when it lands.
> >
> > And there would be a command-line flag that would allow "downgrading" the
> > error to a warning?
> >
> > > > From looking at the code, one erroneous code sample that I expected the
> > > > current code to allow is `export __declspec(deprecated) namespace foo
> > > > {}`, but it appears that we don't (https://godbolt.org/z/b4sE8E8GG). So
> > > > maybe there's no concern here?
> > >
> > > MS attributes are `[`...`]` attributes,
> >
> > Oops *facepalm* A closer look at `ParseMicrosoftAttributes` could have told
> > me that. I didn't even know of these until now TBH.
> >
> > > so the testcase would be `export [blah] namespace foo {}`, which we do
> > > currently erroneously accept. Still, I think that's highly unlikely to be
> > > a concern. Our support for C++20 modules is new, experimental, and
> > > incomplete; there's no need to worry about backwards-incompatibility here.
> >
> > Got it. I'll submit a separate small patch that removes those two calls to
> > `MaybeParseMicrosoftAttributes` then.
> >
> > > > To make sure I understand: What you're saying is that
> > > > `ParseStatementOrDeclarationAfterAttributes` should take two
> > > > `ParsedAttributes` arguments, one for the C++11 attributes and one for
> > > > the GNU attributes?
> > >
> > > I would phrase that as: one for declaration attributes, and one for
> > > decl-specifier attributes (put another way: one parameter for declaration
> > > attributes, and one parameter for the portion of a decl-specifier-seq
> > > that we parsed while looking for a label, which happens to be only GNU
> > > attributes). But yes.
> >
> > Got it, that makes sense.
> >
> > [snip]
> >
> > > > This is essentially what https://reviews.llvm.org/D124919 does. To
> > > > emphasize this (and simplify the code slightly at the same time), I've
> > > > replaced the previous `ExtractDefiniteDeclAttrs()` with a function
> > > > called `SlideAttrsToDeclSpec()` that moves the "sliding" attributes
> > > > from the original `ParsedAttributes` directly to the `DeclSpec`. Is
> > > > this along the lines of what you were thinking of?
> > >
> > > Approximately. I also think we should avoid moving attributes around
> > > between attribute lists entirely, and instead preserve the distinct
> > > attribute lists until we reach the point where we can apply them to a
> > > declaration in `Sema`. I would note that the approach in that patch seems
> > > like it'll reorder attributes, and we have attributes where the order
> > > matters (eg, `[[clang::enable_if(a, "")]] void f [[clang::enable_if(b,
> > > "")]]()`). Also, moving attributes around makes it hard to properly
> > > handle source locations / ranges for attributes.
> >
> > Thanks for pointing out the importance of ordering. I did notice some
> > ordering issues while working on https://reviews.llvm.org/D124919 (see
> > comments in SlideAttrsToDeclSpec() if you're interested), but this drives
> > home the point that we need to be careful about ordering.
> >
> > > In particular: I would make the `ParsingDeclarator` have a pointer to the
> > > declaration attributes in addition to its list of attributes on the
> > > declarator-id. Then, when we apply those attributes to the declaration,
> > > pull them from both places (declaration attributes then declarator-id
> > > attributes, to preserve source order, skipping "slided" type attributes
> > > in the former list). And when we form the type for the declarator, also
> > > apply any attributes from the declaration's attribute list that should be
> > > "slided" on the type.
> >
> > Thanks, I think I can see how that's going to work.
> >
> > I'll attempt to implement this, which will likely take a few days, and will
> > report back when I have something that works and that I'd like to get
> > feedback on. I may also have some followup questions if I run into
> > unanticipated problems (which is more likely than not, I assume).
> @rsmith I have a draft implementation of the idea we discussed above here:
>
> https://reviews.llvm.org/D126061
>
> (All tests pass.)
>
> I'd appreciate your feedback. Does this look like what you had in mind?
>
> There's one main point I'd like to discuss. I've chosen to store the
> declaration attributes in a new field on the `Declarator`, so we now have
> `Declarator::Attrs` and `Declarator::DeclarationAttrs` (and I believe this is
> what we had discussed before). This makes sense from a semantic point of view
> because attributes in both positions have the same meaning.
>
> However there's one point I'm not completely satisfied with: Because a single
> declaration can have multiple declarators, we need to make sure that we
> preserve the declaration attributes when we reuse a `Declarator` for multiple
> declarators in a single declaration. This gives rise to the slightly
> unfortunate `Declarator::clearExceptDeclarationAttrs()`.
>
> All in all, I feel the implementation would be cleaner if we put the
> declaration attributes on the `DeclSpec` instead of the `Declarator`, even if
> the declaration attributes are semantically less related to the `DeclSpec`
> than the `Declarator`. What do you think?
PS Another option would be to introduce a `Declaration` class and put the
declaration attributes on that. The `Declarator` would hold a reference to the
`Declaration`.
This solution would represent the actual entities in the grammar best, but I
wonder if it's worth introducing a `Declarator` class for this purpose alone.
There might also be potential for confusion because we already have a `Decl`
class in the AST.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111548/new/
https://reviews.llvm.org/D111548
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
