Attempt to support Ruby 3.0

[dleidert]

No description provided.

[alexdowad]

Thanks so much for this. Will try to have a good look at this issue later today.

[alexdowad]

Findings thus far:

• Ruby 3.0 removed SortedSet from the standard library and replaced it with a gem.
• The sorted_set gem relies on the rbtree gem for implementing SortedSet.
• With this change, the behavior of SortedSet#eql? has changed. That is what has broken Hamster.
• SortedSet#eql? just delegates to RBTree#eql?.
• RBTree#eql? is just the default implementation of the method.
• I think the best fix would be to implement RBTree#eql? in the rbtree gem, giving it behavior similar to Hash#eql?.
• Just seeing if I can find the repo for that gem and submit a PR there.

[dubek]

do we also need to add a dependency on the sorted_set gem (to support conversion from Hamster::SortedSet to/from ruby SortedSet)?

[alexdowad]

do we also need to add a dependency on the sorted_set gem (to support conversion from Hamster::SortedSet to/from ruby SortedSet)?

I was thinking about that 😄 Many users of Hamster probably don't use Hamster::SortedSet at all, so it's a shame to make them drag in unneeded gems in their projects...

Another point. Hamster.to_ruby was intended to convert Hamster structures into core-Ruby structures, and if SortedSet was originally a gem, I don't think we would have made Hamster.to_ruby produce them at all. At that time, we did so because it was part of Ruby's standard library.

On the other hand, we don't really want to break user code, even if it's only a minority of users whose code is broken. (Although that is exactly what our friends in the Ruby core team have done here...)

I would love to hear what others think. For myself, I think I need to let this issue percolate a bit longer in my brain to come with an opinion.

[alexdowad]

Just found this: knu/sorted_set#3

That issue has been open for almost a year now. 😮 Doesn't look promising.

[alexdowad]

If anyone knows where the repo is for the rbtree gem, please let me know. RubyGems just has a broken link to RubyForge. A Google search reveals skade/rbtree on GitHub, but that doesn't appear to be the repo for the gem. (The code in the repo is not the same as the code on RubyGems.)

[alexdowad]

@dleidert, another thing... could you explain how the code in this PR resolves the problem?

[dubek]

1. Regarding the hamster tests that fail because SortedSet[4,5,6] == SortedSet[4,5,6] is false: we can either implement our own == on SortedSet (just for the test) or use our own function to compare them. Maybe modify the current test case to not include any SortedSet values, and add another test just for that.
2. Regarding Hamster.to_ruby, I think the safest backward-compatible way is to depend on sorted_set and keep generating them in Hamster.to_ruby.
3. Another option is to check at runtime if there's sorted_set: if there is, generate them, otherwise, generate arrays. But that complicates the code and the tests. I don't like this.
4. Another option is to create new major version of Hamster which requires Ruby 3.x. It will always generate arrays and will not depend on sorted_set. It will be a new non-backward-compatible version.
5. I'm not sure what the block.arity == -2 (suggested fix in this PR) has to do with all the above.

[alexdowad]

@dubek I like the way that you are thinking.

As far as SortedSet goes, our code seems to be doing the right thing (as far as I can see right now)... it's just that while we are creating the right SortedSets, we can't confirm that by comparing them with eql?. (Note that the problem is with eql?, not ==.)

Adjusting the test to compare the SortedSets differently makes sense.

[dleidert]

• Ruby 3.0 removed SortedSet from the standard library and replaced it with a gem.
• The sorted_set gem relies on the rbtree gem for implementing SortedSet.
• With this change, the behavior of SortedSet#eql? has changed. That is what has broken Hamster.
• SortedSet#eql? just delegates to RBTree#eql?.
• RBTree#eql? is just the default implementation of the method.

I have opened #251 for this issue. You can relax the test (change from .eql to .eq), but it only works partially.

• I think the best fix would be to implement RBTree#eql? in the rbtree gem, giving it behavior similar to Hash#eql?.
• Just seeing if I can find the repo for that gem and submit a PR there.

The last uploads of this gem were done in 2021. So the maintainer seems to be active again... But I don't know a way to contact them either.

[dleidert]

@dleidert, another thing... could you explain how the code in this PR resolves the problem?

With Ruby 3 block.arity is -2. Thus, the condition tries to execute the else part and leads to an ArgumentError. With the above patch, all tests (except for those affected by #251) were successful. So I thought, it might be the right solution (therefor the "attempt to fix" :))

This patch is not related to the sorted_set issue described in #251.

[dleidert]

@alexdowad, this happens without the patch:

Failures:

  1) Hamster::SortedSet.new can use a block produced by Symbol#to_proc
     Failure/Error: items = items.sort(&block)

     ArgumentError:
       wrong number of arguments (given 1, expected 0)
     # ./lib/hamster/sorted_set.rb:93:in `name'
     # ./lib/hamster/sorted_set.rb:93:in `sort'
     # ./lib/hamster/sorted_set.rb:93:in `initialize'
     # ./lib/hamster/immutable.rb:14:in `new'
     # ./lib/hamster/immutable.rb:14:in `new'
     # ./spec/lib/hamster/sorted_set/new_spec.rb:49:in `block (3 levels) in <top (required)>'

[alexdowad]

@dleidert, thanks for the added information.

Question, please: Would you like me to guide you on how to improve this patch, or would you prefer that I just fix the problem in the most expedient way possible?

[dleidert]

@alexdowad, I'd prefer to leave the correct implementation/fix to the upstream authors :)