Project

General

Profile

Actions

Bug #19301

closed

Fix Data class to report keyrest instead of rest parameters

Added by bkuhlmann (Brooke Kuhlmann) almost 2 years ago. Updated almost 2 years ago.

Status:
Rejected
Assignee:
-
Target version:
-
ruby -v:
ruby 3.2.0 (2022-12-25 revision a528908271) [arm64-darwin22.2.0]
[ruby-core:111581]

Description

Overview

Hello and Happy New Year. 👋

With the new Data class, I'm seeing a discrepancy in parameter behavior compared to a Struct. I understand the original Data feature request made design choices to only accept keyword arguments for the #initialize method but reporting [[rest]] parameters seems misleading to me because it doesn't share the more flexible Struct#initialize behavior and would like to request Data#initialize answer [[keyrest]] for parameters for improved metaprogramming accuracy.

Steps to Recreate

To reproduce, consider the following:

DataExample = Data.define :one, :two
StructExample = Struct.new :one, :two

argument_array = [one: 1, two: 2]
argument_hash = {one: 1, two: 2}

puts "Data (parameters):        #{DataExample.method(:initialize).parameters}"
puts "Struct (parameters):      #{StructExample.method(:initialize).parameters}"
puts "Data (argument hash):     #{DataExample[**argument_hash]}"
puts "Struct (argument array):  #{StructExample[*argument_array]}"
puts "Struct (argument hash):   #{StructExample[**argument_hash]}"

The above will output the following:

Data (parameters):        [[:rest]]
Struct (parameters):      [[:rest]]
Data (argument hash):     #<data DataExample one=1, two=2>
Struct (argument array):  #<struct StructExample one={:one=>1, :two=>2}, two=nil>
Struct (argument hash):   #<struct StructExample one=1, two=2>

The Struct class -- as far as I know -- has always reported [[rest]] parameters even though it can accept positional or keyword arguments without error. ...but this is definitely not the case with the Data class which can be seen when running the following modification to the above:

DemoExample[*argument_array]
# missing keyword: :two (ArgumentError)

The above clearly betrays the [[rest]] parameters response (granted a Struct is slightly devious too but at least happily accepts positional or keyword arguments). With this in mind, could Data#initalize be fixed to at least report [[keyrest]] so we'd have a better chance of metaprogramming the correct argument format based on the #parameters response for initializing a Data instance correctly?

Thanks. 🙇🏻‍♂️

Environment

ruby 3.2.0 (2022-12-25 revision a528908271) [arm64-darwin22.2.0]


Related issues 2 (0 open2 closed)

Related to Ruby master - Bug #19280: Wrong error message about arity of Data::define.newClosedActions
Related to Ruby master - Bug #19278: Constructing subclasses of Data with positional argumentsRejectedActions
Actions #1

Updated by zverok (Victor Shepelev) almost 2 years ago

  • Related to Bug #19280: Wrong error message about arity of Data::define.new added
Actions #2

Updated by zverok (Victor Shepelev) almost 2 years ago

  • Related to Bug #19278: Constructing subclasses of Data with positional arguments added

Updated by bkuhlmann (Brooke Kuhlmann) almost 2 years ago

Apologies, I should have used StructExample = Struct.new :one, :two, keyword_init: true in the above example. ...but this also highlights a new behavioral change with a Struct which doesn't use keyword_init but misleadingly accepts positional arguments incorrectly. Should this be logged as a different issue (I'm not sure if the [[rest]] problem has been reported yet but will look in the meantime?

Updated by zverok (Victor Shepelev) almost 2 years ago

  • Status changed from Open to Rejected

See discussions in #19278 and #19280.

This is a conscious decision of Data.new converting all arguments to keyword ones, made for ease of providing default values or preprocessing of arguments, without caring about the form they were passed in:

DataExample = Data.define(:one, :two) do
  def initialize(one:, two: 0) = super(one: one.to_i, two: two.to_i)
end

DataExample.new('1')
# => #<data DataExample one=1, two=0>
DataExample.new(1)
# => #<data DataExample one=1, two=0>
DataExample.new(one: '1')
# => #<data DataExample one=1, two=0>
DataExample.new(one: '1', two: '2')
# => #<data DataExample one=1, two=2>

This would be very hard to achieve if Data (like Struct) would convert arguments in #initialize.

