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

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

include/clang/Sema/DeclSpec.h

1330–1336

Please add a getExceptionSpecRange function returning the SourceRange...

lib/Sema/SemaDecl.cpp

7447–7450

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

... and use it here.

7454–7455

No newline before else.

7456

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

7456–7458

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

7456–7458

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

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

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

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

lib/Sema/SemaDecl.cpp

7474

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 = ...

7478–7480

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

test/SemaCXX/cxx-concept-declaration.cpp

9–10

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

7478–7480

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

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

[nwilson]

Ping

[rsmith]

include/clang/Sema/DeclSpec.h

1262

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

lib/Sema/SemaDecl.cpp

7477–7479

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

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.

include/clang/Basic/DiagnosticSemaKinds.td

def note_private_extern : Note<
   "use __attribute__((visibility(\"hidden\"))) attribute instead">;
 
 // C++ Concepts TS
 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<

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,

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),

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);

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);

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");

lib/Sema/SemaDecl.cpp

if (isExplicit && !NewFD->isInvalidDecl()) {
                  !isa<CXXConversionDecl>(NewFD)) {
         // 'explicit' was specified on a function that wasn't a constructor
         // or conversion function.
         Diag(D.getDeclSpec().getExplicitSpecLoc(),
              diag::err_explicit_non_ctor_or_conv_function)
           << FixItHint::CreateRemoval(D.getDeclSpec().getExplicitSpecLoc());
       }      
     }
 
     if (isConstexpr) {
       // C++11 [dcl.constexpr]p2: constexpr functions and constexpr constructors
       // are implicitly inline.
       NewFD->setImplicitlyInline();
 
       // 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) {
       // 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());

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));
 

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}}