rust/pin-init/src/macros.rs | 149 ++++++++++++++++++++++++++++-------- 1 file changed, 115 insertions(+), 34 deletions(-)
After initializing a field in an initializer macro, create a variable
holding a reference that points at that field. The type is either
`Pin<&mut T>` or `&mut T` depending on the field's structural pinning
kind.
Link: https://github.com/Rust-for-Linux/pin-init/pull/83/commits/0f658594c39398f58cd5cb99a8141e370e225e74
Signed-off-by: Benno Lossin <lossin@kernel.org>
---
rust/pin-init/src/macros.rs | 149 ++++++++++++++++++++++++++++--------
1 file changed, 115 insertions(+), 34 deletions(-)
diff --git a/rust/pin-init/src/macros.rs b/rust/pin-init/src/macros.rs
index 9ced630737b8..1100c5a0b3de 100644
--- a/rust/pin-init/src/macros.rs
+++ b/rust/pin-init/src/macros.rs
@@ -988,38 +988,56 @@ fn drop(&mut self) {
@pinned($($(#[$($p_attr:tt)*])* $pvis:vis $p_field:ident : $p_type:ty),* $(,)?),
@not_pinned($($(#[$($attr:tt)*])* $fvis:vis $field:ident : $type:ty),* $(,)?),
) => {
- // For every field, we create a projection function according to its projection type. If a
- // field is structurally pinned, then it must be initialized via `PinInit`, if it is not
- // structurally pinned, then it can be initialized via `Init`.
- //
- // The functions are `unsafe` to prevent accidentally calling them.
- #[allow(dead_code)]
- #[expect(clippy::missing_safety_doc)]
- impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
- where $($whr)*
- {
- $(
- $(#[$($p_attr)*])*
- $pvis unsafe fn $p_field<E>(
- self,
- slot: *mut $p_type,
- init: impl $crate::PinInit<$p_type, E>,
- ) -> ::core::result::Result<(), E> {
- // SAFETY: TODO.
- unsafe { $crate::PinInit::__pinned_init(init, slot) }
- }
- )*
- $(
- $(#[$($attr)*])*
- $fvis unsafe fn $field<E>(
- self,
- slot: *mut $type,
- init: impl $crate::Init<$type, E>,
- ) -> ::core::result::Result<(), E> {
- // SAFETY: TODO.
- unsafe { $crate::Init::__init(init, slot) }
- }
- )*
+ $crate::macros::paste! {
+ // For every field, we create a projection function according to its projection type. If a
+ // field is structurally pinned, then it must be initialized via `PinInit`, if it is not
+ // structurally pinned, then it can be initialized via `Init`.
+ //
+ // The functions are `unsafe` to prevent accidentally calling them.
+ #[allow(dead_code)]
+ #[expect(clippy::missing_safety_doc)]
+ impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
+ where $($whr)*
+ {
+ $(
+ $(#[$($p_attr)*])*
+ $pvis unsafe fn $p_field<E>(
+ self,
+ slot: *mut $p_type,
+ init: impl $crate::PinInit<$p_type, E>,
+ ) -> ::core::result::Result<(), E> {
+ // SAFETY: TODO.
+ unsafe { $crate::PinInit::__pinned_init(init, slot) }
+ }
+
+ $(#[$($p_attr)*])*
+ $pvis unsafe fn [<__project_ $p_field>]<'__slot>(
+ self,
+ slot: &'__slot mut $p_type,
+ ) -> ::core::pin::Pin<&'__slot mut $p_type> {
+ ::core::pin::Pin::new_unchecked(slot)
+ }
+ )*
+ $(
+ $(#[$($attr)*])*
+ $fvis unsafe fn $field<E>(
+ self,
+ slot: *mut $type,
+ init: impl $crate::Init<$type, E>,
+ ) -> ::core::result::Result<(), E> {
+ // SAFETY: TODO.
+ unsafe { $crate::Init::__init(init, slot) }
+ }
+
+ $(#[$($attr)*])*
+ $fvis unsafe fn [<__project_ $field>]<'__slot>(
+ self,
+ slot: &'__slot mut $type,
+ ) -> &'__slot mut $type {
+ slot
+ }
+ )*
+ }
}
};
}
@@ -1216,6 +1234,13 @@ fn assert_zeroable<T: $crate::Zeroable>(_: *mut T) {}
// return when an error/panic occurs.
// We also use the `data` to require the correct trait (`Init` or `PinInit`) for `$field`.
unsafe { $data.$field(::core::ptr::addr_of_mut!((*$slot).$field), init)? };
+ // SAFETY:
+ // - the project function does the correct field projection,
+ // - the field has been initialized,
+ // - the reference is only valid until the end of the initializer.
+ #[allow(unused_variables)]
+ let $field = $crate::macros::paste!(unsafe { $data.[< __project_ $field >](&mut (*$slot).$field) });
+
// Create the drop guard:
//
// We rely on macro hygiene to make it impossible for users to access this local variable.
@@ -1247,6 +1272,14 @@ fn assert_zeroable<T: $crate::Zeroable>(_: *mut T) {}
// SAFETY: `slot` is valid, because we are inside of an initializer closure, we
// return when an error/panic occurs.
unsafe { $crate::Init::__init(init, ::core::ptr::addr_of_mut!((*$slot).$field))? };
+
+ // SAFETY:
+ // - the field is not structurally pinned, since the line above must compile,
+ // - the field has been initialized,
+ // - the reference is only valid until the end of the initializer.
+ #[allow(unused_variables)]
+ let $field = unsafe { &mut (*$slot).$field };
+
// Create the drop guard:
//
// We rely on macro hygiene to make it impossible for users to access this local variable.
@@ -1265,7 +1298,48 @@ fn assert_zeroable<T: $crate::Zeroable>(_: *mut T) {}
);
}
};
- (init_slot($($use_data:ident)?):
+ (init_slot(): // No `use_data`, so all fields are not structurally pinned
+ @data($data:ident),
+ @slot($slot:ident),
+ @guards($($guards:ident,)*),
+ // Init by-value.
+ @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
+ ) => {
+ {
+ $(let $field = $val;)?
+ // Initialize the field.
+ //
+ // SAFETY: The memory at `slot` is uninitialized.
+ unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) };
+ }
+
+ #[allow(unused_variables)]
+ // SAFETY:
+ // - the field is not structurally pinned, since no `use_data` was required to create this
+ // initializer,
+ // - the field has been initialized,
+ // - the reference is only valid until the end of the initializer.
+ let $field = unsafe { &mut (*$slot).$field };
+
+ // Create the drop guard:
+ //
+ // We rely on macro hygiene to make it impossible for users to access this local variable.
+ // We use `paste!` to create new hygiene for `$field`.
+ $crate::macros::paste! {
+ // SAFETY: We forget the guard later when initialization has succeeded.
+ let [< __ $field _guard >] = unsafe {
+ $crate::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
+ };
+
+ $crate::__init_internal!(init_slot():
+ @data($data),
+ @slot($slot),
+ @guards([< __ $field _guard >], $($guards,)*),
+ @munch_fields($($rest)*),
+ );
+ }
+ };
+ (init_slot($use_data:ident):
@data($data:ident),
@slot($slot:ident),
@guards($($guards:ident,)*),
@@ -1279,6 +1353,13 @@ fn assert_zeroable<T: $crate::Zeroable>(_: *mut T) {}
// SAFETY: The memory at `slot` is uninitialized.
unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) };
}
+ // SAFETY:
+ // - the project function does the correct field projection,
+ // - the field has been initialized,
+ // - the reference is only valid until the end of the initializer.
+ #[allow(unused_variables)]
+ let $field = $crate::macros::paste!(unsafe { $data.[< __project_ $field >](&mut (*$slot).$field) });
+
// Create the drop guard:
//
// We rely on macro hygiene to make it impossible for users to access this local variable.
@@ -1289,7 +1370,7 @@ fn assert_zeroable<T: $crate::Zeroable>(_: *mut T) {}
$crate::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
};
- $crate::__init_internal!(init_slot($($use_data)?):
+ $crate::__init_internal!(init_slot($use_data):
@data($data),
@slot($slot),
@guards([< __ $field _guard >], $($guards,)*),
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
--
2.50.1Hi Benno, kernel test robot noticed the following build errors: [auto build test ERROR on 8f5ae30d69d7543eee0d70083daf4de8fe15d585] url: https://github.com/intel-lab-lkp/linux/commits/Benno-Lossin/rust-pin-init-add-references-to-previously-initialized-fields/20250905-220242 base: 8f5ae30d69d7543eee0d70083daf4de8fe15d585 patch link: https://lore.kernel.org/r/20250905140047.3325945-1-lossin%40kernel.org patch subject: [PATCH] rust: pin-init: add references to previously initialized fields config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250906/202509062218.Mo9Wsmcd-lkp@intel.com/config) compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261) rustc: rustc 1.88.0 (6b00bc388 2025-06-23) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250906/202509062218.Mo9Wsmcd-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202509062218.Mo9Wsmcd-lkp@intel.com/ All errors (new ones prefixed by >>): >> error[E0308]: mismatched types --> rust/kernel/devres.rs:154:66 | 154 | bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast()) | ---- ^^^^^^^^ expected fn pointer, found `&mut unsafe extern "C" fn(*mut c_void)` | | | arguments to this enum variant are incorrect | = note: expected fn pointer `unsafe extern "C" fn(_)` found mutable reference `&mut unsafe extern "C" fn(_)` help: the type constructed contains `&mut unsafe extern "C" fn(*mut ffi::c_void)` due to the type of the argument passed --> rust/kernel/devres.rs:154:61 | 154 | bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast()) | ^^^^^--------^ | | | this argument influences the type of `Some` note: tuple variant defined here --> /opt/cross/rustc-1.88.0-bindgen-0.72.0/rustup/toolchains/1.88.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:597:5 | 597 | Some(#[stable(feature = "rust1", since = "1.0.0")] T), | ^^^^ help: consider dereferencing the borrow | 154 | bindings::devm_add_action(dev.as_raw(), Some(*callback), inner.cast()) | + -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
On Fri, Sep 05, 2025 at 04:00:46PM +0200, Benno Lossin wrote: > After initializing a field in an initializer macro, create a variable > holding a reference that points at that field. The type is either > `Pin<&mut T>` or `&mut T` depending on the field's structural pinning > kind. > It's hard to review because of lack of examples. Could you or Danilo share some sample usages? Thanks! Regards, Boqun > Link: https://github.com/Rust-for-Linux/pin-init/pull/83/commits/0f658594c39398f58cd5cb99a8141e370e225e74 > Signed-off-by: Benno Lossin <lossin@kernel.org> > --- [...]
(Cc: Alex)
On Fri Sep 5, 2025 at 7:21 PM CEST, Boqun Feng wrote:
> On Fri, Sep 05, 2025 at 04:00:46PM +0200, Benno Lossin wrote:
>> After initializing a field in an initializer macro, create a variable
>> holding a reference that points at that field. The type is either
>> `Pin<&mut T>` or `&mut T` depending on the field's structural pinning
>> kind.
>>
>
> It's hard to review because of lack of examples. Could you or Danilo
> share some sample usages? Thanks!
Sure, here's an example. Eventually, it's going to be a bit more complicated,
but basically that's it.
#[pin_data(PinnedDrop)]
pub(crate) struct Gpu {
spec: Spec,
bar: Arc<Devres<Bar0>>,
sysmem_flush: SysmemFlush,
gsp_falcon: Falcon<Gsp>,
sec2_falcon: Falcon<Sec2>,
#[pin]
gsp: Gsp,
}
impl Gpu {
pub(crate) fn new(
dev: &Device<Bound>,
bar: Arc<Devres<Bar0>>,
) -> impl PinInit<Self, Error> + '_ {
try_pin_init(Self {
bar,
spec: Spec::new(bar.access(dev)?)?,
gsp_falcon: Falcon::<Gsp>::new(dev, spec.chipset)?,
sec2_falcon: Falcon::<Sec2>::new(dev, spec.chipset)?,
sysmem_flush: SysmemFlush::register(dev, bar.access(dev)?, spec.chipset)?
gsp <- Gsp::new(gsp_falcon, sec2_falcon, sysmem_flush)?,
})
}
}
Imagine how much unsafe pointer mess this needs without this patch. :)
On Fri Sep 5, 2025 at 4:00 PM CEST, Benno Lossin wrote:
> After initializing a field in an initializer macro, create a variable
> holding a reference that points at that field. The type is either
> `Pin<&mut T>` or `&mut T` depending on the field's structural pinning
> kind.
>
> Link: https://github.com/Rust-for-Linux/pin-init/pull/83/commits/0f658594c39398f58cd5cb99a8141e370e225e74
> Signed-off-by: Benno Lossin <lossin@kernel.org>
I forgot to test with the right configuration and found some errors with
existing code. Here are their fixes. If I don't need to re-send, I will
add them on apply (if you want a v2, let me know).
---
Cheers,
Benno
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index da18091143a6..91dbf3f4b166 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -134,11 +134,9 @@ pub fn new<'a, E>(
T: 'a,
Error: From<E>,
{
- let callback = Self::devres_callback;
-
try_pin_init!(&this in Self {
dev: dev.into(),
- callback,
+ callback: Self::devres_callback,
// INVARIANT: `inner` is properly initialized.
inner <- {
// SAFETY: `this` is a valid pointer to uninitialized memory.
@@ -151,7 +149,7 @@ pub fn new<'a, E>(
// properly initialized, because we require `dev` (i.e. the *bound* device) to
// live at least as long as the returned `impl PinInit<Self, Error>`.
to_result(unsafe {
- bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
+ bindings::devm_add_action(dev.as_raw(), Some(*callback), inner.cast())
})?;
Opaque::pin_init(try_pin_init!(Inner {
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
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(),
index: *info,
}),
GFP_KERNEL,
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`.
}
(Since I lost tracking of the discussion a bit, maybe there is a
previous discussion I've missed here?)
Regards,
Boqun
> index: *info,
> }),
> GFP_KERNEL,
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:
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:
// 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.
})
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.
> })
This looks good to me as well. But I'm curious what the syntax would be
like in the in-place placement language feature in the future.
Regards,
Boqun
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.
Regards,
Boqun
>
> This looks good to me as well. But I'm curious what the syntax would be
> like in the in-place placement language feature in the future.
>
> Regards,
> Boqun
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.
---
Cheers,
Benno
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.
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.
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
On Sun Sep 7, 2025 at 11:06 PM CEST, Benno Lossin wrote:
> On Sun Sep 7, 2025 at 7:29 PM CEST, Danilo Krummrich wrote:
> 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.
I think that's a pretty good idea, but the part that I think is a little bit
confusing remains: `this` will need to have different fields depending on where
it's accessed.
> 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.
Yeah, it's not great, but given that it's a custom syntax it also does not
create wrong expectations I'd say.
Anyways, I'm fine with either. For now we probably want to land the version as
it is and revisit once you settle on the syntax rework you mentioned above.
On Sun Sep 7, 2025 at 11:39 PM CEST, Danilo Krummrich wrote:
> On Sun Sep 7, 2025 at 11:06 PM CEST, Benno Lossin wrote:
>> On Sun Sep 7, 2025 at 7:29 PM CEST, Danilo Krummrich wrote:
>> 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.
>
> I think that's a pretty good idea, but the part that I think is a little bit
> confusing remains: `this` will need to have different fields depending on where
> it's accessed.
Yeah (that's also the main issue with the macro implementation).
>> 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.
>
> Yeah, it's not great, but given that it's a custom syntax it also does not
> create wrong expectations I'd say.
I'd say it looks like combining the `<-` operation already used by the
`init!` macro & a `let` binding. Thus introducing a local variable
that's (pin) initialized in-place. Not a field of the current struct.
> Anyways, I'm fine with either. For now we probably want to land the version as
> it is and revisit once you settle on the syntax rework you mentioned above.
I actually came up with a third option that looks best IMO:
init!(MyStruct {
x: 42,
#[with_binding]
y: 24,
z: *y,
})
The `#[with_binding]` attribute makes the macro generate a variable `y`.
`x` & `z` don't give access to their value. (we of course should come up
with a better name).
Any thoughts?
---
Cheers,
Benno
On Mon Sep 8, 2025 at 12:51 AM CEST, Benno Lossin wrote:
> I actually came up with a third option that looks best IMO:
>
> init!(MyStruct {
> x: 42,
> #[with_binding]
> y: 24,
> z: *y,
> })
>
> The `#[with_binding]` attribute makes the macro generate a variable `y`.
> `x` & `z` don't give access to their value. (we of course should come up
> with a better name).
>
> Any thoughts?
It may be a bit verbose is some cases, but it makes things pretty obvious, so
LGTM.
How about just #[bind] or #[access]?
On Mon, Sep 08, 2025 at 01:33:26AM +0200, Danilo Krummrich wrote:
> On Mon Sep 8, 2025 at 12:51 AM CEST, Benno Lossin wrote:
> > I actually came up with a third option that looks best IMO:
> >
> > init!(MyStruct {
> > x: 42,
> > #[with_binding]
> > y: 24,
> > z: *y,
> > })
> >
> > The `#[with_binding]` attribute makes the macro generate a variable `y`.
> > `x` & `z` don't give access to their value. (we of course should come up
> > with a better name).
> >
> > Any thoughts?
>
> It may be a bit verbose is some cases, but it makes things pretty obvious, so
> LGTM.
>
> How about just #[bind] or #[access]?
#[shadow] or #[maybe_rebind] ? Or #[pin_ref], the last one is clear
about the purpose.
Regards,
Boqun
On Mon Sep 8, 2025 at 4:08 AM CEST, Boqun Feng wrote:
> On Mon, Sep 08, 2025 at 01:33:26AM +0200, Danilo Krummrich wrote:
>> On Mon Sep 8, 2025 at 12:51 AM CEST, Benno Lossin wrote:
>> > I actually came up with a third option that looks best IMO:
>> >
>> > init!(MyStruct {
>> > x: 42,
>> > #[with_binding]
>> > y: 24,
>> > z: *y,
>> > })
>> >
>> > The `#[with_binding]` attribute makes the macro generate a variable `y`.
>> > `x` & `z` don't give access to their value. (we of course should come up
>> > with a better name).
>> >
>> > Any thoughts?
>>
>> It may be a bit verbose is some cases, but it makes things pretty obvious, so
>> LGTM.
>>
>> How about just #[bind] or #[access]?
I like `#[bind]`.
> #[shadow] or #[maybe_rebind] ? Or #[pin_ref], the last one is clear
> about the purpose.
Hmm in `init!` it's never pinned.
---
Cheers,
Benno
On Mon Sep 8, 2025 at 10:27 AM CEST, Benno Lossin wrote:
> On Mon Sep 8, 2025 at 4:08 AM CEST, Boqun Feng wrote:
>> On Mon, Sep 08, 2025 at 01:33:26AM +0200, Danilo Krummrich wrote:
>>> On Mon Sep 8, 2025 at 12:51 AM CEST, Benno Lossin wrote:
>>> > I actually came up with a third option that looks best IMO:
>>> >
>>> > init!(MyStruct {
>>> > x: 42,
>>> > #[with_binding]
>>> > y: 24,
>>> > z: *y,
>>> > })
>>> >
>>> > The `#[with_binding]` attribute makes the macro generate a variable `y`.
>>> > `x` & `z` don't give access to their value. (we of course should come up
>>> > with a better name).
>>> >
>>> > Any thoughts?
>>>
>>> It may be a bit verbose is some cases, but it makes things pretty obvious, so
>>> LGTM.
>>>
>>> How about just #[bind] or #[access]?
>
> I like `#[bind]`.
>
>> #[shadow] or #[maybe_rebind] ? Or #[pin_ref], the last one is clear
>> about the purpose.
>
> Hmm in `init!` it's never pinned.
I thought about #[shadow] as well, but it is not really accurate I think, as we
might not shadow anything. #[maybe_rebind] sounds a bit like it conditionally
rebinds, as in "it may not do anything", but it always binds.
So, I think it should one clear instruction, i.e. #[bind], #[access], #[ref],
#[use], #[let], etc.
On Mon, Sep 08, 2025 at 10:57:36AM +0200, Danilo Krummrich wrote:
> On Mon Sep 8, 2025 at 10:27 AM CEST, Benno Lossin wrote:
> > On Mon Sep 8, 2025 at 4:08 AM CEST, Boqun Feng wrote:
> >> On Mon, Sep 08, 2025 at 01:33:26AM +0200, Danilo Krummrich wrote:
> >>> On Mon Sep 8, 2025 at 12:51 AM CEST, Benno Lossin wrote:
> >>> > I actually came up with a third option that looks best IMO:
> >>> >
> >>> > init!(MyStruct {
> >>> > x: 42,
> >>> > #[with_binding]
> >>> > y: 24,
> >>> > z: *y,
> >>> > })
> >>> >
> >>> > The `#[with_binding]` attribute makes the macro generate a variable `y`.
> >>> > `x` & `z` don't give access to their value. (we of course should come up
> >>> > with a better name).
> >>> >
> >>> > Any thoughts?
> >>>
> >>> It may be a bit verbose is some cases, but it makes things pretty obvious, so
> >>> LGTM.
> >>>
> >>> How about just #[bind] or #[access]?
> >
> > I like `#[bind]`.
> >
> >> #[shadow] or #[maybe_rebind] ? Or #[pin_ref], the last one is clear
> >> about the purpose.
> >
> > Hmm in `init!` it's never pinned.
>
> I thought about #[shadow] as well, but it is not really accurate I think, as we
> might not shadow anything. #[maybe_rebind] sounds a bit like it conditionally
> rebinds, as in "it may not do anything", but it always binds.
>
> So, I think it should one clear instruction, i.e. #[bind], #[access], #[ref],
> #[use], #[let], etc.
In that sense I think `#[let]` is best? Because it indicates this field
initialization works as a `let`-statement (in term of creating a new
binding), of course I don't have strong ojections against other options.
Regards,
Boqun
On 9/8/25 9:38 PM, Boqun Feng wrote: > On Mon, Sep 08, 2025 at 10:57:36AM +0200, Danilo Krummrich wrote: >> So, I think it should one clear instruction, i.e. #[bind], #[access], #[ref], >> #[use], #[let], etc. > > In that sense I think `#[let]` is best? Because it indicates this field > initialization works as a `let`-statement (in term of creating a new > binding), of course I don't have strong ojections against other options. Same for me, I'm fine with any of this kind. :)
© 2016 - 2025 Red Hat, Inc.