[clang][parser] Allow attributes in explicit template instantiations

[tbaeder]

Summary

This call to ProhibitAttributes() is almost always dead code.

For GNU attributes, it always is, even though they are being used in explicit template instantiations in, e.g. in clang/test/CodeGenCXX/visibility.cpp:

#define DEFAULT __attribute__((visibility("default")))
namespace PR11690 {
  template<class T> struct Class {
    void size() const {
    }
  };
  template class DEFAULT Class<char>;
}

This code is accepted by current g++ and clang++, even though the code claims to reject it because of the attributes.

If the attributes are changed to C++ attributes however, clang++ _does_ reject it:

#define DEFAULT [[]]
namespace PR11690 {
  template<class T> struct Class {
    void size() const {
    }
  };
  template class DEFAULT Class<char>;
}

But g++ still accepts the above code. Simply ignoring the attributes does not work either as it breaks tests (e.g. the visibility test from above).

I am not sure why or if GNU-style and C++ attributes should be handled differently here, so I am open for a discussion but this call seems mislead to me.

Thanks,
Timm

[aaron.ballman]

I don't think that's dead code, but I do think it's unclear code that should be fixed.

The behavior for double square bracket attributes in C++ is correct in Clang and incorrect in GCC, according to: http://eel.is/c++draft/temp.explicit#3.sentence-3. Adding a comment to that effect would be reasonable so there's not confusion again in the future.

Based on empirical testing, I think the GNU-style attribute should continue to be accepted in that position as it has effect in GCC.

[tbaeder]

You are right of course. I changed the diff to allow GNU-style attributes and reject everything else.

This is a bit hairy because [[]] is an empty attribute list but the SourceRange is still valid. We don't know if the source range was for [[]] or __attribute__(()) though. I opted for simply rejecting the empty GNU-style attribute list anyway.

[aaron.ballman]

clang/include/clang/Parse/Parser.h

1586–1589 ↗

