Bug 525739 - Have Open Declaration find potentially-matching definitions for a declaration

The implementation strategy is as follows:

  - Implement an option to find potential matches during an AST
    definition search.

  - Store names that resolve to ProblemBindings in the index,
    as references for the candidate bindings, annotated as being
    potential matches.

  - Add an option to Index.findNames() to include potential matches.

  - Use the added options for the index and AST searches in
    OpenDeclarationsJob, prioritizing them accordingly (e.g. exact index
    matches take precedence over potential AST matches, but if there
    are no exact matches, potential matches from the AST and the index
    are combined).

Change-Id: I19f5c58820eb3ec79a31652d69fd5b86acaba115

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/IASTTranslationUnit.java

@@ -117,15 +117,25 @@ public interface IASTTranslationUnit extends IASTDeclarationListOwner, IFileNomi
      */
     public IName[] getDefinitions(IBinding binding);
 
+	/**
+	 * Equivalent to getDefinitionsInAst(binding, false).
+	 */
+	public IASTName[] getDefinitionsInAST(IBinding binding);
+	
 	/**
      * Returns the array of definitions in this translation unit for the given binding.
      * The array contains the IASTName nodes that define the binding.
 	 * These are part of the AST, no definitions are pulled in from the index.
 	 * 
+	 * If 'permissive' is true, definitions that are not exact matches (for example,
+	 * a method definition with a non-matching signature) are also returned.
+	 * 
 	 * @param binding
+	 * @param permissive
 	 * @return Array of IASTName nodes for the binding's declaration
+	 * @since 6.4
 	 */
-	public IASTName[] getDefinitionsInAST(IBinding binding);
+	public IASTName[] getDefinitionsInAST(IBinding binding, boolean permissive);
 
 	/**
 	 * Returns the list of references in this translation unit to the given

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/index/IIndex.java

@@ -64,6 +64,13 @@ public interface IIndex {
 	 * the c-linkage.
 	 */
 	final int SEARCH_ACROSS_LANGUAGE_BOUNDARIES= 0x8;
+	/**
+	 * Constant to include potential matches in the results of a search.
+	 * An example of a potential match might be a function definition that does match
+	 * a declaration exactly in signature.
+	 * @since 6.4
+	 */
+	final int FIND_POTENTIAL_MATCHES = 0x10;
 	/**
 	 * Constant to search for all declarations including definitions.
 	 */
