Ensure origins of included, prepended, and refined modules #3140

jeremyevans · 2020-05-24T03:29:17Z

This fixes cases where the modules are refined and included into
other modules or classes.

Implement by renaming ensure_origin to rb_ensure_origin, making it
non-static, and calling it directly after Enumerable, Comparable,
and Kernel are created.

This requires a couple of other changes:

rb_undef_methods_from doesn't automatically handle classes with
origins, so pass it the origin for Comparable when undefing
methods in Complex. This fixed a failure in the Complex tests.
When adding a method, the method cache was not be cleared
correctly if klass has an origin. Clear the method cache for
the klass before switching to the origin of klass. This fixed
failures in the autoload tests related to overridding require,
without breaking the optimization tests.

Potentially, this approach could work for other modules (possibly
all modules), but this commit focuses on modules that are included
by default in core classes and are reasonably likely targets for
refinement.

Fixes [Bug #16852]

jeremyevans · 2020-05-25T21:36:33Z

I've added 3 more patches to this. The original patch only fixed Enumerable, Comparable, and Kernel, but the same issues that apply to this affect all modules.

The first added patch fixes various issues when a module is included
in or prepended to a module or class, and then refined, or refined and
then included or prepended to a module or class.

Module#include? is fixed to skip origin iclasses.
Refinements are fixed to use the origin class of the module that
has an origin.
RCLASS_REFINED_BY_ANY is removed as it was only used in a single
place and is no longer needed.
Marshal#dump is fixed to skip iclass origins.
rb_method_entry_make is fixed to handled overridden optimized
methods for modules that have origins.
remove_method is fixed to clear the method cache for both the
module and the origin if they are different.

The second added patches fixes Module#initialize_copy to handle
origins correctly. Previously, Module#initialize_copy did not
handle origins correctly. For example, this code:

module B; end
class A
  def b; 2 end
  prepend B
end
a = A.dup.new
class A
  def b; 1 end
end
p a.b

Printed 1 instead of 2. This is because the super chain for
a.singleton_class was:

a.singleton_class
A.dup
B(iclass)
B(iclass origin)
A(origin) # not A.dup(origin)

The B iclasses would not be modified, so the includer entry would be
still be set to A and not A.dup.

This modifies things so that if the class/module has an origin,
all iclasses between the class/module and the origin are duplicated
and have the correct includer entry set, and the correct origin
is created.

The third added patch strengthens the check in vm_search_super_method.
Previously, the iclass includer was only used for the error message.
This strengthens the check so that the current receiver in the
super call must be an instance of the iclass includer, not mearly
include the same module

jeremyevans · 2020-05-25T21:54:24Z

I'm going to back off the latest patch for now, as it appears to cause problems on mingw at least. It's also a feature improvement and not strictly a bug fix.

mame · 2020-05-28T08:19:42Z

enum.c

 #define rb_intern(str) rb_intern_const(str)

    rb_mEnumerable = rb_define_module("Enumerable");
+    rb_ensure_origin(rb_mEnumerable);


Is this still needed? This is a bit uneasy for me because it looks ad-hoc. And as far as I understand, your patch now ensures the existence of origin whenever the module is include'ed, and the three modules (Enumerable, Comperable, and Kernel) are always include'ed, so I guess they are no longer needed. In fact, the test passes without these calls to rb_ensure_origin().

No, these are no longer needed after the second patch. I'll add another commit to remove them.

mame · 2020-05-28T09:10:00Z

I went through your patch. As far as I understand, the main part of this patch (1) separates origin from module not only when it is prepended but also when it is include'ed (even if not RCLASS_REFINED_BY_ANY), and (2) clears method cache not only of module but also of its origin. There are some other accountings such as Module#dup and Marshal (but I don't understand well).

As I wrote in my review, I'm afraid if ad-hoc calls to rb_ensure_origin for some builtin modules are no longer needed. Except this point, I think this PR is right. But to be honest, I'm not familiar with the implementation detail of refinement. I think @nobu and @ko1 are better persons than me to review this PR.

jeremyevans · 2020-05-28T14:52:16Z

@mame You understand correctly. The changes to Module#dup and Marshal were made to fix bugs that ensuring origins exposed in the tests. These aren't issues introduced by the changes. All bugs in Module#dup and Marshal would affect modules that are prepended in older versions of Ruby.

jeremyevans · 2020-05-28T17:17:15Z

Only failing check is AppVeyor, which had problems with git and didn't run the tests. AppVeyor seems to have general problems if a pull request is not directly based on current master.

jeremyevans · 2020-06-01T22:32:43Z

@ko1 and @nobu if one of you could review this before I commit it, I would appreciate it. I'll wait a few more days before committing.

ko1 · 2020-06-02T19:35:20Z

@nobu could you help us? it is tough to understand the issue and proposed code for me...

nobu

Seems fine.
Copying superclass tree feels somewhat heavy, but I don't have better ideas.

nobu · 2020-06-03T09:31:28Z

class.c

