fix: Decode and Encode derives (#1031) #2940

benluelo · 2023-12-18T00:15:15Z

Reopening #2329

asteinba · 2024-01-07T23:25:56Z

seems like some checks failed because there is a unused import which is treated as an error. Can you fix that? I would love to see this merged as this issue bugs me since a while.

Happy to help out as well :)

abonander · 2024-01-07T23:45:01Z

It's fixed in main, it just needs rebased.

asteinba · 2024-01-08T00:43:11Z

I opened up a PR to your branch with the rebase on main :)

asteinba · 2024-01-09T08:33:27Z

@abonander FYI: Rebase is merged, checks are green :).

asteinba · 2024-01-21T21:36:05Z

@abonander Can you merge this :)?

asteinba · 2024-02-05T17:26:55Z

Don't want to bother but I would really love to see this merged :) Anything that is missing that I can assist with?

asteinba · 2024-02-19T16:31:31Z

@abonander last try before I stop bothering you: Is there anything missing or anything I can help with to get this merged? I would not like to see the PR dying again as @benluelo already reopened that and the value that this PR adds is huge (at least for me, I ran into this "bug" multiple times).

jelacious · 2024-02-20T12:00:27Z

I am going to second that this is a fairly huge PR in terms of usability and would like to see this merged.

Furthermore, the lack of response for a month for such a big project is concerning.

benluelo · 2024-03-03T19:43:04Z

@abonander any update?

abonander · 2024-03-05T05:33:58Z

Strictly speaking, this appears to be a potential breaking change as discussed by @jplatte in #1031 (comment) because it changes lifetime obligations for field types:

If I understand correctly, the previous bounds would potentially allow, e.g. a type that only implemented Decode<'static, Postgres> to be used as it would bubble up the obligation to the derived impl. But after the change, it would require all fields to implement for<'decode> Decode<'decode, Postgres>, either implicitly (by invoking Decode::decode()) or explicitly (on generics).

Would this realistically affect anybody in practice? It seems unlikely, but unless we're confident that this is not a breaking change because code relying on the previous behavior wouldn't have compiled anyways, then it has to wait for 0.8.0.

abonander

I would also like to see a regression test that can be demonstrated to be fixed by this.

jplatte · 2024-03-05T10:40:09Z

Actually no, I wasn't talking about lifetimes. I was talking about something like this:

#[derive(sqlx::Encode)]
struct Foo<T> {
    field: sqlx::types::Json<T>,
}

which currently works because it generates a sqlx::types::Json<T>: sqlx::Encode bound but will fail with this PR because it generates T: sqlx::Encode when actually T: serde::Serialize is required.

This is an actual regression in functionality, but IMO warranted because otherwise private field types leak into the public trait implementation. It's also how virtually all other derives work. See also serde(bound), which is serde's solution for this that does not implicitly leak private implementation details.

abonander · 2024-03-14T22:14:10Z

That is a much better point than I was trying to make. Adding a #[sqlx(bound)] sounds like a good idea.

abonander · 2024-03-22T02:08:57Z

@benluelo we are now merging breaking changes, so if you could rebase and fix the conflict that'd be great.

benluelo · 2024-03-22T04:05:47Z

squashed and rebased, should be good to go (barring any issues I missed post-rebase)

jplatte · 2024-03-22T13:16:25Z

sqlx-macros-core/src/derives/decode.rs

+        // add db type for impl generics & where clause
+        for type_param in &mut generics.type_params_mut() {
+            type_param.bounds.extend::<[TypeParamBound; 2]>([
+                parse_quote!(for<'decode> ::sqlx::decode::Decode<'decode, ::sqlx::Postgres>),


Why is this now a higher-order bound instead of Decode<'r, _> like before?

honestly, i wrote this so long ago, i don't recall any of the reasoning behind my changes. happy to change this though

If it works, I think 'r without the for<> makes more sense. If not, I'd be curious why.

It may have been to get around an "implementation is not general enough" error. People complain about those from time to time.

This change is what I was thinking about in this comment BTW #2940

Which "this comment"? GitHub doesn't highlight anything for me when clicking that link.

And I'm pretty sure the for<'decode> makes the impl less general and can't possibly fix any errors.

The comment I @-mention'd you in above.

Ahh, so you agree with keeping the 'r if possible, right?

jplatte · 2024-03-22T13:16:54Z

sqlx-macros-core/src/derives/decode.rs

        }

-        let (impl_generics, _, where_clause) = generics.split_for_impl();
+        generics.params.push(parse_quote!('r));


I wonder what the idea behind this is (I know it existed before already).

jplatte · 2024-03-22T13:17:04Z

sqlx-macros-core/src/derives/encode.rs

+        // add db type for impl generics & where clause
+        for type_param in &mut generics.type_params_mut() {
+            type_param.bounds.extend::<[TypeParamBound; 2]>([
+                parse_quote!(for<'encode> ::sqlx::encode::Encode<'encode, ::sqlx::Postgres>),


jplatte · 2024-03-22T13:17:07Z

sqlx-macros-core/src/derives/encode.rs

        }

-        let (impl_generics, _, where_clause) = generics.split_for_impl();
+        generics.params.push(parse_quote!('q));


joelawm · 2024-05-03T03:01:24Z

Is there a update to this as this is a blocking issue for a lot of people that want to use composite types in postgres that have null values. If there is anything I can do to help let me know. @abonander

benluelo mentioned this pull request Dec 18, 2023

fix: Decode and Encode derives (#1031) #2329

Closed

abonander added the breaking label Mar 5, 2024

abonander requested changes Mar 5, 2024

View reviewed changes

benluelo force-pushed the fix-encode-decode-derives branch from 39bb514 to 56d5f08 Compare March 22, 2024 04:02

fix: Decode and Encode derives

e5cfa8e

benluelo force-pushed the fix-encode-decode-derives branch from 56d5f08 to e5cfa8e Compare March 22, 2024 04:04

jplatte reviewed Mar 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Decode and Encode derives (#1031) #2940

fix: Decode and Encode derives (#1031) #2940

benluelo commented Dec 18, 2023

asteinba commented Jan 7, 2024

abonander commented Jan 7, 2024

asteinba commented Jan 8, 2024

asteinba commented Jan 9, 2024

asteinba commented Jan 21, 2024

asteinba commented Feb 5, 2024

asteinba commented Feb 19, 2024

jelacious commented Feb 20, 2024

benluelo commented Mar 3, 2024

abonander commented Mar 5, 2024

abonander left a comment

jplatte commented Mar 5, 2024

abonander commented Mar 14, 2024

abonander commented Mar 22, 2024

benluelo commented Mar 22, 2024

jplatte Mar 22, 2024 •

edited

benluelo Mar 22, 2024

jplatte Mar 22, 2024

abonander Mar 22, 2024

jplatte Mar 22, 2024

abonander Mar 22, 2024

jplatte Mar 23, 2024

jplatte Mar 22, 2024

jplatte Mar 22, 2024

jplatte Mar 22, 2024

joelawm commented May 3, 2024

fix: Decode and Encode derives (#1031) #2940

Are you sure you want to change the base?

fix: Decode and Encode derives (#1031) #2940

Conversation

benluelo commented Dec 18, 2023

asteinba commented Jan 7, 2024

abonander commented Jan 7, 2024

asteinba commented Jan 8, 2024

asteinba commented Jan 9, 2024

asteinba commented Jan 21, 2024

asteinba commented Feb 5, 2024

asteinba commented Feb 19, 2024

jelacious commented Feb 20, 2024

benluelo commented Mar 3, 2024

abonander commented Mar 5, 2024

abonander left a comment

Choose a reason for hiding this comment

jplatte commented Mar 5, 2024

abonander commented Mar 14, 2024

abonander commented Mar 22, 2024

benluelo commented Mar 22, 2024

jplatte Mar 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelawm commented May 3, 2024

jplatte Mar 22, 2024 •

edited