Introduce a DEFAULT visibility to indicate unspecified

In jruby#8146 we saw that DelegateClass can be broken by JRuby extensions that do not explicitly set their initialize methods to Visibility.PRIVATE. This is an error-prone requirement as detailed in jruby#8210, since there's no indication that initialize is special and should always be private. This patch introduces a "DEFAULT" visibility to be the default for the annotation. When binding methods using annotation processing, the DEFAULT visibility will translate itself to PRIVATE for the initialize method and PUBLIC for other cases. This ensures that initialize is always set to PRIVATE if no other visibility is specified, avoiding the bug. If initialize is set to some other visibility, it will keep that visibility. Fixes jruby#8210
headius · Apr 25, 2024 · 9317c75 · 9317c75
1 parent e50cdc1
commit 9317c75
Show file tree

Hide file tree

Showing 6 changed files with 24 additions and 9 deletions.
diff --git a/core/src/main/java/org/jruby/anno/AnnotationBinder.java b/core/src/main/java/org/jruby/anno/AnnotationBinder.java
@@ -330,7 +330,7 @@ public void processMethodDeclaration(ExecutableElement method) {
             String implClass = anno.meta() ? "singletonClass" : "cls";
 
             String baseName = getBaseName(anno.name(), method);
-            out.println("        javaMethod = new " + annotatedBindingName + "(" + implClass + ", Visibility." + anno.visibility() + ", \"" + baseName + "\");");
+            out.println("        javaMethod = new " + annotatedBindingName + "(" + implClass + ", Visibility." + anno.visibility().getDefaultVisibilityFor(baseName) + ", \"" + baseName + "\");");
             out.println("        populateMethod(javaMethod, " +
                     join(AnnotationHelper.getArityValue(anno, actualRequired),
                             quote(method.getSimpleName()),
@@ -367,7 +367,7 @@ public void processMethodDeclarationMulti(ExecutableElement method) {
             String implClass = anno.meta() ? "singletonClass" : "cls";
 
             String baseName = getBaseName(anno.name(), method);
-            out.println("        javaMethod = new " + annotatedBindingName + "(" + implClass + ", Visibility." + anno.visibility() + ", \"" + baseName + "\");");
+            out.println("        javaMethod = new " + annotatedBindingName + "(" + implClass + ", Visibility." + anno.visibility().getDefaultVisibilityFor(baseName) + ", \"" + baseName + "\");");
             out.println("        populateMethod(javaMethod, " +
                         join(-1,
                                 quote(method.getSimpleName()),

diff --git a/core/src/main/java/org/jruby/anno/IndyBinder.java b/core/src/main/java/org/jruby/anno/IndyBinder.java
@@ -392,8 +392,9 @@ public void processMethodDeclarationMulti(List<ExecutableElement> methods) {
         mv.dup();
 
         mv.aload(implClass);
-        mv.getstatic(p(Visibility.class), anno.visibility().name(), ci(Visibility.class));
-        mv.ldc(AnnotationBinder.getBaseName(anno.name(), methods.get(0)));
+        String baseName = AnnotationBinder.getBaseName(anno.name(), methods.get(0));
+        mv.getstatic(p(Visibility.class), anno.visibility().getDefaultVisibilityFor(baseName).name(), ci(Visibility.class));
+        mv.ldc(baseName);
         mv.ldc(encodeSignature(0, 0, 0, 0, 0, true, false));
         mv.ldc(true);
         mv.ldc(anno.notImplemented());

diff --git a/core/src/main/java/org/jruby/anno/JRubyMethod.java b/core/src/main/java/org/jruby/anno/JRubyMethod.java
@@ -81,7 +81,7 @@
     /**
      * The visibility of this method.
      */
-    Visibility visibility() default Visibility.PUBLIC;
+    Visibility visibility() default Visibility.DEFAULT;
 
     /**
      * What, if anything, method reads from caller's frame

diff --git a/core/src/main/java/org/jruby/internal/runtime/methods/InvocationMethodFactory.java b/core/src/main/java/org/jruby/internal/runtime/methods/InvocationMethodFactory.java
@@ -357,13 +357,13 @@ public JavaMethod constructJavaMethod(RubyModule implementationClass, JavaMethod
             // Old constructor with no name, use thread-local to pass it.
             JavaMethod.NAME_PASSER.set(name);
             try {
-                ic = (JavaMethod) c.getConstructor(RubyModule_and_Visibility).newInstance(implementationClass, desc.anno.visibility());
+                ic = (JavaMethod) c.getConstructor(RubyModule_and_Visibility).newInstance(implementationClass, desc.anno.visibility().getDefaultVisibilityFor(name));
             } finally {
                 JavaMethod.NAME_PASSER.remove();
             }
         } else {
             // New constructor with name.
-            ic = (JavaMethod) c.getConstructor(RubyModule_and_Visibility_and_Name).newInstance(implementationClass, desc.anno.visibility(), name);
+            ic = (JavaMethod) c.getConstructor(RubyModule_and_Visibility_and_Name).newInstance(implementationClass, desc.anno.visibility().getDefaultVisibilityFor(name), name);
         }
         return ic;
     }

diff --git a/core/src/main/java/org/jruby/internal/runtime/methods/InvokeDynamicMethodFactory.java b/core/src/main/java/org/jruby/internal/runtime/methods/InvokeDynamicMethodFactory.java
@@ -120,7 +120,7 @@ public DynamicMethod getAnnotatedMethod(final RubyModule implementationClass, fi
 
         return new HandleMethod(
                 implementationClass,
-                desc1.anno.visibility(),
+                desc1.anno.visibility().getDefaultVisibilityFor(desc1.name),
                 desc1.name,
                 (min == max) ?
                         org.jruby.runtime.Signature.from(min, 0, 0, 0, 0, org.jruby.runtime.Signature.Rest.NONE, -1).encode() :

diff --git a/core/src/main/java/org/jruby/runtime/Visibility.java b/core/src/main/java/org/jruby/runtime/Visibility.java
@@ -27,7 +27,7 @@
 package org.jruby.runtime;
 
 public enum Visibility {
-    PUBLIC, PROTECTED, PRIVATE, MODULE_FUNCTION, UNDEFINED;
+    PUBLIC, PROTECTED, PRIVATE, MODULE_FUNCTION, UNDEFINED, DEFAULT;
     private static final Visibility[] VALUES = values();
 
     public boolean isPublic() {
@@ -49,4 +49,18 @@ public boolean isModuleFunction() {
     public static Visibility[] getValues() {
         return VALUES;
     }
+
+    public Visibility getDefaultVisibilityFor(String name) {
+        switch (this) {
+            case DEFAULT:
+                switch (name) {
+                    case "initialize":
+                    case "initialize_copy":
+                       return PRIVATE;
+                }
+                return PUBLIC;
+            default:
+                return this;
+        }
+    }
 }