Skip to content

Commit

Permalink
Fix for #1019: improve accuracy of searches for simple method references
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Jan 13, 2020
1 parent 70065b0 commit 53f8893
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 12 deletions.
80 changes: 71 additions & 9 deletions ...ests.builder/src/org/eclipse/jdt/core/groovy/tests/search/MethodReferenceSearchTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -428,10 +428,10 @@ public void testMethodWithDefaultParameters4() throws Exception {
}

@Test
public void testStaticMethodReferenceSearch() throws Exception {
public void testStaticMethodReferenceSearch1() throws Exception {
String contents =
"class Foo {\n" +
" static def bar() {}\n" +
" static bar() {}\n" +
" static {\n" +
" bar()\n" +
" }\n" +
Expand All @@ -444,9 +444,71 @@ public void testStaticMethodReferenceSearch() throws Exception {
factory.createVisitor(match).visitCompilationUnit(new TypeRequestorFactory().createRequestor(match, pattern, searchRequestor));

assertEquals(1, searchRequestor.getMatches().size());
assertEquals(SearchMatch.A_ACCURATE, searchRequestor.getMatch(0).getAccuracy());
assertLocation(searchRequestor.getMatch(0), contents.lastIndexOf("bar"), "bar".length());
}

@Test
public void testStaticMethodReferenceSearch2() throws Exception {
String contents =
"import static Foo.bar\n" +
"class Foo {\n" +
" static bar() {}\n" +
"}\n";
GroovyCompilationUnit foo = createUnit("Foo", contents);

IMethod method = foo.getType("Foo").getMethods()[0];
MockPossibleMatch match = new MockPossibleMatch(foo);
SearchPattern pattern = SearchPattern.createPattern(method, IJavaSearchConstants.REFERENCES);
factory.createVisitor(match).visitCompilationUnit(new TypeRequestorFactory().createRequestor(match, pattern, searchRequestor));

assertEquals(1, searchRequestor.getMatches().size());
assertEquals(SearchMatch.A_ACCURATE, searchRequestor.getMatch(0).getAccuracy());
assertLocation(searchRequestor.getMatch(0), contents.indexOf("bar"), "bar".length());
}

@Test
public void testStaticMethodReferenceSearch3() throws Exception {
String contents =
"import static Foo.bar\n" +
"class Foo {\n" +
" static bar() {}\n" +
" static bar(int i) {}\n" +
"}\n";
GroovyCompilationUnit foo = createUnit("Foo", contents);

IMethod method = foo.getType("Foo").getMethods()[0];
MockPossibleMatch match = new MockPossibleMatch(foo);
SearchPattern pattern = SearchPattern.createPattern(method, IJavaSearchConstants.REFERENCES);
factory.createVisitor(match).visitCompilationUnit(new TypeRequestorFactory().createRequestor(match, pattern, searchRequestor));

assertEquals(1, searchRequestor.getMatches().size());
assertEquals(SearchMatch.A_INACCURATE, searchRequestor.getMatch(0).getAccuracy());
assertLocation(searchRequestor.getMatch(0), contents.indexOf("bar"), "bar".length());
}

@Test
public void testStaticMethodReferenceSearch4() throws Exception {
String contents =
"import static Foo.bar\n" +
"class Foo extends Base {\n" +
" static bar() {}\n" +
"}\n" +
"class Base {\n" +
" static bar(int i) {}\n" +
"}\n";
GroovyCompilationUnit foo = createUnit("Foo", contents);

IMethod method = foo.getType("Foo").getMethods()[0];
MockPossibleMatch match = new MockPossibleMatch(foo);
SearchPattern pattern = SearchPattern.createPattern(method, IJavaSearchConstants.REFERENCES);
factory.createVisitor(match).visitCompilationUnit(new TypeRequestorFactory().createRequestor(match, pattern, searchRequestor));

assertEquals(1, searchRequestor.getMatches().size());
assertEquals(SearchMatch.A_INACCURATE, searchRequestor.getMatch(0).getAccuracy());
assertLocation(searchRequestor.getMatch(0), contents.indexOf("bar"), "bar".length());
}

@Test
public void testExplicitPropertyGetterSearch1() throws Exception {
GroovyCompilationUnit bar = createUnit("foo", "Bar",
Expand All @@ -458,13 +520,13 @@ public void testExplicitPropertyGetterSearch1() throws Exception {
"}\n");
GroovyCompilationUnit baz = createUnit("foo", "Baz",
"package foo\n" +
"import static foo.Bar.getString as blah\n" + // potential
"import static foo.Bar.getString as getS\n" + // potential
"import static foo.Bar.getString as blah\n" + // exact
"import static foo.Bar.getString as getS\n" + // exact
"Bar.getString()\n" + // exact
"Bar.'getString'()\n" + // exact
"str = Bar.string\n" + // potential
"str = Bar.string\n" + // exact
"str = Bar.@string\n" +
"fun = Bar.&getString\n" + // potential
"fun = Bar.&getString\n" + // potential (may refer to category method)
"str = blah()\n" + // exact
"str = s\n"); // exact

Expand All @@ -478,16 +540,16 @@ public void testExplicitPropertyGetterSearch1() throws Exception {
List<SearchMatch> matches = searchRequestor.getMatches();

assertEquals(8, matches.size());
assertEquals(SearchMatch.A_INACCURATE, matches.get(0).getAccuracy());
assertEquals(SearchMatch.A_ACCURATE, matches.get(0).getAccuracy());
assertEquals(String.valueOf(baz.getContents()).indexOf("getString as blah"), matches.get(0).getOffset());
assertEquals(SearchMatch.A_INACCURATE, matches.get(1).getAccuracy());
assertEquals(SearchMatch.A_ACCURATE, matches.get(1).getAccuracy());
assertEquals(String.valueOf(baz.getContents()).indexOf("getString as getS"), matches.get(1).getOffset());
assertEquals(SearchMatch.A_ACCURATE, matches.get(2).getAccuracy());
assertEquals(String.valueOf(baz.getContents()).indexOf("getString()"), matches.get(2).getOffset());
assertEquals(SearchMatch.A_ACCURATE, matches.get(3).getAccuracy());
assertEquals(String.valueOf(baz.getContents()).indexOf("'getString'"), matches.get(3).getOffset());

assertEquals(SearchMatch.A_INACCURATE, matches.get(4).getAccuracy());
assertEquals(SearchMatch.A_ACCURATE, matches.get(4).getAccuracy());
assertEquals(String.valueOf(baz.getContents()).indexOf("Bar.string") + 4, matches.get(4).getOffset());
assertEquals(SearchMatch.A_INACCURATE, matches.get(5).getAccuracy());
assertEquals(String.valueOf(baz.getContents()).indexOf("Bar.&getString") + 5, matches.get(5).getOffset());
Expand Down
14 changes: 13 additions & 1 deletion base/org.eclipse.jdt.groovy.core/src/org/eclipse/jdt/groovy/search/SimpleTypeLookup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.codehaus.groovy.ast.ConstructorNode;
import org.codehaus.groovy.ast.DynamicVariable;
import org.codehaus.groovy.ast.FieldNode;
import org.codehaus.groovy.ast.ImportNode;
import org.codehaus.groovy.ast.MethodNode;
import org.codehaus.groovy.ast.Parameter;
import org.codehaus.groovy.ast.PropertyNode;
Expand Down Expand Up @@ -421,7 +422,9 @@ protected TypeLookupResult findTypeForNameWithKnownObjectExpression(final String
} else if (method.isPrivate() && isThisObjectExpression(scope) && isNotThisOrOuterClass(declaringType, resolvedDeclaringType)) {
// "this.method()" reference to private method of super class yields MissingMethodException; "super.method()" is okay
confidence = TypeConfidence.UNKNOWN;
} else if (isLooseMatch(scope.getMethodCallArgumentTypes(), method.getParameters())) {
} else if (isLooseMatch(scope.getMethodCallArgumentTypes(), method.getParameters()) &&
!(isStaticObjectExpression && isStaticReferenceToUnambiguousMethod(scope, name, declaringType)) &&
!(AccessorSupport.isGetter(method) && !scope.isMethodCall() && scope.getEnclosingNode() instanceof PropertyExpression)) {
// if arguments and parameters are mismatched, a category method may make a better match
confidence = TypeConfidence.LOOSELY_INFERRED;
}
Expand Down Expand Up @@ -981,6 +984,15 @@ protected static boolean isStaticReferenceToInstanceMethod(final VariableScope s
return false;
}

protected static boolean isStaticReferenceToUnambiguousMethod(final VariableScope scope, final String name, final ClassNode type) {
if (scope.getEnclosingNode() instanceof ImportNode) { // import nodes can only refer to static methods of type
long staticMethodCount = getMethods(name, type).stream().filter(meth -> isCompatible(meth, true)).count();
return (staticMethodCount == 1);
}
// TODO: Add case for PropertyExpression, MethodCallExpression or MethodPointerExpression?
return false;
}

protected static boolean isCompatible(final AnnotatedNode declaration, final boolean isStaticExpression) {
if (declaration != null) {
boolean isStatic = false;
Expand Down
8 changes: 6 additions & 2 deletions ...dt.groovy.core/src/org/eclipse/jdt/groovy/search/TypeInferencingVisitorWithRequestor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ public void visitCompilationUnit(final ITypeRequestor requestor) {

this.requestor = requestor;
this.enclosingElement = unit;
VariableScope topLevelScope = new VariableScope(null, enclosingModule, false);
VariableScope topLevelScope = new VariableScope(null, enclosingModule, true);
scopes.add(topLevelScope);

for (ITypeLookup lookup : lookups) {
Expand Down Expand Up @@ -531,9 +531,11 @@ public void visitImports(final ModuleNode node) {
if (imp.getFieldNameExpr() != null) {
completeExpressionStack.add(imp);
try {
scope.setCurrentNode(imp);
primaryTypeStack.add(type);
imp.getFieldNameExpr().visit(this);
visitConstantExpression(imp.getFieldNameExpr());

scope.forgetCurrentNode();
dependentTypeStack.removeLast();
dependentDeclarationStack.removeLast();
} finally {
Expand Down Expand Up @@ -2505,6 +2507,8 @@ private boolean hasStaticObjectExpression(final Expression node, final ClassNode
} else if (objectExpression instanceof ClassExpression || VariableScope.CLASS_CLASS_NODE.equals(primaryType)) {
staticObjectExpression = true; // separate lookup exists for non-static members of Class, Object, or GroovyObject
}
} else if (complete instanceof ImportNode) {
staticObjectExpression = true;
}
}
return staticObjectExpression;
Expand Down

0 comments on commit 53f8893

Please sign in to comment.