Repository navigation
Conversation
Rack::Multipart::UploadedFile forwards missing methods to the wrapped tempfile, but the delegator was never flagged with ruby2_keywords. On Ruby 3.0+, where keywords are separated from a trailing Hash, the keywords are rebuilt as a positional Hash before reaching the tempfile: uploaded_file.readlines(chomp: true) # => TypeError: no implicit conversion of Hash into Integer uploaded_file.gets(chomp: true) # => TypeError: no implicit conversion of Hash into Integer Ruby 2.4 through 2.6 are unaffected, since they do not distinguish a trailing Hash from keywords, which is why this went unnoticed. Flag the delegator, matching BodyProxy#method_missing and MockResponse::Cookie#method_missing. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
… 3.0+ Ruby proposes deprecating and removing ruby2_keywords (Feature #22205). Rack's calls are guarded by respond_to?, so removal would not raise: it would silently skip, and middleware and delegators taking keyword arguments would start receiving a positional Hash instead. Forward keywords explicitly on Ruby 3.0+ at all four sites: Builder#use, BodyProxy#method_missing, MockResponse::Cookie#method_missing and UploadedFile#method_missing. The definitions are version-gated rather than replaced outright. Before Ruby 3.0 a trailing positional Hash is not distinguished from keywords, so a **kwargs parameter captures an options Hash and mangles it: an empty Hash is dropped entirely, and a Hash mixing Symbol and non-Symbol keys is split in two. The pre-3.0 branch keeps ruby2_keywords, which exists on every Ruby that branch can run on. Argument forwarding is byte-identical to the current implementation on 2.4, 2.6, 2.7, 3.0 and 3.4. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
`RUBY_VERSION >= '3.0'` is a string comparison, so it is false on Ruby 10: "10.0.0" sorts before "3.0". The gate would then take the pre-3.0 branch, where `respond_to?(:ruby2_keywords, true)` is also false once Ruby removes the method, so the flag is silently skipped and keyword forwarding breaks again. That is the exact failure this change exists to prevent, so compare the major version numerically here. The string form is kept elsewhere in Rack, where a far-future Ruby only skips an optional method definition. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Thank you for the pull request. I think if Ruby decides to deprecate |
|
Yes, It's the same code at either floor, so 2.7 vs 3.0 stays a policy call rather than a technical one. Explicit Cleaner diff too: no Verification
for v in 2.7 3.0 3.4; do printf "ruby:%-5s " "$v"; docker run --rm ruby:$v ruby -e '
class Dots; def use(m, ...); @p = proc { |app| m.new(app, ...) }; end; def to_app(a); @p[a]; end; end
class Kw; def use(m, *a, **k, &b); @p = proc { |app| m.new(app, *a, **k, &b) }; end; def to_app(a); @p[a]; end; end
class Flag; def use(m, *a, &b); @p = proc { |app| m.new(app, *a, &b) }; end; ruby2_keywords(:use); def to_app(a); @p[a]; end; end
class Pos; def initialize(app, options); $out << options; end; end
o = {}
[Dots, Kw, Flag].each { |c| $out = []
[->(b){ b.use(Pos, o) }, ->(b){ b.use(Pos, {"s" => 1, sym: 2}) }, ->(b){ b.use(Pos, {a: 1}) }].each do |fn|
b = c.new; begin; fn.call(b); b.to_app(:app); rescue ArgumentError; $out << :ArgumentError; end
end
puts "#{c}: #{$out.inspect}" }
'; echo; done2.4-2.6 raise SyntaxError on Also checked against unpatched |
2 participants
Draft, stacked on #2482. The first commit here is that PR; the diff reduces to a single commit once it merges.
Rack calls
ruby2_keywordsin four places, all behindrespond_to?guards. Ruby proposes deprecating and removing it (Feature #22205, currently Open, no target version). Because of the guards, removal would not raise. It would silently skip, and middleware and delegators taking keyword arguments would start receiving a positional Hash.This forwards keywords explicitly on Ruby 3.0+ in
Builder#use,BodyProxy#method_missing,MockResponse::Cookie#method_missingandUploadedFile#method_missing. Argument forwarding is unchanged on every supported Ruby.Why the definitions are version-gated
Adding
**kwargsoutright regresses Ruby 2.4 to 2.7, where a trailing positional Hash is not distinguished from keywords:**kwargsuse M, optswhereopts == {}{}ArgumentError (given 1, expected 2)use M, {"a" => 1, :b => 2}{"a"=>1, :b=>2}ArgumentErrorAn empty options Hash is dropped entirely; a Hash mixing Symbol and non-Symbol keys is split in two. So the pre-3.0 branch keeps
ruby2_keywords, which exists on every Ruby that branch can run on. Follows theif/elseversion-gate style ofUtils#clock_timeandUtils#escape_html.If Rack ever drops Ruby < 3.0
Every gate collapses and each site becomes a single definition with no duplication and no
:nocov:. Not proposing that here; #1601 settled the policy and this patch deliberately keeps 2.4 working.Verification
Compared the values actually received by middleware and delegated objects against unpatched
mainon 2.4, 2.6, 2.7, 3.0 and 3.4: byte-identical for keyword arguments, positional Hash, empty Hash, mixed-key Hash, no-argument middleware and splat middleware. Same forBodyProxy(includingclose-on-raise,to_str, andmethod(:x).call), forUploadedFile, and for consumers that subclassBuilderor prepend a module overuse.Builder#mapflush ordering verified unchanged across fourmap/useinterleavings.Three tests added. They fail against a naive
**kwargsversion (2 errors on 2.6) and against theruby2_keywords-removed scenario.Notes
Builder#use.parametersgains[:keyrest, :kwargs]on 3.0+. Only observable API change.Builder#usenow delegates to a privateadd_middleware, a pure extraction of the existing@map/@uselogic, so the two version branches stay one line each.usewith a bare splat already lose keywords on Ruby 3.0+. Pre-existing, unchanged here.SPEC.rdoccovers the server/application protocol, not the Builder DSL, and the call syntax is unchanged.No urgency, since #22205 is still Open. Happy to hold this until it is accepted, or to split it per file if that reviews more easily.