Conversation
Add Ruby 3.4 to the CI matrix to confirm compatibility. Bump ruby/setup-ruby to v1.288.0 and regenerate lockfiles.
81486f8 to
80b0efe
Compare
Ruby 3.4 may return a different caller frame from prepended warn methods, causing strip_caller_info to fail when the frame points to warning.rb itself instead of the actual caller. Search multiple frames to find the one whose path appears in the warning string, falling back to the first frame.
80b0efe to
1b20df7
Compare
a73c2a8 to
3253fb5
Compare
Remove stubs so ActiveSupport's real deprecator.warn runs, and assert the callstack points to the caller, not deprecatable.rb itself.
Pass explicit caller_locations(1) to deprecator.warn so that ActiveSupport's extract_callstack receives a callstack starting from the actual caller, not from deprecatable.rb itself. Without this, vendored installs report deprecatable.rb as the caller because its path doesn't match ActiveSupport's ignored callstack patterns (RAILS_GEM_ROOT, LIB_DIR).
3fd0b3c to
a231adb
Compare
Assert that no frame in the callstack references deprecatable.rb, not just the first frame. This catches the bug regardless of ActiveSupport's ignored_callstack? path filtering.
caller_locations(1) only skips the current frame, but nested deprecated method calls add additional deprecatable.rb frames higher in the stack (from each wrapper's super call). Reject all deprecatable.rb frames so extract_callstack never picks them up — regardless of install method.
Let the full chain run without stubs: warn() → warning.rb → deprecator.warn. Verify the message passed to deprecator.warn has the caller path stripped and doesn't reference warning.rb.
Temporarily revert find_caller_location to the original caller_locations(1..1).first to see if CI's Ruby 3.4 fails.
Reverted test confirms the fix is needed: Ruby 3.4 + Rails 8.0 fails without it. The extra frame from prepend only manifests on that combination, not on Rails 7.2.
|
|
||
| if str =~ DEPRECATION_PATTERN # rubocop:disable Performance/RegexpMatch | ||
| message = strip_caller_info(str, caller_locations(1..1).first).strip | ||
| cloc = find_caller_location(str, caller_locations(1..5)) |
There was a problem hiding this comment.
Root cause
Uncruft::Warning is prepended onto Warning, Kernel, and Object. On Ruby 3.4 + Rails 8.0, the prepend chain adds an extra frame, shifting caller_locations(1..1).first from the actual caller to warning.rb itself.
How it was proven
- Added an unstubbed spec that lets the full chain run: warn("DEPRECATION: ... #{path}:#{lineno}") → warning.rb → deprecator.warn, then asserts the message passed to deprecator.warn has the caller path
stripped. - Temporarily reverted the fix to
caller_locations(1..1).firstand pushed to CI - Result: Ruby 3.4 + Rails 8.0 failed, all other matrix combinations passed
- Restored the fix, CI passes
The fix
Instead of assuming a fixed frame depth, search frames 1–5 for the one whose path appears in the warning string. This is resilient to frame depth changes across Ruby/Rails versions. Falls back to the
first frame if no match is found.
There was a problem hiding this comment.
We moved away from the dynamic search (clocs.detect { |cl| str.include?(cl.path) }) because while it works, it's the equivalent of Van Halen's crew just sorting through the candy bowl and quietly removing any brown M&Ms — the show goes on, but you never find out whether the promoter actually read the rider.
The dynamic search silently adapts to any frame layout change. If Ruby 3.5 shifts the callstack in a way that happens to still have a matching path somewhere in frames 1–5, it would find it and we'd never know the layout changed. Subtle issues downstream could go unnoticed.
Instead, find_caller_location now checks a version-pinned map (EXPECTED_INTERNAL_FRAMES) that catalogs exactly which internal frames are allowed at clocs[0] for each Ruby/Rails combo. It only skips a frame if it's explicitly expected for that version — no silent adaptation. If a brown M&M shows up (an unexpected frame, or an unknown Ruby/Rails combo), the code doesn't quietly handle it. CI goes red, a developer investigates, adds the entry, and we're back to green.
The tradeoff is intentional: on an uncataloged combo, path stripping degrades benignly (extra path info in the message) rather than silently masking a frame layout change. The brown M&M catches the problem before anything subtler slips through.
| Uncruft.deprecator.warn( | ||
| message, | ||
| caller_locations(1).reject { |loc| loc.path.end_with?("deprecatable.rb") }, | ||
| ) |
There was a problem hiding this comment.
When uncruft is installed via any non-rubygems method (vendored path, GitHub/git source, local path, or custom BUNDLE_PATH), calling a deprecated method reports deprecatable.rb as the caller instead of
the actual call site:
RuntimeError:
DEPRECATION WARNING: Please stop using a_method from MyCode.
(called from block (2 levels) in deprecate_method at
../../../vendor/gems/uncruft/lib/uncruft/deprecatable.rb):19
This is not a regression — it's a gap that has always existed, masked by rubygems installs.
Root cause
Uncruft.deprecator.warn(message) without an explicit callstack lets ActiveSupport compute one via caller_locations(2)
(https://github.com/rails/rails/blob/main/activesupport/lib/active_support/deprecation/reporting.rb#L21). ActiveSupport's extract_callstack then picks the first frame not matching RAILS_GEM_ROOT or
LIB_DIR:
def ignored_callstack?(path)
path.start_with?(RAILS_GEM_ROOT, LIB_DIR) || path.include?("<internal:")
end- Rubygems install: uncruft's path lives under
LIB_DIR(e.g.~/.mise/installs/ruby/3.4.8/lib/ruby/gems/...), sodeprecatable.rbis skipped — bug is masked - Any other install: the path doesn't match either ignore pattern, so
extract_callstackpicksdeprecatable.rbas the "offending line"
Why a fixed frame depth isn't enough
Nested deprecated method calls (e.g. deprecated_method_a calls deprecated_method_b) add multiple deprecatable.rb frames to the callstack. caller_locations(1) only skips the current frame, leaving frames from outer wrappers' super calls:
FRAME 0: deprecatable.rb:18 ← current warn call (skipped by caller_locations(1))
FRAME 1: deprecatable.rb:20 ← super from outer deprecated method wrapper
FRAME 2: my_code.rb:10 ← actual caller
FRAME 3: deprecatable.rb:20 ← super from yet another wrapper
How it was proven
- Applied
caller_locations(1)fix and vendored into an application — CI still failed withdeprecatable.rbas the caller - Added debug logging (
caller_locations(0, 10)) to the vendored gem and pushed to application's CI - Debug output revealed nested
deprecatable.rbframes from chained deprecated method calls - Applied reject fix — application's CI passed
The fix
This removes all deprecatable.rb frames from the callstack. extract_callstack still runs its filtering logic on the remaining frames — we're not bypassing anything, just giving it a cleaner input.
Why the gem's own specs don't catch this
When running uncruft's test suite, the gem is loaded from its own source tree via bundler/setup. The path resolves under LIB_DIR, so ignored_callstack? skips deprecatable.rb and the correct caller is
reported. The bug only surfaces when the gem is loaded from a path outside LIB_DIR.
Precedent in ActiveSupport
ActiveSupport itself passes explicit caller_locations to deprecator.warn in multiple places:
There was a problem hiding this comment.
Pivoted from filtering by filename (end_with?("deprecatable.rb")) to filtering by gem root path (start_with?(Uncruft::GEM_ROOT)), following the same pattern ActiveSupport uses for its own deprecation reporting.
This filters ALL uncruft frames from the callstack (not just deprecatable.rb), making it resilient to file renames and covering edge cases like nested deprecated method calls that chain through multiple gem files.
Temporary debug logging to compare frame layout across Ruby 3.2/3.3/3.4 + Rails 7.2/8.0 matrix in CI.
6d41145 to
36d611f
Compare
Prism polyfill/warn.rb injects +1 frame on Ruby 3.4 + Rails 8.0. find_caller_location search logic handles this correctly.
Replace the dynamic search in find_caller_location with a version-pinned lookup, inspired by the Van Halen brown M&M test: for every supported Ruby/Rails combo, we catalog exactly which internal frames (if any) are allowed at clocs[0]. If the frame layout changes — even benignly — CI fails, signaling that Ruby internals may have shifted and the combo needs investigation. EXPECTED_INTERNAL_FRAMES pins each Ruby/Rails pair to a specific set of allowed internal frames. find_caller_location only skips a frame if it matches the pinned list for the current version. Unknown combos never skip anything — the canary test catches them. Three canary test layers enforce this: 1. "has a version-pinned entry" — fails on unlisted combos 2. "only has expected internal frames" — fails on unexpected frames 3. "strips its path from the message" — verifies resolution works Currently only Ruby 3.4 + Rails 8.0 has a non-empty entry (prism/polyfill/warn.rb), which may or may not be present depending on the call path (warn vs Kernel.warn).
Replace the filename-specific filter (`end_with?("deprecatable.rb")`)
with a gem-root-based filter (`start_with?(Uncruft::GEM_ROOT)`).
This follows the same pattern ActiveSupport uses for its own
deprecation reporting:
RAILS_GEM_ROOT = File.expand_path("../../../..", __dir__) + "/"
def ignored_callstack?(path)
path.start_with?(RAILS_GEM_ROOT, LIB_DIR) || path.include?("<internal:")
end
(activesupport/lib/active_support/deprecation/reporting.rb:143-149)
https://github.com/rails/rails/blob/v8.0.2.1/activesupport/lib/active_support/deprecation/reporting.rb#L143-L149
The GEM_ROOT constant uses `File.expand_path` + trailing "/" which:
- Resolves correctly for rubygems, vendored, git source, and path
gem installations (since `__dir__` resolves at load time)
- Prevents partial prefix matches (e.g. `uncruft-extras/` won't
match `uncruft/`)
- Filters ALL gem frames, not just deprecatable.rb, making the
filter resilient to file renames
smudge
left a comment
There was a problem hiding this comment.
domain LGTM && platform LGTM -- thanks for the rework to encode a bit more of the known behaviors rather than relying on filtering logic every time 👍
Summary
Warning#warn callerframe resolution on Ruby 3.4 + Rails 8.0deprecatable.rbas the caller for non-rubygems installs (bonus).Test plan