When duping Struct accessor methods, ensure they actually dup.

Partial fix for #3834 The main issue here is that we frequently return this for DynamicMethod#dup, rather than return a new instance, and then this gets modified by code expecting it to be something different. This breaks reflective code because many reflective methods will expect the methods in a module's method table to say their implementationClass is that module. The case in #3834 broke because the accessor methods in Struct have this "fake" dup logic, so when the struct class itself was duplicated, the methods got modified implementationClass = clone and seemed to "disappear" from the original. This is only a partial fix because it just fixes those accessors in Struct. The proper fix would be to make sure DynamicMethod#dup returns a real new object for all DynamicMethod (except "dead" ones like UndefinedMethod). This may also fix (or make easier to fix) issues like #2988, where a dup'ed module's methods do not reflect their new implementationClass. Specs pending.
jruby · Apr 28, 2016 · 74d5312 · 74d5312
1 parent 85212ad
commit 74d5312
Showing 1 changed file with 51 additions and 34 deletions.
diff --git a/core/src/main/java/org/jruby/RubyStruct.java b/core/src/main/java/org/jruby/RubyStruct.java
@@ -229,40 +229,8 @@ public static RubyClass newInstance(IRubyObject recv, IRubyObject[] args, Block
             final String memberName = args[i].asJavaString();
             // if we are storing a name as well, index is one too high for values
             final int index = (name == null && !nilName) ? i : i - 1;
-            newStruct.addMethod(memberName, new DynamicMethod(newStruct, Visibility.PUBLIC) {
-                @Override
-                public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args, Block block) {
-                    Arity.checkArgumentCount(context.runtime, name, args, 0, 0);
-                    return ((RubyStruct)self).get(index);
-                }
-
-                @Override
-                public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name) {
-                    return ((RubyStruct)self).get(index);
-                }
-
-                @Override
-                public DynamicMethod dup() {
-                    return this;
-                }
-            });
-            newStruct.addMethod(memberName + '=', new DynamicMethod(newStruct, Visibility.PUBLIC) {
-                @Override
-                public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args, Block block) {
-                    Arity.checkArgumentCount(context.runtime, name, args, 1, 1);
-                    return ((RubyStruct)self).set(args[0], index);
-                }
-
-                @Override
-                public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject arg) {
-                    return ((RubyStruct)self).set(arg, index);
-                }
-
-                @Override
-                public DynamicMethod dup() {
-                    return this;
-                }
-            });
+            newStruct.addMethod(memberName, new Accessor(newStruct, index));
+            newStruct.addMethod(memberName + '=', new Mutator(newStruct, index));
         }
 
         if (block.isGiven()) {
@@ -849,4 +817,53 @@ public IRubyObject initialize_copy(IRubyObject arg) {
         return this;
     }
 
+    private static class Accessor extends DynamicMethod {
+        private final int index;
+
+        public Accessor(RubyClass newStruct, int index) {
+            super(newStruct, Visibility.PUBLIC);
+            this.index = index;
+        }
+
+        @Override
+        public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args, Block block) {
+            Arity.checkArgumentCount(context.runtime, name, args, 0, 0);
+            return ((RubyStruct)self).get(index);
+        }
+
+        @Override
+        public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name) {
+            return ((RubyStruct)self).get(index);
+        }
+
+        @Override
+        public DynamicMethod dup() {
+            return new Accessor((RubyClass) getImplementationClass(), index);
+        }
+    }
+
+    private static class Mutator extends DynamicMethod {
+        private final int index;
+
+        public Mutator(RubyClass newStruct, int index) {
+            super(newStruct, Visibility.PUBLIC);
+            this.index = index;
+        }
+
+        @Override
+        public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args, Block block) {
+            Arity.checkArgumentCount(context.runtime, name, args, 1, 1);
+            return ((RubyStruct)self).set(args[0], index);
+        }
+
+        @Override
+        public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject arg) {
+            return ((RubyStruct)self).set(arg, index);
+        }
+
+        @Override
+        public DynamicMethod dup() {
+            return new Accessor((RubyClass) getImplementationClass(), index);
+        }
+    }
 }