Skip to content

Commit

Permalink
Fix for #1152: make static trait methods private in JDT model (Java 9+)
Browse files Browse the repository at this point in the history 
- copy them (without private modifier) to implementing class(es)
  • Loading branch information
@eric-milles
eric-milles committed Aug 4, 2020
1 parent e459ceb commit cdf2976
Show file tree
Hide file tree
Showing 14 changed files with 339 additions and 71 deletions.
98 changes: 98 additions & 0 deletions ...ore.tests.builder/src/org/eclipse/jdt/core/groovy/tests/search/TraitInferencingTests.java
View file

Large diffs are not rendered by default.

44 changes: 40 additions & 4 deletions ...vy.core.tests.compiler/src/org/eclipse/jdt/groovy/core/tests/basic/GroovySimpleTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -704,8 +704,8 @@ public void testStaticProperty8() {
} else {
runNegativeTest(sources,
"----------\n" +
"1. ERROR in Script.groovy (at line 5)\r\n" +
"\tthis({ -> g() + getF()})\r\n" +
"1. ERROR in Script.groovy (at line 5)\n" +
"\tthis({ -> g() + getF()})\n" +
"\t ^\n" +
"Groovy:Cannot reference 'g' before supertype constructor has been called.\n" +
"----------\n");
Expand Down Expand Up @@ -766,8 +766,8 @@ public void testStaticProperty8b() {
} else {
runNegativeTest(sources,
"----------\n" +
"1. ERROR in Script.groovy (at line 8)\r\n" +
"\tthis(new A(null).with { b = 42; return it })\r\n" +
"1. ERROR in Script.groovy (at line 8)\n" +
"\tthis(new A(null).with { b = 42; return it " + "})\n" +
"\t ^\n" +
"Groovy:Apparent variable 'b' was found in a static scope but doesn't refer to a local variable, static field or class.\n" +
"----------\n");
Expand Down Expand Up @@ -1966,6 +1966,42 @@ public void testOverriding_MissingAnnotation4() {
options);
}

@Test
public void testOverriding_StaticMethodHidesInstanceMethod() {
//@formatter:off
String[] sources = {
"Script.groovy",
"class C {\n" +
" def m() { 'C' }\n" +
"}\n" +
"class D extends C {\n" +
" static m() { 'D' }\n" +
"}\n" +
"print new D().m()\n",
};
//@formatter:on

runConformTest(sources, "D");
}

@Test
public void testOverriding_InstanceMethodCoversStaticMethod() {
//@formatter:off
String[] sources = {
"Script.groovy",
"class C {\n" +
" static m() { 'C' }\n" +
"}\n" +
"class D extends C {\n" +
" def m() { 'D' }\n" +
"}\n" +
"print new D().m()\n",
};
//@formatter:on

runConformTest(sources, "D");
}

@Test
public void testAbstractMethodWithBody1() {
//@formatter:off
Expand Down
116 changes: 71 additions & 45 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 @@ -1116,7 +1116,7 @@ public void testTraits44() {
}

@Test // public method of superclass overridden by static trait method
public void testTraits44a() {
public void testTraits45() {
//@formatter:off
String[] sources = {
"Script.groovy",
Expand All @@ -1127,23 +1127,17 @@ public void testTraits44a() {
" def m() { 'C' }\n" +
"}\n" +
"class D extends C implements T {\n" +
// T.m overrides C.m
"}\n" +
"print new D().m()\n",
};
//@formatter:on

runConformTest(sources, "T");
// runNegativeTest(sources,
// "----------\n" +
// "1. ERROR in Script.groovy (at line 7)\n" +
// "\tclass D extends C implements T {\n" +
// "\t ^\n" +
// "This static method cannot hide the instance method from C\n" +
// "----------\n");
}

@Test // protected method of superclass overridden by trait method
public void testTraits45() {
public void testTraits46() {
//@formatter:off
String[] sources = {
"Script.groovy",
Expand All @@ -1154,6 +1148,7 @@ public void testTraits45() {
" protected def m() { 'C' }\n" +
"}\n" +
"class D extends C implements T {\n" +
// T.m overrides C.m
"}\n" +
"print new D().m()\n",
};
Expand All @@ -1163,7 +1158,7 @@ public void testTraits45() {
}

@Test // package-private method of superclass overridden by trait method
public void testTraits46() {
public void testTraits47() {
//@formatter:off
String[] sources = {
"Script.groovy",
Expand All @@ -1175,6 +1170,7 @@ public void testTraits46() {
" def m() { 'C' }\n" +
"}\n" +
"class D extends C implements T {\n" +
// T.m overrides C.m
"}\n" +
"print new D().m()\n",
};
Expand All @@ -1183,30 +1179,7 @@ public void testTraits46() {
runConformTest(sources, "T");
}

@Test // Test protected method of superclass overriding by trait method - the same package
public void testTraits47() {
//@formatter:off
String[] sources = {
"Script.groovy",
"def myClass = new a.MyClass()\n" +
"print myClass.m()\n",

"Stuff.groovy",
"package a\n" +
"trait MyTrait {\n" +
" def m() { 'a' }\n" +
"}\n" +
"class MySuperClass {\n" +
" protected def m() { 'b' }\n" +
"}\n" +
"class MyClass extends MySuperClass implements MyTrait {}\n",
};
//@formatter:on

runConformTest(sources, "a");
}

@Test // Test protected method of superclass overriding by trait method - different packages
@Test // method of superclass overridden by (different package) trait method
public void testTraits48() {
//@formatter:off
String[] sources = {
Expand Down Expand Up @@ -1607,8 +1580,26 @@ public void testTraits64() {
runConformTest(sources, "T");
}

@Test @Ignore
@Test
public void testTraits65() {
//@formatter:off
String[] sources = {
"Script.groovy",
"trait T {\n" +
" static m() { 'T' }\n" +
"}\n" +
"class C implements T {\n" +
" static m() { 'C' }\n" +
"}\n" +
"print C.m()\n",
};
//@formatter:on

runConformTest(sources, "C");
}

@Test
public void testTraits66() {
//@formatter:off
String[] sources = {
"Main.java",
Expand All @@ -1631,7 +1622,7 @@ public void testTraits65() {
}

@Test
public void testTraits66() {
public void testTraits67() {
//@formatter:off
String[] sources = {
"Script.groovy",
Expand All @@ -1646,13 +1637,13 @@ public void testTraits66() {
}

@Test
public void testTraits67() {
public void testTraits68() {
//@formatter:off
String[] sources = {
"Main.java",
"public class Main {\n" +
" public static void main(String[] args) {\n" +
" System.out.prin(T.m());\n" +
" System.out.print(T.m());\n" +
" }\n" +
"}\n",

Expand All @@ -1663,13 +1654,48 @@ public void testTraits67() {
};
//@formatter:on

runNegativeTest(sources,
"----------\n" +
"1. ERROR in Main.java (at line 3)\n" +
"\tSystem.out.prin(T.m());\n" +
"\t ^\n" +
"The method m() is undefined for the type T\n" +
"----------\n");
if (isAtLeastJava(JDK9)) {
runNegativeTest(sources,
"----------\n" +
"1. ERROR in Main.java (at line 3)\n" +
"\tSystem.out.print(T.m());\n" +
"\t ^\n" +
"The method m() from the type T is not visible\n" +
"----------\n");
} else if (isAtLeastJava(JDK8)) { // TODO: This is not ideal:
runConformTest(sources, "", "java.lang.NoSuchMethodError: T.m()Ljava/lang/Object;");
} else {
runNegativeTest(sources,
"----------\n" +
"1. ERROR in Main.java (at line 3)\n" +
"\tSystem.out.print(T.m());\n" +
"\t ^^^^^\n" +
"Cannot make a static reference to the non-static method m() from the type T\n" +
"----------\n");
}
}

@Test
public void testTraits69() {
//@formatter:off
String[] sources = {
"Main.java",
"public class Main {\n" +
" public static void main(String[] args) {\n" +
" System.out.print(C.m());\n" +
" }\n" +
"}\n",

"C.groovy",
"trait T {\n" +
" static m() { 'T' }\n" +
"}\n" +
"class C implements T {\n" +
"}\n",
};
//@formatter:on

runConformTest(sources, "T");
}

@Test
Expand Down
42 changes: 31 additions & 11 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 @@ -25,7 +25,6 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Stream;

import org.codehaus.groovy.ast.ClassHelper;
import org.codehaus.groovy.ast.PropertyNode;
Expand Down Expand Up @@ -86,7 +85,8 @@ protected MethodBinding[] augmentMethodBindings(final MethodBinding[] methodBind
return methodBindings;
}

ReferenceBinding[] superInterfaces = Optional.ofNullable(typeBinding.superInterfaces).orElse(Binding.NO_SUPERINTERFACES);
ReferenceBinding[] superInterfaces = typeBinding.superInterfaces;
if (superInterfaces == null) superInterfaces = Binding.NO_SUPERINTERFACES;

boolean implementsGroovyLangObject = false;
for (ReferenceBinding face : superInterfaces) {
Expand Down Expand Up @@ -130,9 +130,7 @@ protected MethodBinding[] augmentMethodBindings(final MethodBinding[] methodBind
// create property accessors without resolving the types
if (referenceContext instanceof GroovyTypeDeclaration) {
for (PropertyNode property : ((GroovyTypeDeclaration) referenceContext).getClassNode().getProperties()) {
int modifiers = getModifiers(property);
if (Flags.isPackageDefault(modifiers)) continue;

int modifiers = getModifiers(property); if (Flags.isPackageDefault(modifiers)) continue;
String name = property.getName(), capitalizedName = MetaClassHelper.capitalize(name);

if (ClassHelper.boolean_TYPE.equals(property.getType())) {
Expand Down Expand Up @@ -160,11 +158,16 @@ protected MethodBinding[] augmentMethodBindings(final MethodBinding[] methodBind
if (traitHelper.isTrait(face)) {
ReferenceBinding helperBinding = traitHelper.getHelperBinding(face);
for (MethodBinding method : face.availableMethods()) {
if (!method.isPrivate() && !method.isStatic() && isNotActuallyAbstract(method, helperBinding)) {
if ((method.modifiers & ExtraCompilerModifiers.AccBlankFinal) != 0) { // restore finality
if (!method.isSynthetic() && isNotActuallyAbstract(method, helperBinding)) {
if ((method.modifiers & ExtraCompilerModifiers.AccModifierProblem) != 0) { // Java 7: +static, -abstract
method.modifiers ^= Flags.AccStatic | Flags.AccAbstract | ExtraCompilerModifiers.AccModifierProblem;
}
if ((method.modifiers & ExtraCompilerModifiers.AccBlankFinal) != 0) { // +final
method.modifiers ^= Flags.AccFinal | ExtraCompilerModifiers.AccBlankFinal;
}
traitMethods.put(getMethodAsString(method), method);
if (method.isPublic() || method.isStatic()) {
traitMethods.put(getMethodAsString(method), method);
}
}
}
}
Expand All @@ -182,25 +185,40 @@ protected MethodBinding[] augmentMethodBindings(final MethodBinding[] methodBind
}
for (MethodBinding method : methodBindings) {
if (!method.isConstructor()) {
canBeOverridden.remove(getMethodAsString(method));
String signature = getMethodAsString(method);
canBeOverridden.remove(signature);
traitMethods.remove(signature);
}
}

for (String key : canBeOverridden) {
MethodBinding method = traitMethods.remove(key);
if (method != null) {
// the trait method overrides a superclass method
method = new MethodBinding(method, typeBinding);
method.modifiers &= ~Flags.AccAbstract;
method.modifiers &= ~Flags.AccPrivate;
groovyMethods.add(method);
}
}

for (MethodBinding method : traitMethods.values()) {
method.modifiers &= ~Flags.AccAbstract;
if (!method.isStatic()) {
method.modifiers &= ~Flags.AccAbstract;
} else {
method = new MethodBinding(method, typeBinding);
method.modifiers &= ~Flags.AccPrivate;
groovyMethods.add(method);
}
}
}

return Stream.concat(Stream.of(methodBindings), groovyMethods.stream()).toArray(MethodBinding[]::new);
int m = methodBindings.length, n = m + groovyMethods.size();
MethodBinding[] methods = Arrays.copyOf(methodBindings, n);
for (int i = m, j = 0; i < n; i += 1, j += 1) {
methods[i] = groovyMethods.get(j);
}
return methods;
}

private int getModifiers(final PropertyNode property) {
Expand Down Expand Up @@ -432,6 +450,8 @@ protected void buildFieldsAndMethods() {
@Override
public boolean shouldReport(final int problem) {
switch (problem) {
case IProblem.CannotOverrideAStaticMethodWithAnInstanceMethod:
case IProblem.CannotHideAnInstanceMethodWithAStaticMethod:
case IProblem.EnumConstantMustImplementAbstractMethod:
case IProblem.AbstractMethodMustBeImplemented:
case IProblem.MissingValueForAnnotationMember:
Expand Down
Loading

0 comments on commit cdf2976

Please sign in to comment.