Get rid of RealPathCache and the Kernel.require_relative decorator

Ref: #402
Ref: https://bugs.ruby-lang.org/issues/10222
Ref: ruby/ruby@5754f15
Ref: ruby/ruby@b6d3927

Up to Ruby 2.3, `require` would resolve symlinks, but `require_relative` wouldn't:

```ruby
require 'fileutils'
FileUtils.mkdir_p("realpath")
File.write("realpath/a.rb", "p :a_loaded")
File.symlink("realpath", "symlink") rescue nil

$LOAD_PATH.unshift(File.realpath(__dir__) + "/symlink")
require "a.rb" # load symlink/a.rb in 2.3 and older, load realpath/a.rb on 2.4 and newer
require_relative "realpath/a.rb" # noop on 2.4+
```

This would easily cause double loading issue when `require` and `require_relative`
were mixed, but was fixed in 2.4 (https://bugs.ruby-lang.org/issues/10222).

The problem is that `Bootsnap` kinda negated this fix, because `realpath()`
wouldn't be applied to absolute paths:

```ruby
require 'fileutils'
FileUtils.mkdir_p("realpath")
File.write("realpath/a.rb", "p :a_loaded")
File.symlink("realpath", "symlink") rescue nil

$LOAD_PATH.unshift(File.realpath(__dir__) + "/symlink")
require File.expand_path("symlink/a.rb") # load symlink/a.rb in 3.0 and older, load realpath/a.rb on 3.1 and newer
require_relative "realpath/a.rb" # noop on 3.1+
```

And for performance reasons, Bootsnap tried really hard not to call `realpath`,
as it's a syscall, instead it used `expand_path`, which is entirely in use
space and doesn't reach to the file system. So if you had a `symlink` in
`$LOAD_PATH`, `bootcsnap` would perpetuate this bug, which led to the
addition of #136.

This was ultimately fixed in Ruby 3.1 (https://bugs.ruby-lang.org/issues/17885),
now `realpath` is applied even on absolute paths.

While `realpath` is indeed expensive, I think the performance impact is ok if
we only call it for `$LOAD_PATH` members, rather than for all requirable files.
So if you have X gems, it's going to be more or less X `realpath` calls.

It would stay a problem if a gem actually contained symlinks and used
`require_relative`, but it's quite the stretch, and with 3.1 now
handling it, it's not worth keeping such workaround.

See: #402

CHANGELOG.md

@@ -1,6 +1,9 @@
 # Unreleased
 
-* Drop support for Ruby 2.3. 
+* Get rid of the `Kernel.require_relative` decorator by resolving `$LOAD_PATH` members to their real path.
+  This way we handle symlinks in `$LOAD_PATH` much more efficiently. See #402 for the detailed explanation.
+
+* Drop support for Ruby 2.3 (to allow gettind rid of the `Kernel.require_relative` decorator).
 
 # 1.10.3
 

lib/bootsnap/load_path_cache.rb

@@ -22,7 +22,7 @@ module LoadPathCache
     CACHED_EXTENSIONS = DLEXT2 ? [DOT_RB, DLEXT, DLEXT2] : [DOT_RB, DLEXT]
 
     class << self
-      attr_reader(:load_path_cache, :loaded_features_index, :realpath_cache)
+      attr_reader(:load_path_cache, :loaded_features_index)
 
       def setup(cache_path:, development_mode:)
         unless supported?
@@ -33,7 +33,6 @@ def setup(cache_path:, development_mode:)
         store = Store.new(cache_path)
 
         @loaded_features_index = LoadedFeaturesIndex.new
-        @realpath_cache = RealpathCache.new
 
         @load_path_cache = Cache.new(store, $LOAD_PATH, development_mode: development_mode)
         require_relative("load_path_cache/core_ext/kernel_require")
@@ -55,5 +54,4 @@ def supported?
   require_relative("load_path_cache/store")
   require_relative("load_path_cache/change_observer")
   require_relative("load_path_cache/loaded_features_index")
-  require_relative("load_path_cache/realpath_cache")
 end

lib/bootsnap/load_path_cache/cache.rb

@@ -142,6 +142,8 @@ def push_paths_locked(*paths)
             @has_relative_paths = true if p.relative?
             next if p.non_directory?
 
+            p = p.to_realpath
+
             expanded_path = p.expanded_path
             entries, dirs = p.entries_and_dirs(@store)
             # push -> low precedence -> set only if unset
@@ -157,6 +159,8 @@ def unshift_paths_locked(*paths)
             p = Path.new(path)
             next if p.non_directory?
 
+            p = p.to_realpath
+
             expanded_path = p.expanded_path
             entries, dirs = p.entries_and_dirs(@store)
             # unshift -> high precedence -> unconditional set

lib/bootsnap/load_path_cache/core_ext/kernel_require.rb

@@ -33,15 +33,6 @@ def require(path)
     end
   end
 
-  alias_method(:require_relative_without_bootsnap, :require_relative)
-  def require_relative(path)
-    location = caller_locations(1..1).first
-    realpath = Bootsnap::LoadPathCache.realpath_cache.call(
-      location.absolute_path || location.path, path
-    )
-    require(realpath)
-  end
-
   alias_method(:load_without_bootsnap, :load)
   def load(path, wrap = false)
     if (resolved = Bootsnap::LoadPathCache.load_path_cache.find(path, try_extensions: false))

lib/bootsnap/load_path_cache/path.rb

@@ -21,8 +21,26 @@ def volatile?
 
       attr_reader(:path)
 
-      def initialize(path)
+      def initialize(path, real: false)
         @path = path.to_s.freeze
+        @real = real
+      end
+
+      def to_realpath
+        return self if @real
+
+        realpath = begin
+          File.realpath(path)
+        rescue Errno::ENOENT
+          return self
+        end
+
+        if realpath != path
+          Path.new(realpath, real: true)
+        else
+          @real = true
+          self
+        end
       end
 
       # True if the path exists, but represents a non-directory object
@@ -62,7 +80,11 @@ def entries_and_dirs(store)
       end
 
       def expanded_path
-        File.expand_path(path).freeze
+        if @real
+          path
+        else
+          @expanded_path ||= File.expand_path(path).freeze
+        end
       end
 
       private

test/integration/kernel_test.rb

@@ -0,0 +1,214 @@
+# frozen_string_literal: true
+
+require("test_helper")
+
+module Bootsnap
+  class KernelTest < Minitest::Test
+    include TmpdirHelper
+
+    def test_require_symlinked_file_twice
+      setup_symlinked_files
+      if RUBY_VERSION >= "3.1"
+        # Fixed in https://github.com/ruby/ruby/commit/79a4484a072e9769b603e7b4fbdb15b1d7eccb15 (Ruby 3.1)
+        assert_both_pass(<<~RUBY)
+          require "symlink/test"
+          require "real/test"
+        RUBY
+      else
+        assert_both_pass(<<~RUBY)
+          require "symlink/test"
+          begin
+            require "real/test"
+          rescue RuntimeError
+            exit 0
+          else
+            exit 1
+          end
+        RUBY
+      end
+    end
+
+    def test_require_symlinked_file_twice_aliased
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "test"
+
+        $LOAD_PATH.unshift(File.expand_path("a"))
+        require "test"
+      RUBY
+    end
+
+    def test_require_relative_symlinked_file_twice
+      setup_symlinked_files
+      if RUBY_VERSION >= "3.1"
+        # Fixed in https://github.com/ruby/ruby/commit/79a4484a072e9769b603e7b4fbdb15b1d7eccb15 (Ruby 3.1)
+        assert_both_pass(<<~RUBY)
+          require_relative "symlink/test"
+          require_relative "real/test"
+        RUBY
+      else
+        assert_both_pass(<<~RUBY)
+          require_relative "symlink/test"
+          begin
+            require_relative "real/test"
+          rescue RuntimeError
+            exit 0
+          else
+            exit 1
+          end
+        RUBY
+      end
+    end
+
+    def test_require_and_then_require_relative_symlinked_file
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "test"
+
+        require_relative "real/test"
+      RUBY
+    end
+
+    def test_require_relative_and_then_require_symlinked_file
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        require_relative "real/test"
+
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "test"
+      RUBY
+    end
+
+    def test_require_deep_symlinked_file_twice
+      setup_symlinked_files
+      if RUBY_VERSION >= "3.1"
+        # Fixed in https://github.com/ruby/ruby/commit/79a4484a072e9769b603e7b4fbdb15b1d7eccb15 (Ruby 3.1)
+        assert_both_pass(<<~RUBY)
+          require "symlink/dir/deep"
+          require "real/dir/deep"
+        RUBY
+      else
+        assert_both_pass(<<~RUBY)
+          require "symlink/dir/deep"
+          begin
+            require "real/dir/deep"
+          rescue RuntimeError
+            exit 0
+          else
+            exit 1
+          end
+        RUBY
+      end
+    end
+
+    def test_require_deep_symlinked_file_twice_aliased
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "dir/deep"
+
+        $LOAD_PATH.unshift(File.expand_path("a"))
+        require "dir/deep"
+      RUBY
+    end
+
+    def test_require_relative_deep_symlinked_file_twice
+      setup_symlinked_files
+      if RUBY_VERSION >= "3.1"
+        # Fixed in https://github.com/ruby/ruby/commit/79a4484a072e9769b603e7b4fbdb15b1d7eccb15 (Ruby 3.1)
+        assert_both_pass(<<~RUBY)
+          require_relative "symlink/dir/deep"
+          require_relative "real/dir/deep"
+        RUBY
+      else
+        assert_both_pass(<<~RUBY)
+          require_relative "symlink/dir/deep"
+          begin
+            require_relative "real/dir/deep"
+          rescue RuntimeError
+            exit 0
+          else
+            exit 1
+          end
+        RUBY
+      end
+    end
+
+    def test_require_and_then_require_relative_deep_symlinked_file
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "dir/deep"
+
+        require_relative "real/dir/deep"
+      RUBY
+    end
+
+    def test_require_relative_and_then_require_deep_symlinked_file
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        require_relative "real/dir/deep"
+
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "dir/deep"
+      RUBY
+    end
+
+    private
+
+    def assert_both_pass(source)
+      Help.set_file("without_bootsnap.rb", source)
+      unless execute("without_bootsnap.rb", "debug.txt")
+        flunk "expected snippet to pass WITHOUT bootsnap enabled:\n#{debug_output}"
+      end
+
+      Help.set_file("with_bootsnap.rb", %{require "bootsnap/setup"\n#{source}})
+      unless execute("with_bootsnap.rb", "debug.txt")
+        flunk "expected snippet to pass WITH bootsnap enabled:\n#{debug_output}"
+      end
+    end
+
+    def debug_output
+      File.read("debug.txt")
+    rescue Errno::ENOENT
+    end
+
+    def execute(script_path, output_path)
+      system(
+        {"BOOTSNAP_CACHE_DIR" => "tmp/cache"},
+        RbConfig.ruby, "-I.", script_path,
+        out: output_path, err: output_path
+      )
+    end
+
+    def assert_successful(source)
+      Help.set_file("test_case.rb", source)
+
+      Help.set_file("test_case.rb", %{require "bootsnap/setup"\n#{source}})
+      assert system({"BOOTSNAP_CACHE_DIR" => "tmp/cache"}, RbConfig.ruby, "-I.", "test_case.rb")
+    end
+
+    def setup_symlinked_files
+      skip("Platform doesn't support symlinks") unless File.respond_to?(:symlink)
+
+      Help.set_file("real/test.rb", <<-RUBY)
+        if $test_already_required
+          raise "test.rb required more than once"
+        else
+          $test_already_required = true
+        end
+      RUBY
+
+      Help.set_file("real/dir/deep.rb", <<-RUBY)
+        if $deep_already_required
+          raise "deep.rb required more than once"
+        else
+          $deep_already_required = true
+        end
+      RUBY
+      File.symlink("real", "symlink")
+    end
+  end
+end