[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

1591–1593

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

1840–1842

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

1840–1842

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

1840–1842

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

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

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

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

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

1626

[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–4658

Test coverage for this change?

clang/lib/Parse/ParseDeclCXX.cpp

1936–1937

Test coverage for this change?

[tbaeder]

clang/lib/Parse/ParseDecl.cpp

4656–4658

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

1936–1937

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–4658

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

clang/include/clang/Parse/Parser.h

struct ParsedAttributesWithRange : ParsedAttributes {
 
     void clear() {
       ParsedAttributes::clear();
       Range = SourceRange();
     }
 
     SourceRange Range;
   };
   struct ParsedAttributesViewWithRange : ParsedAttributesView {
     ParsedAttributesViewWithRange() : ParsedAttributesView() {}
     void clearListOnly() {
       ParsedAttributesView::clearListOnly();
       Range = SourceRange();
     }
 
     SourceRange Range;
   };
 
   DeclGroupPtrTy ParseExternalDeclaration(ParsedAttributesWithRange &attrs,
                                           ParsingDeclSpec *DS = nullptr);
   bool isDeclarationAfterDeclarator();
   bool isStartOfFunctionDefinition(const ParsingDeclarator &Declarator);
   DeclGroupPtrTy ParseDeclarationOrFunctionDefinition(
                                                   ParsedAttributesWithRange &attrs,
private:
   }
   void DiagnoseProhibitedAttributes(const SourceRange &Range,
                                     SourceLocation FixItLoc);
 
   // Forbid C++11 and C2x attributes that appear on certain syntactic locations
   // which standard permits but we don't supported yet, for example, attributes
   // appertain to decl specifiers.
   void ProhibitCXX11Attributes(ParsedAttributesWithRange &Attrs,
-                               unsigned DiagID);
+                               unsigned DiagID,
+                               bool DiagnoseEmptyAttrs = false);
 
   /// Skip C++11 and C2x attributes and return the end location of the
   /// last one.
   /// \returns SourceLocation() if there are no attributes.
   SourceLocation SkipCXX11Attributes();
 
   /// Diagnose and skip C++11 and C2x attributes that appear in syntactic
   /// locations where attributes are not allowed.

clang/lib/Parse/ParseDecl.cpp

if (CorrectLocation.isValid()) {
     Diag(CorrectLocation, diag::err_attributes_misplaced)
         << FixItHint::CreateInsertionFromRange(CorrectLocation, AttrRange)
         << FixItHint::CreateRemoval(AttrRange);
   } else
     Diag(Range.getBegin(), diag::err_attributes_not_allowed) << Range;
 }
 
 void Parser::ProhibitCXX11Attributes(ParsedAttributesWithRange &Attrs,
-                                     unsigned DiagID) {
+                                     unsigned DiagID, bool DiagnoseEmptyAttrs) {
+
+  if (DiagnoseEmptyAttrs && Attrs.empty() && Attrs.Range.isValid()) {
+    // An attribute list has been parsed, but it was empty.
+    // This is the case for [[]].
+    const auto &LangOpts = getLangOpts();
+    auto &SM = PP.getSourceManager();
+    Token FirstLSquare;
+    Lexer::getRawToken(Attrs.Range.getBegin(), FirstLSquare, SM, LangOpts);
+
+    if (FirstLSquare.is(tok::l_square)) {
+      llvm::Optional<Token> SecondLSquare =
+          Lexer::findNextToken(FirstLSquare.getLocation(), SM, LangOpts);
+
+      if (SecondLSquare && SecondLSquare->is(tok::l_square)) {
+        // The attribute range starts with [[, but is empty. So this must
+        // be [[]], which we are supposed to diagnose because
+        // DiagnoseEmptyAttrs is true.
+        Diag(Attrs.Range.getBegin(), DiagID) << Attrs.Range;
+        return;
+      }
+    }
+  }
+
   for (const ParsedAttr &AL : Attrs) {
     if (!AL.isCXX11Attribute() && !AL.isC2xAttribute())
       continue;
     if (AL.getKind() == ParsedAttr::UnknownAttribute)
       Diag(AL.getLoc(), diag::warn_unknown_attribute_ignored)
           << AL << AL.getRange();
     else {
       Diag(AL.getLoc(), DiagID) << AL;
void Parser::ParseEnumSpecifier(SourceLocation StartLoc, DeclSpec &DS,
   //   'enum' nested-name-specifier[opt] identifier
   //
   // If we parsed any other bits, reject them now.
   //
   // MSVC and (for now at least) Objective-C permit a full enum-specifier
   // or opaque-enum-declaration anywhere.
   if (IsElaboratedTypeSpecifier && !getLangOpts().MicrosoftExt &&
       !getLangOpts().ObjC) {
-    ProhibitAttributes(attrs);
+    ProhibitCXX11Attributes(attrs, diag::err_attributes_not_allowed,
+                            /*DiagnoseEmptyAttrs=*/true);
     if (BaseType.isUsable())
       Diag(BaseRange.getBegin(), diag::ext_enum_base_in_type_specifier)
           << (AllowEnumSpecifier == AllowDefiningTypeSpec::Yes) << BaseRange;
     else if (ScopedEnumKWLoc.isValid())
       Diag(ScopedEnumKWLoc, diag::ext_elaborated_enum_class)
         << FixItHint::CreateRemoval(ScopedEnumKWLoc) << IsScopedUsingClassTag;
   }
 
   stripTypeAttributesOffDeclSpec(attrs, DS, TUK);

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);
+      ProhibitCXX11Attributes(attrs, diag::err_attributes_not_allowed,
+                              /*DiagnoseEmptyAttrs=*/true);
 
       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>;").
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);
+      ProhibitCXX11Attributes(attrs, diag::err_attributes_not_allowed,
+                              /* DiagnoseEmptyAttrs=*/true);
 
     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

 };
 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 __attribute__((pure)) Template<std::size_t>; // We still allow GNU-style attributes here
 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}}