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 {
----------------
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).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111548/new/
https://reviews.llvm.org/D111548
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
