Uploaded image for project: 'Groovy'
  1. Groovy
  2. GROOVY-8651

Method override weaker access check does not fully account for package-private visibility

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0.0-beta-1, 2.5.7
    • Component/s: None
    • Labels:
      None

      Description

      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:

          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;
                  }
              }
          }
      

      I think that this is the proper set of checks:

                  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);
          }
      

      For these tests, 1b and 2a are failing:

          @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");
          }
      

        Attachments

          Activity

            People

            • Assignee:
              paulk Paul King
              Reporter:
              emilles Eric Milles
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: