Skip to content

Commit

Permalink
Fix for #1130: use field/property of this not accessor from parent class
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Jun 30, 2020
1 parent ecbadfe commit b006b64
Show file tree
Hide file tree
Showing 4 changed files with 251 additions and 49 deletions.
226 changes: 226 additions & 0 deletions ...sts.builder/src/org/eclipse/jdt/core/groovy/tests/search/DeclarationInferencingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,120 @@ public void testGetterAndField13a() {
assertUnknown(contents, "xxx");
}

@Test // https://github.com/groovy/groovy-eclipse/issues/1130
public void testGetterAndField14() {
createUnit("Foo",
//@formatter:off
"class Foo {\n" +
" String getXxx() {\n" +
" }\n" +
"}");
//@formatter:on

String contents =
//@formatter:off
"class Bar extends Foo {\n" +
" private String xxx\n" +
" void meth() {\n" +
" xxx\n" +
" this.xxx\n" +
" }\n" +
"}";
//@formatter:on

int offset = contents.indexOf("xxx", contents.indexOf("meth"));
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);

offset = contents.lastIndexOf("xxx");
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);
}

@Test
public void testGetterAndField14a() {
createJavaUnit("Foo",
//@formatter:off
"interface Foo {\n" +
" default String getXxx() {\n" +
" return \"string\";\n" +
" }\n" +
"}");
//@formatter:on

String contents =
//@formatter:off
"class Bar implements Foo {\n" +
" private String xxx\n" +
" void meth() {\n" +
" xxx\n" +
" this.xxx\n" +
" }\n" +
"}";
//@formatter:on

int offset = contents.indexOf("xxx", contents.indexOf("meth"));
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);

offset = contents.lastIndexOf("xxx");
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);
}

@Test // https://github.com/groovy/groovy-eclipse/issues/1130
public void testGetterAndField15() {
createUnit("Foo",
//@formatter:off
"class Foo {\n" +
" boolean isXxx() {\n" +
" }\n" +
"}");
//@formatter:on

String contents =
//@formatter:off
"class Bar extends Foo {\n" +
" private boolean xxx\n" +
" void meth() {\n" +
" xxx\n" +
" this.xxx\n" +
" }\n" +
"}";
//@formatter:on

int offset = contents.indexOf("xxx", contents.indexOf("meth"));
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);

offset = contents.lastIndexOf("xxx");
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);
}

@Test
public void testGetterAndField15a() {
createJavaUnit("Foo",
//@formatter:off
"interface Foo {\n" +
" default boolean isXxx() {\n" +
" return false;" +
" }\n" +
"}");
//@formatter:on

String contents =
//@formatter:off
"class Bar implements Foo {\n" +
" private boolean xxx\n" +
" void meth() {\n" +
" xxx\n" +
" this.xxx\n" +
" }\n" +
"}";
//@formatter:on

int offset = contents.indexOf("xxx", contents.indexOf("meth"));
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);

offset = contents.lastIndexOf("xxx");
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);
}

@Test
public void testSetterAndField1() {
String contents =
Expand Down Expand Up @@ -617,6 +731,118 @@ public void testSetterAndField2() {
assertDeclaration(contents, offset, offset + 3, "Bar", "yyy", DeclarationKind.PROPERTY);
}

@Test // https://github.com/groovy/groovy-eclipse/issues/1130
public void testSetterAndField3() {
createUnit("Foo",
//@formatter:off
"class Foo {\n" +
" void setXxx(String xxx) {\n" +
" }\n" +
"}");
//@formatter:on

String contents =
//@formatter:off
"class Bar extends Foo {\n" +
" private String xxx\n" +
" void meth() {\n" +
" xxx = 'varX'\n" +
" this.xxx = 'propX'\n" +
" }\n" +
"}";
//@formatter:on

int offset = contents.indexOf("xxx", contents.indexOf("meth"));
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);

offset = contents.lastIndexOf("xxx");
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);
}

@Test
public void testSetterAndField3a() {
createJavaUnit("Foo",
//@formatter:off
"interface Foo {\n" +
" default void setXxx(String xxx) {\n" +
" }\n" +
"}");
//@formatter:on

String contents =
//@formatter:off
"class Bar implements Foo {\n" +
" private String xxx\n" +
" void meth() {\n" +
" xxx = 'varX'\n" +
" this.xxx = 'propX'\n" +
" }\n" +
"}";
//@formatter:on

int offset = contents.indexOf("xxx", contents.indexOf("meth"));
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);

