Bug 402498. Do not rely on promiscuous binding resolution for adding

includes.

Change-Id: Idace39cc9ff9b781bf0de2d0017533548fb69e83

3 files changed, 170 insertions, 22 deletions

diff --git a/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/refactoring/includes/IncludeOrganizerTest.java b/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/refactoring/includes/IncludeOrganizerTest.java
index 98670290a3d..d8d58893b2f 100644
--- a/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/refactoring/includes/IncludeOrganizerTest.java
+++ b/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/refactoring/includes/IncludeOrganizerTest.java
@@ -507,6 +507,7 @@ public class IncludeOrganizerTest extends IncludesTestBase {
 	//class C {};
 
 	//source.cpp
+	//#include "h2.h"
 	//A a;
 	//B* b;
 	//C* c;
diff --git a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/BindingClassifier.java b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/BindingClassifier.java
index 7c1aaf71d6c..f702a38af82 100644
--- a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/BindingClassifier.java
+++ b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/BindingClassifier.java
@@ -698,18 +698,20 @@ public class BindingClassifier {
 				if (isPartOfExternalMacroDefinition(functionNameExpression))
 					return PROCESS_CONTINUE;
 
+				IASTInitializerClause[] arguments = functionCallExpression.getArguments();
 				IBinding binding = getBindingOfExpression(functionNameExpression);
 				if (binding != null) {
-					if (binding instanceof IFunction) {
-						defineForFunctionCall((IFunction) binding, functionCallExpression.getArguments());
-					} else if (binding instanceof ICPPMember) {
-						try {
-							IType memberType = ((ICPPMember) binding).getType();
-							defineIndirectTypes(memberType);
-						} catch (DOMException e) {
+					if (binding instanceof IProblemBinding) {
+						IBinding[] candidates = ((IProblemBinding) binding).getCandidateBindings();
+						if (candidates.length != 0) {
+							for (IBinding candidate : candidates) {
+								defineBindingForFunctionCall(candidate, arguments);
+							}
+						} else {
+							defineBinding(binding);
 						}
-					} else if (binding instanceof ITypedef) {
-						defineBinding(binding);
+					} else {
+						defineBindingForFunctionCall(binding, arguments);
 					}
 				}
 				if (functionCallExpression instanceof IASTImplicitNameOwner) {
@@ -717,7 +719,7 @@ public class BindingClassifier {
 					for (IASTName name : implicitNames) {
 						binding = name.resolveBinding();
 						if (binding instanceof IFunction) {
-							defineForFunctionCall((IFunction) binding, functionCallExpression.getArguments());
+							defineForFunctionCall((IFunction) binding, arguments);
 						}
 					}
 				}
@@ -786,6 +788,20 @@ public class BindingClassifier {
 			return PROCESS_CONTINUE;
 		}
 
+		protected void defineBindingForFunctionCall(IBinding binding, IASTInitializerClause[] arguments) {
+			if (binding instanceof IFunction) {
+				defineForFunctionCall((IFunction) binding, arguments);
+			} else if (binding instanceof ICPPMember) {
+				try {
+					IType memberType = ((ICPPMember) binding).getType();
+					defineIndirectTypes(memberType);
+				} catch (DOMException e) {
+				}
+			} else if (binding instanceof ITypedef) {
+				defineBinding(binding);
+			}
+		}
+
 		@Override
 		public int leave(IASTName name) {
 			if (isPartOfExternalMacroDefinition(name))
@@ -1085,16 +1101,7 @@ public class BindingClassifier {
 	}
 
 	private void addRequiredBindings(IBinding binding, Deque<IBinding> newBindings) {
-		if (binding instanceof ICPPMethod) {
-			newBindings.add(binding);  // Include the method in case we need its inline definition.
-			if (binding instanceof ICPPConstructor)
-				newBindings.add(binding.getOwner()); 
-		} else if (binding instanceof IType) {
-			// Remove type qualifiers.
-			IBinding b = getTypeBinding((IType) binding);
-			if (b != null)
-				newBindings.add(b);
-		} else if (binding instanceof IProblemBinding) {
+		if (binding instanceof IProblemBinding) {
 			IProblemBinding problemBinding = (IProblemBinding) binding;
 
 			IBinding[] candidateBindings = problemBinding.getCandidateBindings();
@@ -1113,6 +1120,15 @@ public class BindingClassifier {
 				} catch (CoreException e) {
 				}
 			}
+		} else if (binding instanceof ICPPMethod) {
+			newBindings.add(binding);  // Include the method in case we need its inline definition.
+			if (binding instanceof ICPPConstructor)
+				newBindings.add(binding.getOwner()); 
+		} else if (binding instanceof IType) {
+			// Remove type qualifiers.
+			IBinding b = getTypeBinding((IType) binding);
+			if (b != null)
+				newBindings.add(b);
 		} else {
 			newBindings.add(binding);
 		}
@@ -1175,7 +1191,9 @@ public class BindingClassifier {
 	 */
 	private boolean canForwardDeclare(IBinding binding) {
 		boolean canDeclare = false;
-		if (binding instanceof ICPPUsingDeclaration) {
+		if (binding instanceof IProblemBinding && ((IProblemBinding) binding).getCandidateBindings().length != 0) {
+			return true;  // Return true to consider delegates later on.
+		} if (binding instanceof ICPPUsingDeclaration) {
 			return true;  // Return true to consider delegates later on.
 		} if (binding instanceof ICompositeType) {
 			canDeclare = fPreferences.forwardDeclareCompositeTypes;
diff --git a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/IncludeCreator.java b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/IncludeCreator.java
index faebb74c581..da3dcfe142b 100644
--- a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/IncludeCreator.java
+++ b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/includes/IncludeCreator.java
@@ -40,6 +40,8 @@ import org.eclipse.text.edits.MultiTextEdit;
 import com.ibm.icu.text.Collator;
 
 import org.eclipse.cdt.core.CCorePlugin;
+import org.eclipse.cdt.core.dom.IName;
+import org.eclipse.cdt.core.dom.ast.DOMException;
 import org.eclipse.cdt.core.dom.ast.IASTDeclaration;
 import org.eclipse.cdt.core.dom.ast.IASTName;
 import org.eclipse.cdt.core.dom.ast.IASTNode;
@@ -51,15 +53,20 @@ import org.eclipse.cdt.core.dom.ast.ICompositeType;
 import org.eclipse.cdt.core.dom.ast.IEnumeration;
 import org.eclipse.cdt.core.dom.ast.IEnumerator;
 import org.eclipse.cdt.core.dom.ast.IFunction;
+import org.eclipse.cdt.core.dom.ast.IProblemBinding;
+import org.eclipse.cdt.core.dom.ast.IScope;
 import org.eclipse.cdt.core.dom.ast.IType;
 import org.eclipse.cdt.core.dom.ast.ITypedef;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTNameSpecifier;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTNamespaceDefinition;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTQualifiedName;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTUsingDeclaration;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTUsingDirective;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPBinding;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPBlockScope;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPConstructor;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPNamespace;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPNamespaceScope;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPSpecialization;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPVariable;
 import org.eclipse.cdt.core.index.IIndex;
@@ -76,6 +83,7 @@ import org.eclipse.cdt.ui.IFunctionSummary;
 import org.eclipse.cdt.ui.IRequiredInclude;
 import org.eclipse.cdt.ui.text.ICHelpInvocationContext;
 
+import org.eclipse.cdt.internal.core.dom.parser.IASTInternalScope;
 import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPVisitor;
 import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.SemanticUtil;
 import org.eclipse.cdt.internal.core.dom.rewrite.commenthandler.ASTCommenter;
@@ -182,7 +190,13 @@ public class IncludeCreator {
 	
 			final ArrayList<IncludeCandidate> candidates = new ArrayList<>(candidatesMap.values());
 			if (candidates.size() > 1) {
-				IncludeCandidate candidate = fAmbiguityResolver.selectElement(candidates);
+				// First, try to resolve the ambiguity by comparing the namespaces of the
+				// candidate bindings to the namespace in which the source name occurs.
+				IncludeCandidate candidate = selectCandidateByNamespace(name, candidates);
+				// If that doesn't disambiguate, fall back to the ambiguity resolver
+				// provided by the user of this class.
+				if (candidate == null)
+					candidate = fAmbiguityResolver.selectElement(candidates);
 				if (candidate == null)
 					return rootEdit;
 				candidates.clear();
@@ -226,6 +240,69 @@ public class IncludeCreator {
 		return createEdit(requiredIncludes, usingDeclarations, ast, selection);
 	}
 
+	private ICPPNamespaceScope getContainingNamespaceScope(IASTNode node) {
+		IScope scope = CPPVisitor.getContainingScope(node);
+		while (scope != null) {
+			if (scope instanceof ICPPNamespaceScope && !(scope instanceof ICPPBlockScope)) {
+				return (ICPPNamespaceScope) scope;
+			}
+			try {
+				scope = scope.getParent();
+			} catch (DOMException e) {
+				return null;
+			}
+		}
+		return null;
+	}
+	
+	private ICPPNamespace getContainingNamespace(IASTName name) {
+		ICPPNamespaceScope scope = getContainingNamespaceScope(name);
+		// TODO(nathanridge): Move this ICPPNamespaceScope -> ICPPNamespace
+		// mapping code to a utility class.
+		if (scope instanceof ICPPNamespace) {
+			return (ICPPNamespace) scope;
+		}
+		if (scope instanceof IASTInternalScope) {
+			IASTNode node = ((IASTInternalScope) scope).getPhysicalNode();
+			if (node instanceof ICPPASTNamespaceDefinition) {
+				IBinding namespace = ((ICPPASTNamespaceDefinition) node).getName().resolveBinding();
+				if (namespace instanceof ICPPNamespace) {
+					return (ICPPNamespace) namespace;
+				}
+			}
+		}
+		return null;
+	}
+	
+	private ICPPNamespace getContainingNamespace(IBinding binding) {
+		while (binding != null) {
+			if (binding instanceof ICPPNamespace) {
+				return (ICPPNamespace) binding;
+			}
+			binding = binding.getOwner();
+		}
+		return null;
+	}
+	
+	private IncludeCandidate selectCandidateByNamespace(IASTName sourceName, ArrayList<IncludeCandidate> candidates) {
+		// If one of the candidates is in the same namespace as the source name,
+		// and the others aren't, prefer that candidate.
+		ICPPNamespace sourceNamespace = getContainingNamespace(sourceName);
+		if (sourceNamespace == null) {
+			return null;
+		}
+		IncludeCandidate winner = null;
+		for (IncludeCandidate candidate : candidates) {
+			if (SemanticUtil.isSameNamespace(sourceNamespace, getContainingNamespace(candidate.binding))) {
+				if (winner != null) {
+					return null;  // ambiguous between 'winner' and 'candidate'
+				}
+				winner = candidate;
+			}
+		}
+		return winner;
+	}
+
 	private MultiTextEdit createEdit(List<IncludeInfo> includes,
 			List<UsingDeclaration> usingDeclarations, IASTTranslationUnit ast,
 			ITextSelection selection) {
@@ -497,11 +574,63 @@ public class IncludeCreator {
 		return name.getLastName().toString().equals(usingChain.get(usingChain.size() - 1 - qualifiers.length));
 	}
 
+	private ArrayList<String> getUsingChainForProblemBinding(IProblemBinding binding) {
+		ArrayList<String> chain = new ArrayList<>(4);
+		chain.add(binding.getName());
+		IASTNode node = binding.getASTNode();
+		if (node.getParent() instanceof ICPPASTQualifiedName) {
+			// If the ProblemBinding is for a name inside the qualified name,
+			// use the chain of preceding segments in the qualifier as the
+			// using chain.
+			ICPPASTQualifiedName qualifiedName = (ICPPASTQualifiedName) node.getParent();
+			ICPPASTNameSpecifier[] qualifier = qualifiedName.getQualifier();
+			int i = qualifier.length;
+			if (node != qualifiedName.getLastName()) {
+				while (--i >= 0) {
+					if (qualifier[i] == node) {
+						break;
+					}
+				}
+			}
+			while (--i >= 0) {
+				chain.add(qualifier[i].resolveBinding().getName());
+			}
+		} else {
+			// Otherwise, fall back to the chain of namespaces that physically
+			// contain the name.
+			ICPPNamespaceScope namespace = getContainingNamespaceScope(node);
+			while (namespace != null) {
+				IName namespaceName = namespace.getScopeName();
+				if (namespaceName != null) {
+					chain.add(new String(namespaceName.getSimpleID()));
+				}
+				try {
+					IScope parent = namespace.getParent();
+					if (parent instanceof ICPPNamespaceScope) {
+						namespace = (ICPPNamespaceScope) parent;
+					} else {
+						break;
+					}
+					
+				} catch (DOMException e) {
+					break;
+				}
+			}
+		}
+		return chain;
+	}
+	
 	/**
 	 * Returns components of the qualified name in reverse order.
 	 * For ns1::ns2::Name, e.g., it returns [Name, ns2, ns1].
 	 */
 	private ArrayList<String> getUsingChain(IBinding binding) {
+		// For ProblemBindings, getOwner() is very heuristic and doesn't
+		// produce the chain of owner bindings we want here, so we
+		// handle it specially.
+		if (binding instanceof IProblemBinding) {
+			return getUsingChainForProblemBinding((IProblemBinding) binding);
+		}
 		ArrayList<String> chain = new ArrayList<>(4);
 		for (; binding != null; binding = binding.getOwner()) {
 			String name = binding.getName();