Project

General

Profile

Actions

Feature #15504

closed

Freeze all Range objects

Added by ko1 (Koichi Sasada) about 5 years ago. Updated over 2 years ago.

Status:
Closed
Target version:
-
[ruby-core:90881]

Description

Abstract

Range is currently non-frozen. How about freezing all Range objects?

Background

We froze some types of objects: Numerics (r47523) and Symbols [Feature #8906]. I believe that making objects immutable solves some kinds of programming difficulties.

Range is mutable at least when written as Range literal. So we can write the following weird program:

2.times{
  r = (1..3)
  p r.instance_variable_get(:@foo)
  #=> 1st time: nil
  #=> 2nd time: :bar
  r.instance_variable_set(:@foo, :bar)
}

In range.c, there is a comment (thanks znz-san):

static void
range_modify(VALUE range)
{
    rb_check_frozen(range);
    /* Ranges are immutable, so that they should be initialized only once. */
    if (RANGE_EXCL(range) != Qnil) {
	rb_name_err_raise("`initialize' called twice", range, ID2SYM(idInitialize));
    }
}

Patch

Index: range.c
===================================================================
--- range.c	(リビジョン 66699)
+++ range.c	(作業コピー)
@@ -45,6 +45,8 @@
     RANGE_SET_EXCL(range, exclude_end);
     RANGE_SET_BEG(range, beg);
     RANGE_SET_END(range, end);
+
+    rb_obj_freeze(range);
 }
 
 VALUE

Discussion

There are several usages of mutable Range in the tests.

  • (1) Taint-flag
  • (2) Add singleton methods.
  • (3) Subclass with mutable states

Maybe (2) and (3) are crucial.

Thanks,
Koichi


Related issues 1 (0 open1 closed)

Related to Ruby master - Feature #17195: Freeze Enumerator::ArithmeticSequence objectsRejectedActions

Updated by marcandre (Marc-Andre Lafortune) about 5 years ago

I think that (2) and (3) are indeed capital points. Freezing range litterals (only) might be a better idea? with an approach like frozen string literals?

Not that even frozen ranges aren't completely immutable:

r = ('a'..'z').freeze
r.end.upcase!
r # => "a".."Z" 

Updated by Eregon (Benoit Daloze) over 4 years ago

I think it would make sense to freeze Range literals.

Adding methods to Range might be reasonable, but singleton methods, I would think much less so.

Updated by mame (Yusuke Endoh) over 4 years ago

I guess @Eregon (Benoit Daloze) came from #15950. Will ary[-3..] be as efficient as ary[-3, 3] by freezing and deduping a literal (-3..)? Looks good if we can confirm it by an experiment.

Some thoughts:

  • Even if a range is frozen, ("a".."z") should not be deduped because of the reason @marcandre (Marc-Andre Lafortune) said.
  • I'm for freezing all Ranges, not only Range literals. I hate the idea of freezing only literals because casually mixing frozen and unfrozen objects leads to hard-to-debug bugs that depend upon data flow.
  • It would be the most elegant if the combination of MJIT and escape analysis solves this kind of performance problems.

Updated by ko1 (Koichi Sasada) over 3 years ago

I got an issue on Ractor.

def test
  (1..2)
end

r1 = test

Ractor.new do
  r2 = test
end.take
  • compiler cached Range (1..2) because they begin and end are frozen literals. The test method returns a cached same range object.
  • it means test returns not-immutable but same object. It violate the Ractor's memory model.

To solve it, there are two options.

(1) avoid cache at compile time.
(2) freeze Range objects which will be cached by th compiler.

For performance reason, I want to choose (2).

After that, could you please discuss all Range objects should be frozen or not.

Thanks,
Koichi

Actions #5

Updated by sawa (Tsuyoshi Sawada) over 3 years ago

  • Subject changed from Freeze all Range object to Freeze all Range objects
  • Description updated (diff)

Updated by Eregon (Benoit Daloze) over 3 years ago

Right, every object cached at parse time must be deeply immutable, I would think that was an oversight.

Updated by matz (Yukihiro Matsumoto) over 3 years ago

I agree with making ranges frozen. I don't see any particular case that could be broken by frozen ranges.
Since there's possiblity of breakage, I'd like to experiment it.

Matz.

Updated by ko1 (Koichi Sasada) over 3 years ago

Ok, I freeze all Ranges except sub-classes.
https://github.com/ruby/ruby/pull/3583

Actions #9

Updated by ko1 (Koichi Sasada) over 3 years ago

  • Status changed from Open to Closed

Applied in changeset git|0096d2b895395df5ab8696d3b6d444dc1b7730b6.


freeze all Range objects.

Matz want to try to freeze all Range objects.
[Feature #15504]

Updated by Eregon (Benoit Daloze) over 3 years ago

Great!

Related, should Enumerator::ArithmeticSequence be frozen too?

Updated by ko1 (Koichi Sasada) over 3 years ago

Eregon (Benoit Daloze) wrote in #note-10:

Related, should Enumerator::ArithmeticSequence be frozen too?

new ticket?

Actions #12

Updated by Eregon (Benoit Daloze) over 3 years ago

  • Related to Feature #17195: Freeze Enumerator::ArithmeticSequence objects added

Updated by Eregon (Benoit Daloze) over 3 years ago

ko1 (Koichi Sasada) wrote in #note-11:

Eregon (Benoit Daloze) wrote in #note-10:

Related, should Enumerator::ArithmeticSequence be frozen too?

new ticket?

I filed #17195

Updated by AlexWayfer (Alexander Popov) about 3 years ago

I can't now mock Range objects with RSpec (gorilla_patch gem). What should I do? I see no work-around, like +'foo' for strings. Range.new gives frozen objects too.

I found a work-around: (5..7).dup, but it's weird anyway.

Updated by zverok (Victor Shepelev) about 3 years ago

@AlexWayfer (Alexander Popov)

https://github.com/AlexWayfer/gorilla_patch/blob/master/lib/gorilla_patch/cover.rb#L8 -- may be for this particular case it is better to have version guard as an outer check?..

if RUBY_VERSION < '2.6'
  def cover?(value)
    #...
  end
end

...and have the same guard in specs?..

Updated by AlexWayfer (Alexander Popov) about 3 years ago

zverok (Victor Shepelev) wrote in #note-15:

@AlexWayfer (Alexander Popov)

https://github.com/AlexWayfer/gorilla_patch/blob/master/lib/gorilla_patch/cover.rb#L8 -- may be for this particular case it is better to have version guard as an outer check?..

if RUBY_VERSION < '2.6'
  def cover?(value)
    #...
  end
end

...and have the same guard in specs?..

Thank you, I agree, it's better. But… if I want to check was called refined method or core? Right now I'm doing it via have_received once or never (depending on Ruby version), and for this RSpec should change the object (Range), but it's frozen since Ruby 3. Do I have other ways to check which implementation of method was used? value.method(:cover?).source_location returns nil in both cases.

Updated by zverok (Victor Shepelev) about 3 years ago

if I want to check was called refined method or core?

It actually might be a good feature proposal for Ruby. Because currently, you can tell whether the method is defined by this class, by its parent, by included module, by singleton class... via Method#owner. But as far as I can recall, there is no way to ask "whether the method is defined by refinement".

But this whole discussion is unrelated to Range frozenness, honestly :)

Updated by AlexWayfer (Alexander Popov) about 3 years ago

zverok (Victor Shepelev) wrote in #note-17:

if I want to check was called refined method or core?

It actually might be a good feature proposal for Ruby. Because currently, you can tell whether the method is defined by this class, by its parent, by included module, by singleton class... via Method#owner. But as far as I can recall, there is no way to ask "whether the method is defined by refinement".

I'm too lazy for such proposals, especially well formatted, but I can try. UPD: Created https://bugs.ruby-lang.org/issues/17674

zverok (Victor Shepelev) wrote in #note-17:

But this whole discussion is unrelated to Range frozenness, honestly :)

No, I disagree! My issue was raised by resolving this issue, even without changes mention in the article about Ruby 3: https://www.ruby-lang.org/en/news/2020/12/25/ruby-3-0-0-released/

Updated by Eregon (Benoit Daloze) over 2 years ago

@ko1 (Koichi Sasada) Do you know why only Range instances and not Range subclass instances were frozen? (https://bugs.ruby-lang.org/issues/15504#note-8)

This issue title is a bit confusing, also https://bugs.ruby-lang.org/issues/15504#note-9.
Probably https://bugs.ruby-lang.org/issues/15504#note-9 was reverted?

I guess "Freeze all Range objects" means all Range instances, literal or not, but still not subclass instances, that confused me.

Updated by ko1 (Koichi Sasada) over 2 years ago

Eregon (Benoit Daloze) wrote in #note-19:

This issue title is a bit confusing, also https://bugs.ruby-lang.org/issues/15504#note-9.
Probably https://bugs.ruby-lang.org/issues/15504#note-9 was reverted?

I guess "Freeze all Range objects" means all Range instances, literal or not, but still not subclass instances, that confused me.

I see. Not sure but maybe it is only implementation issue.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0