Merge pull request #406 from Shopify/rb-get-path

Bind `rb_get_path` to better respect Kernel#require duck-type

CHANGELOG.md

@@ -1,9 +1,13 @@
 # Unreleased
 
+* Better respect `Kernel#require` duck typing. While it almost never comes up in practice, `Kernel#require`
+ follow a fairly intricate duck-typing protocol on its argument implemented as `rb_get_path(VALUE)` in MRI.
+ So when applicable we bind `rb_get_path` and use it for improved compatibility. See #396 and #406.
+
 * 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).
+* Drop support for Ruby 2.3 (to allow getting rid of the `Kernel.require_relative` decorator).
 
 # 1.10.3
 

ext/bootsnap/bootsnap.c

@@ -135,6 +135,12 @@ bs_rb_coverage_running(VALUE self)
  return RTEST(cov) ? Qtrue : Qfalse;
 }
 
+static VALUE
+bs_rb_get_path(VALUE self, VALUE fname)
+{
+ return rb_get_path(fname);
+}
+
 /*
  * Ruby C extensions are initialized by calling Init_<extname>.
  *
@@ -146,6 +152,9 @@ void
 Init_bootsnap(void)
 {
  rb_mBootsnap = rb_define_module("Bootsnap");
+
+ rb_define_singleton_method(rb_mBootsnap, "rb_get_path", bs_rb_get_path, 1);
+
  rb_mBootsnap_CompileCache = rb_define_module_under(rb_mBootsnap, "CompileCache");
  rb_mBootsnap_CompileCache_Native = rb_define_module_under(rb_mBootsnap_CompileCache, "Native");
  rb_cBootsnap_CompileCache_UNCOMPILABLE = rb_const_get(rb_mBootsnap_CompileCache, rb_intern("UNCOMPILABLE"));

lib/bootsnap.rb

@@ -133,5 +133,16 @@ def absolute_path?(path)
  path.start_with?("/")
  end
  end
+
+ # This is a semi-accurate ruby implementation of the native `rb_get_path(VALUE)` function.
+ # The native version is very intricate and may behave differently on windows etc.
+ # But we only use it for non-MRI platform.
+ def rb_get_path(fname)
+ path_path = fname.respond_to?(:to_path) ? fname.to_path : fname
+ String.try_convert(path_path) || raise(TypeError, "no implicit conversion of #{path_path.class} into String")
+ end
+
+ # Allow the C extension to redefine `rb_get_path` without warning.
+ alias_method :rb_get_path, :rb_get_path # rubocop:disable Lint/DuplicateMethods
  end
 end

lib/bootsnap/load_path_cache/core_ext/kernel_require.rb

@@ -6,7 +6,7 @@ module Kernel
  alias_method(:require_without_bootsnap, :require)
 
  def require(path)
- string_path = path.to_s
+ string_path = Bootsnap.rb_get_path(path)
  return false if Bootsnap::LoadPathCache.loaded_features_index.key?(string_path)
 
  resolved = Bootsnap::LoadPathCache.load_path_cache.find(string_path)
@@ -35,7 +35,7 @@ def require(path)
 
  alias_method(:load_without_bootsnap, :load)
  def load(path, wrap = false)
- if (resolved = Bootsnap::LoadPathCache.load_path_cache.find(path, try_extensions: false))
+ if (resolved = Bootsnap::LoadPathCache.load_path_cache.find(Bootsnap.rb_get_path(path), try_extensions: false))
  load_without_bootsnap(resolved, wrap)
  else
  load_without_bootsnap(path, wrap)
@@ -53,7 +53,7 @@ def autoload(const, path)
  # The challenge is that we don't control the point at which the entry gets
  # added to $LOADED_FEATURES and won't be able to hook that modification
  # since it's done in C-land.
- resolved = Bootsnap::LoadPathCache.load_path_cache.find(path)
+ resolved = Bootsnap::LoadPathCache.load_path_cache.find(Bootsnap.rb_get_path(path))
  if Bootsnap::LoadPathCache::FALLBACK_SCAN.equal?(resolved)
  autoload_without_bootsnap(const, path)
  elsif resolved == false

test/integration/kernel_test.rb

@@ -187,7 +187,7 @@ 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")
+ assert system({"BOOTSNAP_CACHE_DIR" => "tmp/cache"}, RbConfig.ruby, "-Ilib:.", "test_case.rb")
  end
 
  def setup_symlinked_files

test/load_path_cache/core_ext/kernel_require_test.rb

@@ -3,33 +3,59 @@
 require("test_helper")
 
 module Bootsnap
- module KernelRequireTest
- class KernelLoadTest < MiniTest::Test
- def setup
- @initial_dir = Dir.pwd
- @dir1 = File.realpath(Dir.mktmpdir)
- FileUtils.touch("#{@dir1}/a.rb")
- FileUtils.touch("#{@dir1}/no_ext")
- @dir2 = File.realpath(Dir.mktmpdir)
- File.open("#{@dir2}/loads.rb", "wb") { |f| f.write("load 'subdir/loaded'\nload './subdir/loaded'\n") }
- FileUtils.mkdir("#{@dir2}/subdir")
- FileUtils.touch("#{@dir2}/subdir/loaded")
- $LOAD_PATH.push(@dir1)
+ class KernelRequireTest < Minitest::Test
+ def test_uses_the_same_duck_type_as_require
+ skip("Need a working Process.fork to test in isolation") unless Process.respond_to?(:fork)
+ begin
+ assert_nil LoadPathCache.load_path_cache
+ cache = Tempfile.new("cache")
+ pid = Process.fork do
+ LoadPathCache.setup(cache_path: cache, development_mode: true)
+ dir = File.realpath(Dir.mktmpdir)
+ $LOAD_PATH.push(dir)
+ FileUtils.touch("#{dir}/a.rb")
+ stringish = mock
+ stringish.expects(:to_str).returns("a").twice # bootsnap + ruby
+ pathish = mock
+ pathish.expects(:to_path).returns(stringish).twice # bootsnap + ruby
+ assert pathish.respond_to?(:to_path)
+ require(pathish)
+ FileUtils.rm_rf(dir)
+ end
+ _, status = Process.wait2(pid)
+ assert_predicate status, :success?
+ ensure
+ cache.close
+ cache.unlink
  end
+ end
+ end
 
- def teardown
- $LOAD_PATH.pop
- Dir.chdir(@initial_dir)
- FileUtils.rm_rf(@dir1)
- FileUtils.rm_rf(@dir2)
- end
+ class KernelLoadTest < MiniTest::Test
+ def setup
+ @initial_dir = Dir.pwd
+ @dir1 = File.realpath(Dir.mktmpdir)
+ FileUtils.touch("#{@dir1}/a.rb")
+ FileUtils.touch("#{@dir1}/no_ext")
+ @dir2 = File.realpath(Dir.mktmpdir)
+ File.open("#{@dir2}/loads.rb", "wb") { |f| f.write("load 'subdir/loaded'\nload './subdir/loaded'\n") }
+ FileUtils.mkdir("#{@dir2}/subdir")
+ FileUtils.touch("#{@dir2}/subdir/loaded")
+ $LOAD_PATH.push(@dir1)
+ end
 
- def test_no_exstensions_for_kernel_load
- assert_raises(LoadError) { load "a" }
- assert(load("no_ext"))
- Dir.chdir(@dir2)
- assert(load("loads.rb"))
- end
+ def teardown
+ $LOAD_PATH.pop
+ Dir.chdir(@initial_dir)
+ FileUtils.rm_rf(@dir1)
+ FileUtils.rm_rf(@dir2)
+ end
+
+ def test_no_exstensions_for_kernel_load
+ assert_raises(LoadError) { load "a" }
+ assert(load("no_ext"))
+ Dir.chdir(@dir2)
+ assert(load("loads.rb"))
  end
  end
 end