init: allow nonstandard_style for generated accessor/value

[Mirko-A]

Allows nonstandard_style lint on accessors/values generated as local variables in init!.

Since the same warning will be reported by the compiler on the struct field, having the extra warning for the generated accessor/value is unnecessary and confusing.

Reported-by: Gary Guo gary@garyguo.net
Link: #125
Closes: https://lore.kernel.org/rust-for-linux/DGTBJBIVFZ2K.2F1ZEFGY0G7NK@garyguo.net/
Fixes: f1b0c3c ("internal: init: remove #[disable_initialized_field_access]")

[Mirko-A]

Tested with the following changes:

diff --git a/examples/big_struct_in_place.rs b/examples/big_struct_in_place.rs
index 80f89b5..5525cae 100644
--- a/examples/big_struct_in_place.rs
+++ b/examples/big_struct_in_place.rs
@@ -13,8 +13,8 @@ pub struct BigStruct {
     a: u64,
     b: u64,
     c: u64,
-    d: u64,
-    managed_buf: ManagedBuf,
+    NONSTANDARD_D: u64,
+    MANAGED_BUF: ManagedBuf,
 }

 #[derive(Debug)]
@@ -37,8 +37,8 @@ fn main() {
             a: 7,
             b: 186,
             c: 7789,
-            d: 34,
-            managed_buf <- ManagedBuf::new(),
+            NONSTANDARD_D: 34,
+            MANAGED_BUF <- ManagedBuf::new(),
         }))
         .unwrap();
         println!("{}", core::mem::size_of_val(&*buf));

Output before the patch:

$ cargo run --example big_struct_in_place
[...]
error: structure field `NONSTANDARD_D` should have a snake case name
  --> examples/big_struct_in_place.rs:16:5
   |
16 |     NONSTANDARD_D: u64,
   |     ^^^^^^^^^^^^^ help: convert the identifier to snake case: `nonstandard_d`
   |
   = note: `-D non-snake-case` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(non_snake_case)]`

error: structure field `MANAGED_BUF` should have a snake case name
  --> examples/big_struct_in_place.rs:17:5
   |
17 |     MANAGED_BUF: ManagedBuf,
   |     ^^^^^^^^^^^ help: convert the identifier to snake case: `managed_buf`

error: variable `NONSTANDARD_D` should have a snake case name
  --> examples/big_struct_in_place.rs:40:28
   |
40 |             NONSTANDARD_D: 34,
   |                            ^^ help: convert the identifier to snake case: `nonstandard_d`

error: variable `NONSTANDARD_D` should have a snake case name
  --> examples/big_struct_in_place.rs:40:13
   |
40 |             NONSTANDARD_D: 34,
   |             ^^^^^^^^^^^^^ help: convert the identifier to snake case: `nonstandard_d`

error: variable `MANAGED_BUF` should have a snake case name
  --> examples/big_struct_in_place.rs:41:13
   |
41 |             MANAGED_BUF <- ManagedBuf::new(),
   |             ^^^^^^^^^^^ help: convert the identifier to snake case: `managed_buf`

error: could not compile `pin-init` (example "big_struct_in_place") due to 5 previous errors

Output after the patch:

$ cargo run --example big_struct_in_place
[...]
error: structure field `NONSTANDARD_D` should have a snake case name
  --> examples/big_struct_in_place.rs:16:5
   |
16 |     NONSTANDARD_D: u64,
   |     ^^^^^^^^^^^^^ help: convert the identifier to snake case: `nonstandard_d`
   |
   = note: `-D non-snake-case` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(non_snake_case)]`

error: structure field `MANAGED_BUF` should have a snake case name
  --> examples/big_struct_in_place.rs:17:5
   |
17 |     MANAGED_BUF: ManagedBuf,
   |     ^^^^^^^^^^^ help: convert the identifier to snake case: `managed_buf`

error: could not compile `pin-init` (example "big_struct_in_place") due to 2 previous errors

I am still quite new to the process of contributing to Rust-For-Linux. Please let me know if I've made any mistakes or if I've missed something.

[nbdd0121]

Please add a test for this

[Mirko-A]

Does something like this look okay?

[nbdd0121]

I think what you want is not a compile_fail test, but rather than the code (with allow on the struct definition side), compiles to completion without warnings? So this should just be a test in src/tests.

[Mirko-A]

Right, that sounds like a better approach.

I've amended the existing test commit to keep the branch clean, hopefully that's okay: 2d194be

So this should just be a test in src/tests.

I put the test under the existing tests directory in the crate root, I hope that's what you meant. If you specifically wanted a new directory in src, I'm happy to move it.

[Mirko-A]

Two more things:

1. The CI failure seems unrelated, running rustup run 1.78 cargo check on main produces the same error for me.
2. Would you like me to add a changelog entry for this fix?

[nbdd0121]

main works fine for me. Did you not add RUSTC_BOOTSTRAP=1? Please do add a changelog entry.

[Mirko-A]

Sorry for the delay - I just pushed a few more commits. Hopefully that covers everything now.

I kept the new commits separate, so that it is easier to see the diffs. Please let me know if you want them squashed.

[nbdd0121]

Please squash the commits. I think for this you can have three commits:

• one for #[pin_data]
• one for init!/pin_init!
• one for the test

Also fix tag is wrong, it should be db96c51

[nbdd0121]

This sentence doesn't need to be here; this is an integral part of the test.

[nbdd0121]

Suggested change

	- `init!` and `pin_init!` no longer produce `nonstandard_style` group warnings at the
	call site when used for structs with non-snake-case field names. Warnings on the
	struct definition are unaffected.
	- `init!` and `pin_init!` no longer produce `non_snake_case` warnings if field names are
	not of snake case. Warnings on the struct definitions are unaffected.

actually, could you update code to use the specific lint, such as non_snake_case for suppression?

Also, could you squash the changelog update into the same commit that update the code please.

You'd also probably want a line about #[pin_data], to mention that lints can now be properly suppressed where it couldn't previously.

[nbdd0121]

Hmm, actually this can cause lints to be suppressed on #value, which is provided by the user.

Code like

init!(foo: {
    let Foo = 1;
    1
})

will be incorrectly suppressed?

[Mirko-A]

I've squashed the commits as suggested. I didn't keep the fixup commits for the most recent round of reviews (there wasn't a nice way to split them per comment but also per commit they should be folded into), so I'll list the changes here:

This sentence doesn't need to be here; this is an integral part of the test.

This comment has been removed.

actually, could you update code to use the specific lint, such as non_snake_case for suppression?

Done. Only non_snake_case warnings were generated by the macros (in the test cases that exist at the moment) so this was pretty much a search and replace.

Also, could you squash the changelog update into the same commit that update the code please.

Done.

You'd also probably want a line about #[pin_data], to mention that lints can now be properly suppressed where it couldn't previously.

Added a line for #[pin_data] in the changelog.

Hmm, actually this can cause lints to be suppressed on #value, which is provided by the user.

Nice catch. I've split up the evaluation of user code and assignment to a local variable, only suppressing warnings for the latter. I've also modified the test to prove this works as intended now.

[nbdd0121]

I've looked into this and the only reason that we need use #value_ident here is actually to support the short-hand syntax. If we just have an addition match, we can avoid create binding with user-supplied name at all:

diff --git a/internal/src/init.rs b/internal/src/init.rs
index aa944de341c8..9b5cd3ea80b0 100644
--- a/internal/src/init.rs
+++ b/internal/src/init.rs
@@ -240,20 +240,22 @@ fn init_fields(
         };
         let init = match kind {
             InitializerKind::Value { ident, value } => {
-                let mut value_ident = ident.clone();
-                let value_prep = value.as_ref().map(|value| &value.1).map(|value| {
-                    // Setting the span of `value_ident` to `value`'s span improves error messages
-                    // when the type of `value` is wrong.
-                    value_ident.set_span(value.span());
-                    quote! {
-                        // Allow `non_snake_case` since the same warning is going to be reported for
-                        // the struct field. Evaluate `#value` first so that user code warnings are not
-                        // suppressed.
-                        let __value_tmp = #value;
-                        #[allow(non_snake_case)]
-                        let #value_ident = __value_tmp;
+                let (value_ident, value_prep) = match value.as_ref() {
+                    None => {
+                        // This is of short-hand syntax, so just use the identifier directly.
+                        (ident.clone(), None)
+                    }
+                    Some((_, value)) => {
+                        // We have an expression. Evaluate it to a local variable first, outside
+                        // `unsafe {}` block.
+                        //
+                        // Setting the span of `value_ident` to `value`'s span improves error messages
+                        // when the type of `value` is wrong.
+                        (format_ident!("value", span = value.span()), Some(quote! {
+                            let value = #value;
+                        }))
                     }
-                });
+                };
                 // Again span for better diagnostics
                 let write = quote_spanned!(ident.span()=> ::core::ptr::write);
                 // NOTE: the field accessor ensures that the initialized field is properly aligned.

(Given this is significant chunk of code, if you take this to your commit makes sure to add Co-developed-by tag according to https://docs.kernel.org/process/submitting-patches.html)

[Mirko-A]

I see, looks like a reasonable change. I've amended the corresponding commit as per your suggestion.