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

[bugfix] Properly detect kwargs hashes vs optional positional args #594

Open
wants to merge 5 commits 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 33 additions & 10 deletions lib/rspec/support/method_signature_verifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,35 @@ def invalid_kw_args_from(given_kw_args)
given_kw_args - @allowed_kw_args
end

# If the last argument is Hash, Ruby will treat only symbol keys as keyword arguments
# the rest will be grouped in another Hash and passed as positional argument.
def has_kw_args_in?(args)
Hash === args.last &&
could_contain_kw_args?(args) &&
(RubyFeatures.kw_arg_separation? || args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) })
# Considering the arg types, are there kw_args?
def has_kw_args_in?(args) # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
if RubyFeatures.kw_arg_separation?
# If the last arg is a hash, depending on the signature it could be kw_args or a positional parameter.
return false unless Hash === args.last && could_contain_kw_args?(args)

# If the position of the hash is beyond the count of required and optional positional
# args then it is the kwargs hash
return true if args.count > @max_non_kw_args

# This is the proper way to disambiguate between positional args and keywords hash
# but relies on beginning of the call chain annotating the method with
# ruby2_keywords, so only use it for positive feedback as without the annotation
# this is always false
return true if Hash.ruby2_keywords_hash?(args[-1])

# Otherwise, the hash could be defined kw_args or an optional positional parameter
# inspect the keys against known kwargs to determine what it is
# Note: the problem with this is that if a user passes only invalid keyword args,
# rspec no longer detects is and will assign this to a positional argument
return arbitrary_kw_args? || args.last.keys.all? { |x| @allowed_kw_args.include?(x) }
else
# Version <= Ruby 2.7
# If the last argument is Hash, Ruby will treat only symbol keys as keyword arguments
# the rest will be grouped in another Hash and passed as positional argument.
Hash === args.last &&
could_contain_kw_args?(args) &&
(args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) })
end
end

# Without considering what the last arg is, could it
Expand Down Expand Up @@ -282,7 +305,7 @@ class MethodSignatureVerifier

def initialize(signature, args=[])
@signature = signature
@non_kw_args, @kw_args = split_args(*args)
@non_kw_args, @kw_args = split_args(args.clone)
@min_non_kw_args = @max_non_kw_args = @non_kw_args
@arbitrary_kw_args = @unlimited_args = false
end
Comment on lines 306 to 311
Copy link
Contributor Author

@malcolmohare malcolmohare Feb 27, 2024

Choose a reason for hiding this comment

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

something had to change here. Splatting the args meant the special marking for the kwargs hash gets lost. This doesnt happen to functions with the ruby2_keywords... except it won't apply here because this function doesn't take the *args input. So either the args array is cloned (which persists the special flagging) or the input to this function needs to swap to using *args. I figured cloning was the less obtrusive change. The split_args function is private so changing its signature was not something that has compatibility concerns.

Expand Down Expand Up @@ -362,7 +385,7 @@ def unlimited_args?
!@unlimited_args || @signature.unlimited_args?
end

def split_args(*args)
def split_args(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) }
Expand Down Expand Up @@ -395,13 +418,13 @@ def split_args(*args)
class LooseSignatureVerifier < MethodSignatureVerifier
private

def split_args(*args)
def split_args(args)
if RSpec::Support.is_a_matcher?(args.last) && @signature.could_contain_kw_args?(args)
args.pop
@signature = SignatureWithKeywordArgumentsMatcher.new(@signature)
end

super(*args)
super(args)
end

# If a matcher is used in a signature in place of keyword arguments, all
Expand Down
2 changes: 2 additions & 0 deletions spec/rspec/support/method_signature_verifier_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ def valid_non_kw_args?(arity)
def valid?(*args)
described_class.new(signature, args).valid?
end
ruby2_keywords(:valid?) if respond_to?(:ruby2_keywords, true)

def error_description
described_class.new(signature).error_message[/Expected (.*),/, 1]
Expand All @@ -21,6 +22,7 @@ def error_description
def error_for(*args)
described_class.new(signature, args).error_message
end
ruby2_keywords(:error_for) if respond_to?(:ruby2_keywords, true)

def signature_description
signature.description
Expand Down
Loading