[SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[cpplearner]

Summary

Given template<class> using Int = int;, the type void(Int<Ts>...) should be treated as a dependent type, even though Int<Ts> is not dependent.

This fixes https://bugs.llvm.org/show_bug.cgi?id=42654

[efriedma]

Is this the only place where a compound type can contain a PackExpansionType?

The code could probably use a brief comment explaining why we need to check this explicitly, even though it isn't listed in [temp.dep.type].

[cpplearner]

In D65050#1596514, @efriedma wrote:

Is this the only place where a compound type can contain a PackExpansionType?

Yes AFAIK. No other place can contain a list or a pack expansion. The closest thing is dynamic exception specification (which isn't part of the type), and it seems to be handled properly.

The code could probably use a brief comment explaining why we need to check this explicitly, even though it isn't listed in [temp.dep.type].

Done.

[cpplearner]

ping

[cpplearner]

ping

[aaron.ballman]

In D65050#1598057, @cpplearner wrote:

In D65050#1596514, @efriedma wrote:

Is this the only place where a compound type can contain a PackExpansionType?

Yes AFAIK. No other place can contain a list or a pack expansion. The closest thing is dynamic exception specification (which isn't part of the type), and it seems to be handled properly.

The noexcept specifier is part of the type these days, is that also handled properly?

[cpplearner]

In D65050#1606022, @aaron.ballman wrote:

The noexcept specifier is part of the type these days, is that also handled properly?

I believe that it's properly handled in this section of FunctionProtoType::FunctionProtoType:

// If this is a canonical type, and its exception specification is dependent,
// then it's a dependent type. This only happens in C++17 onwards.
if (isCanonicalUnqualified()) {
  if (getExceptionSpecType() == EST_Dynamic ||
      getExceptionSpecType() == EST_DependentNoexcept) {
    assert(hasDependentExceptionSpec() && "type should not be canonical");
    setDependent();
  }
} else if (getCanonicalTypeInternal()->isDependentType()) {
  // Ask our canonical type whether our exception specification was dependent.
  setDependent();
}

[aaron.ballman]

I think this is reasonable, but I'd like @rsmith to weigh in -- this looks like it could be a Core Issue.

[efriedma]

this looks like it could be a Core Issue

I think the issue is clang-specific. clang splits the standard notion of a dependent type into two separate bits, for the sake of diagnostics: isDependentType(), and isInstantiationDependentType(). isInstantiationDependentType() reflects the actual standard definition of a dependent type; isDependentType() is a type that can actually vary across instantiations.

[cpplearner]

ping

[cpplearner]

In D65050#1608777, @efriedma wrote:

this looks like it could be a Core Issue

I think the issue is clang-specific. clang splits the standard notion of a dependent type into two separate bits, for the sake of diagnostics: isDependentType(), and isInstantiationDependentType(). isInstantiationDependentType() reflects the actual standard definition of a dependent type; isDependentType() is a type that can actually vary across instantiations.

According to comment in include/clang/AST/Type.h (https://github.com/llvm/llvm-project/blob/llvmorg-8.0.1/clang/include/clang/AST/Type.h#L1426), Dependent reflects the standard definition, while InstantiationDependent reflects "whether this type somehow involves a template parameter, even if the resolution of the type does not depend on a template parameter" (e.g. decltype(sizeof(T))).

Therefore, I agree with Aaron that this could be a Core issue.

[cpplearner]

ping @rsmith

[cpplearner]

ping @rsmith

[cpplearner]

rebase & ping @rsmith

clang/lib/AST/Type.cpp

     ExtraBits.NumExceptionType = epi.ExceptionSpec.Exceptions.size();
   }
 
   // Fill in the trailing argument array.
   auto *argSlot = getTrailingObjects<QualType>();
   for (unsigned i = 0; i != getNumParams(); ++i) {
     addDependence(params[i]->getDependence() &
                   ~TypeDependence::VariablyModified);
+    // A pack expansion with a non-dependent pattern affects the number of
+    // parameters, thus we mark such function type as dependent, even though
+    // this isn't listed in N4861 [temp.dep.type].
+    if (params[i]->getAs<PackExpansionType>())
+      addDependence(TypeDependence::Dependent);
     argSlot[i] = params[i];
   }
 
   // Fill in the exception type array if present.
   if (getExceptionSpecType() == EST_Dynamic) {
     assert(hasExtraBitfields() && "missing trailing extra bitfields!");
     auto *exnSlot =
         reinterpret_cast<QualType *>(getTrailingObjects<ExceptionType>());

clang/test/SemaTemplate/alias-templates.cpp

   int z = Foo(); // expected-error {{use of alias template 'Foo' requires template arguments}}
 
   template<template<typename> class Bar> void f() { // expected-note 3{{here}}
     Bar x; // expected-error {{use of template template parameter 'Bar' requires template arguments}}
     Bar<> y; // expected-error {{too few template arguments for template template parameter 'Bar'}}
     int z = Bar(); // expected-error {{use of template template parameter 'Bar' requires template arguments}}
   }
 }
+
+namespace PR42654 {
+  template<typename> struct function { };
+
+  template<typename...T>
+  struct thing {
+    void f(function<void(T...)>) { }
+  };
+
+  template<typename ...Ts>
+  struct Environment
+  {
+    template<typename T>
+    using Integer = int;
+
+    using Function = function<void(Integer<Ts>...)>;
+    using MyTuple = thing<Integer<Ts>...>;
+
+    void run(Function func)
+    {
+      MyTuple t;
+      t.f(func);
+    }
+  };
+
+  void f()
+  {
+    Environment<int, double> env;
+    env.run({});
+  }
+}