Skip to content

Commit

Permalink
Fix for #1153: improve de-duplication of method completion proposals
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Aug 14, 2020
1 parent c49c154 commit fb50a87
Show file tree
Hide file tree
Showing 5 changed files with 153 additions and 39 deletions.
25 changes: 19 additions & 6 deletions ...rc/org/codehaus/groovy/eclipse/codeassist/tests/DefaultGroovyMethodCompletionTests.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,16 +303,29 @@ final class DefaultGroovyMethodCompletionTests extends CompletionTestSuite {
}
}

@Test // GRECLIPSE-1422
@Test
void testNoDups1() {
ICompletionProposal[] proposals = createProposalsAtOffset('[].collectEnt', 13)
proposalExists(proposals, 'collectEntries', 4) // (), (Closure), (Map), (Map, Closure)
}

@Test // GRECLIPSE-1422
void testNoDups2() {
ICompletionProposal[] proposals = createProposalsAtOffset('[].findA', getIndexOf('[].findA', 'findA'))
// should find 2, not 4. dups removed
proposalExists(proposals, 'findAll', 2)
proposalExists(proposals, 'findAll', 2) // should find 2, not 4
}

@Test
void testNoDups2() {
ICompletionProposal[] proposals = createProposalsAtOffset('[].collectEnt', 13)
proposalExists(proposals, 'collectEntries', 4) // collectEntries(), collectEntries(Closure), collectEntries(Map), collectEntries(Map, Closure)
void testNoDups3() {
ICompletionProposal[] proposals = createProposalsAtOffset('List<String> strings = []; strings.find', 39)
proposalExists(proposals, 'find(Closure closure) : T', 1) // not Object
proposalExists(proposals, 'find() : T', 1) // not Object
}

@Test
void testNoDups4() {
ICompletionProposal[] proposals = createProposalsAtOffset('List<String> strings = []; strings.findA', 40)
proposalExists(proposals, 'findAll(Closure closure) : List<T>', 1) // not Collection<T>
proposalExists(proposals, 'findAll() : List<T>', 1) // not Collection<T>
}
}
4 changes: 2 additions & 2 deletions ...se.codeassist.test/src/org/codehaus/groovy/eclipse/codeassist/tests/RelevanceTests.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ final class RelevanceTests extends CompletionTestSuite {
|}
|'''.stripMargin()
ICompletionProposal[] proposals = orderByRelevance(createProposalsAtOffset(contents, getLastIndexOf(contents, 's')))
assertProposalOrdering(proposals, 'string : String - Foo', 'string : String - Bar', 'setString(String value) : void - Foo', 'setString(String value) : void - Bar', 'setMetaClass')
assertProposalOrdering(proposals, 'string : String - Foo', 'string : String - Bar', 'setString(String value) : void - Foo', 'setString(String value) : void - Bar', 'sleep')
}

@Test
Expand All @@ -158,7 +158,7 @@ final class RelevanceTests extends CompletionTestSuite {
|}
|'''.stripMargin()
ICompletionProposal[] proposals = orderByRelevance(createProposalsAtOffset(contents, getLastIndexOf(contents, 's')))
assertProposalOrdering(proposals, 'string : String - Bar', 'string : String - Foo', 'setString(String value) : void - Bar', 'setString(String value) : void - Foo', 'setMetaClass')
assertProposalOrdering(proposals, 'string : String - Bar', 'string : String - Foo', 'setString(String value) : void - Bar', 'setString(String value) : void - Foo', 'sleep')
}

@Test
Expand Down
6 changes: 6 additions & 0 deletions ...t/src/org/codehaus/groovy/eclipse/codeassist/tests/TriggerCharacterCompletionTests.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import org.eclipse.jface.preference.IPreferenceStore
import org.eclipse.jface.text.Document
import org.eclipse.jface.text.IDocument
import org.eclipse.jface.text.contentassist.ICompletionProposal
import org.junit.After
import org.junit.Assert
import org.junit.Before
import org.junit.Test
Expand Down Expand Up @@ -65,6 +66,11 @@ final class TriggerCharacterCompletionTests extends CompletionTestSuite {
options.each { key, val -> groovyPrefs.setValue(key, val) }
}

