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

Selectable derive does not work with generic embed sub-fields #2950

Closed
Ten0 opened this issue Nov 9, 2021 · 3 comments · Fixed by #2962
Closed

Selectable derive does not work with generic embed sub-fields #2950

Ten0 opened this issue Nov 9, 2021 · 3 comments · Fixed by #2962

Comments

@Ten0
Copy link
Member

Ten0 commented Nov 9, 2021

Setup

Versions

What are you trying to accomplish?

#[derive(Queryable, Selectable)]
#[table_name = "schema::xxx"]
pub struct WithField<T> {
	pub field: i32,
	#[diesel(embed)]
	pub inner: T,
}

What is the expected output?

impl<T, __DB> Selectable<__DB> for WithField<T>
where T: Selectable<__DB> {}

What is the actual output?

impl<T, __DB> Selectable<__DB> for WithField<T> {}

-> fails to compile with "the trait bound T: diesel::Selectable<__DB> is not satisfied".

Solving

Looks like if a field has embed, the Selectable derive should add bounds FieldType: diesel::Selectable<__DB>, regardless of whether the type is generic or not.

This would also allow the Selectable derive to interact as expected with manual implementations of Selectable on a single backend (non-generic).

@weiznich
Copy link
Member

I'm not sure if there is an easy way to fix this issue. We would need to do some simplified form of type resolution there to understand that the type T is a generic parameter to restrict it to T: Selectable. We cannot add this bound for any generic parameter, as this would potentially add the bound to non #[diesel(embedded)] fields as well, which may result in unwanted behaviour there.

To workaround this issue, you can add this bound directly to your struct definition. This should also force the bound in any generated code.

@Ten0
Copy link
Member Author

Ten0 commented Nov 10, 2021

We cannot add this bound for any generic parameter, as this would potentially add the bound to non #[diesel(embedded)] fields as well, which may result in unwanted behaviour there.

My suggestion was to add the bound not on the generic types, but on the field types that are embed.

e.g., here:

#[derive(Selectable)]
struct S<T> {
    #[diesel(embed)]
    a: SomeStruct,
    #[diesel(embed)]
    b: T,
    some_field: i32,
}

the where clause would be where SomeStruct: Selectable<__DB>, T: Selectable<__DB>

This would also allow the Selectable derive to interact as expected with manual implementations of Selectable on a single backend (non-generic).

e.g.

struct S {
   ...
}
impl Selectable<Pg> for S { ... }
#[derive(Selectable)]
struct S2 {
    #[diesel(embed)]
    f: S,
}

To workaround this issue, you can add this bound directly to your struct definition. This should also force the bound in any generated code.

I've considered this but the bound is relative to a __DB type that doesn't exist at that time. For now I've just written the impl manually.

@weiznich
Copy link
Member

My suggestion was to add the bound not on the generic types, but on the field types that are embed.

e.g., here:

#[derive(Selectable)]
struct S<T> {
   #[diesel(embed)]
   a: SomeStruct,
   #[diesel(embed)]
   b: T,
   some_field: i32,
}

the where clause would be where SomeStruct: Selectable<__DB>, T: Selectable<__DB>>

That's a great idea and that should be really simple to add. Basically just add a loop over fields here and add the corresponding bounds to the where clause:

let mut generics = item.generics.clone();
generics
.params
.push(parse_quote!(__DB: diesel::backend::Backend));
let (impl_generics, _, where_clause) = generics.split_for_impl();

for field in model.fields() {
    if field.has_flag("embed") {
        let where_clause = generics.where_clause.get_or_insert(parse_quote!(where));
        let embed_ty = &field.ty;

        where_clause
                .predicates
                .push(parse_quote!(#embed_ty: Selectable<__DB>));
    }
}

Feel free to open a PR with this + a test case showing that it works as expected.

Ten0 added a commit to Ten0/diesel that referenced this issue Nov 21, 2021
Resolves diesel-rs#2950

Previously, fields had `embed`, the `Selectable` derive supposed that
they implemented `Selectable<DB>` for every `DB: Backend`.

If that wasn't the case (generic subfield or backend-specific impl) the
derive would fail.

This commit fixes this issue by introducing the appropriate additional
`where` bounds.
Ten0 added a commit to Ten0/diesel that referenced this issue Nov 21, 2021
Resolves diesel-rs#2950

Previously, fields had `embed`, the `Selectable` derive supposed that
they implemented `Selectable<DB>` for every `DB: Backend`.

If that wasn't the case (generic subfield or backend-specific impl) the
derive would fail.

This commit fixes this issue by introducing the appropriate additional
`where` bounds.
Ten0 added a commit to Ten0/diesel that referenced this issue Nov 21, 2021
Resolves diesel-rs#2950

Previously, if fields had `embed`, the `Selectable` derive supposed that
they implemented `Selectable<DB>` for every `DB: Backend`.

If that wasn't the case (generic subfield or backend-specific impl) the
derive would fail.

This commit fixes this issue by introducing the appropriate additional
`where` bounds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants