Modify DeclaratorChuck::getFunction to be passed an Exception Specification SourceRange

[nwilson]

Summary

• Store the exception specification range's begin and end SourceLocation in DeclaratorChuck::FunctionTypeInfo. These SourceLocations can be used in a FixItHint Range.
• Add diagnostic; function concept having an exception specification.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

1977 ↗

(On Diff #31414)

def err -> def err
can not -> cannot
specifiers -> specification

include/clang/Sema/DeclSpec.h

1330–1336 ↗

(On Diff #31414)

Please add a getExceptionSpecRange function returning the SourceRange...

lib/Sema/SemaDecl.cpp

7447–7450 ↗

(On Diff #31414)

This will assert if there isn't a FunctionTypeInfo for the declaration, which can theoretically happen if it's declared via an (ill-formed today) typedef. (It also might not provide a source range if the exception specification is implicit, for instance because the function template is a destructor or deallocation function, but passing an empty SourceRange to the FixItHint should just result it in being ignored.)

7449–7452 ↗

(On Diff #31414)

... and use it here.

7454–7455 ↗

(On Diff #31414)

No newline before else.

7456 ↗

(On Diff #31414)

Add a comment here indicating why we're doing this. Maybe a snippet of the relevant rule from the TS?

7456–7458 ↗

(On Diff #31414)

Use ASTContext::adjustExceptionSpec(NewFD, EST_BasicNoexcept) for this.

7456–7458 ↗

(On Diff #31414)

You don't seem to have test coverage for this part. Maybe test it with static_assert(noexcept(SomeConcept<T>()));?

[nwilson]

I'll make the fixes based on the comments, but had question about the FunctionTypeInfo in the meantime.

lib/Sema/SemaDecl.cpp

7447–7450 ↗

(On Diff #31414)

Hmm, I'm not sure if we'd run into that case because I don't believe we can have a concept specified as a typedef (check is yet to be added) and a check exists for being in a non-namespace scope. Do you think a check should still be added verifying that the FunctionTypeInfo exists?

[rsmith]

lib/Sema/SemaDecl.cpp

7447–7450 ↗

(On Diff #31414)

Yes, you should check that a FunctionTypeInfo exists (isFunctionDeclarator) rather than assuming that it does. The testcase would look something like this:

typedef int Fn() noexcept;
template<typename T> concept Fn C;

I think we'll discard the noexcept early at the moment, but that's not something you should be subtly relying on here (especially given that http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4533.html is looming on the horizon). Something like:

SourceRange Range;
if (D.isFunctionDeclarator())
  Range = D.getFunctionTypeInfo().getExceptionSpecRange();

... should handle this safely.

[nwilson]

Updated Patch based on comments; fix diagnostic spacing and phrasing, add getExceptionSpecRange, check FunctionTypeInfo exists, use PartialDiagnostic, add static_assert test for function concept being treated as noexcept(true)

[rsmith]

include/clang/Sema/DeclSpec.h

1262 ↗

(On Diff #31488)

Drop bogus "keyword introducing the" here, and drop either the same words from the previous comment or the word "beginning".

lib/Sema/SemaDecl.cpp

7450 ↗

(On Diff #31488)

The D.isFunctionDeclarator() check should be nested within this, as should the Range variable:

if (FPT->hasExceptionSpec()) {
  SourceRange Range;
  if (D.isFunctionDeclarator())
    Range = D.getFunctionTypeInfo().getExceptionSpecRange();
  PartialDiagnostic PD = ...

7454–7456 ↗

(On Diff #31488)

You don't need this if; a FixItHint with an invalid range has no effect.

test/SemaCXX/cxx-concept-declaration.cpp

9–10 ↗

(On Diff #31488)

This isn't quite enough; noexcept will also return true if the expression is a constant expression. Maybe replace the return true with return (throw 0, true).

[nwilson]

lib/Sema/SemaDecl.cpp

7454–7456 ↗

(On Diff #31488)

Ahhh, okay. I misunderstood the original comment about an empty SourceRange apparently.

I'll go back to using Diagnostic rather than PartialDiagnostic as well.

[nwilson]

• Update Patch based on comments; rephrase exception spec comment, adjust nesting of check for FunctionTypeInfo, remove SourceRange check, fix test

[nwilson]

include/clang/Sema/DeclSpec.h

1262 ↗

(On Diff #31502)

Please let me know if there is still an issue with the wording here.

[nwilson]

Ping

[rsmith]

include/clang/Sema/DeclSpec.h

1262 ↗

(On Diff #31502)

It'd be better to use "specification" rather than "specifier" here to match the standard.

lib/Sema/SemaDecl.cpp

7453–7455 ↗

(On Diff #31502)

Move this code out of the if; we should issue an error whether or not the function was spelled with a function declarator.

test/SemaCXX/cxx-concept-declaration.cpp

9 ↗

(On Diff #31502)

I guess this should be either throw 0, true or throw, true.

[nwilson]

Patch addressing comments; fix comment/documentation wording, scoping of diagnostic and setting invalid declaration, and fix test.

I also modified the location of the diagnostic indicating an exception specification is not allowed to point to the function declarator in order to handle a range having an invalid begin location.

The 80 column limit was overrun in SemaType.cpp, so clang format has been run on the problematic lines in the file.

[nwilson]

Ping

[rsmith]

LGTM, sorry for the delay.

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def err_concept_wrong_decl_kind : Error<
   "'concept' can only appear on the definition of a function template or variable template">;
 def err_concept_decls_may_only_appear_in_namespace_scope : Error<
   "concept declarations may only appear in namespace scope">;
 def err_function_concept_not_defined : Error<
   "function concept declaration must be a definition">;
 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">;
 
 // C++11 char16_t/char32_t
 def warn_cxx98_compat_unicode_type : Warning<
   "'%0' type specifier is incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;
  
 // Objective-C++
 def err_objc_decls_may_only_appear_in_global_scope : Error<

cfe/trunk/include/clang/Sema/DeclSpec.h

struct FunctionTypeInfo : TypeInfoCommon {
     ///
     /// If this is an invalid location, there is no restrict-qualifier.
     unsigned RestrictQualifierLoc;
 
     /// \brief The location of the 'mutable' qualifer in a lambda-declarator, if
     /// any.
     unsigned MutableLoc;
 
-    /// \brief The location of the keyword introducing the spec, if any.
-    unsigned ExceptionSpecLoc;
+    /// \brief The beginning location of the exception specification, if any.
+    unsigned ExceptionSpecLocBeg;
+
+    /// \brief The end location of the exception specification, if any.
+    unsigned ExceptionSpecLocEnd;
 
     /// Params - This is a pointer to a new[]'d array of ParamInfo objects that
     /// describe the parameters specified by this function declarator.  null if
     /// there are no parameters specified.
     ParamInfo *Params;
 
     union {
       /// \brief Pointer to a new[]'d array of TypeAndRange objects that
struct FunctionTypeInfo : TypeInfoCommon {
     SourceLocation getEllipsisLoc() const {
       return SourceLocation::getFromRawEncoding(EllipsisLoc);
     }
 
     SourceLocation getRParenLoc() const {
       return SourceLocation::getFromRawEncoding(RParenLoc);
     }
 
-    SourceLocation getExceptionSpecLoc() const {
-      return SourceLocation::getFromRawEncoding(ExceptionSpecLoc);
+    SourceLocation getExceptionSpecLocBeg() const {
+      return SourceLocation::getFromRawEncoding(ExceptionSpecLocBeg);
+    }
+
+    SourceLocation getExceptionSpecLocEnd() const {
+      return SourceLocation::getFromRawEncoding(ExceptionSpecLocEnd);
+    }
+
+    SourceRange getExceptionSpecRange() const {
+      return SourceRange(getExceptionSpecLocBeg(), getExceptionSpecLocEnd());
     }
 
     /// \brief Retrieve the location of the ref-qualifier, if any.
     SourceLocation getRefQualifierLoc() const {
       return SourceLocation::getFromRawEncoding(RefQualifierLoc);
     }
 
     /// \brief Retrieve the location of the 'const' qualifier, if any.
static DeclaratorChunk getFunction(bool HasProto,
                                      unsigned TypeQuals,
                                      bool RefQualifierIsLvalueRef,
                                      SourceLocation RefQualifierLoc,
                                      SourceLocation ConstQualifierLoc,
                                      SourceLocation VolatileQualifierLoc,
                                      SourceLocation RestrictQualifierLoc,
                                      SourceLocation MutableLoc,
                                      ExceptionSpecificationType ESpecType,
-                                     SourceLocation ESpecLoc,
+                                     SourceRange ESpecRange,
                                      ParsedType *Exceptions,
                                      SourceRange *ExceptionRanges,
                                      unsigned NumExceptions,
                                      Expr *NoexceptExpr,
                                      CachedTokens *ExceptionSpecTokens,
                                      SourceLocation LocalRangeBegin,
                                      SourceLocation LocalRangeEnd,
                                      Declarator &TheDeclarator,

cfe/trunk/lib/Parse/ParseDecl.cpp

D.AddTypeInfo(DeclaratorChunk::getFunction(HasProto,
                                              ParamInfo.data(), ParamInfo.size(),
                                              EllipsisLoc, RParenLoc,
                                              DS.getTypeQualifiers(),
                                              RefQualifierIsLValueRef,
                                              RefQualifierLoc, ConstQualifierLoc,
                                              VolatileQualifierLoc,
                                              RestrictQualifierLoc,
                                              /*MutableLoc=*/SourceLocation(),
-                                             ESpecType, ESpecRange.getBegin(),
+                                             ESpecType, ESpecRange,
                                              DynamicExceptions.data(),
                                              DynamicExceptionRanges.data(),
                                              DynamicExceptions.size(),
                                              NoexceptExpr.isUsable() ?
                                                NoexceptExpr.get() : nullptr,
                                              ExceptionSpecTokens,
                                              StartLoc, LocalEndLoc, D,
                                              TrailingReturnType),

cfe/trunk/lib/Parse/ParseExpr.cpp

ParamInfo.AddTypeInfo(DeclaratorChunk::getFunction(/*HasProto=*/true,
                                              /*TypeQuals=*/0,
                                              /*RefQualifierIsLvalueRef=*/true,
                                              /*RefQualifierLoc=*/NoLoc,
                                              /*ConstQualifierLoc=*/NoLoc,
                                              /*VolatileQualifierLoc=*/NoLoc,
                                              /*RestrictQualifierLoc=*/NoLoc,
                                              /*MutableLoc=*/NoLoc,
                                              EST_None,
-                                             /*ESpecLoc=*/NoLoc,
+                                             /*ESpecRange=*/SourceRange(),
                                              /*Exceptions=*/nullptr,
                                              /*ExceptionRanges=*/nullptr,
                                              /*NumExceptions=*/0,
                                              /*NoexceptExpr=*/nullptr,
                                              /*ExceptionSpecTokens=*/nullptr,
                                              CaretLoc, CaretLoc,
                                              ParamInfo),
                           attrs, CaretLoc);

cfe/trunk/lib/Parse/ParseExprCXX.cpp

D.AddTypeInfo(DeclaratorChunk::getFunction(/*hasProto=*/true,
                                            EllipsisLoc, RParenLoc,
                                            DS.getTypeQualifiers(),
                                            /*RefQualifierIsLValueRef=*/true,
                                            /*RefQualifierLoc=*/NoLoc,
                                            /*ConstQualifierLoc=*/NoLoc,
                                            /*VolatileQualifierLoc=*/NoLoc,
                                            /*RestrictQualifierLoc=*/NoLoc,
                                            MutableLoc,
-                                           ESpecType, ESpecRange.getBegin(),
+                                           ESpecType, ESpecRange,
                                            DynamicExceptions.data(),
                                            DynamicExceptionRanges.data(),
                                            DynamicExceptions.size(),
                                            NoexceptExpr.isUsable() ?
                                              NoexceptExpr.get() : nullptr,
                                            /*ExceptionSpecTokens*/nullptr,
                                            LParenLoc, FunLocalRangeEnd, D,
                                            TrailingReturnType),
D.AddTypeInfo(DeclaratorChunk::getFunction(/*hasProto=*/true,
                                                /*TypeQuals=*/0,
                                                /*RefQualifierIsLValueRef=*/true,
                                                /*RefQualifierLoc=*/NoLoc,
                                                /*ConstQualifierLoc=*/NoLoc,
                                                /*VolatileQualifierLoc=*/NoLoc,
                                                /*RestrictQualifierLoc=*/NoLoc,
                                                MutableLoc,
                                                EST_None,
-                                               /*ESpecLoc=*/NoLoc,
+                                               /*ESpecRange=*/SourceRange(),
                                                /*Exceptions=*/nullptr,
                                                /*ExceptionRanges=*/nullptr,
                                                /*NumExceptions=*/0,
                                                /*NoexceptExpr=*/nullptr,
                                                /*ExceptionSpecTokens=*/nullptr,
                                                DeclLoc, DeclEndLoc, D,
                                                TrailingReturnType),
                   Attr, DeclEndLoc);

cfe/trunk/lib/Sema/DeclSpec.cpp

DeclaratorChunk DeclaratorChunk::getFunction(bool hasProto,
                                              SourceLocation ConstQualifierLoc,
                                              SourceLocation
                                                  VolatileQualifierLoc,
                                              SourceLocation
                                                  RestrictQualifierLoc,
                                              SourceLocation MutableLoc,
                                              ExceptionSpecificationType
                                                  ESpecType,
-                                             SourceLocation ESpecLoc,
+                                             SourceRange ESpecRange,
                                              ParsedType *Exceptions,
                                              SourceRange *ExceptionRanges,
                                              unsigned NumExceptions,
                                              Expr *NoexceptExpr,
                                              CachedTokens *ExceptionSpecTokens,
                                              SourceLocation LocalRangeBegin,
                                              SourceLocation LocalRangeEnd,
                                              Declarator &TheDeclarator,
DeclaratorChunk DeclaratorChunk::getFunction(bool hasProto,
   I.Fun.Params                  = nullptr;
   I.Fun.RefQualifierIsLValueRef = RefQualifierIsLvalueRef;
   I.Fun.RefQualifierLoc         = RefQualifierLoc.getRawEncoding();
   I.Fun.ConstQualifierLoc       = ConstQualifierLoc.getRawEncoding();
   I.Fun.VolatileQualifierLoc    = VolatileQualifierLoc.getRawEncoding();
   I.Fun.RestrictQualifierLoc    = RestrictQualifierLoc.getRawEncoding();
   I.Fun.MutableLoc              = MutableLoc.getRawEncoding();
   I.Fun.ExceptionSpecType       = ESpecType;
-  I.Fun.ExceptionSpecLoc        = ESpecLoc.getRawEncoding();
+  I.Fun.ExceptionSpecLocBeg     = ESpecRange.getBegin().getRawEncoding();
+  I.Fun.ExceptionSpecLocEnd     = ESpecRange.getEnd().getRawEncoding();
   I.Fun.NumExceptions           = 0;
   I.Fun.Exceptions              = nullptr;
   I.Fun.NoexceptExpr            = nullptr;
   I.Fun.HasTrailingReturnType   = TrailingReturnType.isUsable() ||
                                   TrailingReturnType.isInvalid();
   I.Fun.TrailingReturnType      = TrailingReturnType.get();
 
   assert(I.Fun.TypeQuals == TypeQuals && "bitfield overflow");

cfe/trunk/lib/Sema/SemaDecl.cpp

if (isConcept) {
       // 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();
       }
 
+      // C++ Concepts TS [dcl.spec.concept]p1: [...] A function concept shall
+      // have no exception-specification and is treated as if it were specified
+      // with noexcept(true) (15.4). [...]
+      if (const FunctionProtoType *FPT = R->getAs<FunctionProtoType>()) {
+        if (FPT->hasExceptionSpec()) {
+          SourceRange Range;
+          if (D.isFunctionDeclarator())
+            Range = D.getFunctionTypeInfo().getExceptionSpecRange();
+          Diag(NewFD->getLocation(), diag::err_function_concept_exception_spec)
+              << FixItHint::CreateRemoval(Range);
+          NewFD->setInvalidDecl();
+        } else {
+          Context.adjustExceptionSpec(NewFD, EST_BasicNoexcept);
+        }
+      }
+
       // C++ Concepts TS [dcl.spec.concept]p2: Every concept definition is
       // implicity defined to be a constexpr declaration (implicitly inline)
       NewFD->setImplicitlyInline();
     }
 
     // If __module_private__ was specified, mark the function accordingly.
     if (D.getDeclSpec().isModulePrivateSpecified()) {
       if (isFunctionTemplateSpecialization) {
D.AddTypeInfo(DeclaratorChunk::getFunction(/*HasProto=*/false,
                                              /*TypeQuals=*/0,
                                              /*RefQualifierIsLvalueRef=*/true,
                                              /*RefQualifierLoc=*/NoLoc,
                                              /*ConstQualifierLoc=*/NoLoc,
                                              /*VolatileQualifierLoc=*/NoLoc,
                                              /*RestrictQualifierLoc=*/NoLoc,
                                              /*MutableLoc=*/NoLoc,
                                              EST_None,
-                                             /*ESpecLoc=*/NoLoc,
+                                             /*ESpecRange=*/SourceRange(),
                                              /*Exceptions=*/nullptr,
                                              /*ExceptionRanges=*/nullptr,
                                              /*NumExceptions=*/0,
                                              /*NoexceptExpr=*/nullptr,
                                              /*ExceptionSpecTokens=*/nullptr,
                                              Loc, Loc, D),
                 DS.getAttributes(),
                 SourceLocation());

cfe/trunk/lib/Sema/SemaType.cpp

declarator.AddInnermostTypeInfo(DeclaratorChunk::getFunction(
       /*RParenLoc=*/NoLoc,
       /*TypeQuals=*/0,
       /*RefQualifierIsLvalueRef=*/true,
       /*RefQualifierLoc=*/NoLoc,
       /*ConstQualifierLoc=*/NoLoc,
       /*VolatileQualifierLoc=*/NoLoc,
       /*RestrictQualifierLoc=*/NoLoc,
       /*MutableLoc=*/NoLoc, EST_None,
-      /*ESpecLoc=*/NoLoc,
+      /*ESpecRange=*/SourceRange(),
       /*Exceptions=*/nullptr,
       /*ExceptionRanges=*/nullptr,
       /*NumExceptions=*/0,
       /*NoexceptExpr=*/nullptr,
       /*ExceptionSpecTokens=*/nullptr,
       loc, loc, declarator));
 
   // For consistency, make sure the state still has us as processing
case DeclaratorChunk::Function: {
         TagDecl *Tag = cast<TagDecl>(D.getDeclSpec().getRepAsDecl());
         S.Diag(Tag->getLocation(), diag::err_type_defined_in_result_type)
           << Context.getTypeDeclType(Tag);
       }
 
       // Exception specs are not allowed in typedefs. Complain, but add it
       // anyway.
       if (IsTypedefName && FTI.getExceptionSpecType())
-        S.Diag(FTI.getExceptionSpecLoc(), diag::err_exception_spec_in_typedef)
+        S.Diag(FTI.getExceptionSpecLocBeg(),
+               diag::err_exception_spec_in_typedef)
             << (D.getContext() == Declarator::AliasDeclContext ||
                 D.getContext() == Declarator::AliasTemplateContext);
 
       // If we see "T var();" or "T var(T());" at block scope, it is probably
       // an attempt to initialize a variable, not a function declaration.
       if (FTI.isAmbiguous)
         warnAboutAmbiguousFunction(S, D, DeclType, T);
 
       FunctionType::ExtInfo EI(getCCForDeclaratorChunk(S, D, FTI, chunkIndex));
 

cfe/trunk/test/SemaCXX/cxx-concept-declaration.cpp

-// RUN:  %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s
+// RUN:  %clang_cc1 -std=c++14 -fconcepts-ts -fcxx-exceptions -x c++ -verify %s
 
 namespace A {
   template<typename T> concept bool C1() { return true; }
 
   template<typename T> concept bool C2 = true;
 }
 
+template<typename T> concept bool C3() { return (throw 0, true); }
+static_assert(noexcept(C3<int>()), "function concept should be treated as if noexcept(true) specified");
+
 template<typename T> concept bool D1(); // expected-error {{function concept declaration must be a definition}}
 
 struct B {
   template<typename T> concept bool D2() { return true; } // expected-error {{concept declarations may only appear in namespace scope}}
 };
 
 struct C {
   template<typename T> static concept bool D3 = true; // expected-error {{concept declarations may only appear in namespace scope}}
 };
 
 concept bool D4() { return true; } // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
 
 concept bool D5 = true; // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
 
 template<typename T>
 concept bool D6; // expected-error {{variable concept declaration must be initialized}}
 
+template<typename T>
+concept bool D7() throw(int) { return true; } // expected-error {{function concept cannot have exception specification}}
+
 // Tag
 concept class CC1 {}; // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
 concept struct CS1 {}; // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
 concept union CU1 {}; // expected-error {{'concept' can only appear on the definition of a function template or variable template}}
 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}}
 
 concept bool; // expected-error {{'concept' can only appear on the definition of a function template or variable template}}