Make JoinWithImplicitOnClause and joins::Inner externally accessible as a sealed trait

[LucaCappelletti94]

Backend

PostgreSQL, MySQL, SQLite, Others

What do you want to add to diesel?

At this time, JoinWithImplicitOnClause is a non exported public trait and joins::Inner is private. This makes it impossible to constraint that a generic T has the inner_join method, as it currently requires T: JoinWithImplicitOnClause<Rhs, joins::Inner>.

I would suggest to:

• make JoinWithImplicitOnClause a sealed externally accessible trait
• export joins::Inner

Implementation notes

No response

Additional details

No response

Checklist

• I have already looked over the issue tracker and the discussion forum for similar possible closed issues.

[weiznich]

I would like to see an specific example of code that cannot expressed without these bounds. As written before in the relevant PR I would not want to expose this due to #3223. We might need to change these internals to address that issue fully.

Now I'm open to discuss this if someone can demonstrate that solving #3223 will not require any changes to the trait/type structure for joins.

[LucaCappelletti94]

I have prepared a minimal documented example here which I report below:

At this time, to my knowledge, it is not possible to write a generic call to the inner_join method as it requires both the JoinWithImplicitOnClause and Inner, both of which are not re-exported in the main Diesel crate.

/// Version of `inner_join` using the main Diesel crate
pub fn inner_join_generic<Lhs, Rhs>(
    left: Lhs,
    right: Rhs,
)
where
    Lhs: diesel::QueryDsl
        // + diesel::query_dsl::JoinWithImplicitOnClause<Rhs, diesel::query_source::joins::Inner>,
{
    let _query = left.inner_join(right);
}

The code above fails with:

error[E0275]: overflow evaluating the requirement `_: Sized`
  --> src/lib.rs:12:23
   |
12 |     let _query = left.inner_join(right);
   |                       ^^^^^^^^^^
   |
   = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`diesel_inner_join`)
   = note: required for `Lhs` to implement `diesel::query_dsl::InternalJoinDsl<_, query_source::joins::Inner, _>`

For more information about this error, try `rustc --explain E0275`.
error: could not compile `diesel-inner-join` (lib) due to 1 previous error

Of course, if we were to uncomment the commented bound, as such:

/// Version of `inner_join` using the main Diesel crate
pub fn inner_join_generic<Lhs, Rhs>(
    left: Lhs,
    right: Rhs,
)
where
    Lhs: diesel::QueryDsl + diesel::query_dsl::JoinWithImplicitOnClause<Rhs, diesel::query_source::joins::Inner>,
{
    let _query = left.inner_join(right);
}

We would get:

error[E0603]: module `joins` is private
  --> src/lib.rs:10:82
   |
10 |         + diesel::query_dsl::JoinWithImplicitOnClause<Rhs, diesel::query_source::joins::Inner>,
   |                                                                                  ^^^^^  ----- struct `Inner` is not publicly re-exported
   |                                                                                  |
   |                                                                                  private module
   |
note: the module `joins` is defined here
  --> /home/lucac/.cargo/git/checkouts/diesel-a2d4519740bc8905/676192a/diesel/src/query_source/mod.rs:8:1
   |
 8 | pub(crate) mod joins;
   | ^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0603`.
error: could not compile `diesel-inner-join` (lib) due to 1 previous error

In the diesel fork at reexport-inner-join, renamed to diesel_custom in the Cargo.toml, those two items are re-exported, so we can write:

/// Version of `inner_join` using a custom re-exported Diesel crate
pub fn inner_join_generic_custom<Lhs, Rhs>(
    left: Lhs,
    right: Rhs,
) -> diesel_custom::helper_types::InnerJoin<Lhs, Rhs>
where
    Lhs: diesel_custom::QueryDsl
        + diesel_custom::query_dsl::JoinWithImplicitOnClause<Rhs, diesel_custom::query_source::Inner>,
{
    left.inner_join(right)
}

which compiles just fine.

[weiznich]

Yes it is correct that you cannot express that with the current public API. That's unfortunate, but I still have the reservations explained above about exposing that as it looses a lot of flexibility on what future changes are allowed to do. Given the linked issue there are at least very specific needs for such a change. Now it might be possible to expose something that allows you to write your generic code and also retains the ability to do such internal changes later. Suggesting something specific requires design work that I currently neither have the capacity for not that would have priority from my side. I'm still open to suggestions, but any proposed solution must allow us to change the internal type structure of joins later in a backward compatible way and it must work without adding even more complexity to this already very complex part of diesel. The other possible way to address my raised concerns are to find a way to solve the linked issue, although I'm not sure to what extent that is even possible without changes to the compiler.

[LucaCappelletti94]

Does the option described below in #4942 (comment) count as "adding even more complexity"?

Otherwise, I believe it might allow to decouple the necessity to express the bound with blocking the JoinWithImplicitOnClause trait API.

[weiznich]

As written before: I do currently not have the capacity to check for each of your proposals if it would be a future compatibility hazard. As hard as it might sound, but there are currently more important things to handle for me at project level than reviewing proposals to expose more internals. This includes fixing regressions like #4948 and bugs like #4941. You are either welcome to help fixing these issues or to perform some level of analysis yourself whether a certain proposal would fit the requirements I stated above. Throwing yet another proposal up to see what's sticking is a really tiring way to work for others.

[LucaCappelletti94]

Regarding PR #4952 for #4941: I’m happy to help if there is any work where another contributor would be useful, and I fully understand you may not be able to review PRs quickly.

As I mentioned before, when I make a change in one of my projects that I believe is better suited or required by Rust’s orphan rules for Diesel, I prefer submitting a PR directly.

I also realize the constraints you mentioned are not strictly testable, so it can be difficult for me to verify whether a change could pose an API limitation. Feel free to tag me in any PRs where you think additional help would be useful. My goal is always to stay aligned with Diesel’s design and long-term priorities.

[LucaCappelletti94]

Alternatively to re-exporting, it could be possible to define a InnerJoinDsl trait defined somewhat as:

pub trait InnerJoinDsl<Rhs> {
    fn inner_join(self, right: Rhs) -> diesel::helper_types::InnerJoin<Lhs, Rhs>;
}

impl<Lhs, Rhs> InnerJoinDsl<Rhs> for Lhs
where
    Lhs: diesel::QueryDsl
        + diesel::query_dsl::JoinWithImplicitOnClause<Rhs, diesel::query_source::inner::Inner>,

{
    fn inner_join(self, right: Rhs) -> diesel::helper_types::InnerJoin<Lhs, Rhs> {
         <Lhs as diesel::QueryDsl>::inner_join(self, right)
    }
}

Positioned somewhere where JoinWithImplicitOnClause and Inner are scoped.

This option should not constrain any change to JoinWithImplicitOnClause, I believe.