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.

Sign up for GitHub

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

Allow re-exporting diesel in other libs #2579

Closed
wants to merge 1 commit into from
Closed

Allow re-exporting diesel in other libs #2579

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions diesel/src/macros/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,15 @@ macro_rules! __diesel_column {
) => {
$($meta)*
#[allow(non_camel_case_types, dead_code)]
#[derive(Debug, Clone, Copy, $crate::query_builder::QueryId, Default)]
#[derive(Debug, Clone, Copy, Default)]
pub struct $column_name;

#[allow(non_camel_case_types)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate why a manual impl here is required? For me that seems just like a stylistic change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained, this is needed for other libs to allow re-exporting of diesel.

Let's assume a crate lib called utils which re exports diesel like following:

pub use diesel;

When these macros are used currently like:

utils::diesel::table! {
    articles (id) {
        id -> Integer,
    }
}

the following code gets generated:

impl ::diesel::query_builder::QueryId for $column_name {
    type QueryId = $column_name;
    const HAS_STATIC_QUERY_ID: bool = true;
}

which complains about diesel not being found in dependencies (since we are using utils as dependency)

 --> src/main.rs:1:1
  |
1 | / b::diesel::table! {
2 | |     articles (id) {
3 | |         id -> Integer,
4 | |     }
5 | | }
  | |_^ could not find `diesel` in `{{root}}`
  |
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

This PR changes the generated code to be:

impl ::utils::diesel::query_builder::QueryId for $column_name {
    type QueryId = $column_name;
    const HAS_STATIC_QUERY_ID: bool = true;
}

which ends up letting us compile the program successfully.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Thanks for the explanation. Another problem: This change will only fix the table! macro. Every derive/proc_macro will be broken, as all of them assume that diesel is some direct dependency. I would rather like to fix that there, instead of having a special fix for table! here.

Copy link
Member Author

@pksunkara pksunkara Nov 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change fixes the non proc-macros and not the derives because anything which is exporting diesel will probably not rely on the derives.

If people want to use derives, then I would say that they need to explicitly add diesel as dependency. At least this was the consensus I understood talking to people on https://rust-lang.zulipchat.com

Copy link
Member Author

@pksunkara pksunkara Nov 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fixing this issue on proc-macros is a much more bigger effort and does not look good ergonomic wise which is also a reason any libs that export diesel will not prefer it.

The way to fix it (from looking at how inventory crate solved it) would be to add a crate attr.

#[derive(Identifiable)]
#[crate = ::utils::diesel]
pub struct User {
  pub id: i32,
}

But as I said, it becomes cumbersome for the end user which is why libs do not prefer it (and there's nothing the libs can do to fix it).

I think we should table the proc-macro correction part until someone needs it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of the derives in diesel_derive already do this as we use them inside of diesel and they also work in third party crates.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the wrap_in_dummy_mod is the wrong approach and ends up giving issues for re-exporting. The correct approach seems to be rust-lang/rust#56409. I am trying to get it working now in diesel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap_in_dummy_mod is there to prevent poluting the outer namespace with identifiers imported internally by the derives. I don't think that's something that need to be changed. The imports done inside of wrap_in_dummy_mod are likely the problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After spending a lot of time debugging, experimenting etc.. this is my final solution.

All non proc_macros currently work correctly with re-exporting

All proc_macros also work correctly with re-exporting but the end user needs to add this single line to their crate

use utils::diesel;

The issue seems to be that using a proc_macro in a non proc_macro leads to the end user needing that line again. So, I think this PR is still valid to reduce the instances where the end user needs that line. And re-exportability is not going to break if we change any of these macros to proc_macros later in the future. This is now just a convenience PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And re-exportability is not going to break if we change any of these macros to proc_macros later in the future.

I don't want to be pedantic here, but how wouldn't re-exportability not change if we change the table! macro to a proc macro in a later version. Given your change it's possible to reexport table! requirements for down stream users on how to use that macro. If we would change now table! to a proc macro that would be a breaking change because down stream code using a reexport would stop to compile as users now would need to add this use utils::diesel; line.

That written I can totally understand that this feature is desirable but I really do not see any good way to implement that without having language support for this.

impl $crate::query_builder::QueryId for $column_name {
type QueryId = $column_name;
const HAS_STATIC_QUERY_ID: bool = true;
}

impl $crate::expression::Expression for $column_name {
type SqlType = $($Type)*;
}
Expand Down Expand Up @@ -653,13 +659,19 @@ macro_rules! __diesel_table_impl {
pub const all_columns: ($($column_name,)+) = ($($column_name,)+);

#[allow(non_camel_case_types)]
#[derive(Debug, Clone, Copy, $crate::query_builder::QueryId)]
#[derive(Debug, Clone, Copy)]
/// The actual table struct
///
/// This is the type which provides the base methods of the query
/// builder, such as `.select` and `.filter`.
pub struct table;

#[allow(non_camel_case_types)]
impl $crate::query_builder::QueryId for table {
type QueryId = table;
const HAS_STATIC_QUERY_ID: bool = true;
}

impl table {
#[allow(dead_code)]
/// Represents `table_name.*`, which is sometimes necessary
Expand Down Expand Up @@ -803,13 +815,19 @@ macro_rules! __diesel_table_impl {
$($imports)*

#[allow(non_camel_case_types, dead_code)]
#[derive(Debug, Clone, Copy, $crate::query_builder::QueryId)]
#[derive(Debug, Clone, Copy)]
/// Represents `table_name.*`, which is sometimes needed for
/// efficient count queries. It cannot be used in place of
/// `all_columns`, and has a `SqlType` of `()` to prevent it
/// being used that way
pub struct star;

#[allow(non_camel_case_types)]
impl $crate::query_builder::QueryId for star {
type QueryId = star;
const HAS_STATIC_QUERY_ID: bool = true;
}

impl<__GB> $crate::expression::ValidGrouping<__GB> for star
where
($($column_name,)+): $crate::expression::ValidGrouping<__GB>,
Expand Down