You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "Paul King (JIRA)" <ji...@apache.org> on 2019/04/30 09:10:00 UTC
[jira] [Resolved] (GROOVY-8651) Method override weaker access check
does not fully account for package-private visibility
[ https://issues.apache.org/jira/browse/GROOVY-8651?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Paul King resolved GROOVY-8651.
-------------------------------
Resolution: Fixed
Assignee: Paul King
Fix Version/s: 2.5.7
3.0.0-beta-1
Well spotted. Some of that code was written before @PackageScope was introduced and has obviously never been updated.
> 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
> Assignee: Paul King
> Priority: Minor
> Fix For: 3.0.0-beta-1, 2.5.7
>
>
> 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)