(On Diff #326882)

1591–1593 ↗

(On Diff #326882)

I would argue that this function should return false in this case because it has no attributes and since it has no attributes, it does not contain *only* GNU attributes. There shouldn't be a need to check Range.isInvalid() at all but could instead return !empty(); and this functionality could be moved to ParsedAttributes instead of requiring there to be a range.

However, I don't know that this function needs to exist at all (see below).

clang/lib/Parse/ParseDeclCXX.cpp

1827–1829

We really should be using ProhibitCXX11Attributes() here, but that doesn't currently handle the case where it's an empty attribute list. However, we could use the valid source range information in that case to look at the tokens used to decide whether to diagnose or not.

[tbaeder]

clang/lib/Parse/ParseDeclCXX.cpp

1827–1829

Unfortunately, also diagnosing empty [[]] triggers a whole lot of the tests, e.g. because int [[]] foo; now starts warning.

[aaron.ballman]

clang/lib/Parse/ParseDeclCXX.cpp

1827–1829

Yeah, I would not be surprised that other parts of the system are accidentally relying on silently accepting [[]]. I would guess the warning about int [[]] foo; is because we don't *support* any attributes in that position currently rather than because the attribute list is prohibited from being in that position.

Typically, we use DiagnoseAndSkipCXX11Attributes() when we are not allowed to *parse* the attribute list in a given position and we use ProhibitCXX11Attributes() when we've already done the parsing work but don't support attributes in that position. However, I think this case would be difficult to implement at the parsing level (but maybe I'm wrong).

One possible way forward is to add a parameter to ProhibitCXX11Attributes() for whether to diagnose an empty list or not.

[tbaeder]

Thanks for the suggestions, Aaron. The new version seems to work but I was not sure how to get the token at a source location. I could only find Lexer::findNextToken().

[aaron.ballman]

clang/lib/Parse/ParseDecl.cpp

1618 ↗

(On Diff #327718)

This will incorrectly classify an empty MS attribute [] as being a prohibited [[]] attribute. I think we need something like:

if (Tok && Tok->is(tok::l_square)) {  
 SourceLocation NextTokLoc = Lexer::findLocationAfterToken(Attrs.Range.getBegin(), Tok.getKind(), SM, getLangOpts(), true);
 auto NextTok = Lexer::findNextToken(NextTokLoc, SM, getLangOpts());
 if (NextTok && NextTok->is(tok::l_square)) {
   ...
 }
}

Also, I think it should use DiagID rather than hard-coding the diagnostic to use.

[tbaeder]

clang/lib/Parse/ParseDecl.cpp

1618 ↗

(On Diff #327718)

The findNextToken() here returns the second [ in [[]], so would return the ] for []. The code you proposed doesn't work because the second findNextToken() returns the first ], so not a tok::l_square. I know your code makes more sense since we're looking for [[, but I couldn't find a way to get the first [ from the Lexer.

[tbaeder]

Hey Aaron, do you have any more comments on this?

[aaron.ballman]

clang/lib/Parse/ParseDecl.cpp

1618 ↗

(On Diff #327718)

Hrm, that surprises me -- we have the correct range when printing diagnostics, such as https://godbolt.org/z/r8rTKhY7f.

What happens if you use Lexer::getRawToken() on Attrs.Range.getBegin()? Does that find the first [ or the second one?

[tbaeder]

I'm gonna continue the conversation here if that's ok, the inline comments are rather cramped and confusing.

So, I just double-checked this and Attrs.Range.getBegin() returns the location of the first [, which is correct and not a problem. Lexer::getRawToken(Attrs.Range.getBegin(), ...) then returns the token as expected, no problem either.

The test code I'm looking at now is:

template<typename> struct Template {};
template struct [[]] Template<char>;

and Attrs.Range.getBegin() correctly returns line 2, column 17 and .getEnd() is line 2 column 20. All correct.

But using Lexer::findLocationAfterToken() will return column 19, not 18, so the first ]. However, it seems like directly calling Lexer::findNextToken() with the location of the first token (without a Lexer::findLocationAfterToken() in between) will return column 18, so the second [.

[aaron.ballman]

I think this basically LGTM with a few minor nits, but I'd like to make sure @rsmith doesn't have concerns, so please wait a few days before landing in case he wants to chime in.

clang/lib/Parse/ParseDecl.cpp

1618 ↗

(On Diff #332886)

We may as well hoist the call to getLangOpts() the same as we do for getSourceManager().

1626 ↗

(On Diff #332886)

[tbaeder]

Alright, thanks for the review.

[tbaeder]

Added the changes from https://reviews.llvm.org/D99338 and https://reviews.llvm.org/D99278

[aaron.ballman]

Thanks for merging, I think this seems reasonable but is missing some test coverage still.

clang/lib/Parse/ParseDecl.cpp

4656–4657 ↗

(On Diff #334179)

Test coverage for this change?

clang/lib/Parse/ParseDeclCXX.cpp

1921

Test coverage for this change?

[tbaeder]

clang/lib/Parse/ParseDecl.cpp

4656–4657 ↗

(On Diff #334179)

I did not add tests for these changes since they are NFC and already tested:

1. We test that empty [[]] are not allowed for example in clang/test/Parser/cxx0x-attributes.c:192: enum [[]] E1 e;
2. We test that GNU-style attributes are still allowed for example in clang/test/Sema/ast-print.c:94: enum __attribute__((deprecated)) EnumWithAttributes2 *EnumWithAttributes2Ptr;

clang/lib/Parse/ParseDeclCXX.cpp

1921

1. We already test that empty [[]] is diagnosed for example in clang/test/Parser/cxx0x-attributes.c:178: struct [[]] N::S s;
2. We test that GNU-style attributes are not diagnosed for example in clang/test/Sema/struct-decl.c:71: typedef struct __attribute__((noreturn)) PreserveAttributes PreserveAttributes_t;

[aaron.ballman]

LGTM, thank you for the fixes!

clang/lib/Parse/ParseDecl.cpp

4656–4657 ↗

(On Diff #334179)

Ah, thanks, I missed that we had already tested both kinds of attributes instead of just one. (Same below.)

clang/lib/Parse/ParseDeclCXX.cpp

if (TemplateId) {
     // or explicit instantiation.
     ASTTemplateArgsPtr TemplateArgsPtr(TemplateId->getTemplateArgs(),
                                        TemplateId->NumArgs);
     if (TemplateId->isInvalid()) {
       // Can't build the declaration.
     } else if (TemplateInfo.Kind == ParsedTemplateInfo::ExplicitInstantiation &&
         TUK == Sema::TUK_Declaration) {
       // This is an explicit instantiation of a class template.
-      ProhibitAttributes(attrs);
-
       TagOrTempResult = Actions.ActOnExplicitInstantiation(
           getCurScope(), TemplateInfo.ExternLoc, TemplateInfo.TemplateLoc,
           TagType, StartLoc, SS, TemplateId->Template,
           TemplateId->TemplateNameLoc, TemplateId->LAngleLoc, TemplateArgsPtr,
           TemplateId->RAngleLoc, attrs);
 
       // Friend template-ids are treated as references unless
       // they have template headers, in which case they're ill-formed
       // (FIXME: "template <class T> friend class A<T>::B<int>;").
       // We diagnose this error in ActOnClassTemplateSpecialization.
     } else if (TUK == Sema::TUK_Reference ||
if (TemplateId) {
 
     TagOrTempResult = Actions.ActOnTemplatedFriendTag(
         getCurScope(), DS.getFriendSpecLoc(), TagType, StartLoc, SS, Name,
         NameLoc, attrs,
         MultiTemplateParamsArg(TemplateParams ? &(*TemplateParams)[0] : nullptr,
                                TemplateParams ? TemplateParams->size() : 0));
   } else {
     if (TUK != Sema::TUK_Declaration && TUK != Sema::TUK_Definition)
       ProhibitAttributes(attrs);
 
     if (TUK == Sema::TUK_Definition &&
         TemplateInfo.Kind == ParsedTemplateInfo::ExplicitInstantiation) {
       // If the declarator-id is not a template-id, issue a diagnostic and
       // recover by ignoring the 'template' keyword.
       Diag(Tok, diag::err_template_defn_explicit_instantiation)
         << 1 << FixItHint::CreateRemoval(TemplateInfo.TemplateLoc);
       TemplateParams = nullptr;

clang/test/Parser/cxx0x-attributes.cpp

namespace N {
   struct S {};
 };
 template<typename> struct Template {};
 
 // FIXME: Improve this diagnostic
 struct [[]] N::S s; // expected-error {{an attribute list cannot appear here}}
 struct [[]] Template<int> t; // expected-error {{an attribute list cannot appear here}}
 struct [[]] ::template Template<int> u; // expected-error {{an attribute list cannot appear here}}
-template struct [[]] Template<char>; // expected-error {{an attribute list cannot appear here}}
+template struct [[]] Template<char>;
 template <> struct [[]] Template<void>;
 
 enum [[]] E1 {};
 enum [[]] E2; // expected-error {{forbids forward references}}
 enum [[]] E1;
 enum [[]] E3 : int;
 enum [[]] {
   k_123 [[]] = 123 // expected-warning {{attributes on an enumerator declaration are a C++17 extension}}