offset = contents.lastIndexOf("xxx");
assertDeclaration(contents, offset, offset + 3, "Bar", "xxx", DeclarationKind.FIELD);
}

@Test
public void testGetterAndSetterOverloads1() {
createUnit("Foo",
//@formatter:off
"class Foo {\n" +
" def xxx\n" +
"}");
//@formatter:on

String contents =
//@formatter:off
"class Bar extends Foo {\n" +
" def getXxx() {\n" +
" this.xxx\n" + // recursive!
" }\n" +
" void setXxx(value) {\n" +
" this.xxx = value\n" + // recursive!
" }\n" +
"}";
//@formatter:on

int offset = contents.indexOf("xxx");
assertDeclaration(contents, offset, offset + 3, "Bar", "getXxx", DeclarationKind.METHOD);

offset = contents.lastIndexOf("xxx");
assertDeclaration(contents, offset, offset + 3, "Bar", "setXxx", DeclarationKind.METHOD);
}

@Test
public void testGetterAndSetterOverloads2() {
createUnit("Foo",
//@formatter:off
"class Foo {\n" +
" def xxx\n" +
"}");
//@formatter:on

String contents =
//@formatter:off
"class Bar extends Foo {\n" +
" def getXxx() {\n" +
" super.xxx\n" +
" }\n" +
" void setXxx(value) {\n" +
" super.xxx = value\n" +
" }\n" +
"}";
//@formatter:on

int offset = contents.indexOf("xxx");
assertDeclaration(contents, offset, offset + 3, "Foo", "xxx", DeclarationKind.PROPERTY);

offset = contents.lastIndexOf("xxx");
assertDeclaration(contents, offset, offset + 3, "Foo", "xxx", DeclarationKind.PROPERTY);
}

@Test
public void testLocalAndFieldWithSameName() {
String contents =
Expand Down
38 changes: 2 additions & 36 deletions ...tests.builder/src/org/eclipse/jdt/core/groovy/tests/search/FieldReferenceSearchTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,42 +178,8 @@ public void testFieldReferencesInClass4() {
"}\n");
}

@Test
public void testFieldReferencesInClass5() {
createUnit("other", "Other",
"class Third {\n" +
" private String xxx\n" +
" String getXxx() {\n" +
" xxx\n" +
" }\n" +
" void setXxx(String xxx) {\n" +
" this.xxx = xxx\n" +
" }\n" +
"}\n" +
"\n" +
"@groovy.transform.CompileStatic\n" +
"class Other {" +
" Other() {\n" +
" def t = new Third(xxx: 'abc')\n" +
" def m = t.&setXxx\n" +
" m('xyz')\n" +
" }\n" +
"}\n");

doTestForTwoFieldReferencesInClass(
"class Second extends First {\n" +
" int getXxx() {\n" +
" return this.xxx\n" +
" }\n" +
" def whatever\n" +
" void setXxx(int value) {\n" +
" this.xxx = value\n" +
" }\n" +
"}\n");
}