@@ -387,7 +394,8 @@ public interface IIndex {
 	 *
 	 * @param binding a binding for which names are searched for
 	 * @param flags a combination of {@link #FIND_DECLARATIONS}, {@link #FIND_DEFINITIONS},
-	 * {@link #FIND_REFERENCES} and {@link #SEARCH_ACROSS_LANGUAGE_BOUNDARIES}.
+	 * {@link #FIND_REFERENCES}, {@link #SEARCH_ACROSS_LANGUAGE_BOUNDARIES}, and 
+	 * {@link #FIND_POTENTIAL_MATCHES}.
 	 * @return an array of names
 	 * @throws CoreException
 	 */

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/index/IIndexName.java

@@ -69,6 +69,14 @@ public interface IIndexName extends IName {
 	 */
 	public boolean couldBePolymorphicMethodCall() throws CoreException;
 
+	/**
+	 * Returns whether this name is a potential match for its binding, rather than an exact match.
+	 * An example of a potential match might be a function definition that does match a
+	 * declaration exactly in signature.
+	 * @since 6.4
+	 */
+	public boolean isPotentialMatch() throws CoreException;
+	
 	/**
 	 * Returns whether this name specifies an inline namespace.
 	 * @since 5.3

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/c/CASTTranslationUnit.java

@@ -67,11 +67,17 @@ public class CASTTranslationUnit extends ASTTranslationUnit implements IASTAmbig
 		return CVisitor.getDeclarations(this, binding);
 	}
 
-    @Override
+	@Override
 	public IASTName[] getDefinitionsInAST(IBinding binding) {
+		return getDefinitionsInAST(binding, false);
+	}
+	
+    @Override
+	public IASTName[] getDefinitionsInAST(IBinding binding, boolean permissive) {
 		if (binding instanceof IMacroBinding) {
 			return getMacroDefinitionsInAST((IMacroBinding) binding);
         }
+		// TODO: Implement support for permissive=true.
     	IASTName[] names = CVisitor.getDeclarations(this, binding);
     	for (int i = 0; i < names.length; i++) {
     		if (!names[i].isDefinition())

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTTranslationUnit.java

@@ -130,11 +130,16 @@ public class CPPASTTranslationUnit extends ASTTranslationUnit implements ICPPAST
     }
 
     @Override
-	public IASTName[] getDefinitionsInAST(IBinding binding) {
+    public IASTName[] getDefinitionsInAST(IBinding binding) {
+    	return getDefinitionsInAST(binding, false);
+    }
+    
+    @Override
+	public IASTName[] getDefinitionsInAST(IBinding binding, boolean permissive) {
         if (binding instanceof IMacroBinding) {
         	return getMacroDefinitionsInAST((IMacroBinding) binding);
         }
-    	IASTName[] names = CPPVisitor.getDeclarations(this, binding);
+    	IASTName[] names = CPPVisitor.getDeclarations(this, binding, permissive);
         for (int i = 0; i < names.length; i++) {
             if (!names[i].isDefinition())
                 names[i] = null;

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/CPPVisitor.java

@@ -1508,6 +1508,7 @@ public class CPPVisitor extends ASTQueries {
 		private int kind;
 		private char[] requiredName;
 		private IIndex index;
+		private boolean permissive;
 
 		private static final int KIND_LABEL  = 1;
 		private static final int KIND_OBJ_FN = 2;
@@ -1516,10 +1517,11 @@ public class CPPVisitor extends ASTQueries {
 		private static final int KIND_COMPOSITE = 5;
 		private static final int KIND_TEMPLATE_PARAMETER = 6;
 
-		public CollectDeclarationsAction(IBinding binding) {
+		public CollectDeclarationsAction(IBinding binding, boolean permissive) {
 			shouldVisitTranslationUnit = true;
 			shouldVisitNames = true;
 			this.decls = new IASTName[DEFAULT_LIST_SIZE];
+			this.permissive = permissive;
 
 			final String bname = binding.getName();
 			if (bname.length() > 0 && !bname.startsWith("operator")) { //$NON-NLS-1$
@@ -1645,7 +1647,7 @@ public class CPPVisitor extends ASTQueries {
 		private boolean isDeclarationBinding(IBinding nameBinding) {
 			if (nameBinding != null) {
 				for (IBinding binding : bindings) {
-					if (areEquivalentBindings(nameBinding, binding, index)) {
+					if (areEquivalentBindings(nameBinding, binding, index, permissive)) {
 						return true;
 					}
 					// A using declaration is a declaration for the references of its delegates.
@@ -1667,6 +1669,20 @@ public class CPPVisitor extends ASTQueries {
 		}
 	}
 
+	private static boolean areEquivalentBindings(IBinding candidate, IBinding target, IIndex index,
+			boolean permissive) {
+		if (permissive && candidate instanceof IProblemBinding && !(target instanceof IProblemBinding)) {
+			IProblemBinding problem = (IProblemBinding) candidate;
+			for (IBinding c : problem.getCandidateBindings()) {
+				if (areEquivalentBindings(c, target, index)) {
+					return true;
+				}
+			}
+			return false;
+		}
+		return areEquivalentBindings(candidate, target, index);
+	}
+	
 	private static boolean areEquivalentBindings(IBinding binding1, IBinding binding2, IIndex index) {
 		if (binding1.equals(binding2)) {
 			return true;
@@ -2759,7 +2775,11 @@ public class CPPVisitor extends ASTQueries {
 	}
 
 	public static IASTName[] getDeclarations(IASTTranslationUnit tu, IBinding binding) {
-	    CollectDeclarationsAction action = new CollectDeclarationsAction(binding);
+		return getDeclarations(tu, binding, false);
+	}
+	
+	public static IASTName[] getDeclarations(IASTTranslationUnit tu, IBinding binding, boolean permissive) {
+	    CollectDeclarationsAction action = new CollectDeclarationsAction(binding, permissive);
 	    tu.accept(action);
 
 		IASTName[] found = action.getDeclarations();

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/IIndexFragment.java

@@ -49,6 +49,10 @@ public interface IIndexFragment {
 	 * @see IIndex#SEARCH_ACROSS_LANGUAGE_BOUNDARIES
 	 */
 	final int SEARCH_ACROSS_LANGUAGE_BOUNDARIES = IIndex.SEARCH_ACROSS_LANGUAGE_BOUNDARIES;
+	/**
+	 * @see IIndex#FIND_POTENTIAL_MATCHES
+	 */
+	final int FIND_POTENTIAL_MATCHES = IIndex.FIND_POTENTIAL_MATCHES;
 	/**
 	 * @see IIndex#FIND_DECLARATIONS_DEFINITIONS
 	 */
@@ -210,7 +214,7 @@ public interface IIndexFragment {
 	 * references, declarations or definitions, or a combination of those.
 	 * @param binding a binding for which names are searched for
 	 * @param flags a combination of {@link #FIND_DECLARATIONS}, {@link #FIND_DEFINITIONS},
-	 *     {@link #FIND_REFERENCES} and {@link #FIND_NON_LOCAL_ONLY}
+	 *     {@link #FIND_REFERENCES}, {@link #FIND_NON_LOCAL_ONLY}, and {@link #FIND_POTENTIAL_MATCHES}.
 	 * @return an array of names
 	 * @throws CoreException
 	 */

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/PDOM.java

@@ -1234,27 +1234,28 @@ public class PDOM extends PlatformObject implements IPDOM {
 		PDOMName name;
 		if ((options & FIND_DECLARATIONS) != 0) {
 			for (name= pdomBinding.getFirstDeclaration(); name != null; name= name.getNextInBinding()) {
-				if (isCommitted(name)) {
+				if (isCommitted(name) && !name.isPotentialMatch()) {
 					names.add(name);
 				}
 			}
 		}
 		if ((options & FIND_DEFINITIONS) != 0) {
 			for (name = pdomBinding.getFirstDefinition(); name != null; name= name.getNextInBinding()) {
-				if (isCommitted(name)) {
+				boolean findPotentialMatches = (options & FIND_POTENTIAL_MATCHES) != 0;
+				if (isCommitted(name) && (!name.isPotentialMatch() || findPotentialMatches)) {
 					names.add(name);
 				}
 			}
 		}
 		if ((options & FIND_REFERENCES) != 0) {
 			for (name = pdomBinding.getFirstReference(); name != null; name= name.getNextInBinding()) {
-				if (isCommitted(name)) {
+				if (isCommitted(name) && !name.isPotentialMatch()) {
 					names.add(name);
 				}
 			}
 			for (IPDOMIterator<PDOMName> iterator = pdomBinding.getExternalReferences(); iterator.hasNext();) {
 				name = iterator.next();
-				if (isCommitted(name))
+				if (isCommitted(name) && !name.isPotentialMatch())
 					names.add(name);
 			}
 		}

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMFile.java

@@ -29,6 +29,7 @@ import org.eclipse.cdt.core.dom.ast.IASTPreprocessorUndefStatement;
 import org.eclipse.cdt.core.dom.ast.IBinding;
 import org.eclipse.cdt.core.dom.ast.IMacroBinding;
 import org.eclipse.cdt.core.dom.ast.IParameter;
+import org.eclipse.cdt.core.dom.ast.IProblemBinding;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPUsingDirective;
 import org.eclipse.cdt.core.index.IIndexFileLocation;
 import org.eclipse.cdt.core.index.IIndexInclude;
@@ -514,9 +515,27 @@ public class PDOMFile implements IIndexFragmentFile {
 			}
 			PDOMBinding pdomBinding = linkage.addBinding(name);
 			if (pdomBinding != null) {
-				final PDOMName result= new PDOMName(fLinkage, name, this, pdomBinding, caller);
+				final PDOMName result= new PDOMName(fLinkage, name, this, pdomBinding, caller, 
+						false /* exact match */);
 				linkage.onCreateName(this, name, result);
 				return result;
+			} else {
+				IBinding b = name.resolveBinding();
+				if (b instanceof IProblemBinding) {
+					IIndexFragmentName result = null;
+					for (IBinding candidate : ((IProblemBinding) b).getCandidateBindings()) {
+						pdomBinding = linkage.adaptBinding(candidate);
+						if (pdomBinding != null) {
+							final PDOMName pdomName = new PDOMName(fLinkage, name, this, pdomBinding, caller,
+									true /* potential match */);
+							linkage.onCreateName(this, name, pdomName);
+							if (result == null) {
+								result = pdomName;
+							}
+						}
+					}
+					return result;
+				}
 			}
 		} catch (CoreException e) {
 			final IStatus status = e.getStatus();

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMMacroDefinitionName.java

@@ -34,6 +34,11 @@ class PDOMMacroDefinitionName implements IIndexFragmentName {
 		return false;
 	}
 	
+	@Override
+	public boolean isPotentialMatch() throws CoreException {
+		return false;
+	}
+
 	@Override
 	public IIndexName[] getEnclosedNames() throws CoreException {
 		return IIndexName.EMPTY_ARRAY;

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMMacroReferenceName.java

@@ -173,6 +173,11 @@ public final class PDOMMacroReferenceName implements IIndexFragmentName {
 		return false;
 	}
 	
+	@Override
+	public boolean isPotentialMatch() throws CoreException {
+		return false;
+	}
+
 	@Override
 	public boolean isInlineNamespaceDefinition() {
 		return false;

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMName.java

@@ -56,9 +56,17 @@ public final class PDOMName implements IIndexFragmentName {
 	public static final int COULD_BE_POLYMORPHIC_METHOD_CALL	= 0x10;
 	public static final int READ_ACCESS 						= 0x20;
 	public static final int WRITE_ACCESS 						= 0x40;
+	// Whether this name is a potential match for its binding, rather than an exact match.
+	// Potential matches are recorded in the index so that we can e.g. offer best-effort
+	// navigation even in the presence of errors in the code (or an incomplete project
+	// configuration), but they are annotated as such so exact matches can be preferred
+	// where appropriate.
+	public static final int IS_POTENTIAL_MATCH                  = 0x80;
+	// Note: There is no room in the flags byte for more flags. If more flags are
+	//       needed, the flag byte needs to be expanded to a short.
 
-	public PDOMName(PDOMLinkage linkage, IASTName name, PDOMFile file, PDOMBinding binding, PDOMName caller)
+	public PDOMName(PDOMLinkage linkage, IASTName name, PDOMFile file, PDOMBinding binding, PDOMName caller,
-			throws CoreException {
+			boolean isPotentialMatch) throws CoreException {
 		this.linkage = linkage;
 		Database db = linkage.getDB();
 		record = db.malloc(RECORD_SIZE);
@@ -66,6 +74,10 @@ public final class PDOMName implements IIndexFragmentName {
 		// What kind of name are we
 		int flags= getRoleOfName(name);
 
+		if (isPotentialMatch) {
+			flags |= IS_POTENTIAL_MATCH;
+		}
+		
 		flags |= binding.getAdditionalNameFlags(flags, name);
 		db.putByte(record + FLAGS, (byte) flags);
 
@@ -286,6 +298,11 @@ public final class PDOMName implements IIndexFragmentName {
 		return getFlags(COULD_BE_POLYMORPHIC_METHOD_CALL) == COULD_BE_POLYMORPHIC_METHOD_CALL;
 	}
 	
+	@Override
+	public boolean isPotentialMatch() throws CoreException {
+		return getFlags(IS_POTENTIAL_MATCH) == IS_POTENTIAL_MATCH;
+	}
+
 	@Override
 	public boolean isReadAccess() throws CoreException {
 		return getFlags(READ_ACCESS) == READ_ACCESS;

core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/selection/CPPSelectionTestsIndexer.java

@@ -26,6 +26,7 @@ import org.eclipse.cdt.core.CCorePlugin;
 import org.eclipse.cdt.core.dom.IPDOMManager;
 import org.eclipse.cdt.core.dom.ast.IASTFileLocation;
 import org.eclipse.cdt.core.dom.ast.IASTName;
+import org.eclipse.cdt.core.dom.ast.IASTNameOwner;
 import org.eclipse.cdt.core.dom.ast.IASTNode;
 import org.eclipse.cdt.core.dom.ast.IBinding;
 import org.eclipse.cdt.core.dom.ast.IMacroBinding;
@@ -1336,4 +1337,46 @@ public class CPPSelectionTestsIndexer extends BaseSelectionTestsIndexer {
         IASTNode def = testF3(file, offset + 1);
         assertTrue(def instanceof IASTName);
     }
+    
+	//	class Waldo {
+	//	    void find();
+	//	};
+    
+    //	#include "test.hpp"
+	//	int Waldo::find() {}
+	public void testNavigationToDefinitionWithWrongSignature_525739() throws Exception {
+        StringBuilder[] buffers = getContents(3);
+        String hpp = buffers[0].toString();
+        String cpp = buffers[1].toString();
+        IFile hppFile = importFile("test.hpp", hpp);
+        IFile cppFile = importFile("test.cpp", cpp);
+        waitUntilFileIsIndexed(index, cppFile);
+
+        // We should find the definition, even though the signature doesn't match exactly.
+        IASTNode target = testF3(hppFile, hpp.indexOf("void find") + 6);
+		assertInstance(target, IASTName.class);
+		assertEquals(IASTNameOwner.r_definition, ((IASTName) target).getRoleOfName(false));
+	}
+
+	//	class Waldo {
+	//	    void find();
+	//	};
+    
+	//	#include "test.hpp"
+	//	void Waldo::find() {}
+	//	int Waldo::find() {}
+	public void testNavigationPrefersCorrectDefinition_525739() throws Exception {
+        StringBuilder[] buffers = getContents(3);
+        String hpp = buffers[0].toString();
+        String cpp = buffers[1].toString();
+        IFile hppFile = importFile("test.hpp", hpp);
+        IFile cppFile = importFile("test.cpp", cpp);
+        waitUntilFileIsIndexed(index, cppFile);
+
+        // We should find the definition that's an exact match, rather than asking the
+        // user to disambiguate between two alternatives.
+        IASTNode target = testF3(hppFile, hpp.indexOf("void find") + 6);
+		assertInstance(target, IASTName.class);
+		assertEquals(IASTNameOwner.r_definition, ((IASTName) target).getRoleOfName(false));
+	}
 }

core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/selection/CPPSelectionTestsNoIndexer.java

@@ -37,6 +37,7 @@ import org.eclipse.cdt.core.CCorePlugin;
 import org.eclipse.cdt.core.dom.IPDOMManager;
 import org.eclipse.cdt.core.dom.ast.IASTElaboratedTypeSpecifier;
 import org.eclipse.cdt.core.dom.ast.IASTName;
+import org.eclipse.cdt.core.dom.ast.IASTNameOwner;
 import org.eclipse.cdt.core.dom.ast.IASTNode;
 import org.eclipse.cdt.core.dom.ast.IBinding;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPConstructor;
@@ -1402,4 +1403,18 @@ public class CPPSelectionTestsNoIndexer extends BaseSelectionTests {
 		int offset = code.indexOf("auto") - 2;
 		IASTNode target = testF3(file, offset);
 	}
+	
+	//	class Waldo {
+	//	    void find();
+	//	};
+	//	int Waldo::find() {}
+	public void testNavigationToDefinitionWithWrongSignature_525739() throws Exception {
+		String code = getAboveComment();
+		IFile file = importFile("testBug525739.cpp", code);
+		
+		int offset = code.indexOf("void find") + 6;
+		IASTNode target = testF3(file, offset);
+		assertInstance(target, IASTName.class);
+		assertEquals(IASTNameOwner.r_definition, ((IASTName) target).getRoleOfName(false));
+	}
 }

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/search/actions/OpenDeclarationsJob.java

@@ -406,9 +406,15 @@ class OpenDeclarationsJob extends Job implements ASTRunnable {
 	}
 
 	private IName[] findDefinitions(IIndex index, IASTTranslationUnit ast, IBinding binding) throws CoreException {
-		List<IASTName> declNames= new ArrayList<>();
+		// The priority of matches are as follows:
-		declNames.addAll(Arrays.asList(ast.getDefinitionsInAST(binding)));
+		//  - If there are exact AST matches, those are returned.
-		for (Iterator<IASTName> i = declNames.iterator(); i.hasNext();) {
+		//  - Otherwise, if there are exact index matches, those are returned.
+		//  - Otherwise, permissive matches from the AST and index, if any, are
+		//    combined and returned.
+		List<IASTName> exactAstMatches = new ArrayList<>();
+		List<IName> permissiveMatches = new ArrayList<>();
+		exactAstMatches.addAll(Arrays.asList(ast.getDefinitionsInAST(binding, /* permissive = */ true)));
+		for (Iterator<IASTName> i = exactAstMatches.iterator(); i.hasNext();) {
 			IASTName name= i.next();
 			final IBinding b2 = name.resolveBinding();
 			if (b2 instanceof ICPPUsingDeclaration) {
@@ -427,13 +433,36 @@ class OpenDeclarationsJob extends Job implements ASTRunnable {
 					i.remove();
 				}
 			}
+			if (b2 instanceof IProblemBinding) {
+				permissiveMatches.add(name);
+				i.remove();
+			}
 		}
-		if (!declNames.isEmpty()) {
+		if (!exactAstMatches.isEmpty()) {
-			return declNames.toArray(new IASTName[declNames.size()]);
+			return exactAstMatches.toArray(new IASTName[exactAstMatches.size()]);
 		}
 
-		// 2. Try definition in index.
+		// Try definition in index.
-		return index.findNames(binding, IIndex.FIND_DEFINITIONS | IIndex.SEARCH_ACROSS_LANGUAGE_BOUNDARIES);
+		IName[] indexMatches = index.findNames(binding, IIndex.FIND_DEFINITIONS | 
+				IIndex.SEARCH_ACROSS_LANGUAGE_BOUNDARIES | IIndex.FIND_POTENTIAL_MATCHES);
+		List<IName> exactIndexMatches = new ArrayList<>();
+		exactIndexMatches.addAll(Arrays.asList(indexMatches));
+		for (Iterator<IName> i = exactIndexMatches.iterator(); i.hasNext();) {
+			IName name = i.next();
+			if (name instanceof IIndexName && ((IIndexName) name).isPotentialMatch()) {
+				permissiveMatches.add(name);
+				i.remove();
+			}
+		}
+		if (!exactIndexMatches.isEmpty()) {
+			return exactIndexMatches.toArray(new IName[exactIndexMatches.size()]);
+		}
+		
+		if (!permissiveMatches.isEmpty()) {
+			return permissiveMatches.toArray(new IName[permissiveMatches.size()]);
+		}
+		
+		return IName.EMPTY_ARRAY;
 	}
 
 	private IName[] findDeclarations(IIndex index, IASTTranslationUnit ast, IBinding binding) throws CoreException {