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

RubyStruct.java gives ArgumentError: unknown keywords: keyword #5448

Closed
dr-itz opened this Issue Nov 14, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@dr-itz
Copy link
Contributor

dr-itz commented Nov 14, 2018

Environment

  • jruby 9.2.4.0 (2.5.0) 2018-11-13 8faff06 Java HotSpot(TM) 64-Bit Server VM 25.162-b12 on 1.8.0_162-b12 +jit [darwin-x86_64]
  • Running AR-JDBC 52-stable tests

Behavior

Running:

RAILS=../rails52 rake rails:test_sqlite3 TEST=test/cases/type/adapter_specific_registry_test.rb

(../rails52 is checked out at v5.2.1)

Gives:

  1) Error:
ActiveRecord::AdapterSpecificRegistryTest#test_construct_args_are_passed_to_the_type:
ArgumentError: unknown keywords: keyword
    org/jruby/RubyStruct.java:403:in `initialize'
    /Users/dritz/devel/open/rails/rails52/activemodel/lib/active_model/type/registry.rb:12:in `block in register'
    /Users/dritz/devel/open/rails/rails52/activerecord/lib/active_record/type/adapter_specific_registry.rb:36:in `call'
    /Users/dritz/devel/open/rails/rails52/activemodel/lib/active_model/type/registry.rb:20:in `lookup'
    /Users/dritz/devel/open/rails/rails52/activerecord/test/cases/type/adapter_specific_registry_test.rb:85:in `block in test_construct_args_are_passed_to_the_type'


  2) Error:
ActiveRecord::AdapterSpecificRegistryTest#test_registering_adapter_specific_modifiers:
ArgumentError: unknown keywords: keyword
    org/jruby/RubyStruct.java:403:in `initialize'
    /Users/dritz/devel/open/rails/rails52/activemodel/lib/active_model/type/registry.rb:12:in `block in register'
    /Users/dritz/devel/open/rails/rails52/activerecord/lib/active_record/type/adapter_specific_registry.rb:36:in `call'
    /Users/dritz/devel/open/rails/rails52/activemodel/lib/active_model/type/registry.rb:20:in `lookup'
    /Users/dritz/devel/open/rails/rails52/activerecord/lib/active_record/type/adapter_specific_registry.rb:105:in `call'
    /Users/dritz/devel/open/rails/rails52/activemodel/lib/active_model/type/registry.rb:20:in `lookup'
    /Users/dritz/devel/open/rails/rails52/activerecord/test/cases/type/adapter_specific_registry_test.rb:127:in `block in test_registering_adapter_specific_modifiers'

This passes on 9.2.0.0/9.1.17.0

I haven't looked any closer since it's the middle of the night again. Thought I report it since I don't know when I get the chance to look closer...might be a few days...

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 14, 2018

Thanks for the report! Something with keyword args must have regressed in 9.2.x.

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 19, 2018

Reproduce locally.

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 19, 2018

Ok, I have a simple reproduction:

Struct.new(:args).new(*[{keyword: :arg}])

This only affects the splatted path, which has to make a determination about the list of incoming arguments as to whether the keywords are being used to initialize the struct fields, or whether any incoming Hash is just intended to initialize one field.

headius added a commit to headius/jruby that referenced this issue Nov 19, 2018

headius added a commit to headius/jruby that referenced this issue Nov 19, 2018

Init of non-kwarg Struct with splatted args containing Hash.
This case should continue to pass the Hash as a normal field init
arg, and not interpret it as keyword-based init.

See jruby#5448.

headius added a commit to headius/jruby that referenced this issue Nov 19, 2018

headius added a commit to headius/jruby that referenced this issue Nov 19, 2018

Init of non-kwarg Struct with splatted args containing Hash.
This case should continue to pass the Hash as a normal field init
arg, and not interpret it as keyword-based init.

See jruby#5448.

headius added a commit to headius/jruby that referenced this issue Nov 19, 2018

headius added a commit to headius/jruby that referenced this issue Nov 19, 2018

Init of non-kwarg Struct with splatted args containing Hash.
This case should continue to pass the Hash as a normal field init
arg, and not interpret it as keyword-based init.

See jruby#5448.

@headius headius closed this in 5c4838f Nov 19, 2018

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Nov 20, 2018

@dr-itz you happen to have any inkling how often we/rails passes a ruby struct around in AR? This is not related to the bug but I am wondering how hot Ruby struct's get.

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 20, 2018

@enebo I have suspected Struct could be a bottleneck for us for some time. It has not had any recent optimization work and I see tons of opportunity there. It also needs to start packing fields like objects do.

@dr-itz

This comment has been minimized.

Copy link
Contributor Author

dr-itz commented Nov 21, 2018

@enebo I don't really know, but it's an interesting question..so I had a quick look. We have

  • The ContentType header in each HTTP response
  • ParamsWrapper for its options, built once per controller, accessed on each request
  • Two uses in routing that I haven't looked at
  • ActionView in format lookup, but that's mostly cached
  • ActiveSupport Callbacks, ExecutionWrapper hooks etc....
  • Lots of uses in testing

In ActiveRecord

  • finder methods: join dependency
  • reflection: for joins
  • The whole schema definition stuff used in migrations, so not hot
  • PG Array and Point classes

So there are a few uses, but I don't think they're really hot..

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Nov 21, 2018

@dr-itz thanks for that comment. With recent object shaping work we can definitely speed up struct and the cost of doing it is small so knowing rails is leaning on it in various ways is helpful.

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 21, 2018

@enebo We might actually do better with a pure-Ruby Struct now, since it would use normal attr accessors and ivars.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Nov 22, 2018

@headius I also thought about just generating a java class ala RubyClass with fields for each struct field.

eregon added a commit to ruby/spec that referenced this issue Nov 27, 2018

Splatted kwarg struct init (jruby/jruby#5453)
* Handle splatted and unsplatted kwarg Struct init the same.

Fixes jruby/jruby#5448.

* Init of non-kwarg Struct with splatted args containing Hash.

This case should continue to pass the Hash as a normal field init
arg, and not interpret it as keyword-based init.

See jruby/jruby#5448.

@enebo enebo added this to the JRuby 9.2.5.0 milestone Nov 28, 2018

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.