From 3dd3ea092acead6179033f2c95525ffc5b8bb6ff Mon Sep 17 00:00:00 2001
From: Alan Wu <XrXr@users.noreply.github.com>
Date: Thu, 17 Jun 2021 17:47:11 -0400
Subject: [PATCH] Use Module#ancestors order in recursive constant lookup

Before this commit, const_get with inherit=true and constant lookup
expressions searched the ancestors of the starting point in an order
different from `starting_point.ancestors`.

Items in the ancestry list introduced through prepend were searched
after searching the module they were prepended into. This oddity allowed
for situations where constant lookups gave different results even though
`starting_point.ancestors` is the same.

Do the lookup in the same order as `starting_point.ancestors` by
skipping classes and modules that have an origin iclass. The origin
iclass is in the super chain after the prepended modules.

Note that just like before this commit, the starting point of the
constant lookup is always the first item that we search, regardless of
the presence of any prepended modules.

[Bug #17887]
---
 test/ruby/test_module.rb | 55 ++++++++++++++++++++++++++++++++++++++++
 variable.c               | 24 ++++++++++++++----
 2 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/test/ruby/test_module.rb b/test/ruby/test_module.rb
index 3411c3d701..2d7bdb47fc 100644
--- a/test/ruby/test_module.rb
+++ b/test/ruby/test_module.rb
@@ -2890,6 +2890,61 @@ class TestModule < Test::Unit::TestCase
     }
   end
 
+  def test_prepend_constant_lookup
+    m = Module.new do
+      const_set(:C, :m)
+    end
+    c = Class.new do
+      const_set(:C, :c)
+      prepend m
+    end
+    sc = Class.new(c)
+    # Situation from [Bug #17887]
+    assert_equal(sc.ancestors.take(3), [sc, m, c])
+    assert_equal(:m, sc.const_get(:C))
+    assert_equal(:m, sc::C)
+
+    assert_equal(:c, c::C)
+
+    m.send(:remove_const, :C)
+    assert_equal(:c, sc.const_get(:C))
+    assert_equal(:c, sc::C)
+
+    # Same ancestors, built with include instead of prepend
+    m = Module.new do
+      const_set(:C, :m)
+    end
+    c = Class.new do
+      const_set(:C, :c)
+    end
+    sc = Class.new(c) do
+      include m
+    end
+
+    assert_equal(sc.ancestors.take(3), [sc, m, c])
+    assert_equal(:m, sc.const_get(:C))
+    assert_equal(:m, sc::C)
+
+    m.send(:remove_const, :C)
+    assert_equal(:c, sc.const_get(:C))
+    assert_equal(:c, sc::C)
+
+    # Situation from [Bug #17887], but with modules
+    m = Module.new do
+      const_set(:C, :m)
+    end
+    m2 = Module.new do
+      const_set(:C, :m2)
+      prepend m
+    end
+    c = Class.new do
+      include m2
+    end
+    assert_equal(c.ancestors.take(3), [c, m, m2])
+    assert_equal(:m, c.const_get(:C))
+    assert_equal(:m, c::C)
+  end
+
   def test_inspect_segfault
     bug_10282 = '[ruby-core:65214] [Bug #10282]'
     assert_separately [], "#{<<~"begin;"}\n#{<<~'end;'}"
diff --git a/variable.c b/variable.c
index eef19ec237..0e442241ea 100644
--- a/variable.c
+++ b/variable.c
@@ -2569,16 +2569,31 @@ rb_const_get_0(VALUE klass, ID id, int exclude, int recurse, int visibility)
 static VALUE
 rb_const_search_from(VALUE klass, ID id, int exclude, int recurse, int visibility)
 {
-    VALUE value, tmp;
+    VALUE value, current;
+    bool first_iteration = true;
 
-    tmp = klass;
-    while (RTEST(tmp)) {
+    for (current = klass;
+            RTEST(current);
+            current = RCLASS_SUPER(current), first_iteration = false) {
+        VALUE tmp;
 	VALUE am = 0;
 	rb_const_entry_t *ce;
 
+        if (!first_iteration && RCLASS_ORIGIN(current) != current) {
+            // This item in the super chain has an origin iclass
+            // that comes later in the chain. Skip this item so
+            // prepended modules take precedence.
+            continue;
+        }
+
+        // Do lookup in original class or module in case we are at an origin
+        // iclass in the chain.
+        tmp = current;
+        if (BUILTIN_TYPE(tmp) == T_ICLASS) tmp = RBASIC(tmp)->klass;
+
+        // Do the lookup. Loop in case of autoload.
 	while ((ce = rb_const_lookup(tmp, id))) {
 	    if (visibility && RB_CONST_PRIVATE_P(ce)) {
-		if (BUILTIN_TYPE(tmp) == T_ICLASS) tmp = RBASIC(tmp)->klass;
 		GET_EC()->private_const_reference = tmp;
 		return Qundef;
 	    }
@@ -2599,7 +2614,6 @@ rb_const_search_from(VALUE klass, ID id, int exclude, int recurse, int visibilit
 	    return value;
 	}
 	if (!recurse) break;
-	tmp = RCLASS_SUPER(tmp);
     }
 
   not_found: