Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distinct last positional argument hashes from keywords on Ruby 3 #537

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 49 additions & 3 deletions lib/rspec/support/method_signature_verifier.rb
Expand Up @@ -85,12 +85,58 @@ def has_kw_args_in?(args)
(args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) })
end

# Without considering what the last arg is, could it
JonRowe marked this conversation as resolved.
Show resolved Hide resolved
# contain keyword arguments?
def could_contain_kw_args?(args)
# If the arg count is less than the required positional args there are no keyword args
return false if args.count <= min_non_kw_args

@allows_any_kw_args || @allowed_kw_args.any?
# If there is no keyword splat or allowed keywords there are no keyword args
return false unless @allows_any_kw_args || @allowed_kw_args.any?

# If the args captured are more than the positional arguments then there are keywords
return true if args.count > max_non_kw_args

# Otherwise we need to check wether the last argument is a positional non default hash,
# e.g. that this:
#
# def capture(_, last = {}, keyword: 2)
#
# Is valid with both:
#
# [1, {:some => :hash}]
# [1, {:keyword => :value}]
#
# but for different reasons.

# If the keyword splat is allowed, and there are no specified
# keywords args then we will either have returned true on L100
# or this is invalid.
return false if @allowed_kw_args.empty? && @allows_any_kw_args

if Hash === args.last
last_arg_matches_any_keywords = !(args.last.keys & @allowed_kw_args).empty?
has_extra_keys = !(args.last.keys - @allowed_kw_args).empty?

# If we allow the keyword splat, and the last hash contains
# a specified keyword, then this hash contains keywords
return true if @allows_any_kw_args && last_arg_matches_any_keywords

# If we don't allow the keyword splat, and the last hash contains extra keys
# then it is a positional hash.
return false if !@allows_any_kw_args && has_extra_keys
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there's a problem between 2.7 and 3.0+ with (spec/rspec/support/method_signature_verifier_spec.rb:372)

> def arity_kw(x, y = {}, z:2); end

> arity_kw(nil, {a: 1}, {z: 2})

it works fine on 2.7 but fails on 3.0+:

ArgumentError: wrong number of arguments (given 3, expected 1..2)

To me, it seems that this expectation is not justified on Ruby 2.7:

expect(valid?(nil, :a => 1)).to eq(false)

as y is initialized with {:a => 1}.

I didn't look at other few failing examples yet.


# Otherwise assume we have keywords
return true
else
# If the last argument is a matcher nd we're on Ruby 3 which won't mix keyword args
# and a normal hash...
if RubyFeatures.distincts_kw_args_from_positional_hash?
# we've either returned true above on L100 or this is a positional arg matcher
return false
else
# However on older rubies this could contain keyword arguments
return true
end
end
end

def arbitrary_kw_args?
Expand Down
53 changes: 40 additions & 13 deletions spec/rspec/support/method_signature_verifier_spec.rb
Expand Up @@ -379,22 +379,49 @@ def arity_kw(x, y = {}, z:2); end
expect(valid?(nil, nil)).to eq(true)
end

it 'does not allow an invalid keyword arguments' do
expect(valid?(nil, nil, :a => 1)).to eq(false)
expect(valid?(nil, :a => 1)).to eq(false)
it 'treats the final positional argument as a hash' do
expect(valid?(1, { :z => 42 })).to eq(true)
expect(valid?(1, { :foo => 'bar', :baz => 'eggs'})).to eq(true)
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)
end
if RubyFeatures.distincts_kw_args_from_positional_hash?
it 'does not allow an invalid keyword arguments' do
expect(valid?(nil, nil, :a => 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: b")
it 'always treats non matching hashes as positional' do
# Basically on Ruby 3 this will always be a hash.
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(true)
expect(valid?(nil, 'a' => 1, :b => 2, :z => 3)).to eq(true)
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")

# Ruby 3 will treat this as a positional hash, so they are valid.
expect(error_for(1, :a => 0)).to eq(nil)
expect(error_for(1, 'a' => 0, :b => 0)).to eq(nil)
end
else
it 'does not allow an invalid keyword arguments' do
expect(valid?(nil, nil, :a => 1)).to eq(false)
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)
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")
end
end

it 'describes invalid arity precisely' do
Expand Down