Drop GAT workarounds #3395

Elrendio · 2022-11-06T19:26:12Z

What it does:

Migrates GatWorkarounds to actual GATs

Good to know:

Trait ConnectionSealed is kept to keep the feature gate around the Connection implementation.
I had to keep the associated type on RowFieldLifetimeHelper in order to express FieldRet without the lifetime of Row
This increases the minimal rust version from 1.60.0 to 1.65.0

Elrendio · 2022-11-06T19:39:22Z

Tests seems to fail because of GAT not stable 🤔

Ten0 · 2022-11-06T19:44:36Z

Tests seems to fail because of GAT not stable

diesel/.github/workflows/ci.yml

Line 298 in f560c29

toolchain: nightly-2022-08-12

Ten0 · 2022-11-06T19:46:58Z

diesel/src/row.rs

        /// Field type returned by a `Row` implementation
        ///
        /// * Crates using existing backend should not concern themself with the
        ///   concrete type of this associated type.
        ///
        /// * Crates implementing custom backends should provide their own type
        ///   meeting the required trait bounds
-        type Field: Field<'a, DB>;
+        type Field<'a>: Field<'a, DB>;


why doesn't this move on the Row trait?

I had to keep the associated type on RowSealed in order to express FieldRet without the lifetime of Row

explained in the commit message and additional doc

Can you check if pub type FieldRet<'a, R, DB> = <R as Row<'a, DB>>::Field<'a>; works for all provided cases? If that's the case I would rather move the Field GAT to Row instead of having another trait for this.

diesel/src/row.rs

diesel/src/pg/connection/mod.rs

Ten0 · 2022-11-06T22:06:47Z

Likely needs some msrv upgrade, compile tests output regeneration and some clippy stuff

Elrendio · 2022-11-06T22:17:20Z

Clippy warnings seems to be mostly unrelevant. For the elided lifetime, it is required and I really don't think '_ is clearer than 'conn

Elrendio · 2022-11-07T11:30:24Z

The compile error comes from a warning that's also generated and seems to be a bug from Rust, as it complains for the experimental feature when it's actually feature-flagged out.

weiznich

Thanks for opening this PR 👍
I've left a number of comments about which parts should be changed so that it is clearer for potential users what the the corresponding types look like.

For the clippy/ci stuff: I've opened #3396 to address that.

weiznich · 2022-11-07T13:21:09Z

.github/workflows/ci.yml

@@ -295,7 +295,7 @@ jobs:
      - uses: actions/checkout@v3
      - uses: dtolnay/rust-toolchain@master
        with:
-          toolchain: nightly-2022-08-12
+          toolchain: nightly-2022-11-05


This likely requires rerunning the compile tests with TRYBUILD=overwrite + committing all the changes.

weiznich · 2022-11-07T13:21:28Z

.github/workflows/ci.yml

@@ -423,11 +423,11 @@ jobs:
        run: cargo +nightly -Z build-std test --manifest-path diesel/Cargo.toml --no-default-features --features "sqlite extras libsqlite3-sys libsqlite3-sys/bundled libsqlite3-sys/with-asan" --target x86_64-unknown-linux-gnu

  minimal_rust_version:
-    name: Check Minimal supported rust version (1.60.0)
+    name: Check Minimal supported rust version (1.65.0)


This requires an change log entry.

diesel/src/connection/mod.rs

weiznich · 2022-11-07T13:26:35Z

diesel/src/connection/mod.rs

+    /// The cursor type returned by [`LoadConnection::load`]
+    ///
+    /// Users should handle this as opaque type that implements [`Iterator`]
+    type Cursor<'conn, 'query>: Iterator<Item = QueryResult<Self::Row<'conn, 'query>>>;


The LoadConnection::load method should be changed so that it returns QueryResult<Self::Cursor<'conn, 'query> now. I think that's much easier to understand for potential users + it's the same type as before.

weiznich · 2022-11-07T13:27:32Z

diesel/src/connection/mod.rs