+                rb_bug("non iclass between module/class and origin");
+            }
+            clone_p = class_alloc(RBASIC(p)->flags, RBASIC(p)->klass);
+            if (prev_clone_p) {


This check would not be necessary if prev_clone_p is initialized to clone, unless I miss something.

I agree. I'll make this change.

nobu · 2020-06-03T09:31:47Z

class.c

+        RCLASS_SET_SUPER(clone, RCLASS_SUPER(orig));
+    }
+    else {
+        VALUE p = RCLASS_SUPER(orig); 


A space at the EOL.

nobu · 2020-06-03T09:32:33Z

class.c

+            RCLASS_CONST_TBL(clone_p) = RCLASS_CONST_TBL(p);
+            RCLASS_IV_TBL(clone_p) = RCLASS_IV_TBL(p);
+            RCLASS_EXT(clone_p)->allocator = RCLASS_EXT(p)->allocator;
+            if (RB_TYPE_P(clone, T_CLASS)) { 


A space here, too.

nobu · 2020-06-03T09:35:46Z

compar.c


 #include "id.h"
 #include "internal.h"
+#include "internal/class.h"


Is this line needed?
Seems nothing changes...

This and the enum.c change was needed in the first commit, but is not needed any more. I'll push a rebased version that removes these.

nobu · 2020-06-03T09:37:39Z

enum.c


 #include "id.h"
 #include "internal.h"
+#include "internal/class.h"


Same as compar.c.

This fixes various issues when a module is included in or prepended to a module or class, and then refined, or refined and then included or prepended to a module or class. Implement by renaming ensure_origin to rb_ensure_origin, making it non-static, and calling it when refining a module. Fix Module#initialize_copy to handle origins correctly. Previously, Module#initialize_copy did not handle origins correctly. For example, this code: ```ruby module B; end class A def b; 2 end prepend B end a = A.dup.new class A def b; 1 end end p a.b ``` Printed 1 instead of 2. This is because the super chain for a.singleton_class was: ``` a.singleton_class A.dup B(iclass) B(iclass origin) A(origin) # not A.dup(origin) ``` The B iclasses would not be modified, so the includer entry would be still be set to A and not A.dup. This modifies things so that if the class/module has an origin, all iclasses between the class/module and the origin are duplicated and have the correct includer entry set, and the correct origin is created. This requires other changes to make sure all tests still pass: * rb_undef_methods_from doesn't automatically handle classes with origins, so pass it the origin for Comparable when undefing methods in Complex. This fixed a failure in the Complex tests. * When adding a method, the method cache was not cleared correctly if klass has an origin. Clear the method cache for the klass before switching to the origin of klass. This fixed failures in the autoload tests related to overridding require, without breaking the optimization tests. Also clear the method cache for both the module and origin when removing a method. * Module#include? is fixed to skip origin iclasses. * Refinements are fixed to use the origin class of the module that has an origin. * RCLASS_REFINED_BY_ANY is removed as it was only used in a single place and is no longer needed. * Marshal#dump is fixed to skip iclass origins. * rb_method_entry_make is fixed to handled overridden optimized methods for modules that have origins. Fixes [Bug #16852]

jeremyevans · 2020-06-03T16:50:30Z

@mame and @nobu Thank you very much for your reviews.

jeremyevans requested a review from nobu May 24, 2020 03:30

jeremyevans force-pushed the ensure-origin-16852 branch 4 times, most recently from a0b1888 to 10f320c Compare May 24, 2020 15:29

jeremyevans force-pushed the ensure-origin-16852 branch from c1bdf1d to 2053fbf Compare May 25, 2020 21:53

jeremyevans force-pushed the ensure-origin-16852 branch 2 times, most recently from fc31d62 to 4c225a2 Compare May 25, 2020 22:15

jeremyevans changed the title ~~Ensure origins of Enumerable, Comparable, and Kernel after creation~~ Ensure origins of included, prepended, and refined modules May 26, 2020

jeremyevans requested a review from mame May 26, 2020 14:48

mame reviewed May 28, 2020

View reviewed changes

jeremyevans requested a review from ko1 May 28, 2020 14:53

nobu approved these changes Jun 3, 2020

View reviewed changes

jeremyevans force-pushed the ensure-origin-16852 branch from e46f2c6 to d6a7b7c Compare June 3, 2020 15:20

jeremyevans merged commit 98286e9 into ruby:master Jun 3, 2020

Ensure origins of included, prepended, and refined modules #3140

Ensure origins of included, prepended, and refined modules #3140

Uh oh!

Conversation

jeremyevans commented May 24, 2020

Uh oh!

jeremyevans commented May 25, 2020

Uh oh!

jeremyevans commented May 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mame commented May 28, 2020

Uh oh!

jeremyevans commented May 28, 2020

Uh oh!

jeremyevans commented May 28, 2020

Uh oh!

jeremyevans commented Jun 1, 2020

Uh oh!

ko1 commented Jun 2, 2020

Uh oh!

nobu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremyevans commented Jun 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants