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

Promoting a writable Slice to a read-only one #12133

Open
HertzDevil opened this issue Jun 17, 2022 · 4 comments
Open

Promoting a writable Slice to a read-only one #12133

HertzDevil opened this issue Jun 17, 2022 · 4 comments

Comments

@HertzDevil
Copy link
Contributor

There is surprisingly no safe way to produce a read-only Slice from a writable x : Slice that refers to the same elements:

# calls unsafe method
Slice.new(x.to_unsafe, x.size, read_only: true)

# refers to different elements
x.map(read_only: true, &.itself)

# not read-only
x.dup
x.clone

# propagates `@read_only`
x[..]
x + 0

There is no reason this would be considered unsafe. I propose wrapping the first expression into a method:

struct Slice(T)
  # Returns a new read-only `Slice` referring to the same elements as `self`.
  #
  # This method does not copy any elements, nor modify `self`; if `self` is
  # writable, it remains so after this method returns.
  #
  # ```
  # x = Slice[1, 2, 3]
  # y = x.freeze
  # y[0] = 4 # raises Exception
  # x[0] = 4 # okay
  # ```
  def freeze : Slice(T)
    Slice.new(@pointer, @size, read_only: true)
  end
end

If this sounds familiar, the name freeze comes from Ruby's Object#freeze.

With #freeze we can simplify a few other methods that take a read_only argument:

# before
Int32.slice(1, 2, 3, read_only: true)
Slice[1, 2, 3, read_only: true]
x.map(read_only: true, &.itself)
x.map_with_index(read_only: true) { 0 }

# after
Int32.slice(1, 2, 3).freeze
Slice[1, 2, 3].freeze
x.map(&.itself).freeze
x.map_with_index { 0 }.freeze

So I suggest we deprecate the read_only parameter in the above methods too. The ones in Slice's constructors can probably stay, although strictly speaking, only #initialize needs this parameter and the other .new overloads don't.

@Fryguy
Copy link
Contributor

Fryguy commented Jun 17, 2022

I love the freeze method, but my only concern here is the deprecation of the parameter, because I can imagine future internal optimizations with the parameter form that I don't think can be done in the method form.

x.map(read_only: true, &.itself) # Here you know within the map call that the result will be frozen
x.map(&.itself).freeze           # Here you _don't_ know within the map call that the result will be frozen

@straight-shoota
Copy link
Member

I think there is actually a somewhat dangerous aspect to it. Not necessarily unsafe, but you need to be aware of when using this.

When a slice points to memory that is writable through some other means, the slice itself is read-only, but the data may still change. When read_only? == true you can only know that this window to that memory is read-only.

Currently, you can only create a read-only Slice when allocating the memory directly in one of the constructors, or when constructing a slice from a pointer (which is inherently unsafe and requires attention about its semantics).
Thus there's high confidence that the memory which a read-only slice points to is actually not writable (assuming only safe operations).

If we allow arbitrarily creating a read-only slice as a read-only window to writable memory, we're weakening the assumptions you can make about Slice#read_only?.

I think there are good reasons for each use case - read-only memory as well as a read-only window to writable memory in order to protect it from write access where it's not meant to be writable.
The read-only memory part is certainly an imperfect approach to guarantee immutability.
So perhaps we can evolve in that direction (for example with #10944) and broaden read-only semantics of Slice to only apply to that memory window.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Feb 14, 2023

I thought about this a bit more, and my conclusion is that to create a read-only window one could do it generically:

module Indexable(T)
  record View(I, T), indexable : I do
    include Indexable(T)
    delegate unsafe_fetch, size, to: @indexable

    def view
      self
    end
  end

  # it is tempting to erase the `I` type above, but the following would produce a
  # recursive struct because `@indexable`'s type includes `View(T)` itself
  # it also forces method dispatch on `@indexable` where often unnecessary
  # record View(T), indexable : Indexable(T)

  def view
    View(self, T).new(self)
  end
end

The above works also on types whose elements aren't laid out contiguously in memory, like Tuple (because it is an aggregate rather than an array), Deque (because as a circular buffer its elements could form two contiguous slides instead), and BitArray (because its elements are not byte-aligned).

By that reasoning, there is actually no need to use owning Slices as a writable data structure at all when Array exists. One could simply write ([1, 2, 3] of UInt8).view instead of Bytes[1, 2, 3, read_only: true], and this #view can also completely replace the #freeze suggested in the OP. Then Slice could be completely reserved for views of read-only memory.

@straight-shoota
Copy link
Member

straight-shoota commented Feb 14, 2023

Just thinking aloud here: if View would also incorporate index projection, it would be a nice implementation of #3386 and #12390 (for read-only use cases of course).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants