Truffleruby: dry-types: some arguments disappear from keyword arguments

[deepj]

I'm not sure how I would describe and explain this issue. But I've met with while running dry-types specs

To reproduce via dry-spec specs

git clone https://github.com/dry-rb/dry-types.git
cd dry-types
bundle
bundle exec rspec spec/dry/types/constructor_spec.rb:67

Error:

1) Dry::Types::Constructor#constructor returns a new constructor
     Failure/Error: expect(upcaser.options[:id]).to be(:upcaser)

       expected #<Symbol:9316> => :upcaser
            got #<NilClass:4> => nil

       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about
       object identity in this example.
     # ./spec/dry/types/constructor_spec.rb:71:in `block (3 levels) in <top (required)>'

The issue is when some keyword arguments disappear when Dry::Types::Constructor.new is called. This doesn't happened under MRI.

This is a minimal reproducible example taken from their specs

require 'dry/types'

type = Dry::Types::Constructor.new(Dry::Types['string'], fn: Kernel.method(:String))

upcaser = type.constructor(-> s { s.upcase }, id: :upcaser)

puts upcaser.options[:id].inspect

This would print upcaser like under MRI, but not under Truffleruby, nil is printed instead.

Step by step calls

1. https://github.com/dry-rb/dry-types/blob/master/lib/dry/types/constructor.rb#L76 (no issue, new options variable has fn and id keywords)
2. https://github.com/dry-rb/dry-types/blob/master/lib/dry/types/options.rb#L17 (no issue, new_options variable has fn and id keywords)
3. https://github.com/dry-rb/dry-types/blob/master/lib/dry/types/constructor.rb#L22 (issue, options variable has only fn keyword, but not id keyword)

[deepj]

@chrisseaton I guess this would be interesting issue. It looks like some kind of bug in "interpreter".

The problem in the third step

def self.new(input, options = {}, &block)
  type = input.is_a?(Builder) ? input : Definition.new(input)
  super(type, options, &block)
end

The options above contains {:fn=>#<Proc:0x243c@~/dev/bugs/truffleruby/dry-types/lib/dry/types/constructor.rb:77 (lambda)>} but it would be {:fn=>#<Proc:0x243c@~/dev/bugs/truffleruby/dry-types/lib/dry/types/constructor.rb:77 (lambda)>, :id => :upcaser}.

In the second step

def with(new_options)
  self.class.new(*@__args__, **options, meta: @meta, **new_options)
end

Here new_options still is {:fn=>#<Proc:0x243c@~/dev/bugs/truffleruby/dry-types/lib/dry/types/constructor.rb:77 (lambda)>, :id => :upcaser}

I hope it's clear.

A motivation is a fact, dry-rb gems use quite not much common Ruby constructions. Some dry-rb gems pass their specs under Truffleruby, but plenty of them not. All of them are pure Ruby.

[eregon]

@deepj Thanks for the detailed report and debugging!
This seems most likely some incorrect handling of keyword splats in the parser .

[deepj]

@eregon could you please check if this has been fixed in https://github.com/oracle/truffleruby/commit/a73127dc74f2fa0b2b28d33d67b16a882b6af9a9?

[eregon]

@deepj I think the issue here is related to splatted keyword arguments (and so that fix won't help), most likely this line doesn't work as expected:
https://github.com/dry-rb/dry-types/blob/858d7ab3c7de23a43fe016f918056fcfb7c01da2/lib/dry/types/options.rb#L17

[eregon]

The issue still happens, using your 4 commands at the top, although the failing specs changed lines.
Commit tested: dry-rb/dry-types@858d7ab3c7de23a43fe016f918056fcfb7c01da2

$ bundle exec rspec spec/dry/types/constructor_spec.rb

Randomized with seed 19877
.........F..........FF........

Failures:

  1) Dry::Types::Constructor equality counts meta
     Failure/Error: expect(type.constructor(to_i).meta(pos: :left)).not_to eql(type.constructor(to_i).meta(pos: :right))

       expected: value != #<Dry::Types::Constructor type=#<Dry::Types::Definition primitive=String options={} meta={}> options={:fn=>#<Proc:0x8d18(&:to_i)>} meta={}>
            got: #<Dry::Types::Constructor type=#<Dry::Types::Definition primitive=String options={} meta={}> options={:fn=>#<Proc:0x8d18(&:to_i)>} meta={}>

       (compared using eql?)

       Diff:

     # ./spec/dry/types/constructor_spec.rb:152:in `block (3 levels) in <top (required)>'

  2) Dry::Types::Constructor#constructor returns a new constructor
     Failure/Error: expect(upcaser.options[:id]).to be(:upcaser)

       expected #<Symbol:36680> => :upcaser
            got #<NilClass:4> => nil

       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about
       object identity in this example.
     # ./spec/dry/types/constructor_spec.rb:95:in `block (3 levels) in <top (required)>'

  3) Dry::Types::Constructor#constructor accepts a block
     Failure/Error: expect(upcaser.options[:id]).to be(:upcaser)

       expected #<Symbol:36680> => :upcaser
            got #<NilClass:4> => nil

       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about
       object identity in this example.
     # ./spec/dry/types/constructor_spec.rb:102:in `block (3 levels) in <top (required)>'

Finished in 0.196 seconds (files took 0.868 seconds to load)
30 examples, 3 failures

Failed examples:

rspec ./spec/dry/types/constructor_spec.rb:148 # Dry::Types::Constructor equality counts meta
rspec ./spec/dry/types/constructor_spec.rb:91 # Dry::Types::Constructor#constructor returns a new constructor
rspec ./spec/dry/types/constructor_spec.rb:98 # Dry::Types::Constructor#constructor accepts a block

Randomized with seed 19877

[deepj]

@eregon thank you for testing!

[deepj]

@eregon @chrisseaton could this be a priority for RC14, please?

[deepj]

@eregon I know I'm annoying right now, but can you take a look at this in near future? Due this e.g. dry-validation specs fail in almost 2000 cases

[chrisseaton]

We'll see if we can squeeze it in.

Thanks again for your reports.

[aardvark179]

I've found the problem. It was a bug in the concatenation of multiple **args containing duplicate keys. I'm testing a fix in our CI environment now.

[aardvark179]

Fix is now merged.

[deepj]

I can confirm this is fixed. I used @chrisseaton nightly build and all dry-types' specs passed.

Thank you @aardvark179