mboehme added a comment. Is there anything else that needs to happen on this change before it's ready to land?
There is some breakage in the pre-merge checks, but it looks as if it's pre-existing breakage. See the comment-only change here that looks as if it exhibits similar breakage: https://reviews.llvm.org/D127494 ================ 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: > > 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. Some notes for those following along: @rsmith and @aaron.ballman are both in the process of reviewing https://reviews.llvm.org/D126061 and have stated that they think this is the right direction. I also wanted to clarify how I want to proceed with this change and https://reviews.llvm.org/D126061. I believe I originally said that I wanted to land https://reviews.llvm.org/D126061 first, then this change afterwards. However, I've now changed my mind about this. We want `annotate_type` to be available in https://reviews.llvm.org/D126061 because `annotate_type` is the first type attribute that doesn't exhibit the legacy "sliding" behavior. Without it, https://reviews.llvm.org/D126061 can't introduce some of the new behavior it introduces. So my plan is now to wait until both this change and https://reviews.llvm.org/D126061 are ready to land, and then land them in sequence (this change first, then https://reviews.llvm.org/D126061). Repository: rG LLVM Github Monorepo 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
