Fixes for keyword argument handling #1054

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@97jaz
Contributor
97jaz commented Sep 30, 2013

[Fixes #1047, #1049. This supersedes my previous PR: 1047-kwargs.]

  • If you pass an explicit nil as the value of a keyword argument, we need to use that nil rather than the default value. Example:
def test(foo: 10)
  foo
end

test(foo: nil)

... should produce nil.

  • The kw assignment code in ArgsNode.java would take any hash that is the last argument to a method expecting kw arguments to be necessarily a hash of kw values. However, this isn't always true. E.g.:
def test(foo, bar: 10)
  [foo, bar]
end

test(no: "way")

...should produce [{:no=>"way"}, 10]

  • The arity checks occurring at lines 285 and 303 in Arity.java defer a check to kw assignment time, because not enough information is provided to the checking method to determine if there is an error. (Maybe this should be changed, but that's a much larger change than what I'm proposing.) However, that deferred check was not actually being performed. Example:
def test(foo: 10)
  foo
end

test(3)

... should raise an ArgumentError.

  • Keyword values passed to a method could be treated both as keyword arguments and as optional or rest arguments, because the code responsible for binding optional and rest arguments was not aware of keyword arguments. Example:
def test(a = true, foo: 10)
  [a, foo]
end

test(foo: 78)

... should produce [true, 78]

and

def test(*opts, foo: 10)
  [opts, foo]
end

test(foo: 3)

... should produce [[], 3]


I would be more than happy to add these examples as tests, but I can't tell how I would add 2.0-specific tests. If someone could point me in the right direction, I'll add tests to this PR.

@97jaz 97jaz Fixes JRuby #1047 - incorrect test for whether a kw arg was provided.
Formerly, the code attempted to extract the argument value from the
hash of provided values and test it against nil. This fails in the
case of an explicitly passed nil. The new version instead tests
whether the hash contains the desired key.
21248c4
@sluukkonen
Contributor

@97jaz if rubyspec doesn't already cover these cases, you should try sending a pull request there.

https://github.com/rubyspec/rubyspec/blob/master/language/def_spec.rb

@97jaz
Contributor
97jaz commented Sep 30, 2013

Thanks. I'll take a look.

@97jaz
Contributor
97jaz commented Sep 30, 2013

I submitted a PR to rubyspecs. See: https://github.com/rubyspec/rubyspec/pull/255

@97jaz 97jaz Fixes #1049: incorrect arity checking for kw arguments
There were two basic problems with the existing logic:
* Arguments could be treated both as kw args and as, e.g.,
optional args.
* An arity check for methods with kw arguments in the Arity class
is deferred to asignment time, but the check was not being performed
there, either.
5c5bf13
@headius
Member
headius commented Oct 1, 2013

Thanks! The changes look good. We also need to fix the compiler side. You can look into that or I can do it.

We're releasing 1.7.5 within the next day, so I'm going to make sure this doesn't affect non-2.0 modes at all before merging. I don't think it will, since there's no keywords in 1.8 or 1.9 mode.

@headius
Member
headius commented Oct 1, 2013

Merged, thanks! When we start hitting 2.0/2.1 logic hard after 1.7.5, I'll get the compiler bits worked out...or you can have a look if you like :-)

@headius headius closed this Oct 1, 2013
@97jaz
Contributor
97jaz commented Oct 1, 2013

Thanks! I haven't looked at the compiler yet, but I'll try to take a look at it tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment