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.

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

Future incompatibility with Rust RFC 3373: Avoid non-local definitions in functions #60

Closed
Urgau opened this issue Feb 4, 2024 · 3 comments · Fixed by #62
Closed

Future incompatibility with Rust RFC 3373: Avoid non-local definitions in functions #60

Urgau opened this issue Feb 4, 2024 · 3 comments · Fixed by #62
Assignees

Comments

@Urgau
Copy link

Urgau commented Feb 4, 2024

Rust RFC 3373: Avoid non-local definitions in functions was accepted and it's implementation at rust-lang/rust#120393 found that this crate would be affected by it.

To be more precise users of this crate would be affected by it, in the form of a warn-by-default lint. This is because the derive macros from this crate use impl in a local context, const _IMPL_NUM_???_FOR_???:

num-derive/src/lib.rs

Lines 102 to 109 in 50ecdb1

fn dummy_const_trick(trait_: &str, name: &Ident, exp: TokenStream2) -> TokenStream2 {
let dummy_const = Ident::new(
&format!("_IMPL_NUM_{}_FOR_{}", trait_, unraw(name)),
Span::call_site(),
);
quote! {
#[allow(non_upper_case_globals, unused_qualifications)]
const #dummy_const: () = {

Fortunately a simple fix exist for this crate, by using a const-anon instead of named one:

 fn dummy_const_trick(trait_: &str, name: &Ident, exp: TokenStream2) -> TokenStream2 {
-    let dummy_const = Ident::new(
-        &format!("_IMPL_NUM_{}_FOR_{}", trait_, unraw(name)),
-        Span::call_site(),
-    );
    quote! {
        #[allow(non_upper_case_globals, unused_qualifications)]
-        const #dummy_const: () = {
+        const _: () = {

I would suggest applying some form of the patch above as well as releasing a patch version of this crate, as to have a fix available for users as soon as possible.

cc @cuviper

@cuviper
Copy link
Member

cuviper commented Feb 5, 2024

Ok, I'll get to this soon - thanks for the heads up!

@cuviper
Copy link
Member

cuviper commented Feb 6, 2024

num-derive v0.4.2 should be fixed for this.

@Urgau
Copy link
Author

Urgau commented Feb 6, 2024

Great, thks for fixing it @cuviper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants