aaron.ballman 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:
> 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.
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
