-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Implement #[derive(From)]
#144922
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
base: master
Are you sure you want to change the base?
Implement #[derive(From)]
#144922
Conversation
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
@@ -0,0 +1,6 @@ | |||
//@ edition: 2021 |
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.
(question about all tests)
Is there a particular reason to have only 2021 edition tests?
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 mentioned this in the PR description (This has worked well, although it only works in edition 2021+. Not sure if I botched the prelude somehow and it should live elsewhere (?).), I probably did something bad in the prelude, because the From
macro can only be resolved in edition 2021+. Probably there's a way to make it work on 2015+ (?).
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.
Hm, that's interesting, I'll try it locally, maybe I'll get lucky
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.
Ok, I now fully understand the problem and see how tricky it actual is
Like, we want to re-export From
here
// Re-exported built-in macros
#[stable(feature = "builtin_macro_prelude", since = "1.38.0")]
#[doc(no_inline)]
pub use core::prelude::v1::{
assert, cfg, column, compile_error, concat, env, file, format_args,
format_args_nl, include, include_bytes, include_str, line, log_syntax, module_path, option_env,
stringify, trace_macros, Clone, Copy, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd,
};
but it conflict with convert::From
I would propose limiting this feature to new editions until we are able to resolve this name resolution issue. I believe this is a good temporary solution, and personally, I wouldn't consider this problem a blocker
Also I'd love to continue work on stabilizing it and pushing forward
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.
But the thing I still not figured out is that how Clone
does resolve it? I believe that both trait and derive macro of Clone
is in prelude right?
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.
The problem is not with it being in prelude, but with the fact that the From trait is stable, but the From macro is unstable.
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 tried to make the derive macro stable, but it didn’t give me any positive results (it might require more work than just switch unstable to stable 😅). The only thing that worked for me was adding From as DeriveFrom
to the standard prelude re-export - but, of course, that's a renaming, which we don’t want, since the macro’s name should stay consistent with the trait
Also, based on what I’ve learned and seen in today’s research, I’d say the implementation looks good. I still don’t have the ability to r+, so all I can for now is just say Looks Good To Me!
Implements the
#[derive(From)]
feature (tracking issue, RFC).It allows deriving the
From
impl on structs and tuple structs with exactly one field. Some implementation notes:span
everywhere. I don't know if it's the Right Thing To Do. In particular the errors when#[derive(From)]
is used on a struct with an unsized field are weirdly duplicated.macro From
tocore::convert
, previously working code likeuse std::convert::From
would suddenly require an unstable feature gate, because rustc would think that you're trying to import the unstable macro. @petrochenkov suggested that I add the macro the the core prelude instead. This has worked well, although it only works in edition 2021+. Not sure if I botched the prelude somehow and it should live elsewhere (?).Ty::AstTy
, because thefrom
function receives an argument with the type of the single field, and the existing variants of theTy
enum couldn't represent an arbitrary type.