Add Set-like object interoperation specs

[ivoanjo]

As discussed in jruby/jruby#5227, the persistent-💎 gem takes advantage of the fact that Ruby's set.rb uses is_a?(Set) to recognize other sets to enable Persistent💎::Set instances to be compared, intersected, disjointed, and flattened as if they were normal Ruby sets.

These specs cover the is_a?(Set) set.rb interoperation behaviors, and are broken in JRuby 9.2.0.0 by the new higher-performance Set implementation.

Issue jruby/jruby#5227

[eregon]

This is interesting, I am not opposed to these specs but I'd like to give a think on how to achieve this possibly better.
Overriding is_a? seems fragile, e.g., kind_of? should probably be overridden too for consistency but Module#=== like Set === persistentSet will not work (unless monkey patched or overridden on Set which feels too much of a hack).

How about having PersistentSet inherit from Set, like SortedSet does, even though it might not share much of the internal representation (the same applies for SortedSet when rbtree is installed)?
Then PersistentSet would naturally be "is_a?" Set.
Would that be problematic or work well?

[headius]

FYI we requested that @ivoanjo add these specs in response to jruby/jruby#5227 but we do not necessarily agree with the behavior.

My thoughts:

is_a? has generally been considered an antipattern in Ruby code. The preferred way to treat an object like a given type is to just let it duck-type as that object. The use of is_a? here is narrowing the range of duckable types to those that either descend from Set (a hard static typing restriction that's weird for Ruby) or those that override is_a? to pretend they descend from Set (which is really lying...because changes to the Set class would not be visible on these types, I do not consider them to be is_a? Set).

I would propose that we try to modify the behavior so that rather than using is_a? it uses either respond_to? for the methods in question or it just goes right ahead and calls them.

@ivoanjo Thank you for putting the specs together! They will help us have a discussion about what would be "correct" behavior for this logic.

[ivoanjo]

Thanks @headius for the added context! I was a bit too terse ;)

I also don't really like this solution -- for sure is a hack. On the other hand, it's an useful(ish) hack: I was able to get it to work on MRI 1.9+; JRuby 1.7+ and TruffleRuby (with an extra hack for JRuby 9.2).

Subclassing Set would be another option, but it's a very awkward one at best -- it would make Set like a very awkward Java interface, but one that has lots of default behavior that needs to be defused. (Would be extra awkward in my case, as Persistent💎::Set extends Hamster::Set from the Hamster gem, adding the extra interoperability code and some extra bits).

I'm not really sure what was the intent behind the is_a?(Set) tests was; I wonder if the author wanted to avoid accidental interactions with the Array class, as it would be terribly inefficient?

I think a nice solution would be to have a to_set() implicit conversion method, like the usual to_ary() and to_hash(). There seems to be precedent on classes that only provide an implicit conversion method, without an explicit one. In fact, the Persistent💎 already supports converting objects that respond_to(:to_set) into Persistent💎::Set -- see the "converting from existing structures" section of the documentation; I wanted to avoid the same issue I faced to when I wrote the code I already included that hook for api clients to use.

Now I'm not sure -- what's the best way to take this forward? Raise this in the Ruby issue tracker?

[eregon]

Which Set methods accept a Set-like arguments and what are the methods called on the arguments? Is it just essentially subset? and similar, expecting an Enumerable argument with size, include?, any? and all? ? In any case it seems good to bring this to the MRI issue tracker, and make a concrete proposal to solve this cleanly, for instance using respond_to? :to_set. It's a good occasion to also ask the maintainer why an ArgumentError is raised instead of just calling methods on the arguments (which would be simplest and is done for & for instance). BTW, Set#to_set exists. Like other implicit conversion methods, to_set must not necessarily return an instance of Set but an object with the same methods. However, this solution is unlikely to be backported, yet it would obviously be good to support PersistentSet's use case on Ruby versions < 2.6. The current workaround seems reasonable in this context and so I propose to merge these specs. When the clean solution is decided, we can extend those specs to e.g. add #to_set to SetLike and let Ruby implementations support either of these two ways.

