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:
> 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?
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
