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

Add support of keyword arguments. #3629

Closed
wants to merge 32 commits into from
Closed

Conversation

take-cheeze
Copy link
Contributor

@take-cheeze take-cheeze commented Apr 22, 2017

Thank @keizo042 and @ksss for sending me Ruby Under a Microscope via amazon.co.jp. The book helped a lot to start implementing this.
This PR isn't squashed since its modification is too big and may need more changes.

  • The parser is mostly ported from CRuby's parse.y. Thanks to CRuby developers.
    • Though AST generating is completely different since data structure of mruby AST is different from CRuby.
  • Most test from ruby/spec is passing.
  • Before Optimize keyword arguments., I've implemented handling keyword arguments only with Hash.
    • After adding OP_SENDK, it uses Array to handle passed keyword arguments instead. Though the old way is still used to handle arguments with OP_SEND or OP_SENDB opcode.
    • struct mrb_callinfo size is increased for optimization.
      I've re-reduced size of mrb_callinfo by reducing size of acc field since it's always 9-bit integer(GETARG_A). Thanks to @miura1729 for some advice.
      And kwds is removed since it's value is from kdict.
  • Some codedump.c improvements is added to know what is on the stack.
    • Name block, rest arguments, keyword rest arguments with &, *, ** when there is no specific variable name.
    • Print local variable names with VM code dumps.
  • This PR breaks VM compatibilities mostly in 4 opcodes(3 new and 1 changed).
    • OP_KARG and OP_KDICT's behavior is changed from the previous documentation to make keyword arguments handling easier. See updated documents for detail.
    • OP_SENDK is added to call method with optimized keyword arguments passing.
      • Instead of building hash, it creates array of keyword symbol and value pairs terminated with nil object.
      • In OP_KARG keyword is linear searched instead of mrb_hash_get.
    • OP_ENTER's arguments specifier's layout has changed. And now maybe it's got a big VM instruction.
      • In old layout there was no difference between required keyword and keyword with default argument.
      • Post required arguments is decreased to 4 bit(max: 31 -> 15).
      • Required keyword arguments and optional keyword arguments is 3 bit (max: 7) each.
      • After changing it I've noticed that adding another opcode like OP_ENTERK may make VM compatible.
  • Differences between CRuby:
    • Hash#fetch(implemented in mruby-hash-ext), Hash#delete, Hash#key? isn't used to handle keyword arguments. So Hash methods overwriting example in Ruby Under a Microscope can't be reproduced in this implementation.
  • mrb_get_args can only handle keyword arguments with last hash argument.
    • Should we add another C API equivalent to OP_KARG and OP_KDICT?.

@ksss
Copy link
Contributor

ksss commented Apr 22, 2017

Great works!

I fount some incompatibility with CRuby.

$ ruby -e 'p ->(a:){}.arity'
1

$ mruby -e 'p ->(a:){}.arity'
0

$ ruby -e 'p ->(a:1){}.arity'
-1

$ mruby -e 'p ->(a:1){}.arity'
0

$ ruby -e 'p ->(**){}.arity'
-1

$ mruby -e 'p ->(**){}.arity'
0
$ ruby -e 'p ->(a, b:){}.call(1, b: 2)'
nil

$ mruby -e 'p ->(a, b:){}.call(1, b: 2)'
trace:
	[0] -e:1
-e:1: unhandled keyword arguments found: {:b=>2} (ArgumentError)

@take-cheeze
Copy link
Contributor Author

@ksss @miura1729
Fixed some bugs and add some improvements mrb_callinfo.

@ksss
Copy link
Contributor

ksss commented Apr 23, 2017

@take-cheeze I found another bugs may about GC.

$ ruby -e 'p ->(a:, **k) { [a, k] }.call(a: 1, b: 2, c: 3)'
[1, {:b=>2, :c=>3}]

$ mruby -e 'p ->(a:, **k) { [a, k] }.call(a: 1, b: 2, c: 3)'
[1, {:b=>2}]

And interpretation for argument.

$ ruby -e 'p ->(a = nil, **k) { [a, k] }.call({a: 1}, {})'
[{:a=>1}, {}]

$ mruby -e 'p ->(a = nil, **k) { [a, k] }.call({a: 1}, {})'
trace:
	[0] -e:1
-e:1: 'call': wrong number of arguments (2 for 0) (ArgumentError)

mruby: e7bab50

build_config.rb

MRuby::Build.new do |conf|
  toolchain :gcc
  enable_debug
  conf.gembox 'default'
  conf.cc.defines << %w(MRB_GC_STRESS)
end

@take-cheeze
Copy link
Contributor Author

@ksss Should be fixed with updates.

@ksss
Copy link
Contributor

ksss commented Apr 23, 2017

@take-cheeze Great!
I found more diffs. (using fedae3f)

$ cat t.rb
p ["-> (a:, b:) { }.arity", -> (a:, b:) { }.arity]
p ["-> (a, b, c:, d: 1) { }.arity", -> (a, b, c:, d: 1) { }.arity]
p ["-> (a, b=1, *c, d:, e:, f: 2, **k, &l) { }.arity", -> (a, b=1, *c, d:, e:, f: 2, **k, &l) { }.arity]

$ ruby t.rb
["-> (a:, b:) { }.arity", 1]
["-> (a, b, c:, d: 1) { }.arity", 3]
["-> (a, b=1, *c, d:, e:, f: 2, **k, &l) { }.arity", -3]

$ mruby t.rb
["-> (a:, b:) { }.arity", 2]
["-> (a, b, c:, d: 1) { }.arity", -4]
["-> (a, b=1, *c, d:, e:, f: 2, **k, &l) { }.arity", -4]

See also
https://github.com/ruby/spec/blob/50357ca5ac97798a4eeb3cfb41680e1982510f9e/core/proc/arity_spec.rb#L44
https://github.com/ruby/spec/blob/50357ca5ac97798a4eeb3cfb41680e1982510f9e/core/proc/arity_spec.rb#L62
https://github.com/ruby/spec/blob/50357ca5ac97798a4eeb3cfb41680e1982510f9e/core/proc/arity_spec.rb#L203

@take-cheeze
Copy link
Contributor Author

@ksss I should read description of Proc#arity more closely.
Should be fixed with update.

@ksss
Copy link
Contributor

ksss commented Apr 24, 2017

@take-cheeze Thank you for responding.
I could't find bugs about kw_args anymore!

@take-cheeze
Copy link
Contributor Author

@ksss That's nice!
Thank you for many reviews!

@take-cheeze
Copy link
Contributor Author

Rebased and fixed a GC issue in C function.

@matz matz mentioned this pull request Jul 4, 2017
@katzer
Copy link
Contributor

katzer commented Jul 27, 2018

@matz @take-cheeze What's the status about keyword arguments? It was announced that they will arrive with 1.4.0 and 1.4.1 is out already but without them.

@take-cheeze
Copy link
Contributor Author

@katzer I don't know the detailed progress but this can't be applied without an incompatible changes to VM instructions so I'm waiting for the new VM instruction sets to come.

@matz
Copy link
Member

matz commented Jul 27, 2018

We are going to make big changes in the next release and name it mruby 2.0!
The keyword arguments will be among those changes. It is working on my machine.

@katzer
Copy link
Contributor

katzer commented Jul 27, 2018

Whats the big picture for mruby 2.0?

@matz
Copy link
Member

matz commented Jul 27, 2018

The current plan is:

  • bytecode, instead of word instruction
  • new mrb binary format
  • keyword arguments

matz added a commit that referenced this pull request Jul 30, 2018
The implementation of keyword arguments is heavily rely on the prototype
made by @take-cheeze in #3629.
@katzer
Copy link
Contributor

katzer commented Jul 31, 2018

That sounds all great, however what about better threading support?

@matz
Copy link
Member

matz commented Aug 1, 2018

@katzer Unfortunately there's no plan for the better threading support. Some platforms mruby supports do not provide threading functionality (e.g. small devices without OS). That means mruby cannot support threading by the core, which is needed for the better treading support, in my opinion.

@matz
Copy link
Member

matz commented Aug 1, 2018

Since the keyword argument is merged in the mruby2-draft branch, I close this issue.

@matz matz closed this Aug 1, 2018
ksekimoto added a commit to ksekimoto/mruby that referenced this pull request Jul 16, 2021
The implementation of keyword arguments is heavily rely on the prototype
made by @take-cheeze in mruby#3629.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants