[GR-38307] Fix loading issue when multiple feature paths overlap

PullRequest: truffleruby/3313

Author: bjfish

CHANGELOG.md

@@ -36,6 +36,7 @@ Bug fixes:
 * Fix `Float#round` to avoid losing precision during the rounding process (@aardvark179).
 * Fix `String#insert` to not call a subclassed string method (@bjfish).
 * Fix `rb_obj_call_init` to pass any block argument to the `initialize` method (#2675, @aardvark179).
+* Fix issue with feature loading not detecting a previously loaded feature (#2677, @bjfish).
 
 Compatibility:
 

spec/ruby/core/kernel/shared/require.rb

@@ -568,6 +568,25 @@
 
       -> { @object.require("unicode_normalize") }.should raise_error(LoadError)
     end
+
+    ruby_version_is "3.0" do
+      it "does not load a file earlier on the $LOAD_PATH when other similar features were already loaded" do
+        Dir.chdir CODE_LOADING_DIR do
+          @object.send(@method, "../code/load_fixture").should be_true
+        end
+        ScratchPad.recorded.should == [:loaded]
+
+        $LOAD_PATH.unshift "#{CODE_LOADING_DIR}/b"
+        # This loads because the above load was not on the $LOAD_PATH
+        @object.send(@method, "load_fixture").should be_true
+        ScratchPad.recorded.should == [:loaded, :loaded]
+
+        $LOAD_PATH.unshift "#{CODE_LOADING_DIR}/c"
+        # This does not load because the above load was on the $LOAD_PATH
+        @object.send(@method, "load_fixture").should be_false
+        ScratchPad.recorded.should == [:loaded, :loaded]
+      end
+    end
   end
 
   describe "(shell expansion)" do

spec/ruby/fixtures/code/c/load_fixture.rb

@@ -0,0 +1 @@
+ScratchPad << :loaded

spec/truffle/kernel/feature_loader_spec.rb

@@ -15,27 +15,37 @@
     current_index.clear
 
     Truffle::FeatureLoader.features_index_add('foo', 0)
-    current_index[Truffle::FeatureLoader::FeatureEntry.new('foo')].should == [0]
+    current_index['foo'][0].index.should == 0
 
     Truffle::FeatureLoader.features_index_add('foo', 1)
-    current_index[Truffle::FeatureLoader::FeatureEntry.new('foo')].should == [0, 1]
+    current_index['foo'][1].index.should == 1
     current_index.clear
 
     Truffle::FeatureLoader.features_index_add('foo.rb', 0)
     Truffle::FeatureLoader.features_index_add('foo.rb', 1)
-    current_index[Truffle::FeatureLoader::FeatureEntry.new('foo')].should == [0, 1]
-    current_index[Truffle::FeatureLoader::FeatureEntry.new('foo.rb')].should == [0, 1]
+    current_index['foo'][0].index.should == 0
+    current_index['foo'][1].index.should == 1
     current_index.clear
 
     Truffle::FeatureLoader.features_index_add('one/two/foo.rb', 0)
     ['foo', 'foo.rb', 'two/foo', 'two/foo.rb', 'one/two/foo.rb', 'one/two/foo'].each do |feature|
-      current_index[Truffle::FeatureLoader::FeatureEntry.new(feature)].should == [0]
+      current_index['foo'].any? do |fe|
+        if fe.include?(Truffle::FeatureLoader::FeatureEntry.new(feature))
+          fe.index.should == 0
+          true
+        end
+      end.should == true
     end
     current_index.clear
 
     Truffle::FeatureLoader.features_index_add('/two/foo.rb', 0)
     ['foo', 'foo.rb', 'two/foo', 'two/foo.rb', '/two/foo.rb', '/two/foo'].each do |feature|
-      current_index[Truffle::FeatureLoader::FeatureEntry.new(feature)].should == [0]
+      current_index['foo'].any? do |fe|
+        if fe.include?(Truffle::FeatureLoader::FeatureEntry.new(feature))
+          fe.index.should == 0
+          true
+        end
+      end.should == true
     end
     current_index.clear
   ensure
@@ -89,48 +99,35 @@
   it "only a nested path at the end should match" do
     hash = {}
     stored_entry = Truffle::FeatureLoader::FeatureEntry.new("path/to/feature")
-    hash[stored_entry] = true
-    stored_entry.part_of_index = true
+    hash[stored_entry.base] = [stored_entry]
 
     short_lookup_entry = Truffle::FeatureLoader::FeatureEntry.new("to/feature")
-    hash[short_lookup_entry].should be_true
+    hash['feature'].any? { |fe| fe.include?(short_lookup_entry) }.should be_true
 
     exact_lookup_entry = Truffle::FeatureLoader::FeatureEntry.new("path/to/feature")
-    hash[exact_lookup_entry].should be_true
-
+    hash['feature'].any? { |fe| fe.include?(exact_lookup_entry) }.should be_true
 
     longer_lookup_entry = Truffle::FeatureLoader::FeatureEntry.new("long/path/to/feature")
-    hash[longer_lookup_entry].should be_nil
+    hash['feature'].any? { |fe| fe.include?(longer_lookup_entry) }.should be_false
 
     prefix_lookup_entry = Truffle::FeatureLoader::FeatureEntry.new("path/to")
-    hash[prefix_lookup_entry].should be_nil
+    hash['feature'].any? { |fe| fe.include?(prefix_lookup_entry) }.should be_false
   end
 
   describe "when stored has an extension" do
     it "matches a lookup with or without extension" do
       hash = {}
       stored_entry = Truffle::FeatureLoader::FeatureEntry.new("path/to/feature.so")
-      hash[stored_entry] = true
-      stored_entry.part_of_index = true
-
+      hash['feature'] = [stored_entry]
 
       lookup_entry_no_ext = Truffle::FeatureLoader::FeatureEntry.new("to/feature")
-      hash[lookup_entry_no_ext].should be_true
+      hash['feature'].any? { |fe| fe.include?(lookup_entry_no_ext) }.should be_true
 
       lookup_entry_ext = Truffle::FeatureLoader::FeatureEntry.new("to/feature.so")
-      hash[lookup_entry_ext].should be_true
+      hash['feature'].any? { |fe| fe.include?(lookup_entry_ext) }.should be_true
 
       lookup_entry_wrong_ext = Truffle::FeatureLoader::FeatureEntry.new("to/feature.rb")
-      hash[lookup_entry_wrong_ext].should be_nil
+      hash['feature'].any? { |fe| fe.include?(lookup_entry_wrong_ext) }.should be_false
     end
   end
-
-  it "raises an error when both keys are lookup" do
-    hash = {}
-    stored_entry = Truffle::FeatureLoader::FeatureEntry.new("path/to/feature")
-    hash[stored_entry] = true
-
-    short_lookup_entry = Truffle::FeatureLoader::FeatureEntry.new("to/feature")
-    -> { hash[short_lookup_entry] }.should raise_error
-  end
 end

src/main/ruby/truffleruby/core/truffle/feature_loader.rb

@@ -13,7 +13,24 @@ module FeatureLoader
 
     DOT_DLEXT = ".#{Truffle::Platform::DLEXT}"
 
-    # FeatureEntry => [*index_inside_$LOADED_FEATURES]
+    #
+    # The following $LOADED_FEATURES:
+    # ["/one/path.rb", "/two/path.so", "three/path" ]
+    #
+    # would create the @loaded_features_index:
+    #
+    # {
+    #     "path" => [
+    #             #<FeatureEntry @feature="/one/path.rb" @index=0>,
+    #             #<FeatureEntry @feature="/two/path.so" @index=1>,
+    #             #<FeatureEntry @feature="/three/path" @index=2>,
+    #         ]
+    # }
+    #
+    # For the feature "path" or "nested/path", all three feature entries are in the same hash key and `include?`
+    # the feature. However, for feature "path.rb", only the first entry with the same extension would be at the same
+    # hash key and not `include?` the feature.
+    #
     @loaded_features_index = {}
     # A snapshot of $LOADED_FEATURES, to check if the @loaded_features_index cache is up to date.
     @loaded_features_version = -1
@@ -33,43 +50,24 @@ def self.clear_cache
     end
 
     class FeatureEntry
-      attr_reader :feature, :ext, :feature_no_ext
-      attr_accessor :part_of_index
+      attr_accessor :index
+      attr_reader :feature, :ext, :feature_no_ext, :base
 
       def initialize(feature)
-        @part_of_index = false
         @ext = Truffle::FeatureLoader.extension(feature)
         @feature = feature
         @feature_no_ext = @ext ? feature[0...(-@ext.size)] : feature
         @base = File.basename(@feature_no_ext)
+        @index = nil
       end
 
-      def ==(other)
-        # The looked up feature has to be the trailing part of an already-part_of_index entry.
-        # We always want to check part_of_index_feature.end_with?(lookup_feature).
-        # We compare extensions only if the lookup_feature has an extension.
-
-        if @part_of_index
-          stored = self
-          lookup = other
-        elsif other.part_of_index
-          stored = other
-          lookup = self
-        else
-          raise 'Expected that at least one of the FeatureEntry instances is part of the index'
-        end
-
+      def include?(lookup)
         if lookup.ext
-          stored.feature.end_with?(lookup.feature)
+          feature.end_with?(lookup.feature)
         else
-          stored.feature_no_ext.end_with?(lookup.feature_no_ext)
+          feature_no_ext.end_with?(lookup.feature_no_ext)
         end
       end
-      alias_method :eql?, :==
-
-      def hash
-        @base.hash
-      end
     end
 
     def self.find_file(feature)
@@ -161,30 +159,32 @@ def self.feature_provided?(feature, expanded)
       with_synchronized_features do
         get_loaded_features_index
         feature_entry = FeatureEntry.new(feature)
-        if @loaded_features_index.key?(feature_entry)
-          @loaded_features_index[feature_entry].each do |i|
-            loaded_feature = $LOADED_FEATURES[i]
+        if @loaded_features_index.key?(feature_entry.base)
+          @loaded_features_index[feature_entry.base].each do |fe|
+            if fe.include?(feature_entry)
+              loaded_feature = $LOADED_FEATURES[fe.index]
 
-            next if loaded_feature.size < feature.size
-            feature_path = if loaded_feature.start_with?(feature)
-                             feature
-                           else
-                             if expanded
-                               nil
+              next if loaded_feature.size < feature.size
+              feature_path = if loaded_feature.start_with?(feature)
+                               feature
                              else
-                               loaded_feature_path(loaded_feature, feature, get_expanded_load_path)
+                               if expanded
+                                 nil
+                               else
+                                 loaded_feature_path(loaded_feature, feature, get_expanded_load_path)
+                               end
                              end
-                           end
-            if feature_path
-              loaded_feature_ext = extension_symbol(loaded_feature)
-              if !loaded_feature_ext
-                return :unknown unless feature_ext
-              else
-                if (!feature_has_rb_ext || !feature_ext) && binary_ext?(loaded_feature_ext)
-                  return :so
-                end
-                if (feature_has_rb_ext || !feature_ext) && loaded_feature_ext == :rb
-                  return :rb
+              if feature_path
+                loaded_feature_ext = extension_symbol(loaded_feature)
+                if !loaded_feature_ext
+                  return :unknown unless feature_ext
+                else
+                  if (!feature_has_rb_ext || !feature_ext) && binary_ext?(loaded_feature_ext)
+                    return :so
+                  end
+                  if (feature_has_rb_ext || !feature_ext) && loaded_feature_ext == :rb
+                    return :rb
+                  end
                 end
               end
             end
@@ -225,7 +225,7 @@ def self.relative_feature(expanded_path)
       if !load_path_entries.empty?
         load_path_entry = load_path_entries.max_by(&:length)
         before_dot_rb = expanded_path.end_with?('.rb') ? -4 : -1
-        expanded_path[load_path_entry.size+1..before_dot_rb]
+        expanded_path[load_path_entry.size + 1..before_dot_rb]
       else
         nil
       end
@@ -297,13 +297,14 @@ def self.get_loaded_features_index
 
     # MRI: features_index_add
     # always called inside #with_synchronized_features
+    #
     def self.features_index_add(feature, offset)
       feature_entry = FeatureEntry.new(feature)
-      if @loaded_features_index.key?(feature_entry)
-        @loaded_features_index[feature_entry] << offset
+      feature_entry.index = offset
+      if @loaded_features_index.key?(feature_entry.base)
+        @loaded_features_index[feature_entry.base] << feature_entry
       else
-        @loaded_features_index[feature_entry] = [offset]
-        feature_entry.part_of_index = true
+        @loaded_features_index[feature_entry.base] = [feature_entry]
       end
     end