Improve refinements implementation and enable MRI test.

jruby · Aug 26, 2015 · f3b41ae · f3b41ae
1 parent 4ef1514
commit f3b41ae
Showing 8 changed files with 364 additions and 31 deletions.
diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
@@ -68,6 +68,7 @@
 import org.jruby.runtime.BlockBody;
 import org.jruby.runtime.CallSite;
 import org.jruby.runtime.ClassIndex;
+import org.jruby.runtime.DynamicScope;
 import org.jruby.runtime.Helpers;
 import org.jruby.runtime.IRBlockBody;
 import org.jruby.runtime.MethodFactory;
@@ -105,6 +106,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.IdentityHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -558,13 +560,13 @@ private String calculateAnonymousName() {
     }
 
 
-    @JRubyMethod(name = "refine", required = 1)
+    @JRubyMethod(name = "refine", required = 1, reads = SCOPE)
     public IRubyObject refine(ThreadContext context, IRubyObject classArg, Block block) {
         if (!block.isGiven()) throw context.runtime.newArgumentError("no block given");
         if (block.isEscaped()) throw context.runtime.newArgumentError("can't pass a Proc as a block to Module#refine");
         if (!(classArg instanceof RubyClass)) throw context.runtime.newTypeError(classArg, context.runtime.getClassClass());
-        if (refinements == Collections.EMPTY_MAP) refinements = new HashMap<>();
-        if (activatedRefinements == Collections.EMPTY_MAP) activatedRefinements = new HashMap<>();
+        if (refinements == Collections.EMPTY_MAP) refinements = new IdentityHashMap<>();
+        if (activatedRefinements == Collections.EMPTY_MAP) activatedRefinements = new IdentityHashMap<>();
 
         RubyClass classWeAreRefining = (RubyClass) classArg;
         RubyModule refinement = refinements.get(classWeAreRefining);
@@ -582,7 +584,8 @@ public IRubyObject refine(ThreadContext context, IRubyObject classArg, Block blo
     }
 
     private RubyModule createNewRefinedModule(ThreadContext context, RubyClass classWeAreRefining) {
-        RubyModule newRefinement = new RubyModule(context.runtime, classWeAreRefining);
+        RubyModule newRefinement = new RubyModule(context.runtime);
+        newRefinement.setSuperClass(classWeAreRefining);
         newRefinement.setFlag(REFINED_MODULE_F, true);
         newRefinement.setFlag(RubyObject.USER7_F, false); // Refinement modules should not do implementer check
         newRefinement.refinedClass = classWeAreRefining;
@@ -604,14 +607,14 @@ private void yieldRefineBlock(ThreadContext context, RubyModule refinement, Bloc
     // 3. refinement is already in the refinementwrapper so we do not need to add it to the wrapper again: return null
     private RubyClass getAlreadyActivatedRefinementWrapper(RubyClass classWeAreRefining, RubyModule refinement) {
         // We have already encountered at least one refine on this class.  Return that wrapper.
-        RubyClass moduleWrapperForRefinment = activatedRefinements.get(classWeAreRefining);
-        if (moduleWrapperForRefinment == null) return classWeAreRefining;
+        RubyClass moduleWrapperForRefinement = activatedRefinements.get(classWeAreRefining);
+        if (moduleWrapperForRefinement == null) return classWeAreRefining;
 
-        for (RubyModule c = moduleWrapperForRefinment; c != null && c.isIncluded(); c = c.getSuperClass()) {
+        for (RubyModule c = moduleWrapperForRefinement; c != null && c.isIncluded(); c = c.getSuperClass()) {
             if (c.getNonIncludedClass() == refinement) return null;
         }
 
-        return moduleWrapperForRefinment;
+        return moduleWrapperForRefinement;
     }
 
     /*
@@ -621,24 +624,34 @@ private RubyClass getAlreadyActivatedRefinementWrapper(RubyClass classWeAreRefin
      */
     // MRI: add_activated_refinement
     private void addActivatedRefinement(ThreadContext context, RubyClass classWeAreRefining, RubyModule refinement) {
-        RubyClass superClass = getAlreadyActivatedRefinementWrapper(classWeAreRefining, refinement);
-        if (superClass == null) return; // already been refined and added to refinementwrapper
-
+//        RubyClass superClass = getAlreadyActivatedRefinementWrapper(classWeAreRefining, refinement);
+//        if (superClass == null) return; // already been refined and added to refinementwrapper
+        RubyClass superClass = null;
+        RubyClass c = activatedRefinements.get(classWeAreRefining);
+        if (c != null) {
+            superClass = c;
+            while (c != null && c.isIncluded()) {
+                if (((IncludedModuleWrapper)c).getNonIncludedClass() == refinement) {
+            		/* already used refinement */
+                    return;
+                }
+                c = c.getSuperClass();
+            }
+        }
         refinement.setFlag(IS_OVERLAID_F, true);
         IncludedModuleWrapper iclass = new IncludedModuleWrapper(context.runtime, superClass, refinement);
-        RubyClass c = iclass;
+        c = iclass;
         c.refinedClass = classWeAreRefining;
         for (refinement = refinement.getSuperClass(); refinement != null; refinement = refinement.getSuperClass()) {
             refinement.setFlag(IS_OVERLAID_F, true);
-            RubyClass superClazz = c.getSuperClass();
-            c.setModuleSuperClass(new IncludedModuleWrapper(context.runtime, c.getSuperClass(), refinement));
+            c.setSuperClass(new IncludedModuleWrapper(context.runtime, c.getSuperClass(), refinement));
+            c = c.getSuperClass();
             c.refinedClass = classWeAreRefining;
-            c = superClazz;
         }
         activatedRefinements.put(classWeAreRefining, iclass);
     }
 
-    @JRubyMethod(name = "using", required = 1, frame = true)
+    @JRubyMethod(name = "using", required = 1, frame = true, reads = SCOPE)
     public IRubyObject using(ThreadContext context, IRubyObject refinedModule) {
         if (context.getFrameSelf() != this) throw context.runtime.newRuntimeError("Module#using is not called on self");
         // FIXME: This is a lame test and I am unsure it works with JIT'd bodies...
@@ -647,7 +660,9 @@ public IRubyObject using(ThreadContext context, IRubyObject refinedModule) {
         }
 
         // I pass the cref even though I don't need to so that the concept is simpler to read
-        usingModule(context, this, refinedModule);
+        StaticScope staticScope = context.getCurrentStaticScope();
+        RubyModule overlayModule = staticScope.getOverlayModule(context);
+        usingModule(context, overlayModule, refinedModule);
 
         return this;
     }

diff --git a/core/src/main/java/org/jruby/RubyNumeric.java b/core/src/main/java/org/jruby/RubyNumeric.java
@@ -583,8 +583,8 @@ public RubyNumeric asNumeric() {
      *
      */
     @JRubyMethod(name = "singleton_method_added")
-    public IRubyObject sadded(IRubyObject name) {
-        throw getRuntime().newTypeError("can't define singleton method " + name + " for " + getType().getName());
+    public static IRubyObject sadded(IRubyObject self, IRubyObject name) {
+        throw self.getRuntime().newTypeError("can't define singleton method " + name + " for " + self.getType().getName());
     }
 
     /** num_init_copy

diff --git a/core/src/main/java/org/jruby/TopSelfFactory.java b/core/src/main/java/org/jruby/TopSelfFactory.java
@@ -109,7 +109,7 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz
             @Override
             public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args) {
                 Arity.checkArgumentCount(context.runtime, args, 1, 1);
-                RubyModule cref = context.getCurrentStaticScope().getModule();
+                RubyModule cref = context.getCurrentStaticScope().getOverlayModule(context);
                 // unclear what the equivalent check would be for us
 //                rb_control_frame_t * prev_cfp = previous_frame(GET_THREAD());
 //

diff --git a/core/src/main/java/org/jruby/parser/StaticScope.java b/core/src/main/java/org/jruby/parser/StaticScope.java
@@ -45,6 +45,7 @@
 import org.jruby.lexer.yacc.ISourcePosition;
 import org.jruby.runtime.DynamicScope;
 import org.jruby.runtime.Signature;
+import org.jruby.runtime.ThreadContext;
 import org.jruby.runtime.builtin.IRubyObject;
 import org.jruby.runtime.scope.DummyDynamicScope;
 
@@ -97,6 +98,8 @@ public class StaticScope implements Serializable {
     // scope refers to a method scope or to determined IRScope of the parent of a compiling eval.
     private IRScope irScope;
 
+    private RubyModule overlayModule;
+
     public enum Type {
         LOCAL, BLOCK, EVAL;
 
@@ -530,4 +533,12 @@ public StaticScope duplicate() {
 
         return dupe;
     }
+
+    public RubyModule getOverlayModule(ThreadContext context) {
+        RubyModule omod = overlayModule;
+        if (omod == null) {
+            overlayModule = omod = RubyModule.newModule(context.runtime);
+        }
+        return omod;
+    }
 }
diff --git a/core/src/main/java/org/jruby/runtime/callsite/RefinedCachingCallSite.java b/core/src/main/java/org/jruby/runtime/callsite/RefinedCachingCallSite.java
@@ -33,7 +33,7 @@ public IRubyObject call(ThreadContext context, IRubyObject caller, IRubyObject s
 
         while (refinedScope != null &&
                 (
-                        (refinements = refinedScope.getModule().getRefinements()).isEmpty() ||
+                        (refinements = refinedScope.getOverlayModule(context).getRefinements()).isEmpty() ||
                                 (refinement = refinements.get(selfType)) == null ||
                                 (method = refinement.searchMethod(methodName)).isUndefined())
                 ) {
@@ -56,7 +56,7 @@ public IRubyObject call(ThreadContext context, IRubyObject caller, IRubyObject s
 
         while (refinedScope != null &&
                 (
-                        (refinements = refinedScope.getModule().getRefinements()).isEmpty() ||
+                        (refinements = refinedScope.getOverlayModule(context).getRefinements()).isEmpty() ||
                                 (refinement = refinements.get(selfType)) == null ||
                                 (method = refinement.searchMethod(methodName)).isUndefined())
                 ) {
@@ -79,7 +79,7 @@ public IRubyObject call(ThreadContext context, IRubyObject caller, IRubyObject s
 
         while (refinedScope != null &&
                 (
-                        (refinements = refinedScope.getModule().getRefinements()).isEmpty() ||
+                        (refinements = refinedScope.getOverlayModule(context).getRefinements()).isEmpty() ||
                                 (refinement = refinements.get(selfType)) == null ||
                                 (method = refinement.searchMethod(methodName)).isUndefined())
                 ) {
@@ -102,7 +102,7 @@ public IRubyObject call(ThreadContext context, IRubyObject caller, IRubyObject s
 
         while (refinedScope != null &&
                 (
-                        (refinements = refinedScope.getModule().getRefinements()).isEmpty() ||
+                        (refinements = refinedScope.getOverlayModule(context).getRefinements()).isEmpty() ||
                                 (refinement = refinements.get(selfType)) == null ||
                                 (method = refinement.searchMethod(methodName)).isUndefined())
                 ) {
@@ -125,7 +125,7 @@ public IRubyObject call(ThreadContext context, IRubyObject caller, IRubyObject s
 
         while (refinedScope != null &&
                 (
-                        (refinements = refinedScope.getModule().getRefinements()).isEmpty() ||
+                        (refinements = refinedScope.getOverlayModule(context).getRefinements()).isEmpty() ||
                                 (refinement = refinements.get(selfType)) == null ||
                                 (method = refinement.searchMethod(methodName)).isUndefined())
                 ) {
@@ -148,7 +148,7 @@ public IRubyObject call(ThreadContext context, IRubyObject caller, IRubyObject s
 
         while (refinedScope != null &&
                 (
-                        (refinements = refinedScope.getModule().getRefinements()).isEmpty() ||
+                        (refinements = refinedScope.getOverlayModule(context).getRefinements()).isEmpty() ||
                                 (refinement = refinements.get(selfType)) == null ||
                                 (method = refinement.searchMethod(methodName)).isUndefined())
                 ) {
@@ -171,7 +171,7 @@ public IRubyObject call(ThreadContext context, IRubyObject caller, IRubyObject s
 
         while (refinedScope != null &&
                 (
-                        (refinements = refinedScope.getModule().getRefinements()).isEmpty() ||
+                        (refinements = refinedScope.getOverlayModule(context).getRefinements()).isEmpty() ||
                                 (refinement = refinements.get(selfType)) == null ||
                                 (method = refinement.searchMethod(methodName)).isUndefined())
                 ) {
@@ -194,7 +194,7 @@ public IRubyObject call(ThreadContext context, IRubyObject caller, IRubyObject s
 
         while (refinedScope != null &&
                 (
-                        (refinements = refinedScope.getModule().getRefinements()).isEmpty() ||
+                        (refinements = refinedScope.getOverlayModule(context).getRefinements()).isEmpty() ||
                                 (refinement = refinements.get(selfType)) == null ||
                                 (method = refinement.searchMethod(methodName)).isUndefined())
                 ) {
@@ -217,7 +217,7 @@ public IRubyObject call(ThreadContext context, IRubyObject caller, IRubyObject s
 
         while (refinedScope != null &&
                 (
-                        (refinements = refinedScope.getModule().getRefinements()).isEmpty() ||
+                        (refinements = refinedScope.getOverlayModule(context).getRefinements()).isEmpty() ||
                                 (refinement = refinements.get(selfType)) == null ||
                                 (method = refinement.searchMethod(methodName)).isUndefined())
                 ) {
@@ -240,7 +240,7 @@ public IRubyObject call(ThreadContext context, IRubyObject caller, IRubyObject s
 
         while (refinedScope != null &&
                 (
-                        (refinements = refinedScope.getModule().getRefinements()).isEmpty() ||
+                        (refinements = refinedScope.getOverlayModule(context).getRefinements()).isEmpty() ||
                                 (refinement = refinements.get(selfType)) == null ||
                                 (method = refinement.searchMethod(methodName)).isUndefined())
                 ) {

diff --git a/test/mri.index b/test/mri.index
@@ -76,6 +76,7 @@ ruby/test_rational2.rb
 ruby/test_readpartial.rb
 ruby/test_regexp.rb
 ruby/test_require.rb
+ruby/test_refinement.rb
 ruby/test_rubyoptions.rb
 # Removed until we can implement the remaining features (#2143)
 #ruby/test_settracefunc.rb

diff --git a/test/mri/excludes/TestRefinement.rb b/test/mri/excludes/TestRefinement.rb
@@ -0,0 +1,30 @@
+exclude :test_adding_private_method, "needs investigation"
+exclude :test_call_refined_method_in_duplicate_module, "needs investigation"
+exclude :test_case_dispatch_is_aware_of_refinements, "needs investigation"
+exclude :test_include_into_refinement, "needs investigation"
+exclude :test_inspect, "needs investigation"
+exclude :test_making_private_method_public, "needs investigation"
+exclude :test_module_inclusion, "needs investigation"
+exclude :test_module_inclusion2, "needs investigation"
+exclude :test_module_using, "needs investigation"
+exclude :test_module_using_class, "needs investigation"
+exclude :test_module_using_in_method, "needs investigation"
+exclude :test_module_using_invalid_self, "needs investigation"
+exclude :test_new_method, "needs investigation"
+exclude :test_new_method_on_subclass, "needs investigation"
+exclude :test_override, "needs investigation"
+exclude :test_prepend_into_refinement, "needs investigation"
+exclude :test_refine_after_using, "needs investigation"
+exclude :test_refine_in_class, "needs investigation"
+exclude :test_refine_mutual_recursion, "needs investigation"
+exclude :test_refine_recursion, "needs investigation"
+exclude :test_refine_scoping, "needs investigation"
+exclude :test_refine_with_proc, "needs investigation"
+exclude :test_refined_method_defined, "needs investigation"
+exclude :test_remove_refined_method, "needs investigation"
+exclude :test_remove_undefined_refined_method, "needs investigation"
+exclude :test_singleton_method_should_not_use_refinements, "needs investigation"
+exclude :test_super, "needs investigation"
+exclude :test_undefined_refined_method_defined, "needs investigation"
+exclude :test_using_in_method, "needs investigation"
+exclude :test_using_in_module, "needs investigation"
diff --git a/test/mri/ruby/test_refinement.rb b/test/mri/ruby/test_refinement.rb
@@ -1,5 +1,4 @@
 require 'test/unit'
-require_relative 'envutil'
 
 # to supress warnings for future calls of Module#refine
 EnvUtil.suppress_warning do
@@ -1164,6 +1163,283 @@ def foo
 
     assert_raise(NoMethodError, bug10106) {Object.new.foo}
     end;
+
+    assert_separately([], <<-"end;")
+    bug10707 = '[ruby-core:67389] [Bug #10707]'
+    module RefinementBug
+      refine BasicObject do
+        def foo
+        end
+      end
+    end
+
+    assert(methods, bug10707)
+    assert_raise(NameError, bug10707) {method(:foo)}
+    end;
+  end
+
+  def test_change_refined_new_method_visibility
+    assert_separately([], <<-"end;")
+    bug10706 = '[ruby-core:67387] [Bug #10706]'
+    module RefinementBug
+      refine Object do
+        def foo
+        end
+      end
+    end
+
+    assert_raise(NameError, bug10706) {private(:foo)}
+    end;
+  end
+
+  def test_alias_refined_method
+    assert_separately([], <<-"end;")
+    bug10731 = '[ruby-core:67523] [Bug #10731]'
+
+    class C
+    end
+
+    module RefinementBug
+      refine C do
+        def foo
+        end
+
+        def bar
+        end
+      end
+    end
+
+    assert_raise(NameError, bug10731) do
+      class C
+        alias foo bar
+      end
+    end
+    end;
+  end
+
+  def test_singleton_method_should_not_use_refinements
+    assert_separately([], <<-"end;")
+    bug10744 = '[ruby-core:67603] [Bug #10744]'
+
+    class C
+    end
+
+    module RefinementBug
+      refine C.singleton_class do
+        def foo
+        end
+      end
+    end
+
+    assert_raise(NameError, bug10744) { C.singleton_method(:foo) }
+    end;
+  end
+
+  def test_refined_method_defined
+    assert_separately([], <<-"end;")
+    bug10753 = '[ruby-core:67656] [Bug #10753]'
+
+    c = Class.new do
+      def refined_public; end
+      def refined_protected; end
+      def refined_private; end
+
+      public :refined_public
+      protected :refined_protected
+      private :refined_private
+    end
+
+    m = Module.new do
+      refine(c) do
+        def refined_public; end
+        def refined_protected; end
+        def refined_private; end
+
+        public :refined_public
+        protected :refined_protected
+        private :refined_private
+      end
+    end
+
+    using m
+
+    assert_equal(true, c.public_method_defined?(:refined_public), bug10753)
+    assert_equal(false, c.public_method_defined?(:refined_protected), bug10753)
+    assert_equal(false, c.public_method_defined?(:refined_private), bug10753)
+
+    assert_equal(false, c.protected_method_defined?(:refined_public), bug10753)
+    assert_equal(true, c.protected_method_defined?(:refined_protected), bug10753)
+    assert_equal(false, c.protected_method_defined?(:refined_private), bug10753)
+
+    assert_equal(false, c.private_method_defined?(:refined_public), bug10753)
+    assert_equal(false, c.private_method_defined?(:refined_protected), bug10753)
+    assert_equal(true, c.private_method_defined?(:refined_private), bug10753)
+    end;
+  end
+
+  def test_undefined_refined_method_defined
+    assert_separately([], <<-"end;")
+    bug10753 = '[ruby-core:67656] [Bug #10753]'
+
+    c = Class.new
+
+    m = Module.new do
+      refine(c) do
+        def undefined_refined_public; end
+        def undefined_refined_protected; end
+        def undefined_refined_private; end
+        public :undefined_refined_public
+        protected :undefined_refined_protected
+        private :undefined_refined_private
+      end
+    end
+
+    using m
+
+    assert_equal(false, c.public_method_defined?(:undefined_refined_public), bug10753)
+    assert_equal(false, c.public_method_defined?(:undefined_refined_protected), bug10753)
+    assert_equal(false, c.public_method_defined?(:undefined_refined_private), bug10753)
+
+    assert_equal(false, c.protected_method_defined?(:undefined_refined_public), bug10753)
+    assert_equal(false, c.protected_method_defined?(:undefined_refined_protected), bug10753)
+    assert_equal(false, c.protected_method_defined?(:undefined_refined_private), bug10753)
+
+    assert_equal(false, c.private_method_defined?(:undefined_refined_public), bug10753)
+    assert_equal(false, c.private_method_defined?(:undefined_refined_protected), bug10753)
+    assert_equal(false, c.private_method_defined?(:undefined_refined_private), bug10753)
+    end;
+  end
+
+  def test_remove_refined_method
+    assert_separately([], <<-"end;")
+    bug10765 = '[ruby-core:67722] [Bug #10765]'
+
+    class C
+      def foo
+        "C#foo"
+      end
+    end
+
+    module RefinementBug
+      refine C do
+        def foo
+          "RefinementBug#foo"
+        end
+      end
+    end
+
+    using RefinementBug
+
+    class C
+      remove_method :foo
+    end
+
+    assert_equal("RefinementBug#foo", C.new.foo, bug10765)
+    end;
+  end
+
+  def test_remove_undefined_refined_method
+    assert_separately([], <<-"end;")
+    bug10765 = '[ruby-core:67722] [Bug #10765]'
+
+    class C
+    end
+
+    module RefinementBug
+      refine C do
+        def foo
+        end
+      end
+    end
+
+    using RefinementBug
+
+    assert_raise(NameError, bug10765) {
+      class C
+        remove_method :foo
+      end
+    }
+    end;
+  end
+
+  module NotIncludeSuperclassMethod
+    class X
+      def foo
+      end
+    end
+
+    class Y < X
+    end
+
+    module Bar
+      refine Y do
+        def foo
+        end
+      end
+    end
+  end
+
+  def test_instance_methods_not_include_superclass_method
+    bug10826 = '[ruby-dev:48854] [Bug #10826]'
+    assert_not_include(NotIncludeSuperclassMethod::Y.instance_methods(false),
+                       :foo, bug10826)
+    assert_include(NotIncludeSuperclassMethod::Y.instance_methods(true),
+                   :foo, bug10826)
+  end
+
+  def test_call_refined_method_in_duplicate_module
+    bug10885 = '[ruby-dev:48878]'
+    assert_in_out_err([], <<-INPUT, [], [], bug10885)
+      module M
+        refine Object do
+          def raise
+            # do nothing
+          end
+        end
+
+        class << self
+          using M
+          def m0
+            raise
+          end
+        end
+
+        using M
+        def M.m1
+          raise
+        end
+      end
+
+      M.dup.m0
+      M.dup.m1
+    INPUT
+  end
+
+  def test_check_funcall_undefined
+    bug11117 = '[ruby-core:69064] [Bug #11117]'
+
+    x = Class.new
+    Module.new do
+      refine x do
+        def to_regexp
+          //
+        end
+      end
+    end
+
+    assert_nothing_raised(NoMethodError, bug11117) {
+      assert_nil(Regexp.try_convert(x.new))
+    }
+  end
+
+  def test_funcall_inherited
+    bug11117 = '[ruby-core:69064] [Bug #11117]'
+
+    Module.new {refine(Dir) {def to_s; end}}
+    x = Class.new(Dir).allocate
+    assert_nothing_raised(NoMethodError, bug11117) {
+      x.inspect
+    }
   end
 
   private