[cfe-commits] Possible fix for PR9685

[Douglas Gregor]

On Oct 17, 2011, at 10:45 PM, David Blaikie wrote:

> I'll be the first to admit I'm not entirely confident that this fix is the right one - but at the very least it's a starting point. It fixes the bug and doesn't regress any other clang tests.
> 
> If there are things this change breaks, I'll be happy to add tests for those cases & work on a fix that accommodates them, or if there's just a more appropriate way to express the required semantics, I'm all ears.
> 
> [basically, members of local structs were being instantiated in the context of their reference, not their definition, and failing because the Decl corresponding to the template of the member wasn't in the context of the instantiation scope (forEach in the attached example). By extending the lifetime of the LocalInstantiationScope and walking up past uncombined scopes in LocalInstantiationScope::findInstantiationOf it's now able to find the relevant Decl & instantiate correctly]

I see why this appears to be working, but it's not actually the right fix. Let's walk into what it's actually doing:

Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===================================================================
--- lib/Sema/SemaTemplateInstantiateDecl.cpp	(revision 142336)
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp	(working copy)
@@ -2540,7 +2540,6 @@
   // This class may have local implicit instantiations that need to be
   // instantiation within this scope.
   PerformPendingInstantiations(/*LocalOnly=*/true);
-  Scope.Exit();

This tells Clang not to exit the instantiation scope, i.e., the thing that maps from entities in the template to entities that map into the current instantiation. So, we'll end up leaving the mapping for this instantiation (doIt<int>) available while we instantiate other templates (including completely unrelated templates whose instantiation happens to have shown up in the list of instantiations).

Now let's look at:

Index: lib/Sema/SemaTemplateInstantiate.cpp
===================================================================
--- lib/Sema/SemaTemplateInstantiate.cpp	(revision 142336)
+++ lib/Sema/SemaTemplateInstantiate.cpp	(working copy)
@@ -2291,10 +2291,6 @@
       else
         CheckD = 0;
     } while (CheckD);
-    
-    // If we aren't combined with our outer scope, we're done. 
-    if (!Current->CombineWithOuterScope)
-      break;
   }

This change tells Clang to keep looking into outer instantiation scopes when it can't find what it's looking for in the current instantiation scope. That CombineWithOuterScope barrier is important, however, to make sure that we don't ever end up using bindings from other instantiations of the same template that are further up on the stack. This can become problematic with, e.g., label declarations, which are instantiated on demand.

I don't have a killer test case for you that breaks when you make this change; I suspect that that test case is out there, but that it is going to be very hard to write. However, let's see how things went wrong:

  - In Sema::MarkDeclarationReferenced, not how around line 9365 we check whether the context of an instantiable function is a local class and, if so, push it into the set of pending *local* instantiations rather than the set of pending (global) instantiations).
  - In Sema::InstantiateFunctionDefinition, we create a LocalInstantiationScope and again check for a local class. If we're instantiating something in a local class, then we merge our current instantiation scope with its parent scope, which turns on that CombineWithOuterScope flag (i.e., skipping the break in the second patch snippet above).
  - Late in Sema::InstantiateFunctionDefinition, we instantiate all of the pending local instantiations while we're still inside the instantiation scope of the original function. So, in combination with the previous bullet, this means that those instantiations can access our mappings from local names (e..g, Functor) to their instantiations (the instantiated Functor).

Now, things break in your test case because we're forEach<Functor> is not classified as a local instantiation, so it doesn't get instantiated while we're still inside doIt<int>'s scope. Hence, when forEach<Functor> then triggers an instantiation of doIt<int>::Functor::operator(), we're not longer

What your patch is doing is essentially treating *all* instantiations like they are local instantiations, so everything gets instantiated while doIt<int> is on the current instantiation stack, and all of the instantiation scopes are effectively merged.

I *think* the right answer is that, when there are local classes involved in the instantiation, we're going to want to keep around the contents of the LocalInstantiationScope in some side data structure. Then, when we're instantiating something that somehow refers to the local class (or a member of it), we'll have to re-create a LocalInstantiationScope as a parent LocalInstantiationScope, so that the right set of bindings are available.

	- Doug