Eric Milles created GROOVY-8651:
-----------------------------------
Summary: Method override weaker access check does not fully
account for package-private visibility
Key: GROOVY-8651
URL: https://issues.apache.org/jira/browse/GROOVY-8651
Project: Groovy
Issue Type: Bug
Reporter: Eric Milles
If override method is package-private (via {{PackageScope}}) and super method
is public (or protected?), no error is produced. If protected is indeed more
visible than package-private, there may be some additional cases uncovered
below.
org.codehaus.groovy.classgen.ClassCompletionVerifier:
{code:java}
private void checkMethodForWeakerAccessPrivileges(MethodNode mn, ClassNode
cn) {
if (mn.isPublic()) return;
Parameter[] params = mn.getParameters();
for (MethodNode superMethod :
cn.getSuperClass().getMethods(mn.getName())) {
Parameter[] superParams = superMethod.getParameters();
if (!hasEqualParameterTypes(params, superParams)) continue;
if ((mn.isPrivate() && !superMethod.isPrivate()) ||
(mn.isProtected() && superMethod.isPublic())) {
addWeakerAccessError(cn, mn, params, superMethod);
return;
}
}
}
{code}
I think that this is the proper set of checks:
{code:java}
if ((mn.isPrivate() && !superMethod.isPrivate()) ||
(mn.isProtected() && !superMethod.isProtected() &&
!superMethod.isPrivate()) ||
(!mn.isPrivate() && !mn.isProtected() && !mn.isPublic() &&
(superMethod.isPublic() || superMethod.isProtected()))) {
addWeakerAccessError(cn, mn, params, superMethod);
...
// also the error message need some change to indicate default/package-private
private void addWeakerAccessError(ClassNode cn, MethodNode method,
Parameter[] parameters, MethodNode superMethod) {
StringBuilder msg = new StringBuilder();
msg.append(method.getName());
appendParamsDescription(parameters, msg);
msg.append(" in ");
msg.append(cn.getName());
msg.append(" cannot override ");
msg.append(superMethod.getName());
msg.append(" in ");
msg.append(superMethod.getDeclaringClass().getName());
msg.append("; attempting to assign weaker access privileges; was ");
// GRECLIPSE edit
//msg.append(superMethod.isPublic() ? "public" : "protected");
msg.append(superMethod.isPublic() ? "public" :
(superMethod.isProtected() ? "protected" : "package-private"));
// GRECLIPSE end
addError(msg.toString(), method);
}
{code}
For these tests, 1b and 2a are failing:
{code:java}
@Test
public void testOverriding_ReducedVisibility1() {
runNegativeTest(new String[] {
"Bar.groovy",
"class Bar { public def baz() {} }\n",
"Foo.groovy",
"class Foo extends Bar { private def baz() {}\n }\n",
}, "----------\n" +
"1. ERROR in Foo.groovy (at line 1)\n" +
"\tclass Foo extends Bar { private def baz() {}\n" +
"\t ^^^^^^^^^^^^^^^^^^^^\n" +
"Groovy:baz() in Foo cannot override baz in Bar; attempting to
assign weaker access privileges; was public\n" +
"----------\n");
}
@Test
public void testOverriding_ReducedVisibility1a() {
runNegativeTest(new String[] {
"Bar.groovy",
"class Bar { public def baz() {} }\n",
"Foo.groovy",
"class Foo extends Bar { protected def baz() {}\n }\n",
}, "----------\n" +
"1. ERROR in Foo.groovy (at line 1)\n" +
"\tclass Foo extends Bar { protected def baz() {}\n" +
"\t ^^^^^^^^^^^^^^^^^^^^^^\n" +
"Groovy:baz() in Foo cannot override baz in Bar; attempting to
assign weaker access privileges; was public\n" +
"----------\n");
}
@Test
public void testOverriding_ReducedVisibility1b() {
runNegativeTest(new String[] {
"Bar.groovy",
"class Bar { public def baz() {} }\n",
"Foo.groovy",
"class Foo extends Bar { @groovy.transform.PackageScope def baz()
{}\n }\n",
}, "----------\n" +
"1. ERROR in Foo.groovy (at line 1)\n" +
"\tclass Foo extends Bar { @groovy.transform.PackageScope def baz()
{}\n" +
"\t
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" +
"Groovy:baz() in Foo cannot override baz in Bar; attempting to
assign weaker access privileges; was public\n" +
"----------\n");
}
@Test
public void testOverriding_ReducedVisibility2() {
runNegativeTest(new String[] {
"Bar.groovy",
"class Bar { protected def baz() {} }\n",
"Foo.groovy",
"class Foo extends Bar { private def baz() {}\n }\n",
}, "----------\n" +
"1. ERROR in Foo.groovy (at line 1)\n" +
"\tclass Foo extends Bar { private def baz() {}\n" +
"\t ^^^^^^^^^^^^^^^^^^^^\n" +
"Groovy:baz() in Foo cannot override baz in Bar; attempting to
assign weaker access privileges; was protected\n" +
"----------\n");
}
@Test
public void testOverriding_ReducedVisibility2a() {
runNegativeTest(new String[] {
"Bar.groovy",
"class Bar { protected def baz() {} }\n",
"Foo.groovy",
"class Foo extends Bar { @groovy.transform.PackageScope def baz()
{}\n }\n",
}, "----------\n" +
"1. ERROR in Foo.groovy (at line 1)\n" +
"\tclass Foo extends Bar { @groovy.transform.PackageScope def baz()
{}\n" +
"\t
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" +
"Groovy:baz() in Foo cannot override baz in Bar; attempting to
assign weaker access privileges; was protected\n" +
"----------\n");
}
@Test
public void testOverriding_ReducedVisibility3() {
runNegativeTest(new String[] {
"Bar.groovy",
"class Bar { @groovy.transform.PackageScope def baz() {} }\n",
"Foo.groovy",
"class Foo extends Bar { private def baz() {}\n }\n",
}, "----------\n" +
"1. ERROR in Foo.groovy (at line 1)\n" +
"\tclass Foo extends Bar { private def baz() {}\n" +
"\t ^^^^^^^^^^^^^^^^^^^^\n" +
"Groovy:baz() in Foo cannot override baz in Bar; attempting to
assign weaker access privileges; was protected\n" +
"----------\n");
}
@Test
public void testOverriding_ReducedVisibility3a() {
runNegativeTest(new String[] {
"Bar.java",
"public class Bar { Object baz() { return null; } }\n",
"Foo.groovy",
"class Foo extends Bar { private def baz() {}\n }\n",
}, "----------\n" +
"1. ERROR in Foo.groovy (at line 1)\n" +
"\tclass Foo extends Bar { private def baz() {}\n" +
"\t ^^^^^^^^^^^^^^^^^^^^\n" +
"Groovy:baz() in Foo cannot override baz in Bar; attempting to
assign weaker access privileges; was package-private\n" +
"----------\n");
}
{code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