@@ -40,7 +40,17 @@ pub trait SimpleConnection {
 ///
 /// Users should threat this type as `impl Iterator<Item = QueryResult<impl Row<DB>>>`
 pub type LoadRowIter<'conn, 'query, C, DB, B = DefaultLoadingMode> =


This type can be deprecated. (It should be behind the with-deprecated feature flag + removed for the without-deprecated flag)

weiznich · 2022-11-07T13:32:43Z

diesel/src/row.rs

@@ -29,15 +29,15 @@ pub trait RowIndex<I> {
 /// Return type of [`Row::get`]
 ///
 /// Users should threat this as opaque [`impl Field<DB>`](Field) type.
-pub type FieldRet<'a, R, DB> = <R as RowGatWorkaround<'a, DB>>::Field;
+pub type FieldRet<'a, R, DB> = <R as RowFieldLifetimeHelper<DB>>::Field<'a>;


This type should be deprecated as well

weiznich · 2022-11-07T13:37:34Z

diesel/src/row.rs

        /// Field type returned by a `Row` implementation
        ///
        /// * Crates using existing backend should not concern themself with the
        ///   concrete type of this associated type.
        ///
        /// * Crates implementing custom backends should provide their own type
        ///   meeting the required trait bounds
-        type Field: Field<'a, DB>;
+        type Field<'a>: Field<'a, DB>;


Can you check if pub type FieldRet<'a, R, DB> = <R as Row<'a, DB>>::Field<'a>; works for all provided cases? If that's the case I would rather move the Field GAT to Row instead of having another trait for this.

diesel/src/connection/mod.rs

Ten0 · 2022-11-09T19:06:18Z

It looks like HasBindCollector may be a gat workaround too

weiznich · 2022-11-09T19:43:12Z

Yes HasBindCollector and also HasRawValue are GAT workarounds. Both can be added directly to Backend.

weiznich · 2022-11-25T18:14:22Z

@Elrendio Do you plan to follow up onto the comments above?

weiznich · 2023-01-05T08:02:05Z

This is currently blocked on rust-lang/rust#106417 and on a trybuild release containing dtolnay/trybuild#213

diesel/src/connection/mod.rs

diesel/src/query_dsl/load_dsl.rs

diesel/src/row.rs

Elrendio marked this pull request as ready for review November 6, 2022 19:38

Ten0 reviewed Nov 6, 2022

View reviewed changes

diesel/src/pg/connection/mod.rs Outdated Show resolved Hide resolved

Ten0 changed the title ~~Drop gat workarounds~~ Drop GAT workarounds Nov 6, 2022

weiznich requested changes Nov 7, 2022

View reviewed changes

Ten0 reviewed Nov 9, 2022

View reviewed changes

diesel/src/connection/mod.rs Outdated Show resolved Hide resolved

Elrendio and others added 16 commits December 27, 2022 20:29

drops

1941723

remove .vscode

e2b32e6

renaming + explanation

6ce7874

bump ci toolchain

b9bc660

reduce ConnectionSealed

45cae5f

doc

0c327f3

fix nightly link

5db34fd

few clippy hints

8024c27

increase rust minimal version

434fcc0

Explicitly deprecate LoadRowIter

09759dc

Explicitly deprecate FieldRet

4478f1e

Explicitly deprecate RowIter

26da914

Port MultiConnection to the GAT based connection implementation

98f03a3

Update compile tests

062b236

Replace HasRawValue trait with a GAT at the Backend trait

f0f7a92

Replace the HasBindCollector trait with a GAT on Backend

464ef34

weiznich force-pushed the drop_gat_workarounds branch from ad8c0b6 to 464ef34 Compare December 30, 2022 17:13

weiznich added 5 commits January 2, 2023 09:55

Minor test fixes

58df7d5

More fixes

b3db6f9

More fixes

22d155e

More documentation fixes

b5568a5

Compile test changes

8a9c304

weiznich force-pushed the drop_gat_workarounds branch from 7fe9dd4 to 8a9c304 Compare January 2, 2023 14:09

weiznich force-pushed the drop_gat_workarounds branch from 146b3b7 to 16433b6 Compare January 10, 2023 09:41

Bump trybuild and rustversion again to fix compile tests

6921fe2

weiznich force-pushed the drop_gat_workarounds branch from 16433b6 to 6921fe2 Compare January 10, 2023 10:19

weiznich reviewed Jan 11, 2023

View reviewed changes

diesel/src/connection/mod.rs Outdated Show resolved Hide resolved

diesel/src/query_dsl/load_dsl.rs Show resolved Hide resolved

diesel/src/row.rs Outdated Show resolved Hide resolved

diesel/src/row.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

cab0bdc

weiznich force-pushed the drop_gat_workarounds branch from 076ea8a to cab0bdc Compare January 11, 2023 10:15

weiznich merged commit b48925f into diesel-rs:master Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop GAT workarounds #3395

Drop GAT workarounds #3395

Elrendio commented Nov 6, 2022 •

edited

Elrendio commented Nov 6, 2022

Ten0 commented Nov 6, 2022 •

edited

Ten0 Nov 6, 2022

Elrendio Nov 6, 2022

Ten0 Nov 6, 2022

weiznich Nov 7, 2022

Ten0 commented Nov 6, 2022

Elrendio commented Nov 6, 2022

Elrendio commented Nov 7, 2022 •

edited by Ten0

weiznich left a comment

weiznich Nov 7, 2022

weiznich Nov 7, 2022

weiznich Nov 7, 2022

weiznich Nov 7, 2022

weiznich Nov 7, 2022

weiznich Nov 7, 2022

Ten0 commented Nov 9, 2022

weiznich commented Nov 9, 2022

weiznich commented Nov 25, 2022

weiznich commented Jan 5, 2023

Drop GAT workarounds #3395

Drop GAT workarounds #3395

Conversation

Elrendio commented Nov 6, 2022 • edited

Elrendio commented Nov 6, 2022

Ten0 commented Nov 6, 2022 • 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

Ten0 commented Nov 6, 2022

Elrendio commented Nov 6, 2022

Elrendio commented Nov 7, 2022 • edited by Ten0

weiznich left a 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

Ten0 commented Nov 9, 2022

weiznich commented Nov 9, 2022

weiznich commented Nov 25, 2022

weiznich commented Jan 5, 2023

Elrendio commented Nov 6, 2022 •

edited

Ten0 commented Nov 6, 2022 •

edited

Elrendio commented Nov 7, 2022 •

edited by Ten0