Override freeze-related behaviour in RubyRange for compatibility with Ruby 3.0

[MattAlp]

Update
As stated in the discussion below, this PR is now an optimization + compatibility change that modifies the behaviour of RubyRange and how it is used.

---

While working on #2453 and updating Range's behaviour to comply with 3.0, I discovered that RubyRange inherits from RubyDynamicObject, which exports freeze and isFrozen methods that dynamically set the frozen? property on the object.

This proposed change implements the same pattern as seen in RubyString, which exposes a frozen parameter in the constructor and overrides the freeze and isFrozen methods to explicitly work on the boolean frozen field. By doing so, the field is always present on the object and can default to true, rather than needing to be instantiated as a dynamic property. This also allows us to set the property at the appropriate call sites (i.e. to false when creating an object in Ruby that inherits from Range).

[nirvdrum]

Is this to handle freeze state in Range subclasses better? I think that's why RubyString has a local field like that.

[chrisseaton]

Extra context is that because RubyRange does not inherit from RubyBasicObject, it does not have any field slots. So the frozen field goes into an extension array, which is indirect and wasteful. Adding this field in here like this, like RubyString does avoids that extension array being used.

This is preliminary work to fixing the frozen behaviour of Range.

Note that another conversation to have is that Range, String etc also store their object IDs in extension arrays, for the same reason. We should probably fix that?

[eregon]

Ah, yes, not all Range are frozen, only instances of Range but not instances of subclasses of Ranges: https://bugs.ruby-lang.org/issues/15504#note-19
(Otherwise we'd just always return true for isFrozen on RubyRange and call it a day)

I think this is something to fix in CRuby at some point, it's a weird exception to immutable classes (https://gist.github.com/eregon/bce555fa9e9133ed27fbfc1deb181573).

[eregon]

Note that another conversation to have is that Range, String etc also store their object IDs in extension arrays, for the same reason. We should probably fix that?

So we'd have long objectID in RubyDynamicObject? The trade-off is higher memory footprint for all Ruby objects even if they never need the object_id. I think few objects actually need an object_id, but it would be good to have some data on that if we do such a change.
If the object_id computations come mostly from Kernel#inspect we might be able to instead use the Java identityHashCode for that purpose, which is somewhat what CRuby does IIRC (#inspect hexa identity code != object_id).

[chrisseaton]

So we'd have long objectID in RubyDynamicObject?

No, in RubyString, RubyRange, etc. Maybe a new base class for objects that don't expect lots of instance variables, but might need frozen and object_id?

[MattAlp]

Benoit and I briefly chatted in the Graal Slack. I'm going to go ahead and update this PR with additional changes that bring in the .frozen? compatibility change, including the call site changes, untag, and new spec.

[eregon]

No, in RubyString, RubyRange, etc. Maybe a new base class for objects that don't expect lots of instance variables, but might need frozen and object_id?

Yeah, that could work. It'd still increase the footprint for all instances of these. I'm not sure which builtin classes are likely to be frozen (but not always frozen) and/or object_id, would be good to have some data on that.
For now I don't see a strong need to share the trivial logic between RubyString and RubyRange.

[chrisseaton]

Need to fix the spec.

[eregon]

.dup should actually create an unfrozen copy:

irb(main):001:0> RUBY_VERSION
=> "3.0.2"
irb(main):002:0> (1..2).frozen?
=> true
irb(main):003:0> (1..2).dup.frozen?
=> false

Could you add specs for that (if there isn't already) and fix the behavior?
For the int and long ranges, we need to create non-frozen variants as well then (or create RubyObjectRange, but that seems not advantageous).

[eregon]

This looks great, thank you for the PR!

[eregon]

This should move to spec/ruby/core/range/dup_spec.rb, I'll do that.

[eregon]

I just remembered, there is a follow-up optimization related to this, range literals can be embedded in the AST:

$ ruby -ve '2.times { p (1..2).object_id }'  
ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux]
60
60

However this is a problem for AST sharing (sharing the same AST for multiple execution contexts, which have different Ruby classes, etc), because RubyRange objects are currently not context-independent, they point to a given RubyClass (private RubyClass metaClass; in RubyDynamicObject).
So to solve this we would need RubyIntRange/RubyLongRange to no longer inherit from RubyRange (we'd also just remove RubyRange), and instead inherit from ImmutableRubyObject.
Then in the translator when building a literal Range with int or long literals for both begin & end (non-beginless & non-endless) we could create a RubyIntRange/RubyLongRange directly in the translator (in org.truffleruby.parser.BodyTranslator#visitDotNode) and use an ObjectLiteralNode to embed the range instance in the AST.
For the cases we need to create a non-frozen range with int/long begin/end we'd go to ObjectLiteralNode then (e.g., for Range#dup).

@MattAlp Would you like to try that (separate PR of course) or you would prefer if somebody else works on that? It is a bit more involved than the change here but I think also not very difficult either.

=> moved to #2622

[MattAlp]

Sure, I'll take it @eregon