(You can try yourself with a Struct to define the #initialize which does the same.)

I will work on improving the implicitness of this in the documentation, though.

I am not sure about your DemoExample which raises (I can't see anything called DemoExample in your definitions). This works for me, if that was meant:

DataExample[1, 2]
#=> #<data DataExample one=1, two=2>

To see (for metaprogramming, say) "all parameters that Data.new accepts", you can do this:

DataExample.method(:new).parameters
#=> [[:rest]]

...which would be like Struct's [[:rest]] (and not that informative, but that's typically this for C-defined methods.)

Updated by bkuhlmann (Brooke Kuhlmann) almost 2 years ago

Hey Victor, thanks. I was aware of #19278 but hadn't noticed #19280.

You are correct, when I used DemoExample[*argument_array] earlier, that was a typo. I mean to use DataExample[*argument_array].

Ah, so you are saying this is a problem with C-defined methods. I hadn't realized that distinction before. Is there no way to allow C-defined methods to at least attempt to report the correct parameters because only answering [[rest]] isn't accurate?

I suppose this goes way beyond this issue but was hoping for something kind of like this (not identical to what I'm requesting but more accurate parameter-wise):

class Demo
  def self.new(**) = allocate(**)

  def initialize one: 1, two: 2
    super one:, two:
  end

  private

  attr_reader :one, :two
end

puts "Demo Parameters (new): #{Demo.method(:new).parameters}"
puts "Demo Parameters (initialize): #{Demo.instance_method(:initialize).parameters}"

# Demo Parameters (new): [[:keyrest, :**]]
# Demo Parameters (initialize): [[:key, :one], [:key, :two]]

Updated by zverok (Victor Shepelev) almost 2 years ago

I am a bit lost with what you are expecting, but to clarify a bit:

  1. Data.new and Data#initialize have different signatures
  2. If they would be defined in Ruby, the signatures would be: for .new: [[:rest], [:keyrest]], for #initialize: [[:keyrest]]
  3. But C-defined methods have somewhat loose reporting on their signatures, that's why .new reports itself as just [[:rest]], I am not sure it can be made better (mnemonically, can be treated as "accepts any number of arguments, if the last of them is Hash, considers it keyword args"); anyway, it is unrelated to Data implementation.
  4. What is reported for Data#initialize is correct, it really accepts only keyword args. I just published an article explaining this design decision :)

Updated by bkuhlmann (Brooke Kuhlmann) almost 2 years ago

Yeah, let me respond in order:

  1. Correct. No issues there.
  2. Correct. My problem was thinking in terms of pure Ruby, not C. That was my source of confusion, originally, but you've resolved my confusion.
  3. Correct. However, I think there is still an outstanding issue, architecturally, but I'll explain shortly.
  4. Thanks, yes, I read your article in detail and linked to it from my Ruby Data article as well.

I understand more clearly where you are coming from but am still concerned how to dynamically build a proper argument array when Struct#initialize and Data#initialize always return [[rest]] for parameters. I now realize that is how C-backed objects work but that also means Method#parameters is unreliable in terms of dealing with Struct and Data classes. For example, consider the following:

DataExample = Data.define :one, :two
StructAny = Struct.new :one, :two
StructKeywordOnly = Struct.new :one, :two, keyword_init: true

models = [DataExample, StructAny, StructKeywordOnly]
arguments = [{one: 1, two: 2}]

models.each do |model|
  print "#{model}: "
  puts model[*arguments]
rescue ArgumentError => error
  puts error.message
end

# DataExample: missing keyword: :two
# StructAny: #<struct StructAny one={:one=>1, :two=>2}, two=nil>
# StructKeywordOnly: #<struct StructKeywordOnly one=1, two=2>

In all three models, asking model.method(:initialize).parameters will always answer [[rest]] for parameters so when I build my arguments (i.e. [{one: 1, two: 2}]) in the same format as dictated from the Method#parameters response then the only model that gives me the correct instance is the StructKeywordOnly model because using keyword_init: true does that coercion for me. 😅

I originally logged this issue because I was thinking Data should behave more like how a Struct works when used with keyword_init: true since Data enforces keyword arguments for #initialize by default but I think this is a bigger issue than Data alone.

Would it be better if I move this discussion to a new issue where the focus is on asking if Method#parameters could properly answer the argument signature for C objects? Maybe that's too big of an ask?

Updated by Eregon (Benoit Daloze) almost 2 years ago

bkuhlmann (Brooke Kuhlmann) wrote in #note-7:

Would it be better if I move this discussion to a new issue where the focus is on asking if Method#parameters could properly answer the argument signature for C objects? Maybe that's too big of an ask?

Yes I think this would be best as a new issue, unless there is already an existing issue about this.

Updated by bkuhlmann (Brooke Kuhlmann) almost 2 years ago

Benoit, thanks. I found what I was looking for in #8088. I've added a comment to that issue referring back to this issue.

Victor, thanks for discussion on this and learning just how far down this C-based implementation problem really goes. 🙃

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0