Skip to content

Match absolute paths in the irb_require already-loaded check - #1254

Open
youdie006 wants to merge 2 commits into
ruby:masterfrom
youdie006:irb-require-absolute-loaded-features
Open

youdie006 wants to merge 2 commits into
ruby:masterfrom
youdie006:irb-require-absolute-loaded-features

Conversation

@youdie006

Copy link
Copy Markdown

#1253 anchored the already-loaded check with \A, but $LOADED_FEATURES holds absolute paths for anything the real require loaded, so the guard can no longer match them:

require 'json'          # $" gets "/usr/lib/ruby/3.2.0/json.rb"
irb_require 'json'      # the guard misses, so this loads and evaluates it again

Measured against the shapes that actually appear in $":

file_name $" entry before #1253 on master with this change
foo /tmp/x/foo.rb matches misses matches
foo foo.rb (what irb_require itself pushes) matches matches matches
foo /tmp/x/foo2.rb matches (the #1252 bug) misses misses
foo /tmp/x/barfoo.rb matches (same bug) misses misses

Anchoring on a path boundary rather than the string start keeps both: absolute entries match again, and the foo/foo2 substring match #1253 was fixing stays rejected.

Verification

Base d05df55, Docker ruby:3.4.

row load.rb md5 new test #1253's test_irb_require_file_matches_exact_name
master 3b81c5e261 1 failure passes
this PR f15815509a passes passes
drop the trailing \z ec0bb20c9e 1 failure passes
drop the boundary entirely 4f7a97b201 1 failure passes

The two mutants are killed by different rows of the new test: the \z by the irb_require 'irb_require' case, the boundary by the irb_require 'probe' case.

Full suite with the fix: 409 tests, 2496 assertions, 1 failure. That one is test_use_tracer_enabled_when_gem_is_available, which also fails on unmodified d05df55 with the unmodified test file (408 tests, 1 failure) because the tracer gem is not installed in the container; load.rb does not mention tracer. rubocop: 116 files inspected, no offenses.

Not run here: rake test_in_isolation, the yamatanooroti integration tests (need libvterm, no sudo), the debug-compat job, and the Ruby 2.7 to 3.3 and prism 1.3.0/1.7.0 matrix rows. The change is a regex literal, so cross-version risk is low, but I have not verified it.

One observation while writing the test, offered as something worth extending rather than a complaint. test_irb_require_file_matches_exact_name still passes with the complete pre-#1253 regex restored, so it does not reproduce #1252: it loads foo then foo2, and foo is not a substring of foo.rb at that point. Reproducing #1252 needs foo2 loaded first, since foo is a substring of foo2.rb.

I also left irb_require pushing relative names into $" at lib/irb/command/load.rb:55 and :66 alone, and did not widen the extension list beyond the existing (?:\.o|\.rb)?, so a .so feature still will not match. Both seemed like separate scope.


Disclosure: written with the help of Claude (an AI assistant). Every number above is from runs on my machine.

$LOADED_FEATURES holds absolute paths for anything the real require
loaded, so anchoring the check with \A made it unable to match them.
require 'x' followed by irb_require 'x' re-evaluates x.rb in the session.

Anchor on a path boundary instead, which still rejects the foo/foo2
substring match ruby#1253 was fixing.
The probe file read an uninitialized global, which warns on stderr under
-w and tripped the test's own assert_empty(err) on the CI rows that
enable warnings.
@youdie006

Copy link
Copy Markdown
Author

Pushed a fix for the red rows, which were mine: the probe file read $irb_require_probe_count before it was assigned, so under -w Ruby warns on stderr and the test's own assert_empty(err) caught it. The counter is now initialised in the session first.

My local run had not enabled warnings, which is why I missed it; re-ran the whole matrix with RUBYOPT=-w this time.

row new test
master 1 failure
this PR passes
drop the trailing \z 1 failure
drop the boundary 1 failure

Full suite under -w: 409 tests, 2496 assertions, 1 failure, which is the pre-existing test_use_tracer_enabled_when_gem_is_available (the tracer gem is absent in my container; unmodified d05df55 gives 408 tests and the same single failure).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant