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

JRuby accepts wrong method arguments when mixing positional with defaults and keywords #4186

Closed
ivoanjo opened this Issue Sep 28, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@ivoanjo
Contributor

ivoanjo commented Sep 28, 2016

Environment

Running jruby 9.1.5.0 (2.3.1) 2016-09-07 036ce39 OpenJDK 64-Bit Server VM 25.91-b14 on 1.8.0_91-8u91-b14-3ubuntu1~16.04.1-b14 [linux-x86_64] on Linux maruchan 4.8.0-16-generic #17-Ubuntu SMP Thu Sep 22 22:48:49 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux.

Expected Behavior

Example code:

def foo(a = 'default a', b:)
  [a, b]
end  

def bar
  foo(a: 'some value', b: 'some value')
end

bar

Output in MRI 2.3.1:

ArgumentError: unknown keyword: a

Actual Behavior

Output in JRuby (return from function)

["default a", "some value"]
@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Sep 28, 2016

Member

ok I know the problem and I will describe it as the actual fix might take a little thought. We allocate lvars and kwargs using the same StaticScope storage. We depend on strings which represent the variable name. So we do not complain about 'a:' since opt arg 'a=' exists. This seemed safe since you cannot actually have a method like:

def foo(a=1, a:)
end

The parser realizes that you are trying to use the same name in two different ways which is ambiguous. What the current solution did not consider is that we need to know if a particular name is a kwarg or an ordinary arg. So there are many ways to skin this cat but it means some extra state somewhere. Most obvious thought would be a kwargs start at offset x field. Then if we find 'a' exists before that we know it is not a kwarg?

Member

enebo commented Sep 28, 2016

ok I know the problem and I will describe it as the actual fix might take a little thought. We allocate lvars and kwargs using the same StaticScope storage. We depend on strings which represent the variable name. So we do not complain about 'a:' since opt arg 'a=' exists. This seemed safe since you cannot actually have a method like:

def foo(a=1, a:)
end

The parser realizes that you are trying to use the same name in two different ways which is ambiguous. What the current solution did not consider is that we need to know if a particular name is a kwarg or an ordinary arg. So there are many ways to skin this cat but it means some extra state somewhere. Most obvious thought would be a kwargs start at offset x field. Then if we find 'a' exists before that we know it is not a kwarg?

@enebo enebo added this to the JRuby 9.1.6.0 milestone Sep 28, 2016

@enebo enebo closed this in 7b93e08 Sep 28, 2016

@ivoanjo

This comment has been minimized.

Show comment
Hide comment
@ivoanjo

ivoanjo Sep 28, 2016

Contributor

Wow, blazing fast fix! Thanks! 😄

Contributor

ivoanjo commented Sep 28, 2016

Wow, blazing fast fix! Thanks! 😄

enebo added a commit that referenced this issue Sep 28, 2016

Fallout from #4186. JRuby AOT and IR persistence needs to persist the…
… new

fistKeywordIndex field in StaticScope.
@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Sep 29, 2016

Member

@ivoanjo I thought I had fixed all keyword argument bugs relating to methods (we have some other issues still with blocks) in our last point release so I was determined!

Member

enebo commented Sep 29, 2016

@ivoanjo I thought I had fixed all keyword argument bugs relating to methods (we have some other issues still with blocks) in our last point release so I was determined!

@ivoanjo

This comment has been minimized.

Show comment
Hide comment
@ivoanjo

ivoanjo Sep 30, 2016

Contributor

Contributor

ivoanjo commented Sep 30, 2016

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