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

parameters on non-Ruby method with special arity will NPE #3040

Closed
enebo opened this Issue Jun 12, 2015 · 7 comments

Comments

Projects
None yet
3 participants
@enebo
Copy link
Member

enebo commented Jun 12, 2015

jruby  -e '[].method(:shuffle!).parameters'

@cheald I think found this indirectly via Symbol.to_proc on one of these methods.

@enebo enebo added this to the JRuby 9.0.0.0.rc2 milestone Jun 12, 2015

@enebo enebo closed this in e1c0f7d Jun 12, 2015

@cheald

This comment has been minimized.

Copy link
Contributor

cheald commented Jun 12, 2015

I don't think this is resolved yet.

The method in this case is an instance of MixedModeIRMethod, which is in fact a Ruby method from my code.

ArgumentDescriptor is being given a null name with type rest from Helpers.methodToArgumentDescriptors. The method passed is a WrapperMethod, which wraps a MixedModeIRMethod. This causes it to enter the else branch, where it returns an array of ArgumentDescriptor of type rest with null names. Later on, toArrayForm is called on these ArgumentDescriptor, but name is null and is passed through where it's eventually attempted to look it up as a symbol, which produces the NPE.

However, the method that parameters is being called on, when invoked manually from a console, produces:

> Kerrigan::Model::Page.method(:for_url).parameters
 => [[:req, :url], [:opt, :create_if_missing], [:opt, :updates]]

No rest parameters. Odd.

This is called from INTERPRET_METHOD, which has debug data:

/usr/local/rvm/gems/jruby-1.7.19/gems/rspec-support-3.2.1/lib/rspec/support/method_signature_verifier.rb:76 classify_parameters
  0 %self = recv_self()
  1 %v_0 = load_implicit_closure()
  2 %current_scope = copy(scope<0>)
  3 %current_module = copy(module<0>)
  4 check_arity(;req: 0, opt: 0, *r: false, kw: false)
  5 line_num(;n: 77)
  6 put_field(%self, Fixnum:0 ;name: @min_non_kw_args)
  7 optional_non_kw_args(0:0) = copy(Fixnum:0)
  8 line_num(;n: 78)
  9 put_field(%self, false ;name: @allows_any_kw_args)
  10  line_num(;n: 80)
  11  %v_3 = get_field(%self ;name: @method)
  12  %v_4 = call_0o(%v_3 ;n:parameters, t:NO, cl:false)
  13  %v_5 = call(%v_4, %self:CLOSURE classify_parameters_CLOSURE_1[/usr/local/rvm/gems/jruby-1.7.19/gems/rspec-support-3.2.1/lib/rspec/support/method_signature_verifier.rb:80] ;n:each, t:NO, cl:true)
  14  line_num(;n: 97)
  15  %v_6 = get_field(%self ;name: @min_non_kw_args)
  16  %v_7 = call_1o(%v_6, optional_non_kw_args(0:0) ;n:+, t:NO, cl:false)
  17  put_field(%self, %v_7 ;name: @max_non_kw_args)
  18  line_num(;n: 98)
  19  %v_8 = get_field(%self ;name: @required_kw_args)
  20  %v_9 = get_field(%self ;name: @optional_kw_args)
  21  %v_10 = call_1o(%v_8, %v_9 ;n:+, t:NO, cl:false)
  22  put_field(%self, %v_10 ;name: @allowed_kw_args)
  23  return(%v_10)

As I mentioned earlier, this only happens in aggregate suite runs - single runs pass - so it may be JIT related or something.

@dekz

This comment has been minimized.

Copy link
Contributor

dekz commented Jun 13, 2015

I am also seeing some strange behaviour which I'm able to reproduce simply from executing pry:

https://gist.github.com/dekz/c96aac5d62f9ae548531

@cheald

This comment has been minimized.

Copy link
Contributor

cheald commented Jun 13, 2015

According to bisection, 49bf2c1 is the responsible commit.

@cheald

This comment has been minimized.

Copy link
Contributor

cheald commented Jun 13, 2015

And confirmed, reverting that commit on HEAD fixes my issues.

@enebo

This comment has been minimized.

Copy link
Member Author

enebo commented Jun 14, 2015

ok the bug now is crystal clear, but the solution (or right solution) may require some refactoring. The reason we see rest in your case is the WrapperMethod is not MethodArgs2 or IRMethodArgs instance so it travels down an else branch on line 2526 on Helpers.java. Notice we do instanceof checks for this helper method but a WrapperMethod can wrap any type of method.

I can hack this to ask if it is a WrapperMethod or possibly I can add getRealMethod(). If getRealMethod does what I think then perhaps that is the simplest fix...

@enebo

This comment has been minimized.

Copy link
Member Author

enebo commented Jun 14, 2015

@dekz that is something else. I believe there may be an open issue on some pry issues, but if you cannot find anything then I recommend opening up a new issue.

@enebo

This comment has been minimized.

Copy link
Member Author

enebo commented Jun 14, 2015

@cheald haha...I did not realize that was a PR to fix this. This on the surface seems like the correct fix. I think we are seeing something like WrapperMethod(XMethod(YMethod)). My only reservation is that perhaps we want to return XMethod in some case? Don't know, but it is early enough in RC1 and it feels right. In the majority of cases getRealMethod -> self for most wrapped methods. So this would be an error where we have something like: WrapprMethod(AliasMethod(DynamicMethod)) and we want name from Alias method but we return what it is aliasing.

@headius can you think of any cases where wrapper should not just real method the method it is holding? CI is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.