@Test // https://github.com/groovy/groovy-eclipse/issues/891
public void testFieldReferencesInClass6() {
public void testFieldReferencesInClass5() {
String contents =
"@groovy.transform.CompileStatic\n" +
"class Pogo {\n" +
Expand Down Expand Up @@ -241,7 +207,7 @@ public void testFieldReferencesInClass6() {
}

@Test // https://github.com/groovy/groovy-eclipse/issues/935
public void testFieldReferencesInClass7() {
public void testFieldReferencesInClass6() {
GroovyCompilationUnit pogo = createUnit("Pogo",
"class Pogo {\n" +
" boolean flag\n" +
Expand Down
2 changes: 1 addition & 1 deletion base/org.eclipse.jdt.groovy.core/src/org/eclipse/jdt/groovy/search/AccessorSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public static Stream<MethodNode> findAccessorMethodsForPropertyName(String name,
String methodName = kind.prefix + suffix;
methods = Stream.concat(methods, findAccessorMethodsForMethodName(methodName, declaringType, isCategory, kind));

// abstract types do not track undeclared abstract methods
// abstract types do not track undeclared abstract methods; concrete types do not track interface default methods
if (declaringType.isAbstract() || declaringType.isInterface() || GroovyUtils.implementsTrait(declaringType)) {
Set<ClassNode> faces = new LinkedHashSet<>();
VariableScope.findAllInterfaces(declaringType, faces, true);
Expand Down
34 changes: 22 additions & 12 deletions 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 @@ -16,10 +16,12 @@
package org.eclipse.jdt.groovy.search;

import static org.codehaus.groovy.runtime.DefaultGroovyMethods.last;
import static org.codehaus.groovy.runtime.DefaultGroovyMethods.unique;
import static org.eclipse.jdt.groovy.core.util.GroovyUtils.implementsTrait;

import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.LinkedHashSet;
import java.util.LinkedList;
import java.util.List;
Expand Down Expand Up @@ -618,8 +620,8 @@ protected ASTNode findDeclaration(final String name, final ClassNode declaringTy
}

// look for canonical accessor method
Optional<MethodNode> accessor = findPropertyAccessorMethod(name, declaringType, isLhsExpression, isStaticExpression, methodCallArgumentTypes);
if (accessor.filter(it -> !isSynthetic(it) && !(directFieldAccess && declaringType.equals(it.getDeclaringClass()))).isPresent()) {
Optional<MethodNode> accessor = findPropertyAccessorMethod(name, declaringType, isLhsExpression, isStaticExpression, methodCallArgumentTypes).filter(it -> !isSynthetic(it));
if (accessor.isPresent() && !directFieldAccess) {
return accessor.get();
}

Expand All @@ -633,17 +635,24 @@ protected ASTNode findDeclaration(final String name, final ClassNode declaringTy
property = Optional.ofNullable(type.redirect().<List<PropertyNode>>getNodeMetaData("trait.properties"))
.flatMap(list -> list.stream().filter(prop -> prop.getName().equals(name)).findFirst()).orElse(null);
}
if (isCompatible(property, isStaticExpression)) {
if (isCompatible(property, isStaticExpression) && (!accessor.isPresent() ||
directFieldAccess && declaringType.equals(property.getDeclaringClass()))) {
return property;
}
if (property != null) break;
}

// look for field
FieldNode field = declaringType.getField(name);
if (isCompatible(field, isStaticExpression)) {
if (isCompatible(field, isStaticExpression) && (!accessor.isPresent() ||
directFieldAccess && declaringType.equals(field.getDeclaringClass()))) {
return field;
}

if (accessor.isPresent()) {
return accessor.get();
}

typeHierarchy.clear();
VariableScope.findAllInterfaces(declaringType, typeHierarchy, true);

Expand All @@ -658,11 +667,6 @@ protected ASTNode findDeclaration(final String name, final ClassNode declaringTy
}
}

// look for static or synthetic accessor
if (accessor.isPresent()) {
return accessor.get();
}

// look for member in outer classes
if (getBaseDeclaringType(declaringType).getOuterClass() != null) {
// search only for static declarations if inner class is static
Expand Down Expand Up @@ -837,8 +841,7 @@ protected static ClassNode getBaseDeclaringType(final ClassNode declaringType) {

protected static List<MethodNode> getMethods(final String name, final ClassNode type) {
List<MethodNode> methods = type.getMethods(name);
List<MethodNode> traitMethods =
type.redirect().getNodeMetaData("trait.methods");
List<MethodNode> traitMethods = type.redirect().getNodeMetaData("trait.methods");
if (traitMethods != null) {
methods = new ArrayList<>(methods);
for (MethodNode method : traitMethods) {
Expand All @@ -847,7 +850,14 @@ protected static List<MethodNode> getMethods(final String name, final ClassNode
}
}
}
return methods;
return methods.size() <= 1 ? methods : unique(methods, Comparator.comparing(m -> {
StringBuilder sb = new StringBuilder();
for (Parameter p : m.getParameters()) {
sb.append(p.getType().getName());
sb.append(',');
}
return sb.toString();
}));
}

/**
Expand Down

0 comments on commit b006b64

Please sign in to comment.