Associated Reference Types

[danii]

I have added different associated types for the reference types returned from trait methods such as Get::get, using generic associated types to enable patterns such as collections that return guarded references. I've done this in anticipation of Rust stabilizing GATs soon, as their blog post has led me to believe. To further emphasize the pattern of collections that are locks, I've added Lock traits which provide the same interface as their non-Lock counterparts, only behind an immutable reference rather than a mutable one. To showcase these new features I've also added implementations for the traits on structures in the popular dashmap crate.

I'll ready this pull request if and when (hopefully just a matter of when) GATs are stabilized.

[timothee-haudebourg]

Hi, thanks for the pull request! I think adding support for GATs is a great idea, however I've made a series of remarks before I can pull. In particular, I think I'd like to see a separate pull request for the Lock Traits part.

[timothee-haudebourg]

I there a reason why you choose to add the Self: 'a where clause? Why not just

type ItemRef<'a>: 'a + Deref<Target = Self::Item>

[danii]

I there a reason why you choose to add the Self: 'a where clause? Why not just

type ItemRef<'a>: 'a + Deref<Target = Self::Item>

I chose to as Self: 'a because any generics placed in the type must live as long as 'a. 'a in this context is unbounded, so it could be anything, which would require that generics placed in the type must live long as 'static.

In theory, the bound Self::Item: 'a would work fine, but in practice, some reference types may hold more references to generic types than just Self::Item, e.g. DashMap's reference type also holds a reference to the key.

The most correct solution would probably be to add another type for every reference, where implementors could provide all the generics their reference type uses, so we could add a bound Self::ItemRefGenerics: 'a or something like that. But just adding Self: 'a probably works just as well and is more elegant.

[timothee-haudebourg]

I'm not sure to see what you mean. dashmap::mapref::one::Ref<'a, K, V, S> has only one lifetime parameter here. If we take the Get trait for instance with the get function:

fn get<'a>(&'a self, key: T) -> Option<Self::ItemRef<'a>>;

The 'a lifetime on self should imply Self: 'a. Moreover I think associated type lifetimes are implicitly bound by the implementor's lifetime right? Here is an example:

#![feature(generic_associated_types)]
pub trait Foo {
  type T<'a>;
}

impl<'t, T> Foo for &'t T {
  type T<'a> = Self;
}

This compiles even though I never explicitly specified Self: 'a for Foo:T<'a>. But it automatically derived &'t T: 'a even though &'t T is obviously not 'static.

[timothee-haudebourg]

In fact, following this reasoning I even wonder if specifying Deref<Target = Self::Item> is enough, instead of 'a + Deref<Target = Self::Item>...

[timothee-haudebourg]

Hu... you are right, the Self: 'a bound seems required when the implementor has type parameters. The implicit bound that seems to be automatically derived for Self in my example is not automatically derived for the type parameters. How weird.

[timothee-haudebourg]

No it is even weirder. In my example It doesn't work if we replace Self with &'a Self. So it can detect Self: 'a but not &'a Self: 'a? I feel like my example should not be accepted in the first place...

[timothee-haudebourg]

My confusion has been resolved in the forum. We will need to wait for #87479 to make an actual decision here about the where Self: 'a bound.

[timothee-haudebourg]

You don't need to use an explicit lifetime here (and other similar functions) and instead use the placeholder lifetime '_ to make it less verbose:

fn get(&self, key: T) -> Option<Self::ItemRef<'_>>;

[timothee-haudebourg]

So you want to define interfaces for concurrent data structures. I'm not a fan of using the term Lock in the name of the trait since such data structure may be implemented without any lock. Maybe ConcurrentGet (a bit long) or ConstGet (maybe ambiguous) would be more appropriate? Anyway I think introducing concurrent traits is well beyond just adding GATs and should be part of another pull request so we can discuss these details.

[danii]

So you want to define interfaces for concurrent data structures. I'm not a fan of using the term Lock in the name of the trait since such data structure may be implemented without any lock. Maybe ConcurrentGet (a bit long) or ConstGet (maybe ambiguous) would be more appropriate? Anyway I think introducing concurrent traits is well beyond just adding GATs and should be part of another pull request so we can discuss these details.

Okay, I'll split this into two PRs, I'll keep this PR on GATs.

[timothee-haudebourg]

Do you have a motivation for adding a lifetime to the associated Output types?

[danii]

Do you have a motivation for adding a lifetime to the associated Output types?

I added it in case some insert operations returns a reference to the data back or something; although I should add Self: 'a here too.

[timothee-haudebourg]

Thinking about your proposition, I think I would prefer to break the Collection trait into smaller parts to preserve the spirit of this crate (small, composable traits). Not all collections can provide references to its items, and even less collections are mutable. So in many case, providing ItemRef and ItemMut will not be possible. I propose to split your Collection definition into smaller traits:

pub trait Collection {
  type Item;
}

pub trait CollectionRef: Collection {
  type ItemRef<'a>: ...;
}

pub trait CollectionMut: Collection {
  type ItemMut<'a>: ...;
}

[danii]

Agh I just pushed broken code because I didn't put the Self: 'a bounds on Output in the implementations! 😱
Don't review just yet

[danii]

Thinking about your proposition, I think I would prefer to break the Collection trait into smaller parts to preserve the spirit of this crate (small, composable traits). Not all collections can provide references to its items, and even less collections are mutable. So in many case, providing ItemRef and ItemMut will not be possible. I propose to split your Collection definition into smaller traits:

pub trait Collection {
  type Item;
}

pub trait CollectionRef {
  type ItemRef<'a>: ...;
}

pub trait CollectionMut {
  type ItemMut<'a>: ...;
}

That is probably for the better, I'll add that too.

[timothee-haudebourg]

Almost there. There is still an issue with the example the slab impl, and then we must wait GATs stabilization...

[timothee-haudebourg]

My confusion has been resolved in the forum. We will need to wait for #87479 to make an actual decision here about the where Self: 'a bound.

[timothee-haudebourg]

Since I would prefer for this crate to remain usable with stable rust, we'll need to wait for GATs stabilization and remove this before merging release.

[timothee-haudebourg]

@danii can you fix the Slab implementation so I can merge? I'm actually going to need these changes very soon.

[timothee-haudebourg]

Looks fine to me. Thanks! I won't release it until GATs are stabilized though.

[danii]

@danii can you fix the Slab implementation so I can merge? I'm actually going to need these changes very soon.

Okay, done, sorry I keep missing some things. 😅
If you'd like to merge it now, I'd recommend to merge it into a different branch until GATs are stabilized.

[timothee-haudebourg]

@danii FIY I think I'm going to drop the lifetime parameter for the Output types as we don't seem to have a use case for it and it messes up the trait aliases where I use an equality bound Output=bool. As far as I know, equality bounds are not supported for GATs yet.