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

WIP: exploring workaround for slice first design #21

Open
wants to merge 6 commits into
base: slice_first_design
Choose a base branch
from

Conversation

RishabhRD
Copy link
Owner

WARN: current code doesn't even compile

This branch tries to compile slice first design with ideal code with original idea from #19 . I am making this separate branch because I am not sure if this would work, this is just a fight with borrow checker.
Few things taken from #19 discussion is:

  1. Inversion of control is necessary
  2. Lazy Collections are not required
  3. There is nothing as non-reorderable mutable range.

If somehow this works, this would be the stable code for slice_first_design then on which algorithms can be developed.

@RishabhRD
Copy link
Owner Author

The current code has some lifetime issues most probably. I have pushed same intent code in ideal_slice_code branch. I don't know the exact difference between both and I can achieve ideal_source_code thing in workaround code or not. But if this gets solved, then we can move forward with testing the abstraction ideas in terms of code and how they workout in different usecases.

@dabrahams
Copy link

dabrahams commented Feb 21, 2025

I don't know if you've asked real Rust experts about this, but I was very surprised that (while this doesn't match the trait), this change allows the slice function to compile:

diff --git a/src/array_slice.rs b/src/array_slice.rs
index 09b02d0..c3c7bbb 100644
--- a/src/array_slice.rs
+++ b/src/array_slice.rs
@@ -111,7 +111,7 @@ impl<'this, T> CollectionLifetime<'_> for MutableArraySlice<'this, T> {
     type MutableSlice = MutableArraySlice<'this, T>;
 }
 
-impl<T> Collection for MutableArraySlice<'_, T> {
+impl<'b, T> Collection for MutableArraySlice<'b, T> {
     fn start(&self) -> Self::Position {
         self.m_start
     }
@@ -141,6 +141,7 @@ impl<T> Collection for MutableArraySlice<'_, T> {
     where
         Callback: FnMut(&<Self as CollectionLifetime<'a>>::Slice) -> ReturnType,
         Self: 'a,
+        'a: 'b
     {
         let slice = ArraySlice::new(self.m_slice, from, to);
         callback(&slice)

And when I change it so that 'b encloses 'a, it fails to compile. To me that seems clearly backwards. The new slice's lifetime 'a needs to be enclosed by that of the outer slice. Actually I was trying to constrain the lifetimes to match, which you can do with 'a:'b, 'b:'a, but I don't understand how to get 'b represented in the trait. If you can't do that, this may be a dead end.

@dabrahams
Copy link

this suggests to me that maybe (at least for the callback form) maybe slices shouldn't conform to collection, but only /references/ to slices. Since the actual slice isn't going to escape, it might not matter.

@dabrahams
Copy link

this (which is in Rust now) might be helpful, but my Rust-fu is way too weak to understand how.

@RishabhRD
Copy link
Owner Author

@dabrahams I was trying to solve this lifetime problem this past 2 days. I got some interesting results. Will be answering your comments in separate comment.

So, I pushed the updated code in this branch. I started with dropping all the requirements on Slice and MutableSlice associated type. Interestingly, I was able to make ArraySlice and MutableArraySlice compile without those requirements with little bit of changes on impl blocks. However, I am still trying to figure out how to enforce the requirement of slice of a slice is slice itself. Not sure if this improves the situation a little.

Current code contains a quick sort implementation that doesn't compile because of lack of those slice requirements.

Coming to changes, earlier I did something similar to:

impl<'a, T> RangeLifetime<'_> for MutableArraySlice<'a, T> {
    type Slice = ArraySlice<'a, T>;

    type MutableSlice = MutableArraySlice<'a, T>;
}

It has the problem that it enforces associated type Slice has lifetime of 'a which is lifetime of MutableArraySlice.m_slice.
I corrected it with:

impl<'a, T> RangeLifetime<'a> for MutableArraySlice<'_, T> {
    type Slice = ArraySlice<'a, T>;

    type MutableSlice = MutableArraySlice<'a, T>;
}

With this updated code, lifetime of slice is associated with lifetime of RangeLifetime<'a> on construction site.

Another major problem was how I was trying to enforce requirements on Slice.

pub trait RangeLifetime<'this, ImplicitBounds: Sealed = Bounds<&'this Self>>:
    RangeBase
{
    /// Type of slice of range.
    type Slice: Range<Slice = Self::Slice>;
}

However, this is totally wrong. I realized from the code of ideal_slice_code:

pub trait Collection {
    type Slice<'a>: Collection<
          Slice<'a> = Self::Slice<'a>
    >;
}

In ideal code, Slice is a type constructor not type and in while enforcing bounds we are actually enforcing equality of type constructor not type. However, in the first code, we are enforcing type equality instead of type constructor equality. This is a big problem as Slice has a lifetime attached to it that is inherited from RangeLifetime's lifetime and that would lead RangeLifetime implementation not matching the lifetime mentioned in trait.
This is not the case with Swift as in swift, lifetimes are not mixed with types and thus enforcing bounds on slice is enforcing type equality. Mixing lifetime annotations with types actually made really complex in this case.

With this, I am sure that its not possible to enforce requirements on Slice in RangeLifetime trait so, it should be outside. Now, I am not sure how to check equality of type constructors in where clause. I am sure your workaround would be required for that, but I am still struggling with rust syntax for checking type constructor equality.

@RishabhRD
Copy link
Owner Author

RishabhRD commented Feb 22, 2025

Now answering your comments:

I don't know if you've asked real Rust experts about this

Nope, I am also new to rust community so I don't know many people in rust community right now. Maybe #guild-rustlang might help not sure.

To me that seems clearly backwards.

I was struggling with same for many days. Its really painful to work with lifetimes 😅 .

this (which is in Rust now) might be helpful, but my Rust-fu is way too weak to understand how.

I have used the same in my ideal_slice_code, however, it gets hit with language limitation while writing traits like BidirectionalCollection. I described a little about it here. Specifically, writing BidirectionalCollection enforces that implementation struct should have static lifetime (i.e., they should live for full program lifetime). I should create a new issue on rust repo for this I guess. This is a well known issue in rust community, just that I am not sure if there is any ongoing work to fix the same.

For what it's worth, at some point I suggest you look very carefully at my workarounds and consider translating them because they are quite evolved to create a good user experience.

I have this realization, just that I was little busy in trying to figure out this lifetime issue recently. I am going to work on translating this workaround as I have this feeling that type constructor equality requirement on slice would be easier to write in this form. Just that I need to figure out, if this type constructor equality thing is feasible or not.

If this generic associated types issue would have been fixed, the whole thing would have been really easy.

@dabrahams
Copy link

I just found, in my working copy of this repo, a not-checked-in file called proto.rs that looks a little bit like the code in my gist. I don't know where it came from but I'll have to assume you wrote it.

I don't know what a type constructor equality requirement is, so I'm having a hard time understanding some of what you wrote.

I had no idea you were an Adobe person! My suggestion is to boldly ask questions in all the places (our slack, rust community); that's what I do. The sooner the better, because someone may have my answer before I waste more time flailing about in the dark.

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.

2 participants