[Concepts] Diagnose when 'concept' is specified on a specialization

[hubert.reinterpretcast]

test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.concept/p7.cpp

13 ↗

(On Diff #36270)

"extern template" is a form of explicit instantiation as well. I suggest testing that for both the function concept and variable concept cases.

[hubert.reinterpretcast]

test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.concept/p7.cpp

4 ↗

(On Diff #36270)

This is not a declaration (never mind definition) of a function template or variable template, but of a specialization. Presumably, this violates [dcl.spec.concept]p1. Perhaps a test for p7 should omit the concept specifier? The same logic may apply to the explicit specialization cases.

[rsmith]

lib/Sema/SemaDecl.cpp

5992–6005

Maybe combine these into a single check, and use << (IsPartialSpecialization ? 2 : 1) for the specialization kind.

7658

Missing period.

7994

I'm not convinced this is going to work: if the declaration is invalid because it's declared concept /and/ for another reason, we presumably want to use the same error recovery path as before.

test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.concept/p7.cpp

1 ↗

(On Diff #36270)

This file is in the wrong directory; the relevant section is [dcl.spec.concept], not [dcl.concept].

4 ↗

(On Diff #36270)

Right, there are two different ways things can go wrong here:

1. concept is specified on a non-template (such as in an explicit specialization or explicit instantiation declaration) or a variable template partial specialization
2. an explicit or partial specialization or an explicit instantiation declares (*without* the concept keyword) a specialization of a template that was declared as a concept.

This patch is only checking for the former case, whereas the rule in [dcl.spec.concept]p7 says that the latter case is ill-formed. So these tests are at least in the wrong file. To check for violations of p7, you'll need to look at whether the template's pattern is marked as being a concept.

I'm also not sure where we say that a partial specialization of a variable template cannot be declared concept; that seems to be a defect in the TS wording.

[nwilson]

Moving tests to the correct file.
Modifying the diagnostic id and message so it's more applicable to the checks in this Patch. Update the quoted sections of the standard as well.
Use ternary operator for Partial Specialization check.
Add missing period in comment.

[nwilson]

lib/Sema/SemaDecl.cpp

7994

Maybe there could be a problem further down if we think there aren't explicit template args and there really are. We could do a check similar to the isFriend check in the same block below like this:

else if (isConcept && isFunctionTemplateSpecialization) {

HasExplicitTemplateArgs = true;

}

But it seems like that's essentially the same thing.

[nwilson]

Addressing Richard's other comment regarding the check that the FunctionDecl is not a concept. Also, not marking the specialization as invalid if it's declared with the concept specifier since the downstream parts of Clang should look at the primary template.

[faisalv]

include/clang/AST/Decl.h

1577 ↗

(On Diff #37529)

My inclination would have been to add this bit to FunctionTemplateDecl, instead of to every FunctionDecl - since not every kind of FunctionDecl can be a concept ... My first instinct would have been to add an enum to TemplateKind, and then forward isConcept() to check if we are a template and if so, through it, if concept is specified?

But I suppose that adds more complexity, and you trade space for speed - For my own edification, could I ask you or Richard to comment on the cons of that approach - and why the current approach is preferred? (i.e. simplicity over complexity or space over time etc.)

[nwilson]

include/clang/AST/Decl.h

1577 ↗

(On Diff #37529)

Sorry for the slow reply. Yeah, I thought it was a little more straightforward having isConcept as a member function of FunctionDecl (and the bit). It also seemed that we'd be okay in terms of size since 17 bits are already being used here and adding one more wouldn't go over a byte boundary. Maybe Richard has other thoughts though?

[hubert.reinterpretcast]

include/clang/AST/Decl.h

1577 ↗

(On Diff #37529)

I am not sure what the original justification was for IsTrivial to be a bit here instead of in CXXMethodDecl, but it seems that similar considerations apply.

lib/Sema/SemaDecl.cpp

5999

Should we mention that the partial specialization case is pending additional wording (under Concepts TS Issue 15)?

[nwilson]

Updating to r251898

[nwilson]

lib/Sema/SemaDecl.cpp

5999

Hmm, I'd lean toward leaving it as is until the wording is sorted out. I don't feel too strongly either way though.

[nwilson]

• Remove marking a variable concept invalid when specialized since we'll only look at the primary template downstream. This removal let's us use the same recovery path as before when 'concept' is specified on a non-template.

[hubert.reinterpretcast]

Comment added inline. Otherwise, LGTM.

lib/Sema/SemaDecl.cpp

7727

I don't think the declaration should still be marked as a concept in this case.

[aaron.ballman]

Aside from one question, LGTM.

~Aaron

include/clang/Basic/DiagnosticSemaKinds.td

2072

Is this an extraneous %?

[nwilson]

include/clang/Basic/DiagnosticSemaKinds.td

2072

Good catch. I'll remove it when making the commit.

lib/Sema/SemaDecl.cpp

7727

Hmm, Richard - did you have any thoughts about this? IIRC, we might be okay here by only looking at the concept flag of the primary template.

[hubert.reinterpretcast]

lib/Sema/SemaDecl.cpp

7727

A consideration:
When processing the body associated with the specialization, should the requirements for a function concept body be checked?

[rsmith]

lib/Sema/SemaDecl.cpp

7727

I think the concept flag logically belongs on the template declaration rather than on the templated declaration; moving it there would make this question irrelevant =)

[nwilson]

lib/Sema/SemaDecl.cpp

7727

Sorry for the slow reply.

Okay. That makes sense. So, we'd be okay putting the IsConcept bit (and the associated member functions) in TemplateDecl then?

You'd be okay with the extra byte used?

[nwilson]

Updating to r254337

[nwilson]

• Move IsConcept bit and associated member functions from FunctionDecl to FunctionTemplateDecl.
• Set the IsConcept flag using getDescribedFunctionTemplate.

[nwilson]

Updating to r256247

[rsmith]

include/clang/AST/DeclTemplate.h

836

This might make more sense on TemplateDecl, since we also want this flag for VarTemplateDecls. In any case, please use some existing spare bit for this rather than making all FunctionTemplateDecls 8 bytes larger by putting it here.

986

Do we need a setter for this? (Can it change after the decl is constructed?)

lib/Sema/SemaDecl.cpp

7727

Yes, I think that's a reasonable way forward. You can change either TemplatedDecl or TemplateParams into a PointerIntPair to avoid making TemplateDecl larger.

lib/Sema/SemaTemplate.cpp

7672–7674

I don't think we need to quote the second sentence here.

7677

No space after ::

[faisalv]

include/clang/AST/DeclTemplate.h

836

@Nate - since you asked before, if I was to try and do this (now that we know Richard cares strongly about the extra space), I would probably risk complexity (and insult readability) by attempting to smuggle the extra bit into RedeclararableTemplateDecl::CommonBase::InstantiatedFromMember or perhaps within the low bits of the pointer to ReDeclarableTemplateDecl::CommonBase itself (i.e. Common)
Let's see if Richard has a preference - or suggests a completely different strategy for squirreling.

[nwilson]

include/clang/AST/DeclTemplate.h

836

@rsmith - Yeah, I originally thought to put it in TemplateDecl but reconsidered since ClassTemplateDecls would get it as well . However, I guess by making TemplateDecl::TemplatedDecl an IntPointerPair that concern is irrelevant.

@faisalv - Thanks for the info and suggestion! This helped with Richard's suggestion (below). FYI - It looks like that IntPointerPair, InstantiatedFromMember, is for explicit specializations, and the pointer to CommonBase (Common) is null for a function concept's primary template declaration.

986

The setter is used below when we see that concept is specified on the declaration. So, it wouldn't be initialized (or set) to true when the decl is constructed. Or, do you have a different opinion on where to set IsConcept to true such as how IsConstExpr is initialized in FunctionDecls being passed as a param in the constructor?

lib/Sema/SemaDecl.cpp

7727

Thanks for the suggestion. I'll go with TemplatedDecl and see what you guys think.

[nwilson]

• Store the IsConcept boolean flag in TemplateDecl by making TemplatedDecl an IntPointerPair, and move the associated member functions into TemplateDecl.
• Remove unnecessary quoted comment.
• Remove an extra space where the diagnostic is used.

[nwilson]

Ping. Now that the holidays are over-ish, as Aaron said in one of his Patches.

[hubert.reinterpretcast]

include/clang/AST/DeclTemplate.h

375

This code is not limited to function concepts.

377

The parameter is now unused and should be removed.

[nwilson]

include/clang/AST/DeclTemplate.h

377

Hmm, I think I should actually use that parameter since it's a bug as is (and would still be if I removed the parameter). I'll plan on doing that unless there is another thought about this.

[hubert.reinterpretcast]

include/clang/AST/DeclTemplate.h

377

My interpretation of http://reviews.llvm.org/D13357?id=43469#inline-129592 is that the function should take no parameters (there is no need for a way to set the property to false). Perhaps I misunderstood Richard's intent.

[nwilson]

include/clang/AST/DeclTemplate.h

377

I can't seem to follow the link properly, but I'm assuming it's supposed to be where Richard is asking about whether we need a setter. I thought he meant whether we needed the setter function at all. Perhaps I misunderstood though.

But it seems to be the convention to give a boolean (or some other type of param) to a setter. I'd be fine either way though because as you said, there isn't a need to set the property to false. As a point of reference RedeclarableTemplateDecl::setMemberSpecialization doesn't take any params either.

[nwilson]

Ping.

@rsmith - would you also mind clarifying the comment regarding setConcept(bool IC) at to whether it should exist at all or simply not have any params?

[nwilson]

• Fix a couple of comments to reflect the Patch.
• Clang-format the changes in this Patch.

[nwilson]

Marking some comments Done which were fixed in previous updates.

[rsmith]

include/clang/AST/DeclTemplate.h

377

I would prefer to not have a setter at all, but if it's awkward to pass this flag into the constructor, then a setter is fine (and I don't mind whether or not it takes a parameter). We should definitely not take a parameter and ignore it though.

[nwilson]

include/clang/AST/DeclTemplate.h

377

Ah, gotcha. Yeah, it seemed somewhat awkward to me to pass the flag all the way through. As a note it would be VarTemplateDecl -> RedeclarableTemplateDecl -> TemplateDecl. Also, RedeclarableTemplateDecl also currently currently takes 7 parameters.

If you feel strongly, I can move it to the constructor. Otherwise, I'll take the parameter out of the setter as both you and Hubert have pointed out the property will not need to set to false.

[nwilson]

• This update removes the parameter in TemplateDecl::setConcept since there isn't a need to mark a concept false.

[hubert.reinterpretcast]

Minor comments; otherwise, LGTM.

lib/Sema/SemaDecl.cpp

5993

We do not need to quote the second sentence here. The ellipsis in the first sentence should be expanded since the full list is necessary to conclude that the cases being diagnosed here are excluded.

7724

We do not need to quote the second sentence here. The ellipsis in the first sentence should be expanded since the full list is necessary to conclude that the cases being diagnosed here are excluded.

[nwilson]

• Address Hubert's comments about the quoted section of the TS.

[rsmith]

LGTM

include/clang/AST/DeclTemplate.h

static bool classofKind(Kind K) {
     return K >= firstTemplate && K <= lastTemplate;
   }
 
   SourceRange getSourceRange() const override LLVM_READONLY {
     return SourceRange(TemplateParams->getTemplateLoc(),
                        TemplatedDecl->getSourceRange().getEnd());
   }
 
 protected:
   NamedDecl *TemplatedDecl;
   TemplateParameterList* TemplateParams;
 
 public:
   /// \brief Initialize the underlying templated declaration and
   /// template parameters.
   void init(NamedDecl *templatedDecl, TemplateParameterList* templateParams) {
     assert(!TemplatedDecl && "TemplatedDecl already set!");
     assert(!TemplateParams && "TemplateParams already set!");
     TemplatedDecl = templatedDecl;
getTemplateArgs(FunctionTemplateSpecializationInfo *I) {
     return I->TemplateArguments->asArray();
   }
 };
 
 /// Declaration of a template function.
 class FunctionTemplateDecl : public RedeclarableTemplateDecl {
   static void DeallocateCommon(void *Ptr);
 
+  bool IsConcept : 1;
 protected:
   /// \brief Data that is common to all of the declarations of a given
   /// function template.
   struct Common : CommonBase {
     Common() : InjectedArgs(), LazySpecializations() { }
 
     /// \brief The function template specializations for this function
     /// template, including explicit specializations and instantiations.
public:
 
   /// \brief Create an empty function template node.
   static FunctionTemplateDecl *CreateDeserialized(ASTContext &C, unsigned ID);
 
   // Implement isa/cast/dyncast support
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == FunctionTemplate; }
 
+  /// Whether this is a (C++ Concepts TS) function concept.
+  bool isConcept() const { return IsConcept; }
+  void setConcept(bool IC) { IsConcept = IC; }
+
   friend class ASTDeclReader;
   friend class ASTDeclWriter;
 };
 
 //===----------------------------------------------------------------------===//
 // Kinds of Template Parameters
 //===----------------------------------------------------------------------===//
 

include/clang/Basic/DiagnosticSemaKinds.td

def err_var_concept_not_initialized : Error<
   "variable concept declaration must be initialized">;
 def err_function_concept_exception_spec : Error<
   "function concept cannot have exception specification">;
 def err_concept_decl_invalid_specifiers : Error<
   "%select{variable|function}0 concept cannot be declared "
   "'%select{thread_local|inline|friend|constexpr}1'">;
 def err_function_concept_with_params : Error<
   "function concept cannot have any parameters">;
+def err_concept_specified_specialization : Error<
+  "'concept' cannot be applied on an "
+  "%select{explicit instantiation|explicit specialization|partial specialization}0">;
 
 // C++11 char16_t/char32_t
 def warn_cxx98_compat_unicode_type : Warning<
   "'%0' type specifier is incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;
 
 // __make_integer_seq
 def err_integer_sequence_negative_length : Error<

lib/Sema/SemaDecl.cpp

if (D.getDeclSpec().isConceptSpecified()) {
       }
 
       if (D.getDeclSpec().isConstexprSpecified()) {
         Diag(D.getDeclSpec().getConstexprSpecLoc(),
              diag::err_concept_decl_invalid_specifiers)
             << 0 << 3;
         NewVD->setInvalidDecl(true);
       }
+
+      // C++ Concepts TS [dcl.spec.concept]p1: The concept specifier shall be
+      // applied only to the definition of a [...] variable template, declared
+      // in namespace scope. [...] A concept definition refers to [...] a
+      // variable concept and its initializer.
+      if (IsVariableTemplateSpecialization) {
+        Diag(D.getDeclSpec().getConceptSpecLoc(),
+             diag::err_concept_specified_specialization)
+          << (IsPartialSpecialization ? 2 : 1);
+      }
     }
   }
 
   // Set the lexical context. If the declarator has a C++ scope specifier, the
   // lexical context will be different from the semantic context.
   NewVD->setLexicalDeclContext(CurContext);
   if (NewTemplate)
     NewTemplate->setLexicalDeclContext(CurContext);
 
   if (IsLocalExternDecl)
     NewVD->setLocalExternDecl();
 
   bool EmitTLSUnsupportedError = false;
if (isConstexpr) {
       // C++11 [dcl.constexpr]p3: functions declared constexpr are required to
       // be either constructors or to return a literal type. Therefore,
       // destructors cannot be declared constexpr.
       if (isa<CXXDestructorDecl>(NewFD))
         Diag(D.getDeclSpec().getConstexprSpecLoc(), diag::err_constexpr_dtor);
     }
 
     if (isConcept) {
+      // This is a function concept.
+      if (FunctionTemplateDecl *FTD = NewFD->getDescribedFunctionTemplate())
+        FTD->setConcept(true);
+
       // C++ Concepts TS [dcl.spec.concept]p1: The concept specifier shall be
       // applied only to the definition of a function template [...]
       if (!D.isFunctionDefinition()) {
         Diag(D.getDeclSpec().getConceptSpecLoc(),
              diag::err_function_concept_not_defined);
         NewFD->setInvalidDecl();
       }
 
if (isConcept) {
       }
 
       if (isConstexpr) {
         Diag(D.getDeclSpec().getConstexprSpecLoc(),
              diag::err_concept_decl_invalid_specifiers)
             << 1 << 3;
         NewFD->setInvalidDecl(true);
       }
+
+      // C++ Concepts TS [dcl.spec.concept]p1: The concept specifier shall be
+      // applied only to the definition of a function template [...], declared
+      // in namespace scope. [...] A concept definition refers to either a
+      // function concept and its definition [...].
+      if (isFunctionTemplateSpecialization) {
+        Diag(D.getDeclSpec().getConceptSpecLoc(),
+             diag::err_concept_specified_specialization) << 1;
+      }
     }
 
     // If __module_private__ was specified, mark the function accordingly.
     if (D.getDeclSpec().isModulePrivateSpecified()) {
       if (isFunctionTemplateSpecialization) {
         SourceLocation ModulePrivateLoc
           = D.getDeclSpec().getModulePrivateSpecLoc();
         Diag(ModulePrivateLoc, diag::err_module_private_specialization)
if (!getLangOpts().CPlusPlus) {
     if (D.getName().getKind() == UnqualifiedId::IK_TemplateId) {
       TemplateIdAnnotation *TemplateId = D.getName().TemplateId;
       TemplateArgs.setLAngleLoc(TemplateId->LAngleLoc);
       TemplateArgs.setRAngleLoc(TemplateId->RAngleLoc);
       ASTTemplateArgsPtr TemplateArgsPtr(TemplateId->getTemplateArgs(),
                                          TemplateId->NumArgs);
       translateTemplateArguments(TemplateArgsPtr,
                                  TemplateArgs);
-    
       HasExplicitTemplateArgs = true;
-    
       if (NewFD->isInvalidDecl()) {
         HasExplicitTemplateArgs = false;
       } else if (FunctionTemplate) {
         // Function template with explicit template arguments.
         Diag(D.getIdentifierLoc(), diag::err_function_template_partial_spec)
           << SourceRange(TemplateId->LAngleLoc, TemplateId->RAngleLoc);
 
         HasExplicitTemplateArgs = false;
       } else {
         assert((isFunctionTemplateSpecialization ||

lib/Sema/SemaTemplate.cpp

Diag(D.getDeclSpec().getInlineSpecLoc(),
            diag::warn_explicit_instantiation_inline_0x)
       << FixItHint::CreateRemoval(D.getDeclSpec().getInlineSpecLoc());
   if (D.getDeclSpec().isConstexprSpecified() && R->isFunctionType())
     // FIXME: Add a fix-it to remove the 'constexpr' and add a 'const' if one is
     // not already specified.
     Diag(D.getDeclSpec().getConstexprSpecLoc(),
          diag::err_explicit_instantiation_constexpr);
 
+  // C++ Concepts TS [dcl.spec.concept]p1: The concept specifier shall be
+  // applied only to the definition of a function template or variable template,
+  // declared in namespace scope. A concept definition refers to either a
+  // function concept and its definition or a variable concept and its
+  // initializer.
+  if (D.getDeclSpec().isConceptSpecified()) {
+    Diag(D.getDeclSpec().getConceptSpecLoc(),
+         diag:: err_concept_specified_specialization) << 0;
+    return true;
+  }
+
   // C++0x [temp.explicit]p2:
   //   There are two forms of explicit instantiation: an explicit instantiation
   //   definition and an explicit instantiation declaration. An explicit
   //   instantiation declaration begins with the extern keyword. [...]
   TemplateSpecializationKind TSK
     = ExternLoc.isInvalid()? TSK_ExplicitInstantiationDefinition
                            : TSK_ExplicitInstantiationDeclaration;
 

test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.spec.concept/p1.cpp

 concept enum CE1 {}; // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
 template <typename T> concept class TCC1 {}; // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
 template <typename T> concept struct TCS1 {}; // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
 template <typename T> concept union TCU1 {}; // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
 typedef concept int CI; // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
 void fpc(concept int i) {} // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
 
 concept bool; // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
+
+template<typename T> concept bool VCEI { true };
+template concept bool VCEI<int>; // expected-error {{'concept' cannot be applied on an explicit instantiation}}
+extern template concept bool VCEI<int>; // expected-error {{'concept' cannot be applied on an explicit instantiation}}
+
+template<typename T> concept bool VCPS { true };
+template<typename T> concept bool VCPS<T*> { true }; // expected-error {{'concept' cannot be applied on an partial specialization}}
+
+template<typename T> concept bool VCES { true };
+template<> concept bool VCES<int> { true }; // expected-error {{'concept' cannot be applied on an explicit specialization}}
+
+template<typename T> concept bool FCEI() { return true; }
+template concept bool FCEI<int>(); // expected-error {{'concept' cannot be applied on an explicit instantiation}}
+extern template concept bool FCEI<int>(); // expected-error {{'concept' cannot be applied on an explicit instantiation}}
+
+template<typename T> concept bool FCES() { return true; }
+template<> concept bool FCES<bool>() { return true; } // expected-error {{'concept' cannot be applied on an explicit specialization}}