@After
void tearDown() {
System.clearProperty(AssistOptions.PROPERTY_SubstringMatch)
}

/**
* Tries each completion proposal for <code>sort</code> using the trigger character <code>'{'</code>.
*/
Expand Down
114 changes: 87 additions & 27 deletions ...deassist/src/org/codehaus/groovy/eclipse/codeassist/creators/CategoryProposalCreator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,15 @@
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Queue;
import java.util.Set;

import org.codehaus.groovy.ast.ClassHelper;
import org.codehaus.groovy.ast.ClassNode;
import org.codehaus.groovy.ast.FieldNode;
import org.codehaus.groovy.ast.MethodNode;
Expand Down Expand Up @@ -57,7 +62,7 @@ public class CategoryProposalCreator extends AbstractProposalCreator {
protected static final String GROOVY_METHOD_CONTRIBUTOR = "Groovy";

@Override
public List<IGroovyProposal> findAllProposals(ClassNode selfType, Set<ClassNode> categories, String prefix, boolean isStatic, boolean isPrimary) {
public List<IGroovyProposal> findAllProposals(final ClassNode selfType, final Set<ClassNode> categories, final String prefix, final boolean isStatic, final boolean isPrimary) {
if (categories == null || categories.isEmpty()) {
return Collections.emptyList();
}
Expand All @@ -75,45 +80,106 @@ public List<IGroovyProposal> findAllProposals(ClassNode selfType, Set<ClassNode>
if (method.isStatic() && method.isPublic()) {
Parameter[] params = method.getParameters();

if (matcher.test(prefix, methodName)) {
if (params.length > 0 && GroovyUtils.isAssignable(selfType, params[0].getType())) {
proposals.add(new CategoryMethodProposal(method));
}
if (params.length > 0 && matcher.test(prefix, methodName) &&
GroovyUtils.isAssignable(selfType, params[0].getType())) {
CategoryMethodProposal proposal = new CategoryMethodProposal(method);
proposal.setRelevanceMultiplier(tweakRelevance(method, selfType));
proposals.add(proposal);
}

if (params.length == 1 && findLooselyMatchedAccessorKind(prefix, methodName, true).isAccessorKind(method, true) &&
hasNoField(selfType, methodName) && GroovyUtils.isAssignable(selfType, params[0].getType()) &&
(isStatic || !GeneralUtils.isOrImplements(selfType, VariableScope.MAP_CLASS_NODE)) &&
(isDefaultCategory || !methodName.startsWith("is"))) { // GROOVY-5245
// add property variant of accessor method
proposals.add(new CategoryPropertyProposal(method));
// add property variant of accessor category method
CategoryPropertyProposal proposal = new CategoryPropertyProposal(method);
proposal.setRelevanceMultiplier(tweakRelevance(method, selfType));
proposals.add(proposal);
}
}
}
}
return proposals;
}

protected boolean isDefaultCategory(ClassNode category) {
protected boolean isDefaultCategory(final ClassNode category) {
return (VariableScope.DGM_CLASS_NODE.equals(category) || (currentScope != null && currentScope.isDefaultCategory(category)));
}

protected boolean isDefaultStaticCategory(ClassNode category) {
protected boolean isDefaultStaticCategory(final ClassNode category) {
return (VariableScope.DGSM_CLASS_NODE.equals(category) || (currentScope != null && currentScope.isDefaultStaticCategory(category)));
}

//--------------------------------------------------------------------------
protected static boolean isObjectOrPrimitiveArray(final ClassNode type) {
return (VariableScope.OBJECT_CLASS_NODE.equals(type.getComponentType()) || ClassHelper.isPrimitiveType(type.getComponentType()));
}

protected class CategoryMethodProposal extends GroovyMethodProposal {
protected float tweakRelevance(final MethodNode method, final ClassNode selfType) {
float relevanceMultiplier = isDefaultCategory(method.getDeclaringClass()) ? 0.1f : 5.0f;

protected CategoryMethodProposal(MethodNode method) {
super(method, GROOVY_METHOD_CONTRIBUTOR);
ClassNode firstParamType = method.getParameters()[0].getType();
if (!selfType.equals(firstParamType)) {
int distance = 0;

if (isDefaultCategory(method.getDeclaringClass())) {
setRelevanceMultiplier(0.1f);
if (!firstParamType.isInterface()) {
ClassNode next = selfType;
do {
distance += 1;
next = next.isArray() && !isObjectOrPrimitiveArray(next)
? VariableScope.OBJECT_CLASS_NODE.makeArray() : next.getSuperClass();
} while (next != null && !next.equals(firstParamType));
if (next == null) distance = 5; // arbitrary for implicit Object
} else if (firstParamType.equals(VariableScope.GROOVY_OBJECT_CLASS_NODE)) {
distance = 5; // arbitrary for explicit or implicit GroovyObject
} else {
setRelevanceMultiplier(5);
Queue<ClassDepth> todo = new LinkedList<>();
Set<ClassNode> visited = new HashSet<>();
todo.add(new ClassDepth(selfType, 0));

out: while (!todo.isEmpty()) {
ClassDepth next = todo.remove();
visited.add(next.clazz.redirect());
for (ClassNode face : next.clazz.getInterfaces()) {
if (firstParamType.equals(face)) {
distance = next.depth + 1;
break out;
}
if (!visited.contains(face.redirect())) {
todo.add(new ClassDepth(face, next.depth + 1));
}
}

Optional.ofNullable(next.clazz.getSuperClass())
.filter(sc -> !sc.equals(VariableScope.OBJECT_CLASS_NODE))
.map(sc -> new ClassDepth(sc, next.depth + 1)).ifPresent(todo::add);
}
}

while (distance-- > 0) {
relevanceMultiplier *= 0.88f;
}
}

return relevanceMultiplier;
}

private static class ClassDepth { // TODO: class->record

ClassDepth(final ClassNode t, final int n) {
clazz = t;
depth = n;
}

final ClassNode clazz;
final int depth;
}

//--------------------------------------------------------------------------

protected class CategoryMethodProposal extends GroovyMethodProposal {

protected CategoryMethodProposal(final MethodNode method) {
super(method, GROOVY_METHOD_CONTRIBUTOR);
}

@Override
Expand All @@ -131,17 +197,17 @@ protected char[] createMethodSignature() {
}

@Override
protected char[][] getParameterNames(Parameter[] parameters) {
protected char[][] getParameterNames(final Parameter[] parameters) {
return (char[][]) ArrayUtils.remove(super.getParameterNames(parameters), 0);
}

@Override
protected char[][] getParameterTypeNames(Parameter[] parameters) {
protected char[][] getParameterTypeNames(final Parameter[] parameters) {
return (char[][]) ArrayUtils.remove(super.getParameterTypeNames(parameters), 0);
}

@Override
public IJavaCompletionProposal createJavaProposal(CompletionEngine engine, ContentAssistContext context, JavaContentAssistInvocationContext javaContext) {
public IJavaCompletionProposal createJavaProposal(final CompletionEngine engine, final ContentAssistContext context, final JavaContentAssistInvocationContext javaContext) {
IJavaCompletionProposal javaProposal = super.createJavaProposal(engine, context, javaContext);
if (javaProposal instanceof LazyJavaCompletionProposal) {
//ProposalInfo proposalInfo = ((LazyJavaCompletionProposal) javaProposal).getProposalInfo();
Expand Down Expand Up @@ -199,22 +265,16 @@ protected IMember resolveMember() throws JavaModelException {

protected class CategoryPropertyProposal extends GroovyFieldProposal {

protected CategoryPropertyProposal(MethodNode method) {
protected CategoryPropertyProposal(final MethodNode method) {
super(createMockField(method));

if (!isDefaultStaticCategory(method.getDeclaringClass())) {
getField().setModifiers(getField().getModifiers() & ~Flags.AccStatic);
}

if (isDefaultCategory(method.getDeclaringClass())) {
setRelevanceMultiplier(0.1f);
} else {
setRelevanceMultiplier(5);
}
}

@Override
protected StyledString createDisplayString(FieldNode field) {
protected StyledString createDisplayString(final FieldNode field) {
return super.createDisplayString(field).append(new StyledString(
" (" + GROOVY_METHOD_CONTRIBUTOR + ")", StyledString.DECORATIONS_STYLER));
}
Expand Down
43 changes: 39 additions & 4 deletions ...ehaus/groovy/eclipse/codeassist/processors/StatementAndExpressionCompletionProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
package org.codehaus.groovy.eclipse.codeassist.processors;

import static org.codehaus.groovy.ast.tools.GeneralUtils.isOrImplements;
import static org.codehaus.groovy.runtime.StringGroovyMethods.find;
import static org.codehaus.groovy.transform.trait.Traits.isTrait;

import java.util.ArrayList;
Expand Down Expand Up @@ -343,7 +342,7 @@ private List<ICompletionProposal> createJavaProposals(final Collection<IGroovyPr
}
}

String signature = javaProposal.getDisplayString().split(" : ")[0]; // TODO: remove parameter generics and names
String signature = getProposalSignature(javaProposal.getDisplayString());
//javaProposals.computeIfAbsent(signature, k -> new ArrayList<>()).add(javaProposal);
javaProposals.merge(signature, Collections.singletonList(javaProposal), (list, one) -> {
if (list.size() == 1) list = new ArrayList<>(list);
Expand All @@ -364,8 +363,12 @@ private List<ICompletionProposal> createJavaProposals(final Collection<IGroovyPr
} else { // de-duplicate the proposal group
Map<String, IJavaCompletionProposal> map = new HashMap<>(n);
for (IJavaCompletionProposal jcp : group) {
String key = jcp.getDisplayString().split(" - ")[1];
key = find((CharSequence) key, "\\w+(?=\\s|$)");
String key = jcp.getDisplayString();
int i = key.indexOf(" - ") + 3;
int j = key.indexOf(' ', i);
if (j < 0) j = key.length();
key = key.substring(i, j);

map.merge(key, jcp, (one, two) -> {
// TODO: break ties between unqualified and fully-qualified declaring types
return (one.getRelevance() > two.getRelevance() ? one : two);
Expand Down Expand Up @@ -474,6 +477,38 @@ private static ClassNode getCompletionType(final ASTNode completionNode, final C
return completionType;
}

private static String getProposalSignature(final String completionProposalDescription) {
String sig = completionProposalDescription;
int idx = sig.indexOf(" : "); // type, etc.
if (idx != -1) sig = sig.substring(0, idx);

if (sig.endsWith(")") && !sig.endsWith("()")) {
// remove parameter generics and names
String[] tokens = sig.split("[()]|, ");

StringBuilder sb = new StringBuilder(tokens[0]).append('(');
for (int i = 1, n = tokens.length; i < n; i += 1) {
int j = tokens[i].indexOf('<');
if (j < 0) {
j = tokens[i].lastIndexOf(' ');
if (j < 0) j = tokens[i].length();
sb.append(tokens[i], 0, j); // type name
} else {
sb.append(tokens[i], 0, j); // type name

j = tokens[i].lastIndexOf('>');
int k = tokens[i].indexOf(' ', j);
if (k < 0) k = tokens[i].length();
sb.append(tokens[i], j + 1, k); // "[]" or "..."
}
sb.append(',');
}
sb.setCharAt(sb.length() - 1, ')');
sig = sb.toString();
}
return sig;
}

private static VariableScope createTopLevelScope(final ClassNode completionType) {
return new VariableScope(null, completionType, false);
}
Expand Down

0 comments on commit fb50a87

Please sign in to comment.