From 671d1d4dc13526e09d6ac956bfafc255561c46d8 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Fri, 28 Jan 2022 15:40:01 +0100 Subject: [PATCH] Bind `rb_get_path` to better respect Kernel#require duck-type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Kernel#require turns its argument into a path by calling `to_path` if it responds to it, and then into a String (the argument or the return value of `to_path`) by using implicit conversion (`to_str`). Co-authored-by: Étienne Barrié --- CHANGELOG.md | 6 +- ext/bootsnap/bootsnap.c | 9 +++ lib/bootsnap.rb | 11 +++ .../core_ext/kernel_require.rb | 6 +- test/integration/kernel_test.rb | 2 +- .../core_ext/kernel_require_test.rb | 74 +++++++++++++------ 6 files changed, 79 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 209560e2..94a15b7b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/ext/bootsnap/bootsnap.c b/ext/bootsnap/bootsnap.c index 33e9b76d..fd97709a 100644 --- a/ext/bootsnap/bootsnap.c +++ b/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_. * @@ -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")); diff --git a/lib/bootsnap.rb b/lib/bootsnap.rb index f49dabcd..13000478 100644 --- a/lib/bootsnap.rb +++ b/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 diff --git a/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb b/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb index c06960fa..16a5749c 100644 --- a/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb +++ b/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 diff --git a/test/integration/kernel_test.rb b/test/integration/kernel_test.rb index e9410255..7c1abb8a 100644 --- a/test/integration/kernel_test.rb +++ b/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 diff --git a/test/load_path_cache/core_ext/kernel_require_test.rb b/test/load_path_cache/core_ext/kernel_require_test.rb index 9fc89ba4..072f0f04 100644 --- a/test/load_path_cache/core_ext/kernel_require_test.rb +++ b/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