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

Attempt to support Ruby 3.0 #252

Open
wants to merge 1 commit into
base: core
Choose a base branch
from
Open

Attempt to support Ruby 3.0 #252

wants to merge 1 commit into from

Conversation

dleidert
Copy link

No description provided.

@alexdowad
Copy link
Contributor

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

@alexdowad
Copy link
Contributor

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
Copy link
Contributor

dubek commented Nov 28, 2021

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

@alexdowad
Copy link
Contributor

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
Copy link
Contributor

Just found this: knu/sorted_set#3

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

@alexdowad
Copy link
Contributor

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
Copy link
Contributor

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

@dubek
Copy link
Contributor

dubek commented Nov 28, 2021

  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
Copy link
Contributor

@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
Copy link
Author

  • 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
Copy link
Author

dleidert commented Nov 28, 2021

@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
Copy link
Author

@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
Copy link
Contributor

@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
Copy link
Author

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

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.

3 participants