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

TypeError raised by JRuby during keyword argument checking while the call is perfectly legit #3760

Closed
doudou opened this Issue Mar 27, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@doudou

doudou commented Mar 27, 2016

Environment

jruby 9.0.5.0 (2.2.3) 2016-01-26 7bee00d Java HotSpot(TM) 64-Bit Server VM 25.77-b03 on 1.8.0_77-b03 +jit [linux-amd64] on Linux squidock 4.5.0-040500-generic #201603140130 SMP Mon Mar 14 05:32:22 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux (Kubuntu 15.10)

Ran with --debug -G, but the problem also appears with no command line options.

Noteworthy gems: facets (but not only very little of it activated), concurrent-ruby. The code that causes the problem is not multithreaded.

Expected Behaviour

I'm calling a method whose signature is def process_events_synchronous(seeds = Hash.new, initial_errors = Array.new, enable_scheduler: false) The call line is engine.process_events_synchronous(seeds)

Links to the actual called method and the calling site

To reproduce,

git clone https://github.com/rock-core/tools-roby -b roby3
cd tools-roby
bundle install
ruby test/test_event_generator.rb -n /test_emit_without_propagation/

I expect to enter the called method.

Actual Behaviour

JRuby raises a TypeError exception complaining that XXXX is not a string where 'XXX' looks like the result of the caller's #inspect. The first line of the TypeError backtrace on the Ruby side is bogus (see #3624). Removing the keyword argument enable_scheduler makes the call pass as expected. The full backtrace reports

    java/lang/Thread.java:1552:in `getStackTrace'
    org/jruby/runtime/backtrace/TraceType.java:215:in `getBacktraceData'
    org/jruby/runtime/backtrace/TraceType.java:47:in `getBacktrace'
    org/jruby/RubyException.java:225:in `prepareBacktrace'
    org/jruby/exceptions/RaiseException.java:229:in `preRaise'
    org/jruby/exceptions/RaiseException.java:196:in `preRaise'
    org/jruby/exceptions/RaiseException.java:111:in `<init>'
    org/jruby/Ruby.java:4066:in `newRaiseException'
    org/jruby/Ruby.java:3828:in `newTypeError'
    org/jruby/RubyBasicObject.java:689:in `asJavaString'
    org/jruby/ir/runtime/IRRuntimeHelpers.java:592:in `visit'
    org/jruby/RubyHash.java:650:in `visitLimited'
    org/jruby/RubyHash.java:636:in `visitAll'
    org/jruby/ir/runtime/IRRuntimeHelpers.java:589:in `checkForExtraUnwantedKeywordArgs'
    org/jruby/ir/runtime/IRRuntimeHelpers.java:543:in `checkArity'
    org/jruby/ir/instructions/CheckArityInstr.java:69:in `checkArity'
    org/jruby/ir/interpreter/InterpreterEngine.java:390:in `processBookKeepingOp'
    org/jruby/ir/interpreter/StartupInterpreterEngine.java:111:in `interpret'
    org/jruby/ir/interpreter/InterpreterEngine.java:86:in `interpret'
    org/jruby/internal/runtime/methods/InterpretedIRMethod.java:193:in `INTERPRET_METHOD'
    /home/doudou/dev/rock/toolchain-devel/jruby/tools-roby/lib/roby/execution_engine.rb:661:in `process_events_synchronous'
    org/jruby/internal/runtime/methods/InterpretedIRMethod.java:180:in `call'
    org/jruby/internal/runtime/methods/DynamicMethod.java:197:in `call'
    org/jruby/runtime/callsite/CachingCallSite.java:313:in `cacheAndCall'
    org/jruby/runtime/callsite/CachingCallSite.java:163:in `call'
    org/jruby/ir/interpreter/InterpreterEngine.java:316:in `processCall'
    org/jruby/ir/interpreter/StartupInterpreterEngine.java:77:in `interpret'
    org/jruby/ir/interpreter/InterpreterEngine.java:80:in `interpret'
    org/jruby/internal/runtime/methods/InterpretedIRMethod.java:165:in `INTERPRET_METHOD'
    /home/doudou/dev/rock/toolchain-devel/jruby/tools-roby/lib/roby/event_generator.rb:661:in `emit'
@doudou

This comment has been minimized.

Show comment
Hide comment
@doudou

doudou Mar 28, 2016

OK, so narrowed it down to JRuby having different rules than MRI when a method is given a hash as argument, and also has keyword arguments.

I've been testing a few cases, and it seems that MRI's rule in this case is that any key that is not a symbol is fed into the argument (that does include strings), any key that is a symbol is interpreted as a keyword argument

Testcase:

def test(argument = Hash.new, keyword_arg: true)
    puts "ARGS: #{argument.inspect}"
    puts "KW: #{keyword_arg}"
end
test(Hash[Object.new => 10])

MRI:

ruby 2.1.6p336 (2015-04-13 revision 50298) [x86_64-linux]
ARGS: {#<Object:0x00000001bace70>=>10}
KW: true

JRuby 9.0.5.0

jruby 9.0.5.0 (2.2.3) 2016-01-26 7bee00d Java HotSpot(TM) 64-Bit Server VM 25.77-b03 on 1.8.0_77-b03 +jit [linux-amd64]
TypeError: #<Object:0x6f1fba17> is not a string
  <top> at jruby_bug.rb:5

doudou commented Mar 28, 2016

OK, so narrowed it down to JRuby having different rules than MRI when a method is given a hash as argument, and also has keyword arguments.

I've been testing a few cases, and it seems that MRI's rule in this case is that any key that is not a symbol is fed into the argument (that does include strings), any key that is a symbol is interpreted as a keyword argument

Testcase:

def test(argument = Hash.new, keyword_arg: true)
    puts "ARGS: #{argument.inspect}"
    puts "KW: #{keyword_arg}"
end
test(Hash[Object.new => 10])

MRI:

ruby 2.1.6p336 (2015-04-13 revision 50298) [x86_64-linux]
ARGS: {#<Object:0x00000001bace70>=>10}
KW: true

JRuby 9.0.5.0

jruby 9.0.5.0 (2.2.3) 2016-01-26 7bee00d Java HotSpot(TM) 64-Bit Server VM 25.77-b03 on 1.8.0_77-b03 +jit [linux-amd64]
TypeError: #<Object:0x6f1fba17> is not a string
  <top> at jruby_bug.rb:5
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 28, 2016

Member

Thanks for the reduction, @doudou. I'll poke at this a bit this morning.

Member

headius commented Mar 28, 2016

Thanks for the reduction, @doudou. I'll poke at this a bit this morning.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 28, 2016

Member

The problem appears to be that we try to coerce keys to a string-like thing when we are walking keys we're not sure should be in the kwargs processing. We need the string for the hash lookup. I believe we should only perform this search for keys that actually are string-like, or perhaps only symbols.

Member

headius commented Mar 28, 2016

The problem appears to be that we try to coerce keys to a string-like thing when we are walking keys we're not sure should be in the kwargs processing. We need the string for the hash lookup. I believe we should only perform this search for keys that actually are string-like, or perhaps only symbols.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 28, 2016

Member

This is unlikely to make 9.1 because of some key issues we have to fix:

  • Only symbols should ever be processed as kwarg keys. We're processing both symbols and strings currently.
  • Any incoming hash in the proper position to be processed as kwargs (last non-required argument) has potential to be split, with symbolic keys going to kwargs and non-symbolic keys remaining part of the normal arg-passed hash. Currently, once we've decided a hash is in the right position to be used for kwargs, we don't use it for anything else (leading to this error).

The main piece that's missing is that we don't treat an incoming kwarg-able hash as potentially containing both kwarg symbols and non-kwarg pairs. We need a step before arity-checking and argument assignment that breaks apart such hashes into kwargable and non-kwargable parts.

Member

headius commented Mar 28, 2016

This is unlikely to make 9.1 because of some key issues we have to fix:

  • Only symbols should ever be processed as kwarg keys. We're processing both symbols and strings currently.
  • Any incoming hash in the proper position to be processed as kwargs (last non-required argument) has potential to be split, with symbolic keys going to kwargs and non-symbolic keys remaining part of the normal arg-passed hash. Currently, once we've decided a hash is in the right position to be used for kwargs, we don't use it for anything else (leading to this error).

The main piece that's missing is that we don't treat an incoming kwarg-able hash as potentially containing both kwarg symbols and non-kwarg pairs. We need a step before arity-checking and argument assignment that breaks apart such hashes into kwargable and non-kwargable parts.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 22, 2016

Member

I think @enebo's nearly-ready args work will probably fix this.

Member

headius commented Aug 22, 2016

I think @enebo's nearly-ready args work will probably fix this.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Aug 22, 2016

Member

Fixed in commit c183943.

Member

enebo commented Aug 22, 2016

Fixed in commit c183943.

@enebo enebo closed this Aug 22, 2016

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