…

On Tue, Oct 16, 2018, 02:36 Ivo Anjo ***@***.***> wrote: Thanks @headius <https://github.com/headius> for the added context! I was a bit too terse ;) I also don't really like this solution -- for sure is a hack. On the other hand, it's an useful(ish) hack: I was able to get it to work on MRI 1.9+; JRuby 1.7+ and TruffleRuby (with an extra hack for JRuby 9.2). Subclassing Set would be another option, but it's a very awkward one at best -- it would make Set like a very awkward Java interface, but one that has lots of default behavior that needs to be defused. (Would be extra awkward in my case, as Persistent💎::Set extends Hamster::Set from the Hamster <https://github.com/hamstergem/hamster> gem, adding the extra interoperability code and some extra bits). I'm not really sure what was the intent behind the is_a?(Set) tests was; I wonder if the author wanted to avoid accidental interactions with the Array class, as it would be terribly inefficient? I think a nice solution would be to have a to_set() implicit conversion method, like the usual to_ary() and to_hash(). There seems to be precedent <https://idiosyncratic-ruby.com/54-try-converting.html> on classes that only provide an implicit conversion method, without an explicit one. In fact, the Persistent💎 already supports converting objects that respond_to(:to_set) into Persistent💎::Set -- see the "converting from existing structures" section of the documentation <https://gitlab.com/ivoanjo/persistent-dmnd/blob/master/README.adoc>; I wanted to avoid the same issue I faced to when I wrote the code I already included that hook for api clients to use. Now I'm not sure -- what's the best way to take this forward? Raise this in the Ruby issue tracker? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#629 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKTlmqodnMRBFuJ66QLFcrWPMIeTOHQks5ulNWugaJpZM4XbGsY> .

[ivoanjo]

Which Set methods accept a Set-like arguments and what are the methods called on the arguments? Is it just essentially subset? and similar, expecting an Enumerable argument with size, include?, any? and all? ?

Yes, they don't do anything special that anything that implements the usual Enumerable methods won't be able to handle. I believe I covered all of them in the specs, here's a quick list:

• #==
• #flatten
• #flatten!
• #intersect?
• #disjoint?
• #subset?
• #proper_subset?
• #superset?
• #proper_superset?

I admit I didn't even remember (knew?) about Set#to_set. Indeed if it already exists, it seems really straightforward to replace the is_a?(Set) with respond_to(:to_set) && .to_set.

However, this solution is unlikely to be backported, yet it would obviously be good to support PersistentSet's use case on Ruby versions < 2.6. The current workaround seems reasonable in this context and so I propose to merge these specs. When the clean solution is decided, we can extend those specs to e.g. add #to_set to SetLike and let Ruby implementations support either of these two ways.

Sounds like a plan. The gem is really niche so I can definitely live with the workarounds I have right now, but it would indeed be nice to solve this as sets rarely get any love in Ruby.

Shall I raise the ticket on the Ruby issue tracker, or d'you prefer going for it?

[eregon]

Shall I raise the ticket on the Ruby issue tracker, or d'you prefer going for it?

@ivoanjo Please create the ticket.

[ivoanjo]

Issue raised upstream @ https://bugs.ruby-lang.org/issues/15240.

[eregon]

Given that is_a? seems the only way to achieve this (except subclassing which is not practical in some cases like here), I'm tempted to merge these specs as it is the de facto way to make Set handle Set-like objects. And then we can evolve them when a proper solution is added in Ruby.
I'll wait a couple more days to see if there are more comments on the Ruby tracker.

[ivoanjo]

Slight delay but I'm back from holidays! Happy new year, btw!

I've made the requested changes, and also rebased on top of current master.

[eregon]

@ivoanjo Thanks for the PR! We'll need to keep pushing on https://bugs.ruby-lang.org/issues/15240 to get a proper solution, but this is now merged to ensure the current workaround keeps working.

[eregon]

Note: these specs were partially disabled in ruby/ruby@dc7785e

[ivoanjo]

Thanks for the heads-up!