From bd26d93d5634efda89b4c47b447a2779566af5f8 Mon Sep 17 00:00:00 2001 From: Malcolm O'Hare Date: Mon, 15 Jan 2024 13:30:12 -0500 Subject: [PATCH 1/5] Allow string keys for method kwarg splats --- .../support/method_signature_verifier.rb | 6 +-- .../support/method_signature_verifier_spec.rb | 54 ++++++++++++++++--- 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/lib/rspec/support/method_signature_verifier.rb b/lib/rspec/support/method_signature_verifier.rb index f35243038..dd6bb971e 100644 --- a/lib/rspec/support/method_signature_verifier.rb +++ b/lib/rspec/support/method_signature_verifier.rb @@ -84,7 +84,7 @@ def invalid_kw_args_from(given_kw_args) def has_kw_args_in?(args) Hash === args.last && could_contain_kw_args?(args) && - (args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) }) + (args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) || x.is_a?(String) }) end # Without considering what the last arg is, could it @@ -365,12 +365,12 @@ def unlimited_args? def split_args(*args) kw_args = if @signature.has_kw_args_in?(args) last = args.pop - non_kw_args = last.reject { |k, _| k.is_a?(Symbol) } + non_kw_args = last.reject { |k, _| k.is_a?(Symbol) || k.is_a?(String) } if non_kw_args.empty? last.keys else args << non_kw_args - last.select { |k, _| k.is_a?(Symbol) }.keys + last.select { |k, _| k.is_a?(Symbol) || k.is_a(String) }.keys end else [] diff --git a/spec/rspec/support/method_signature_verifier_spec.rb b/spec/rspec/support/method_signature_verifier_spec.rb index d342425a1..e995f73a4 100644 --- a/spec/rspec/support/method_signature_verifier_spec.rb +++ b/spec/rspec/support/method_signature_verifier_spec.rb @@ -384,17 +384,21 @@ def arity_kw(x, y = {}, z:2); end expect(valid?(nil, :a => 1)).to eq(false) end - it 'treats symbols as keyword arguments and the rest as optional argument' do - expect(valid?(nil, 'a' => 1)).to eq(true) - expect(valid?(nil, 'a' => 1, :z => 3)).to eq(true) - expect(valid?(nil, 'a' => 1, :b => 3)).to eq(false) - expect(valid?(nil, 'a' => 1, :b => 2, :z => 3)).to eq(false) + it 'treats symbols as keyword arguments' do + expect(valid?(nil, :z => 3)).to eq(true) + expect(valid?(nil, :b => 3)).to eq(false) + expect(valid?(nil, :b => 2, :z => 3)).to eq(false) + end + + it 'treats string keys as invalid keyword arguments' do + expect(valid?(nil, 'a' => 1)).to eq(false) + expect(valid?(nil, 'a' => 1, :z => 3)).to eq(false) end it 'mentions the invalid keyword args in the error', :pending => RSpec::Support::Ruby.jruby? && !RSpec::Support::Ruby.jruby_9000? do expect(error_for(1, 2, :a => 0)).to eq("Invalid keyword arguments provided: a") expect(error_for(1, :a => 0)).to eq("Invalid keyword arguments provided: a") - expect(error_for(1, 'a' => 0, :b => 0)).to eq("Invalid keyword arguments provided: b") + expect(error_for(1, 'a' => 0, :b => 0)).to eq("Invalid keyword arguments provided: a, b") end it 'describes invalid arity precisely' do @@ -656,6 +660,41 @@ def arity_required_kw_splat(w, *x, y:, z:, a: 'default'); end end end + describe 'a method with only a keyword arg splat' do + eval <<-RUBY + def arity_kw_arg_splat(**rest); end + RUBY + + let(:test_method) { method(:arity_kw_arg_splat) } + + it 'allows undeclared keyword args' do + expect(valid?(:x => 1)).to eq(true) + expect(valid?(:x => 1, 'y' => 2)).to eq(true) + end + + it 'mentions the required kw args and keyword splat in the description' do + expect(signature_description).to \ + eq("any additional keyword args") + end + + describe 'with an expectation object' do + it 'allows zero non-kw args' do + expect(validate_expectation 0).to eq(true) + expect(validate_expectation 1).to eq(false) + expect(validate_expectation 0, 0).to eq(true) + expect(validate_expectation 0, 1).to eq(false) + end + + it 'does not match unlimited arguments' do + expect(validate_expectation :unlimited_args).to eq(false) + end + + it 'matches arbitrary keywords' do + expect(validate_expectation :arbitrary_kw_args).to eq(true) + end + end + end + describe 'a method with required keyword arguments and a keyword arg splat' do eval <<-RUBY def arity_kw_arg_splat(x:, **rest); end @@ -666,6 +705,7 @@ def arity_kw_arg_splat(x:, **rest); end it 'allows extra undeclared keyword args' do expect(valid?(:x => 1)).to eq(true) expect(valid?(:x => 1, :y => 2)).to eq(true) + expect(valid?(:x => 1, :y => 2, 'z' => 3)).to eq(true) end it 'mentions missing required keyword args in the error' do @@ -730,7 +770,9 @@ def arity_kw_arg_splat(x, **rest); end it 'allows a single arg and any number of keyword args' do expect(valid?(nil)).to eq(true) expect(valid?(nil, :x => 1)).to eq(true) + expect(valid?(nil, 'x' => 1)).to eq(true) expect(valid?(nil, :x => 1, :y => 2)).to eq(true) + expect(valid?(nil, :x => 1, :y => 2, 'z' => 3)).to eq(true) expect(valid?(:x => 1)).to eq(true) expect(valid?(nil, {})).to eq(true) From ab28606bff3ff5fb4baa70b9ae065ed317f7f1bf Mon Sep 17 00:00:00 2001 From: Malcolm O'Hare Date: Tue, 16 Jan 2024 10:02:09 -0500 Subject: [PATCH 2/5] refactor kwargs filtering code path --- lib/rspec/support/method_signature_verifier.rb | 9 +-------- spec/rspec/support/method_signature_verifier_spec.rb | 12 ++++++++++++ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/rspec/support/method_signature_verifier.rb b/lib/rspec/support/method_signature_verifier.rb index dd6bb971e..05fd07e89 100644 --- a/lib/rspec/support/method_signature_verifier.rb +++ b/lib/rspec/support/method_signature_verifier.rb @@ -364,14 +364,7 @@ def unlimited_args? def split_args(*args) kw_args = if @signature.has_kw_args_in?(args) - last = args.pop - non_kw_args = last.reject { |k, _| k.is_a?(Symbol) || k.is_a?(String) } - if non_kw_args.empty? - last.keys - else - args << non_kw_args - last.select { |k, _| k.is_a?(Symbol) || k.is_a(String) }.keys - end + args.pop.select { |k, _| k.is_a?(Symbol) || k.is_a?(String) }.keys else [] end diff --git a/spec/rspec/support/method_signature_verifier_spec.rb b/spec/rspec/support/method_signature_verifier_spec.rb index e995f73a4..f0d0bfff1 100644 --- a/spec/rspec/support/method_signature_verifier_spec.rb +++ b/spec/rspec/support/method_signature_verifier_spec.rb @@ -670,6 +670,18 @@ def arity_kw_arg_splat(**rest); end it 'allows undeclared keyword args' do expect(valid?(:x => 1)).to eq(true) expect(valid?(:x => 1, 'y' => 2)).to eq(true) + expect(valid?('y' => 2)).to eq(true) + end + + it 'does not allow kw args of only unsupported types' do + expect(valid?(3 => 1)).to eq(false) + expect(valid?({"a":"b"} => 1)).to eq(false) + end + + it "ignores kwargs with invalid types if there are any with valid types" do + expect(valid?(:x => 1, 3 => 10, {"a":"b"} => 1)).to eq(true) + expect(valid?(:x => 1, 'y' => 2, 3 => 10, {"a":"b"} => 1)).to eq(true) + expect(valid?('y' => 2, 3 => 10, {"a":"b"} => 1)).to eq(true) end it 'mentions the required kw args and keyword splat in the description' do From a150ae31230f6a903b29a0a9e7cc4edab88ba406 Mon Sep 17 00:00:00 2001 From: Malcolm O'Hare Date: Tue, 16 Jan 2024 11:36:39 -0500 Subject: [PATCH 3/5] remove kwargs parameter that had a hash as a key as its not valid for legacy ruby --- spec/rspec/support/method_signature_verifier_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/rspec/support/method_signature_verifier_spec.rb b/spec/rspec/support/method_signature_verifier_spec.rb index f0d0bfff1..bf04eb695 100644 --- a/spec/rspec/support/method_signature_verifier_spec.rb +++ b/spec/rspec/support/method_signature_verifier_spec.rb @@ -679,9 +679,9 @@ def arity_kw_arg_splat(**rest); end end it "ignores kwargs with invalid types if there are any with valid types" do - expect(valid?(:x => 1, 3 => 10, {"a":"b"} => 1)).to eq(true) - expect(valid?(:x => 1, 'y' => 2, 3 => 10, {"a":"b"} => 1)).to eq(true) - expect(valid?('y' => 2, 3 => 10, {"a":"b"} => 1)).to eq(true) + expect(valid?(:x => 1, 3 => 10)).to eq(true) + expect(valid?(:x => 1, 'y' => 2, 3 => 10)).to eq(true) + expect(valid?('y' => 2, 3 => 10)).to eq(true) end it 'mentions the required kw args and keyword splat in the description' do From 7f30ba303e309b2afc56fe44e67631e9e5b69a72 Mon Sep 17 00:00:00 2001 From: Malcolm O'Hare Date: Wed, 17 Jan 2024 12:52:25 -0500 Subject: [PATCH 4/5] version gate the change and the specs --- .../support/method_signature_verifier.rb | 11 ++- lib/rspec/support/ruby_features.rb | 11 +++ .../support/method_signature_verifier_spec.rb | 77 +++++++++++-------- 3 files changed, 65 insertions(+), 34 deletions(-) diff --git a/lib/rspec/support/method_signature_verifier.rb b/lib/rspec/support/method_signature_verifier.rb index 05fd07e89..21d0bb2da 100644 --- a/lib/rspec/support/method_signature_verifier.rb +++ b/lib/rspec/support/method_signature_verifier.rb @@ -84,7 +84,7 @@ def invalid_kw_args_from(given_kw_args) def has_kw_args_in?(args) Hash === args.last && could_contain_kw_args?(args) && - (args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) || x.is_a?(String) }) + (args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) || RubyFeatures.kw_arg_separation? }) end # Without considering what the last arg is, could it @@ -364,7 +364,14 @@ def unlimited_args? def split_args(*args) kw_args = if @signature.has_kw_args_in?(args) - args.pop.select { |k, _| k.is_a?(Symbol) || k.is_a?(String) }.keys + last = args.pop + non_kw_args = last.reject { |k, _| k.is_a?(Symbol) || RubyFeatures.kw_arg_separation? } + if non_kw_args.empty? + last.keys + else + args << non_kw_args + last.select { |k, _| k.is_a?(Symbol) || RubyFeatures.kw_arg_separation? }.keys + end else [] end diff --git a/lib/rspec/support/ruby_features.rb b/lib/rspec/support/ruby_features.rb index ee98c1795..7ccdb7e26 100644 --- a/lib/rspec/support/ruby_features.rb +++ b/lib/rspec/support/ruby_features.rb @@ -106,6 +106,17 @@ def supports_syntax_suggest? end end + if RUBY_VERSION.to_f >= 3.0 + # https://rubyreferences.github.io/rubychanges/3.0.html#keyword-arguments-are-now-fully-separated-from-positional-arguments + def kw_arg_separation? + true + end + else + def kw_arg_separation? + false + end + end + if RUBY_VERSION.to_f >= 2.7 def supports_taint? false diff --git a/spec/rspec/support/method_signature_verifier_spec.rb b/spec/rspec/support/method_signature_verifier_spec.rb index bf04eb695..670cac78e 100644 --- a/spec/rspec/support/method_signature_verifier_spec.rb +++ b/spec/rspec/support/method_signature_verifier_spec.rb @@ -384,21 +384,37 @@ def arity_kw(x, y = {}, z:2); end expect(valid?(nil, :a => 1)).to eq(false) end - it 'treats symbols as keyword arguments' do - expect(valid?(nil, :z => 3)).to eq(true) - expect(valid?(nil, :b => 3)).to eq(false) - expect(valid?(nil, :b => 2, :z => 3)).to eq(false) - end - - it 'treats string keys as invalid keyword arguments' do - expect(valid?(nil, 'a' => 1)).to eq(false) - expect(valid?(nil, 'a' => 1, :z => 3)).to eq(false) - end + if RubyFeatures.kw_arg_separation? + context "when ruby has kw arg separation" do + it 'treats symbols as keyword arguments' do + expect(valid?(nil, nil, :z => 1)).to eq(true) + expect(valid?(nil, :z => 1)).to eq(true) + end + + it 'fails to match string keys as kw args' do + expect(valid?(nil, nil, 'z' => 1)).to eq(false) + expect(valid?(nil, 'z' => 1)).to eq(false) + end + + it 'mentions the invalid keyword args in the error', :pending => RSpec::Support::Ruby.jruby? && !RSpec::Support::Ruby.jruby_9000? do + expect(error_for(1, 2, :a => 0)).to eq("Invalid keyword arguments provided: a") + expect(error_for(1, :a => 0)).to eq("Invalid keyword arguments provided: a") + expect(error_for(1, 'a' => 0, :b => 0)).to eq("Invalid keyword arguments provided: a, b") + end + end + else + it 'treats symbols as keyword arguments and the rest as optional argument' do + expect(valid?(nil, 'a' => 1)).to eq(true) + expect(valid?(nil, 'a' => 1, :z => 3)).to eq(true) + expect(valid?(nil, 'a' => 1, :b => 3)).to eq(false) + expect(valid?(nil, 'a' => 1, :b => 2, :z => 3)).to eq(false) + end - it 'mentions the invalid keyword args in the error', :pending => RSpec::Support::Ruby.jruby? && !RSpec::Support::Ruby.jruby_9000? do - expect(error_for(1, 2, :a => 0)).to eq("Invalid keyword arguments provided: a") - expect(error_for(1, :a => 0)).to eq("Invalid keyword arguments provided: a") - expect(error_for(1, 'a' => 0, :b => 0)).to eq("Invalid keyword arguments provided: a, b") + it 'mentions the invalid keyword args in the error', :pending => RSpec::Support::Ruby.jruby? && !RSpec::Support::Ruby.jruby_9000? do + expect(error_for(1, 2, :a => 0)).to eq("Invalid keyword arguments provided: a") + expect(error_for(1, :a => 0)).to eq("Invalid keyword arguments provided: a") + expect(error_for(1, 'a' => 0, :b => 0)).to eq("Invalid keyword arguments provided: b") + end end it 'describes invalid arity precisely' do @@ -667,21 +683,18 @@ def arity_kw_arg_splat(**rest); end let(:test_method) { method(:arity_kw_arg_splat) } - it 'allows undeclared keyword args' do - expect(valid?(:x => 1)).to eq(true) - expect(valid?(:x => 1, 'y' => 2)).to eq(true) - expect(valid?('y' => 2)).to eq(true) - end - - it 'does not allow kw args of only unsupported types' do - expect(valid?(3 => 1)).to eq(false) - expect(valid?({"a":"b"} => 1)).to eq(false) - end - - it "ignores kwargs with invalid types if there are any with valid types" do - expect(valid?(:x => 1, 3 => 10)).to eq(true) - expect(valid?(:x => 1, 'y' => 2, 3 => 10)).to eq(true) - expect(valid?('y' => 2, 3 => 10)).to eq(true) + if RubyFeatures.kw_arg_separation? + it 'allows undeclared keyword args' do + expect(valid?(:x => 1)).to eq(true) + expect(valid?(:x => 1, 'y' => 2)).to eq(true) + expect(valid?('y' => 2)).to eq(true) + end + else + it 'allows undeclared symbol keyword args' do + expect(valid?(:x => 1)).to eq(true) + expect(valid?(:x => 1, 'y' => 2)).to eq(false) + expect(valid?('y' => 2)).to eq(false) + end end it 'mentions the required kw args and keyword splat in the description' do @@ -717,7 +730,7 @@ def arity_kw_arg_splat(x:, **rest); end it 'allows extra undeclared keyword args' do expect(valid?(:x => 1)).to eq(true) expect(valid?(:x => 1, :y => 2)).to eq(true) - expect(valid?(:x => 1, :y => 2, 'z' => 3)).to eq(true) + expect(valid?(:x => 1, :y => 2, 'z' => 3)).to eq(RubyFeatures.kw_arg_separation?) end it 'mentions missing required keyword args in the error' do @@ -782,9 +795,9 @@ def arity_kw_arg_splat(x, **rest); end it 'allows a single arg and any number of keyword args' do expect(valid?(nil)).to eq(true) expect(valid?(nil, :x => 1)).to eq(true) - expect(valid?(nil, 'x' => 1)).to eq(true) + expect(valid?(nil, 'x' => 1)).to eq(RubyFeatures.kw_arg_separation?) expect(valid?(nil, :x => 1, :y => 2)).to eq(true) - expect(valid?(nil, :x => 1, :y => 2, 'z' => 3)).to eq(true) + expect(valid?(nil, :x => 1, :y => 2, 'z' => 3)).to eq(RubyFeatures.kw_arg_separation?) expect(valid?(:x => 1)).to eq(true) expect(valid?(nil, {})).to eq(true) From 30dea7db8130232dc40c9e544e6890f1aaac8d89 Mon Sep 17 00:00:00 2001 From: Malcolm O'Hare Date: Wed, 17 Jan 2024 12:58:10 -0500 Subject: [PATCH 5/5] make impl cleaner --- lib/rspec/support/method_signature_verifier.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/rspec/support/method_signature_verifier.rb b/lib/rspec/support/method_signature_verifier.rb index 21d0bb2da..0d5903072 100644 --- a/lib/rspec/support/method_signature_verifier.rb +++ b/lib/rspec/support/method_signature_verifier.rb @@ -84,7 +84,7 @@ def invalid_kw_args_from(given_kw_args) def has_kw_args_in?(args) Hash === args.last && could_contain_kw_args?(args) && - (args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) || RubyFeatures.kw_arg_separation? }) + (RubyFeatures.kw_arg_separation? || args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) }) end # Without considering what the last arg is, could it @@ -363,15 +363,17 @@ def unlimited_args? end def split_args(*args) - kw_args = if @signature.has_kw_args_in?(args) + kw_args = if @signature.has_kw_args_in?(args) && !RubyFeatures.kw_arg_separation? last = args.pop - non_kw_args = last.reject { |k, _| k.is_a?(Symbol) || RubyFeatures.kw_arg_separation? } + non_kw_args = last.reject { |k, _| k.is_a?(Symbol) } if non_kw_args.empty? last.keys else args << non_kw_args - last.select { |k, _| k.is_a?(Symbol) || RubyFeatures.kw_arg_separation? }.keys + last.select { |k, _| k.is_a?(Symbol) }.keys end + elsif @signature.has_kw_args_in?(args) && RubyFeatures.kw_arg_separation? + args.pop.keys else [] end