-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Allow re-exporting diesel in other libs #2579
Conversation
Shout out to @flodiebold for helping me figure this issue out yesterday! |
pub struct $column_name; | ||
|
||
#[allow(non_camel_case_types)] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This change while verbose will allow diesel to be re-exported in other libs to conform with the Rust API guidelines.