Skip to content

Commit

Permalink
Fix for #1158: put traits first in reverse declared order
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Sep 12, 2020
1 parent ad34e58 commit 5083add
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 35 deletions.
35 changes: 27 additions & 8 deletions ...ore.tests.builder/src/org/eclipse/jdt/core/groovy/tests/search/TraitInferencingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,27 @@ public void testProperty12() {
assertExprType(source, "setNumber", "java.lang.Void");
}

@Test // GROOVY-9255
@Test
public void testProperty13() {
//@formatter:off
String source =
"trait A {\n" +
" Number number\n" +
"}\n" +
"trait B {\n" +
" Number number\n" +
"}\n" +
"class C implements A, B {\n" +
" def n = number\n" +
"}\n";
//@formatter:on

assertDeclType(source, "number", "B");
assertExprType(source, "number", "java.lang.Number");
}

@Test // GROOVY-9255
public void testProperty14() {
//@formatter:off
String source =
"trait T {\n" +
Expand All @@ -252,7 +271,7 @@ public void testProperty13() {
}

@Test
public void testProperty14() {
public void testProperty15() {
//@formatter:off
String source =
"trait T {\n" +
Expand All @@ -270,7 +289,7 @@ public void testProperty14() {
}

@Test
public void testProperty15() {
public void testProperty16() {
createUnit("T",
"trait T {\n" +
" Number number\n" +
Expand All @@ -290,7 +309,7 @@ public void testProperty15() {
}

@Test
public void testProperty16() {
public void testProperty17() {
createUnit("T",
"trait T {\n" +
" Number number\n" +
Expand All @@ -310,7 +329,7 @@ public void testProperty16() {
}

@Test
public void testProperty17() {
public void testProperty18() {
createUnit("T",
"trait T {\n" +
" Number number\n" +
Expand All @@ -330,7 +349,7 @@ public void testProperty17() {
}

@Test
public void testProperty18() {
public void testProperty19() {
createUnit("T",
"trait T {\n" +
" Number number\n" +
Expand Down Expand Up @@ -388,7 +407,7 @@ public void testPublicMethod2() {
"}\n";
//@formatter:on

assertDeclType(contents, "method", "A");
assertDeclType(contents, "method", "B");
assertExprType(contents, "method", "java.lang.Void");
}

Expand Down Expand Up @@ -667,7 +686,7 @@ public void testPrivateMethod2() {
"}";
//@formatter:on

assertDeclType(contents, "method", "A");
assertDeclType(contents, "method", "B");
assertExprType(contents, "method", "java.lang.Void");
}

Expand Down
41 changes: 30 additions & 11 deletions ...t.groovy.core.tests.compiler/src/org/eclipse/jdt/groovy/core/tests/basic/TraitsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,18 +441,38 @@ public void testTraits17() {
String[] sources = {
"Script.groovy",
"trait A {\n" +
" String exec() { 'A' }\n" +
" String getIdentity() { 'A' }\n" +
"}\n" +
"trait B {\n" +
" String exec() { 'B' }\n" +
" String getIdentity() { 'B' }\n" +
"}\n" +
"class C implements B, A {}\n" +
"def c = new C()\n" +
"print c.exec()\n",
"class C implements A, B {\n" +
"}\n" +
"print new C().getIdentity()\n",
};
//@formatter:on

runConformTest(sources, "A");
runConformTest(sources, "B");
}

@Test
public void testTraits17a() {
//@formatter:off
String[] sources = {
"Script.groovy",
"trait A {\n" +
" final String identity = 'A'\n" +
"}\n" +
"trait B {\n" +
" final String identity = 'B'\n" +
"}\n" +
"class C implements A, B {\n" +
"}\n" +
"print new C().getIdentity()\n",
};
//@formatter:on

runConformTest(sources, "B");
}

@Test // Multiple inheritance conflicts - User conflict resolution
Expand All @@ -461,16 +481,15 @@ public void testTraits18() {
String[] sources = {
"Script.groovy",
"trait A {\n" +
" String exec() { 'A' }\n" +
" String getIdentity() { 'A' }\n" +
"}\n" +
"trait B {\n" +
" String exec() { 'B' }\n" +
" String getIdentity() { 'B' }\n" +
"}\n" +
"class C implements A, B {\n" +
" String exec() { A.super.exec() }\n" +
" String getIdentity() { A.super.getIdentity() }\n" +
"}\n" +
"def c = new C()\n" +
"print c.exec()\n",
"print new C().getIdentity()\n",
};
//@formatter:on

Expand Down
10 changes: 6 additions & 4 deletions ...e.jdt.groovy.core/src/org/codehaus/jdt/groovy/internal/compiler/ast/GroovyClassScope.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -76,8 +77,9 @@ protected MethodBinding[] augmentMethodBindings(final MethodBinding[] methodBind
return methodBindings;
}

ReferenceBinding[] superInterfaces = typeBinding.superInterfaces;
ReferenceBinding[] superInterfaces = typeBinding.superInterfaces();
if (superInterfaces == null) superInterfaces = Binding.NO_SUPERINTERFACES;
else if (superInterfaces.length > 1) Collections.reverse(Arrays.asList(superInterfaces));

boolean implementsGroovyObject = false;
for (ReferenceBinding face : superInterfaces) {
Expand Down Expand Up @@ -112,7 +114,7 @@ protected MethodBinding[] augmentMethodBindings(final MethodBinding[] methodBind
.ifPresent(groovyMethods::add);
createMethod("setProperty", new TypeBinding[] {bindingJLS, bindingJLO}, TypeBinding.VOID, methodBindings)
.ifPresent(groovyMethods::add);
createMethod("getMetaClass", Binding.NO_TYPES, bindingGLM, methodBindings)
createMethod("getMetaClass", Binding.NO_TYPES, bindingGLM, methodBindings) // TODO: @java.beans.Transient
.ifPresent(groovyMethods::add);
createMethod("setMetaClass", new TypeBinding[] {bindingGLM}, TypeBinding.VOID, methodBindings)
.ifPresent(groovyMethods::add);
Expand All @@ -125,7 +127,7 @@ protected MethodBinding[] augmentMethodBindings(final MethodBinding[] methodBind
if (Flags.isPackageDefault(modifiers)) continue;
String capitalizedName = MetaClassHelper.capitalize(property.getName());

if (ClassHelper.boolean_TYPE.equals(property.getType())) {
if (property.getType().equals(ClassHelper.boolean_TYPE)) {
createGetterMethod(property, "is" + capitalizedName, modifiers, methodBindings)
.ifPresent(binding -> {
groovyMethods.add(binding);
Expand Down Expand Up @@ -158,7 +160,7 @@ protected MethodBinding[] augmentMethodBindings(final MethodBinding[] methodBind
method.modifiers ^= Flags.AccFinal | ExtraCompilerModifiers.AccBlankFinal;
}
if (method.isPublic() || method.isStatic()) {
traitMethods.put(getMethodAsString(method), method);
traitMethods.putIfAbsent(getMethodAsString(method), method);
}
}
}
Expand Down
34 changes: 22 additions & 12 deletions base/org.eclipse.jdt.groovy.core/src/org/eclipse/jdt/groovy/search/VariableScope.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Deque;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.HashSet;
Expand Down Expand Up @@ -79,6 +80,7 @@
import org.codehaus.groovy.runtime.DefaultGroovyMethods;
import org.codehaus.groovy.runtime.DefaultGroovyStaticMethods;
import org.codehaus.groovy.syntax.Token;
import org.codehaus.groovy.transform.trait.Traits;
import org.codehaus.jdt.groovy.internal.compiler.GroovyClassLoaderFactory.GrapeAwareGroovyClassLoader;
import org.codehaus.jdt.groovy.internal.compiler.ast.JDTMethodNode;
import org.eclipse.core.runtime.Assert;
Expand Down Expand Up @@ -991,33 +993,41 @@ public void remove() {

/**
* Finds all interfaces implemented by {@code type} (including itself, if it
* is an interface). The ordering is that the interfaces closest to type are
* first (in declared order) and then interfaces declared on super interfaces
* occur (if they are not duplicates).
* is an interface). The ordering is that the interfaces closest to type are
* first (in declared order with traits reversed) and then any non-duplicate
* interfaces declared on super interfaces.
*
* @param allInterfaces an accumulator set that will ensure that each interface exists at most once and in a predictible order
* @param useResolved whether or not to use the resolved interfaces
*/
public static void findAllInterfaces(ClassNode type, Set<ClassNode> allInterfaces, boolean useResolved) {
public static void findAllInterfaces(ClassNode type, final Set<ClassNode> allInterfaces, final boolean useResolved) {
if (!useResolved) type = type.redirect();
boolean isInterface = type.isInterface();
if (!isInterface || !allInterfaces.contains(type)) {
if (isInterface) {
allInterfaces.add(type);
}
if (!isInterface || allInterfaces.add(type)) {
// Urrrgh...I don't like this.
// Groovy compiler has a different notion of 'resolved' than we do here.
// Groovy compiler considers a resolved ClassNode one that has no redirect.
// However, we consider a ClassNode to be resolved if its type parameters are resolved.
ClassNode[] faces = !useResolved ? type.getInterfaces() : type.getUnresolvedInterfaces();
if (faces != null) {
for (ClassNode face : faces) {
ClassNode[] interfaces = !useResolved ? type.getInterfaces() : type.getUnresolvedInterfaces();
if (interfaces != null && interfaces.length > 0) {
// put traits first in reverse declared order
Deque<ClassNode> deque = new LinkedList<>();
for (ClassNode face : interfaces) {
if (Traits.isTrait(face)) {
deque.addFirst(face);
} else {
deque.addLast(face);
}
}

for (ClassNode face : deque) {
findAllInterfaces(face, allInterfaces, useResolved);
}
}

if (!isInterface) {
ClassNode superType = type.getSuperClass();
if (superType != null && !OBJECT_CLASS_NODE.equals(superType)) {
if (superType != null && !superType.equals(OBJECT_CLASS_NODE)) {
findAllInterfaces(superType, allInterfaces, useResolved);
}
}
Expand Down

0 comments on commit 5083add

Please sign in to comment.