Re: [PATCH] rust: pin-init: add references to previously initialized fields

["Benno Lossin" <lossin@kernel.org>]

On Sun Sep 7, 2025 at 7:29 PM CEST, Danilo Krummrich wrote:
> On Sun Sep 7, 2025 at 10:41 AM CEST, Benno Lossin wrote:
>> On Sun Sep 7, 2025 at 4:07 AM CEST, Boqun Feng wrote:
>>> On Sat, Sep 06, 2025 at 06:57:04PM -0700, Boqun Feng wrote:
>>>> On Sat, Sep 06, 2025 at 12:52:22PM +0200, Danilo Krummrich wrote:
>>>> > On Fri Sep 5, 2025 at 7:44 PM CEST, Boqun Feng wrote:
>>>> > > On Fri, Sep 05, 2025 at 07:18:25PM +0200, Benno Lossin wrote:
>>>> > > [...]
>>>> > >> index 606946ff4d7f..1ac0b06fa3b3 100644
>>>> > >> --- a/samples/rust/rust_driver_pci.rs
>>>> > >> +++ b/samples/rust/rust_driver_pci.rs
>>>> > >> @@ -78,8 +78,8 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
>>>> > >>  
>>>> > >>          let drvdata = KBox::pin_init(
>>>> > >>              try_pin_init!(Self {
>>>> > >> -                pdev: pdev.into(),
>>>> > >>                  bar <- pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci")),
>>>> > >> +                pdev: pdev.into(),
>>>> > >
>>>> > > Ok, this example is good enough for me to express the concern here: the
>>>> > > variable shadowing behavior seems not straightforward (maybe because in
>>>> > > normal Rust initalization expression, no binding is created for
>>>> > > previous variables, neither do we have a `let` here).
>>>> > >
>>>> > > Would the future inplace initialization have the similar behavior? I
>>>> > > asked because a natural resolution is adding a special syntax like:
>>>> > >
>>>> > >     let a = ..;
>>>> > >
>>>> > >     try_pin_init!(Self {
>>>> > >         b: a,
>>>> > > 	let a = a.into(); // create the new binding here.
>>>> > > 	c: a, // <- use the previous initalized `a`.
>>>> > >     }
>>>> > 
>>>> > Can you please clarify the example? I'm a bit confused that this is not a field
>>>> > of Self, so currently this can just be written as:
>>>> > 
>>>> 
>>>> Oh, I could have been more clear: `a` is a field of `Self`, and the
>>>> `let` part initalizes it.
>>>> 
>>>> > 	try_pin_init!(Self {
>>>> > 	   b: a,
>>>> > 	   c: a.into,
>>>> > 	})
>>>> > 
>>>> > Of course assuming that a is Clone, as the code above does as well.
>>>> > 
>>>> > So, if we are concerned by the variable shadowing, which I'm less concerned
>>>> > about, maybe we can do this:
>>>> 
>>>> I'm not that concerned to block this, but it does look to me like we are
>>>> inventing a new way (and even a different syntax because normal Rust
>>>> initialization doesn't create new bindings) to create binding, so I
>>>> think I should bring it up.
>>>> 
>>>> > 
>>>> > 	// The "original" `a` and `b`.
>>>> > 	let a: A = ...;
>>>> > 	let b: B = ...;
>>>> > 
>>>> > 	try_pin_init!(Self {
>>>> > 	   a,                   // Initialize the field only.
>>>> > 	   let b <- b,          // Initialize the field and create a `&B` named `b`.
>>>> > 	   c: a.into(),         // That's the "original" `a`.
>>>> > 	   d <- D::new(b),      // Not the original `b`, but the pin-init one.
>>>> > 	})
>>>
>>> Another idea is using `&this`:
>>>
>>>  	try_pin_init!(&this in Self {
>>>  	   a,                   // Initialize the field only.
>>>  	   b <- b,              // Initialize the field only.
>>>  	   c: a.into(),         // That's the "original" `a`.
>>>  	   d <- D::new(this->b),      // Not the original `b`, but the pin-init one.
>>>  	})
>>>
>>> , like a special field projection during initialization.
>>
>> The main issue with new syntax is the difficulty of implementing it. The
>> let one is fine, but it's pretty jarring & doesn't get formatted by
>> rustfmt (which I want to eventually have). Using `this` does look better
>> IMO, but it's near impossible to implement using declarative macros
>> (even using `syn` it seems difficult to me). So either we find some way
>> to express it in existing rust syntax (maybe use an attribute?), or we
>> just keep it this way.
>>
>> Maybe Gary has some ideas on how to implement it.
>
> I also thought about reusing `this`, but I think we should not reuse it. We
> still need it to get pointers to uninitialized fields.
>
> Surely, we could say that we provide this.as_ptr() to get the NonNull `this`
> is currently defined to be and otherwise make it expose only the initialized
> fields for a certain scope.

I have some ideas of changing the syntax to be more closure-esque:

    init!(|this| -> Result<MyStruct, Error> {
        let x = 42;
        MyStruct {
            x,
        }
    })

There we could add another parameter, that would then serve this
purpose. We should also probably rename `this` to `slot` & then use
`this` for the initialized version.

But as I said before, implementing the `this` thing from a macro
perspective is rather difficult (I have two ideas on how to do it and
both are bad...).

> But as you say, that sounds tricky to implement and is probably not very
> intuitive either. I'd rather say keep it as it is, if we don't want something
> like the `let b <- b` syntax I proposed for formatting reasons.

I don't feel like that's conveying the correct thing, it looks as if you
are only declaring a local variable.

---
Cheers,
Benno