Always build a new TypeSourceInfo for function templates with parameters

[thakis]

Summary

RecursiveASTVisitor::TraverseFunctionHelper() traverses a function's ParmVarDecls by going to the function's getTypeSourceInfo if it exists, and `
DEF_TRAVERSE_TYPELOC(FunctionProtoType` then goes to the function's ParmVarDecls.

For a function template that doesn't have parameters that explicitly depend on the template parameter, we used to be clever and not build a new TypeSourceInfo. That meant that when an instantiation of such a template is visited, its TypeSourceInfo would point to the ParmVarDecls of the template, not of the instantiation, which then confused clients of RecursiveASTVisitor.

So don't be clever for function templates that have parameters, even if none of the parameters depend on the type.

Fixes PR26257.

[thakis]

rnk, I added you because you hacked in this general area a while ago in r187528. Since Richard is out, do you think you can take a look?

[rnk]

The fact that an instantiated type might point to decls from the template pattern seems like expected behavior. The parts of the template that are the same are supposed to be shared.

Can we dig in to what is going wrong in parent map construction?

[thakis]

In D16478#338653, @rnk wrote:

The fact that an instantiated type might point to decls from the template pattern seems like expected behavior. The parts of the template that are the same are supposed to be shared.

Can we dig in to what is going wrong in parent map construction?

"RecursiveASTVisitor::TraverseFunctionHelper() traverses a function's ParmVarDecls by going to the function's getTypeSourceInfo if it exists, and `
DEF_TRAVERSE_TYPELOC(FunctionProtoType` then goes to the function's ParmVarDecls." was my attempt to do that. I guess I can expand this a bit with code snippets.

The parent map uses a RecursiveASTVisitor to build parents: It keeps a stack of nodes it has seen so far, and when something gets visited, it marks parent[something] = seenstack.top(). RecursiveASTVisitor does this for functions:

DEF_TRAVERSE_DECL(FunctionDecl, {
  // We skip decls_begin/decls_end, which are already covered by
  // TraverseFunctionHelper().
  return TraverseFunctionHelper(D);
})

TraverseFunctionHelper goes to the TypeSource if it exists to visit the parameter decls:

bool RecursiveASTVisitor<Derived>::TraverseFunctionHelper(FunctionDecl *D) {
  // ...
  if (TypeSourceInfo *TSI = D->getTypeSourceInfo()) {
    TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));

The TypeLoc then goes to the param's decl if available:

DEF_TRAVERSE_TYPELOC(FunctionProtoType, {
  TRY_TO(TraverseTypeLoc(TL.getReturnLoc()));

  const FunctionProtoType *T = TL.getTypePtr();

  for (unsigned I = 0, E = TL.getNumParams(); I != E; ++I) {
    if (TL.getParam(I)) {

So when the instantiated CXXMethodDecl for quantizedSize is visited, this goes and visits the ParmDecls from the template CXXMethodDecl, not from the instantiated one. Hence, the instantiated ParmDecl for count never gets a parent assigned (since it's never visited).

So at least RecursiveASTVisitor didn't expect the current behavior.

[yaron.keren]

The instantiated does get a new collection of ParmVarDecls while getting
the pattern Type which points to the pattern ParmVarDecls. So the
ParmVarDecls pointed in the instantiated are not the same as those pointed
by its Type.

Traversing by Type or by the Decl finds a different set of ParmVarDecls
which seems an inconsistency or at least a surprise.

[rsmith]

This is a bug -- we intend to reuse Stmt nodes across the template and its instantiations, but we should have separate Decl nodes in each instantiation (otherwise the Decl's parent will be wrong etc).

lib/Sema/SemaTemplateInstantiate.cpp

1519–1521

This doesn't look quite right: IIRC, there can be attributes here as well as parens (in particular, calling convention attributes can appear in this position).

1526

Just return true here?

[thakis]

just return true

[thakis]

lib/Sema/SemaTemplateInstantiate.cpp

1519–1521

Ack.

[thakis]

restore accidentally dropped test

[rsmith]

LGTM

lib/Sema/SemaTemplateInstantiate.cpp

1525–1531

Maybe replace the whole loop with

for (ParmVarDecl *D : FP.getParmArray())
  if (D)
    return true;

(or an STL algorithm for same)?

[thakis]

r259428, thanks! Went with the for-each loop but left the structure as-is -- I find the comments helpful and it's not clear where they should go else.

lib/Sema/SemaTemplateInstantiate.cpp

QualType Sema::SubstType(QualType T,
   if (!T->isInstantiationDependentType() && !T->isVariablyModifiedType())
     return T;
 
   TemplateInstantiator Instantiator(*this, TemplateArgs, Loc, Entity);
   return Instantiator.TransformType(T);
 }
 
 static bool NeedsInstantiationAsFunctionType(TypeSourceInfo *T) {
-  if (T->getType()->isInstantiationDependentType() || 
+  if (T->getType()->isInstantiationDependentType() ||
       T->getType()->isVariablyModifiedType())
     return true;
 
   TypeLoc TL = T->getTypeLoc().IgnoreParens();
   if (!TL.getAs<FunctionProtoTypeLoc>())
     return false;
 
   FunctionProtoTypeLoc FP = TL.castAs<FunctionProtoTypeLoc>();
   for (unsigned I = 0, E = FP.getNumParams(); I != E; ++I) {
-    ParmVarDecl *P = FP.getParam(I);
-
     // This must be synthesized from a typedef.
-    if (!P) continue;
-
-    // The parameter's type as written might be dependent even if the
-    // decayed type was not dependent.
-    if (TypeSourceInfo *TSInfo = P->getTypeSourceInfo())
-      if (TSInfo->getType()->isInstantiationDependentType())
-        return true;
+    if (!FP.getParam(I)) continue;
 
-    // TODO: currently we always rebuild expressions.  When we
-    // properly get lazier about this, we should use the same
-    // logic to avoid rebuilding prototypes here.
-    if (P->hasDefaultArg())
-      return true;
+    // If there are any parameters, a new TypeSourceInfo that refers to the
+    // instantiated parameters must be built.
   }
 
   return false;
 }
 
 /// A form of SubstType intended specifically for instantiating the
 /// type of a FunctionDecl.  Its purpose is solely to force the
 /// instantiation of default-argument expressions and to avoid
 /// instantiating an exception-specification.
 TypeSourceInfo *Sema::SubstFunctionDeclType(TypeSourceInfo *T,
                                 const MultiLevelTemplateArgumentList &Args,
                                 SourceLocation Loc,
                                 DeclarationName Entity,
                                 CXXRecordDecl *ThisContext,
                                 unsigned ThisTypeQuals) {
   assert(!ActiveTemplateInstantiations.empty() &&
          "Cannot perform an instantiation without some context on the "
          "instantiation stack");
-  
   if (!NeedsInstantiationAsFunctionType(T))
     return T;
 
   TemplateInstantiator Instantiator(*this, Args, Loc, Entity);
 
   TypeLocBuilder TLB;
 
   TypeLoc TL = T->getTypeLoc();