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

Associations with generic types #1827

Closed
moulins opened this Issue Aug 23, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@moulins

moulins commented Aug 23, 2018

I have the following two tables (with the corresponding schema!s):

#[derive(Debug, Queryable, Insertable, Identifiable)]
#[table_name = "foos"]
pub struct Foo<'a> {
    id: i32,
    stuff: Cow<'a, String>
}

#[derive(Debug, Queryable, Insertable, Identifiable, Associations)]
#[belongs_to(Foo)]
#[table_name = "bars"]
pub struct Bar {
    id: i32,
    foo_id: i32
}

The #[derive(Associations)] fail with expected lifetime parameter, because there is no way to refer to the generic lifetime 'a of Foo in the #[belongs_to]. Did I overlook something which could allow me to use the derive?


Of course, I can implement BelongsTo manually, and everything seems to work:

impl<'a> BelongsTo<Foo<'a>> for Bar {
    type ForeignKey = i32;
    type ForeignKeyColumn = schema::bars::foo_id;
    
    fn foreign_key(&self) -> Option<&Self::ForeignKey> {
        Some(&self.foo_id)
    }

    fn foreign_key_column() -> Self::ForeignKeyColumn {
        schema::bars::foo_id
    }
}

But it would be nicer if we could specify these kinds of generic lifetimes, maybe with something like: #[belongs_to(type="for<'a> Foo<'a>")]?

@sgrif

This comment has been minimized.

Member

sgrif commented Aug 31, 2018

This is something we should support, but I'd like to see more discussion around what the API should be. We 100% need the lifetime declared in belongs_to, but I'm wondering if just belongs_to("Foo<'a>") makes more sense

@moulins

This comment has been minimized.

moulins commented Sep 1, 2018

We 100% need the lifetime declared in belongs_to, but I'm wondering if just belongs_to("Foo<'a>") makes more sense

Well, I was thinking "What if you want to specify constraints on your lifetimes?"; but when thinking more about it, all diesel objects you'll get back via belongs_to should be 100% owned, so all lifetime parameters can be unconstrained.

Using belongs_to("Foo<'a>") still seems wierd to me, because the 'a isn't defined anywhere, and I'm not sure what belongs_to("Foo<'a, 'a>") should mean:

  • impl<'a> ... for Foo<'a, 'a>, or
  • impl<'a, 'b> ... for Foo<'a, 'b>?

Instead, I would propose belongs_to("Foo<'_>"): the elided lifetime makes it clear that any lifetime is acceptable.

@sgrif

This comment has been minimized.

Member

sgrif commented Sep 6, 2018

I think allowing elided lifetimes as the syntax makes perfect sense.

sgrif added a commit that referenced this issue Sep 12, 2018

Allow associations to structs with lifetime parameters
This was a bit of a pain to implement... I had hoped to just have the
syntax be `#[belongs_to(User<'_>)]`, which works on nightly... but is
feature gated on stable. I think that might be going away in 1.30, but
I'm not 100% certain yet. Second attempt was `#[belongs_to("User<'_>")]`
which works on nightly, but is feature gated on stable.

So the API has to be `#[belongs_to(some_key = "User<'_>")]` for the time
being. I've gone with `parent` as the key. This also makes our parsing
logic much more complicated, since we're now handling multiple forms
potentially given in multiple places. I want to refactor meta handling
in general eventually, but for now a little bit of spaghetti is fine.

The bigger messiness comes from what happens after the parsing. Next we
need to go through the type, and replace all instances of `'_` with some
unique lifetime, and stick that on the generics that we use for the
impl. This would actually look a bit cleaner if we had the users provide
lifetime names, but I'd rather be consistent with Rust syntax, instead
of simplifying our impl.

Fixes #1827.

@sgrif sgrif closed this in #1849 Sep 12, 2018

@moulins

This comment has been minimized.

moulins commented Sep 12, 2018

Nice, thanks for the work!

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