Restore Bootsnap fallback for RBS rewriting#2630
Conversation
| assert_success_status(result) | ||
| end | ||
|
|
||
| it "preserves RBS comment rewriting when the host sets up Bootsnap without TAPIOCA_RBS_CACHE" do |
There was a problem hiding this comment.
Figured the test is worth it given last time this bug went through. It uses a custom compiler so that we don't boot rails but an existing compiler could also show this issue.
|
|
||
| require "require-hooks/setup" | ||
|
|
||
| unless rbs_cache_enabled |
There was a problem hiding this comment.
Used if and unless so that require "require-hooks/setup" call isn't duplicated.
There was a problem hiding this comment.
Eh, I think a single if/else with a repeated require "require-hooks/setup" would be more clear, but that's just me
There was a problem hiding this comment.
Does the order of requires matter here?
There was a problem hiding this comment.
Does the order of requires matter here?
Yes, require-hooks/setup chooses a mode based on bootsnap being loaded or not. In TAPIOCA_RBS_CACHE (if) we want it after loading bootsnap. In else, we want it before bootsnap is loaded.
| module InstructionSequenceMixin | ||
| #: (String) -> RubyVM::InstructionSequence | ||
| def load_iseq(path) | ||
| super if defined?(super) |
There was a problem hiding this comment.
Maybe just clarify:
| super if defined?(super) | |
| super if defined?(super) # Disable Bootsnaps' hook, but trigger any others |
|
|
||
| require "require-hooks/setup" | ||
|
|
||
| unless rbs_cache_enabled |
There was a problem hiding this comment.
Eh, I think a single if/else with a repeated require "require-hooks/setup" would be more clear, but that's just me
PR #2617 moved the old `load_iseq` bypass behind `TAPIOCA_RBS_CACHE=1`, so hosts that call `Bootsnap.setup` without the opt-in cache bypass `RequireHooks` and lose RBS comment signatures. Restore the default bypass after `require-hooks` setup while keeping the opt-in cache path intact. Add a CLI regression test that sets up host `Bootsnap` without `TAPIOCA_RBS_CACHE` and verifies an RBS comment produces a typed RBI. Co-authored-by: Julia Boutin <julia.boutin@shopify.com>
KaanOzkan
force-pushed
the
kaan/fix-rbs-rewriter-bootsnap-default
branch
from
b768059 to
042fc36
Compare
3 participants
Fixes a bug on unreleased
mainthat @dejmedus pointed out in the new rewriter implementation when the ENV variable is omitted.Motivation
PR #2617 moved the old
load_iseqbypass behindTAPIOCA_RBS_CACHE=1, so hosts that callBootsnap.setupwithout the opt-in cache bypassRequireHooksand lose RBS comment signatures. Restore the default bypass afterrequire-hookssetup while keeping the opt-in cache path intact.Implementation
Bring back the overload from
tapioca/lib/tapioca/rbs/rewriter.rb
Lines 23 to 37 in 3fe6bb6
Tests
Included