Bug 516338 - Improve typedef preservation

Besides the UX advantages of typedef preservation (such as refactorings
preserving typedefs), it's important for correctness because the
arguments of template aliases can be subject to SFINAE even if they
don't participate in the target type.

Change-Id: I4e71372553dc418d1b8c3e27bd2c0387a41a3269

4 files changed, 88 insertions, 8 deletions

diff --git a/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ast2/AST2TemplateTests.java b/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ast2/AST2TemplateTests.java
index b1fdf5ce787..47f4a66ef6a 100644
--- a/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ast2/AST2TemplateTests.java
+++ b/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ast2/AST2TemplateTests.java
@@ -4112,7 +4112,7 @@ public class AST2TemplateTests extends AST2CPPTestBase {
 	public void testTypedefPreservation_380498c() throws Exception {
 		BindingAssertionHelper ba= getAssertionHelper();
 		ICPPVariable s = ba.assertNonProblem("s :", "s", ICPPVariable.class);
-		assertTrue(s.getType() instanceof ITypedef);
+		assertInstance(s.getType(), ITypedef.class);
 		assertEquals("string", ASTTypeUtil.getType(s.getType(), false));
 	}
 
diff --git a/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/internal/index/tests/IndexCPPTemplateResolutionTest.java b/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/internal/index/tests/IndexCPPTemplateResolutionTest.java
index d0026141c17..70289ca4118 100644
--- a/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/internal/index/tests/IndexCPPTemplateResolutionTest.java
+++ b/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/internal/index/tests/IndexCPPTemplateResolutionTest.java
@@ -98,7 +98,7 @@ public class IndexCPPTemplateResolutionTest extends IndexBindingResolutionTestBa
 	}
 
 	public IndexCPPTemplateResolutionTest() {
-		setStrategy(new ReferencedProject(true));
+		setStrategy(new SinglePDOMTestStrategy(true));
 	}
 
     // template<typename _TpAllocator>
@@ -3089,6 +3089,55 @@ public class IndexCPPTemplateResolutionTest extends IndexBindingResolutionTestBa
 	public void testRegression_402498() throws Exception {
 		checkBindings();
 	}
+
+	//	template <typename Iterator>
+	//	struct iterator_traits {
+	//	    typedef typename Iterator::value_type value_type;
+	//	};
+	//
+	//	template <typename I>
+	//	struct normal;
+	//
+	//	template <typename T>
+	//	struct normal<T*> {
+	//	    typedef T value_type;
+	//	};
 	
-	
+	//	template <class Iterator>
+	//	struct iterator_value {
+	//	    typedef typename iterator_traits<Iterator>::value_type type;
+	//	};
+	//
+	//	template <typename BidiIter, typename RegexTraits = typename iterator_value<BidiIter>::type>
+	//	struct regex_compiler;
+	//
+	//	typedef normal<char*> Iter;
+	//
+	//	typedef regex_compiler<Iter> sregex_compiler;
+	//
+	//	template<typename Char>
+	//	struct xpression_linker;
+	//
+	//	template<typename BidiIter>
+	//	struct matchable_ex {
+	//	    typedef BidiIter iterator_type;
+	//	    typedef typename iterator_value<iterator_type>::type char_type;
+	//
+	//	    void link(xpression_linker<char_type>);
+	//	};
+	//
+	//	template<typename BidiIter>
+	//	struct sub_match {
+	//	    typedef typename iterator_value<BidiIter>::type value_type;
+	//	    operator value_type() const;
+	//	};
+	//
+	//	void waldo(char);
+	//	        
+	//	void foo(sub_match<Iter> w) {
+	//	    waldo(w);
+	//	}
+	public void testRegression_516338() throws Exception {
+		checkBindings();
+	}
 }
diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/CPPTemplates.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/CPPTemplates.java
index da964ef2b0d..75a83d7d672 100644
--- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/CPPTemplates.java
+++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/CPPTemplates.java
@@ -1450,9 +1450,37 @@ public class CPPTemplates {
 			return new CPPTemplateNonTypeArgument(newEval, context.getPoint());
 		}
 
-		final IType orig= arg.getTypeValue();
-		final IType inst= instantiateType(orig, context);
-		if (orig == inst)
+		// Which to instantiate, getOriginalTypeValue() or getTypeValue()?
+		//
+		// Using getOriginalTypeValue() is better for typedef preservation,
+		// and in the case of alias template instances, also necessary for 
+		// correctness (since an alias template instance could have dependent 
+		// arguments that don't appear in the resulting type, and these 
+		// arguments could SFINAE out during instantiation; the popular 
+		// "void_t" technique relies on this).
+		//
+		// However, caching of template instances is based on the normalized
+		// representation of arguments, which uses getTypeValue(). This,
+		// together with certain deficiencies in ASTTypeUtil (namely, that
+		// template parameters owned by different templates end up with the
+		// same string representation), leads to tricky bugs if we try to
+		// use getOriginalTypeValue() here all the time (observe, e.g., how
+		// IndexCPPTemplateResolutionTest.testRegression_516338 fails if we
+		// unconditionally use getOriginalTypeValue() here).
+		//
+		// As a compromise, we use getOriginalTypeValue() in the case where
+		// it's important for correctness (alias template instances), and
+		// getTypeValue() otherwise.
+		IType type;
+		final IType origType = arg.getOriginalTypeValue();
+		if (origType instanceof ICPPAliasTemplateInstance) {
+			type = origType;
+		} else {
+			type = arg.getTypeValue();
+		}
+		
+		final IType inst= instantiateType(type, context);
+		if (type == inst)
 			return arg;
 		return new CPPTemplateTypeArgument(inst);
 	}
diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/SemanticUtil.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/SemanticUtil.java
index e689d1cc156..15f78fb1fdf 100644
--- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/SemanticUtil.java
+++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/SemanticUtil.java
@@ -438,7 +438,8 @@ public class SemanticUtil {
 		if (!(typedefType instanceof ITypedef))
 			return null;
 		IType nestedType = type;
-		while (!nestedType.isSameType(((ITypedef) typedefType).getType())) {
+		IType typedefTarget = ((ITypedef) typedefType).getType();
+		while (!nestedType.isSameType(typedefTarget)) {
 			if (nestedType instanceof IQualifierType) {
 				nestedType = ((IQualifierType) nestedType).getType();
 			} else if (nestedType instanceof IPointerType) {
@@ -448,7 +449,9 @@ public class SemanticUtil {
 			} else if (nestedType instanceof ICPPReferenceType) {
 				nestedType = ((ICPPReferenceType) nestedType).getType();
 			} else {
-				return null;
+				// The typedef's target could itself be a (pointer or reference
+				// to, or qualified version of) a typedef. Try substituting that too.
+				return substituteTypedef(type, typedefTarget);
 			}
 		}