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 #46

Closed
Urgau opened this issue Feb 4, 2024 · 7 comments · Fixed by #47
Closed

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

Urgau opened this issue Feb 4, 2024 · 7 comments · Fixed by #47

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: non_local_definitions. This is because the derive macros from this crate use impl in a local context, const _DERIVE_Display_FOR_???:

displaydoc/src/expand.rs

Lines 22 to 25 in f0b62a5

let dummy_const = format_ident!("_DERIVE_Display_FOR_{}", input.ident);
Ok(quote! {
#[allow(non_upper_case_globals, unused_attributes, unused_qualifications)]
const #dummy_const: () = {

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

-    let dummy_const = format_ident!("_DERIVE_Display_FOR_{}", input.ident);
     Ok(quote! {
         #[allow(non_upper_case_globals, unused_attributes, 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 @yaahc

@rnbguy
Copy link

rnbguy commented Feb 26, 2024

The RFC impl is merged in the latest nightly. displaydoc dependent nighly crates are getting the warning now.

@Manishearth
Copy link
Collaborator

Does that patch actually work? My understanding is that displaydoc does this scoping so that it can have some local types and impls

@Urgau
Copy link
Author

Urgau commented Jun 18, 2024

Yes, it should work. We do (in rustc) special case const-anon to not lint, given the widespread usage of that pattern.

@Manishearth
Copy link
Collaborator

But it seems like that just hoists all the definitions to the outer scope? So they'll clash with each other?

@Manishearth
Copy link
Collaborator

This should work: #49

@kpreid
Copy link

kpreid commented Jun 18, 2024

But it seems like that just hoists all the definitions to the outer scope? So they'll clash with each other?

An anonymous const item with a block expression still introduces a new scope just like a named const. (In fact, it's the block expression that's doing all the work; the const _: () = is just a way to put a const-evaluated expression in a context that accepts items.)

@Manishearth
Copy link
Collaborator

@kpreid hmm, in that case the error message for this future incompat lint seems incorrect?

rust-lang/rust#120363 (comment)

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.

4 participants