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

[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]

Actually, could you please update the commit message to reflect latest changes please.

Also, the current summary lines are too long (they'll be prepended with rust: pin-init: when patches are synced to kernel).

Something like internal: suppress ... for `#[pin_data]` would work better. Thanks!

[nbdd0121]

Turns out this has to use #value_ident which has the correct span, otherwise the test will fail with the following diff

diff --git a/tests/nonstandard_style.rs b/tests/nonstandard_style.rs                                                                                               
index 33c62db81997..5020961dfde2 100644                                                                                                                            
--- a/tests/nonstandard_style.rs
+++ b/tests/nonstandard_style.rs
@@ -21,6 +21,14 @@ struct Bar {
     Non_Standard_C: usize,
 }

+macro_rules! wrap_init {
+    ($($args:tt)*) => {
+        ::pin_init::init!(
+            $($args)*
+        )
+    }
+}
+
 impl Foo {
     fn new() -> impl Init<Self> {
         init!(Self {
@@ -31,7 +39,7 @@ impl Foo {
                 )]
                 (0..2).map(|NonStandardInUserCode| NonStandardInUserCode + 1).sum()
             },
-            nonStandardB <- init!(Bar { Non_Standard_C: 42 }),
+            nonStandardB <- wrap_init!(Bar {Non_Standard_C: 1}),
         })
     }
 }

(Just wrapping the macro).

I'm kinda surprised that non of our existing test caught this, but when trying to build kernel with this diff there're lots of fallouts.

[Mirko-A]

It didn't occur to me to also try compiling the kernel after every change. Thank you for finding this.

This should fix the issue (will squash after your review): 5b5bb7a

I'm kinda surprised that non of our existing test caught this

I'd be happy to add such a test (calling pin-init macros from within another macro), if you'd like. Perhaps as the next pull request?

[Mirko-A]

After #133, CI wouldn't pass here because it requires that all commits pass. So I've squashed the fixup commit. I hope you had the chance to look at the change.

[nbdd0121]

Thanks. I think this series is basically ready to merge. There're some high priority fixes that need to go in first, so I am holding off merging this one for now. But I'll merge this when the tree reopens.