[PATCH] rust: arc: remove unused PhantomData

[PATCH] rust: arc: remove unused PhantomData

[Tamir Duberstein]

There's no need for this. The type had the same form when it was first
introduced, so it seems this was never necessary.

Fixed: 9dc043655003 ("rust: sync: add `Arc` for ref-counted allocations")
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/sync/arc.rs | 2 --
 1 file changed, 2 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index db9da352d588f65348aa7a5204abbb165b70197f..7e54d31538273d410f80fd65b2070e75e4f69428 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -127,7 +127,6 @@
 /// ```
 pub struct Arc<T: ?Sized> {
     ptr: NonNull<ArcInner<T>>,
-    _p: PhantomData<ArcInner<T>>,
 }
 
 #[pin_data]
@@ -219,7 +218,6 @@ unsafe fn from_inner(inner: NonNull<ArcInner<T>>) -> Self {
         // INVARIANT: By the safety requirements, the invariants hold.
         Arc {
             ptr: inner,
-            _p: PhantomData,
         }
     }
 

---
base-commit: ae7851c29747fa3765ecb722fe722117a346f988
change-id: 20241104-simplify-arc-70c3574b5fac

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

Re: [PATCH] rust: arc: remove unused PhantomData

[Boqun Feng]

Hi Tamir,

On Mon, Nov 04, 2024 at 05:23:44PM -0400, Tamir Duberstein wrote:
> There's no need for this. The type had the same form when it was first

I believe we need this `PhantomData` to inform drop chec [1] that the
drop of `Arc` may result into the drop of an `ArcInner<T>`. Rust std
`Arc` has the similar definition [2], you can find more information
about PhantomData usage on drop checking at [3].

[1]: https://doc.rust-lang.org/nightly/std/ops/trait.Drop.html#drop-check
[2]: https://doc.rust-lang.org/src/alloc/sync.rs.html#245
[3]: https://doc.rust-lang.org/nightly/std/marker/struct.PhantomData.html#ownership-and-the-drop-check

Regards,
Boqun

> introduced, so it seems this was never necessary.
> 
> Fixed: 9dc043655003 ("rust: sync: add `Arc` for ref-counted allocations")
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/sync/arc.rs | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index db9da352d588f65348aa7a5204abbb165b70197f..7e54d31538273d410f80fd65b2070e75e4f69428 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -127,7 +127,6 @@
>  /// ```
>  pub struct Arc<T: ?Sized> {
>      ptr: NonNull<ArcInner<T>>,
> -    _p: PhantomData<ArcInner<T>>,
>  }
>  
>  #[pin_data]
> @@ -219,7 +218,6 @@ unsafe fn from_inner(inner: NonNull<ArcInner<T>>) -> Self {
>          // INVARIANT: By the safety requirements, the invariants hold.
>          Arc {
>              ptr: inner,
> -            _p: PhantomData,
>          }
>      }
>  
> 
> ---
> base-commit: ae7851c29747fa3765ecb722fe722117a346f988
> change-id: 20241104-simplify-arc-70c3574b5fac
> 
> Best regards,
> -- 
> Tamir Duberstein <tamird@gmail.com>
> 

Re: [PATCH] rust: arc: remove unused PhantomData

[Tamir Duberstein]

On Mon, Nov 4, 2024 at 6:42 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Hi Tamir,
>
> On Mon, Nov 04, 2024 at 05:23:44PM -0400, Tamir Duberstein wrote:
> > There's no need for this. The type had the same form when it was first
>
> I believe we need this `PhantomData` to inform drop chec [1] that the
> drop of `Arc` may result into the drop of an `ArcInner<T>`. Rust std
> `Arc` has the similar definition [2], you can find more information
> about PhantomData usage on drop checking at [3].
>
> [1]: https://doc.rust-lang.org/nightly/std/ops/trait.Drop.html#drop-check
> [2]: https://doc.rust-lang.org/src/alloc/sync.rs.html#245
> [3]: https://doc.rust-lang.org/nightly/std/marker/struct.PhantomData.html#ownership-and-the-drop-check
>
> Regards,
> Boqun

Thanks for explaining! Withdrawn.

> > introduced, so it seems this was never necessary.
> >
> > Fixed: 9dc043655003 ("rust: sync: add `Arc` for ref-counted allocations")
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/kernel/sync/arc.rs | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index db9da352d588f65348aa7a5204abbb165b70197f..7e54d31538273d410f80fd65b2070e75e4f69428 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -127,7 +127,6 @@
> >  /// ```
> >  pub struct Arc<T: ?Sized> {
> >      ptr: NonNull<ArcInner<T>>,
> > -    _p: PhantomData<ArcInner<T>>,
> >  }
> >
> >  #[pin_data]
> > @@ -219,7 +218,6 @@ unsafe fn from_inner(inner: NonNull<ArcInner<T>>) -> Self {
> >          // INVARIANT: By the safety requirements, the invariants hold.
> >          Arc {
> >              ptr: inner,
> > -            _p: PhantomData,
> >          }
> >      }
> >
> >
> > ---
> > base-commit: ae7851c29747fa3765ecb722fe722117a346f988
> > change-id: 20241104-simplify-arc-70c3574b5fac
> >
> > Best regards,
> > --
> > Tamir Duberstein <tamird@gmail.com>
> >

Re: [PATCH] rust: arc: remove unused PhantomData

[Miguel Ojeda]

On Mon, Nov 4, 2024 at 11:42 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> I believe we need this `PhantomData` to inform drop chec [1] that the
> drop of `Arc` may result into the drop of an `ArcInner<T>`. Rust std
> `Arc` has the similar definition [2], you can find more information
> about PhantomData usage on drop checking at [3].

Hmm... But they use `may_dangle` in their `Drop` and we don't (and we
have a `Drop` unlike something like `Unique`), no? Or am I confused?
i.e. if I understand correctly, that would have been needed in the
past, but not anymore.

In any case, a comment here clarifying would be good -- the standard
library does that in some of its types, which is a good idea. At the
very least, the name of the field should indicate why we have the
marker. I will add that to the guidelines patch I have to send...

Thanks!

Cheers,
Miguel

Re: [PATCH] rust: arc: remove unused PhantomData

[Tamir Duberstein]

On Tue, Nov 5, 2024 at 8:29 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Mon, Nov 4, 2024 at 11:42 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > I believe we need this `PhantomData` to inform drop chec [1] that the
> > drop of `Arc` may result into the drop of an `ArcInner<T>`. Rust std
> > `Arc` has the similar definition [2], you can find more information
> > about PhantomData usage on drop checking at [3].
>
> Hmm... But they use `may_dangle` in their `Drop` and we don't (and we
> have a `Drop` unlike something like `Unique`), no? Or am I confused?
> i.e. if I understand correctly, that would have been needed in the
> past, but not anymore.

Doing a bit of archaeology I found the reasoning for the presence of
`PhantomData` in std's `Arc`[0]. The TL;DR is that the presence of a
type parameter `T` implies "owns T", but `Arc` owns `ArcInner<T>`
rather than `T`. Thus in order to get correct dropck behavior it is
necessary to opt out of "owns T" using `may_dangle` and opt into "owns
ArcInner<T>" using `PhantomData`.

Please check my understanding; I couldn't find detailed documentation
of the interaction between `may_dangle` and `PhantomData`. If this
sounds correct, should we add `may_dangle` to our dropck? compile-fail
tests would be useful here.

Cheers,
Tamir

Link: https://github.com/rust-lang/rust/pull/46749#issuecomment-352146569 [0]

Re: [PATCH] rust: arc: remove unused PhantomData

[Alice Ryhl]

On Tue, Nov 5, 2024 at 9:13 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Tue, Nov 5, 2024 at 8:29 AM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > On Mon, Nov 4, 2024 at 11:42 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > I believe we need this `PhantomData` to inform drop chec [1] that the
> > > drop of `Arc` may result into the drop of an `ArcInner<T>`. Rust std
> > > `Arc` has the similar definition [2], you can find more information
> > > about PhantomData usage on drop checking at [3].
> >
> > Hmm... But they use `may_dangle` in their `Drop` and we don't (and we
> > have a `Drop` unlike something like `Unique`), no? Or am I confused?
> > i.e. if I understand correctly, that would have been needed in the
> > past, but not anymore.
>
> Doing a bit of archaeology I found the reasoning for the presence of
> `PhantomData` in std's `Arc`[0]. The TL;DR is that the presence of a
> type parameter `T` implies "owns T", but `Arc` owns `ArcInner<T>`
> rather than `T`. Thus in order to get correct dropck behavior it is
> necessary to opt out of "owns T" using `may_dangle` and opt into "owns
> ArcInner<T>" using `PhantomData`.
>
> Please check my understanding; I couldn't find detailed documentation
> of the interaction between `may_dangle` and `PhantomData`. If this
> sounds correct, should we add `may_dangle` to our dropck? compile-fail
> tests would be useful here.

We don't *have* to use #[may_dangle]. Using it may allow more stuff,
but it's not a problem for it to be missing. We probably don't want to
use it since it's unstable.

Since we don't use #[may_dangle], we don't *need* the PhantomData
field. Having it doesn't change anything.

Alice

Re: [PATCH] rust: arc: remove unused PhantomData

[Tamir Duberstein]

On Wed, Nov 6, 2024 at 5:26 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Tue, Nov 5, 2024 at 9:13 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > On Tue, Nov 5, 2024 at 8:29 AM Miguel Ojeda
> > <miguel.ojeda.sandonis@gmail.com> wrote:
> > >
> > > On Mon, Nov 4, 2024 at 11:42 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > >
> > > > I believe we need this `PhantomData` to inform drop chec [1] that the
> > > > drop of `Arc` may result into the drop of an `ArcInner<T>`. Rust std
> > > > `Arc` has the similar definition [2], you can find more information
> > > > about PhantomData usage on drop checking at [3].
> > >
> > > Hmm... But they use `may_dangle` in their `Drop` and we don't (and we
> > > have a `Drop` unlike something like `Unique`), no? Or am I confused?
> > > i.e. if I understand correctly, that would have been needed in the
> > > past, but not anymore.
> >
> > Doing a bit of archaeology I found the reasoning for the presence of
> > `PhantomData` in std's `Arc`[0]. The TL;DR is that the presence of a
> > type parameter `T` implies "owns T", but `Arc` owns `ArcInner<T>`
> > rather than `T`. Thus in order to get correct dropck behavior it is
> > necessary to opt out of "owns T" using `may_dangle` and opt into "owns
> > ArcInner<T>" using `PhantomData`.
> >
> > Please check my understanding; I couldn't find detailed documentation
> > of the interaction between `may_dangle` and `PhantomData`. If this
> > sounds correct, should we add `may_dangle` to our dropck? compile-fail
> > tests would be useful here.
>
> We don't *have* to use #[may_dangle]. Using it may allow more stuff,
> but it's not a problem for it to be missing. We probably don't want to
> use it since it's unstable.
>
> Since we don't use #[may_dangle], we don't *need* the PhantomData
> field. Having it doesn't change anything.

In that case, should we reconsider this patch?

Re: [PATCH] rust: arc: remove unused PhantomData

[Miguel Ojeda]

On Tue, Nov 5, 2024 at 9:13 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Doing a bit of archaeology I found the reasoning for the presence of

What I meant by "in the past" is that, before the Rust RFC that
changed it, the `PhantomData` would have been needed as far as I
understand it, since having the `Drop` wouldn't have been enough.

Cheers,
Miguel

Re: [PATCH] rust: arc: remove unused PhantomData

[Miguel Ojeda]

On Wed, Nov 6, 2024 at 2:45 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> In that case, should we reconsider this patch?

Either that [*] or we could at least add a comment explaining it is
not required for drop check purposes but that we have it anyway for
clarity.

Starting to use `may_dangle` is a third option, but I agree we should
avoid it unless we got at least an indication from upstream Rust that
they want to stabilize it soon in that form (and probably only if we
feel an actual need for it, since it is yet another `unsafe` use).

[*] Well, not this patch exactly -- the commit message should be fixed
to explain things properly (and likely the "Fixes" tag removed) and it
should also mention it double-checked the effect on variance and auto
traits.

Cheers,
Miguel

Re: [PATCH] rust: arc: remove unused PhantomData

[Tamir Duberstein]

On Wed, Nov 6, 2024 at 11:38 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Tue, Nov 5, 2024 at 9:13 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Doing a bit of archaeology I found the reasoning for the presence of
>
> What I meant by "in the past" is that, before the Rust RFC that
> changed it, the `PhantomData` would have been needed as far as I
> understand it, since having the `Drop` wouldn't have been enough.

I would be happy to add the relevant details to the commit message but
this is one citation that I haven't been able to locate. The closest
mention I could find[0] only vaguely mentions that this change was
made, but does not reference a commit (and certainly not an RFC).

Link: https://github.com/rust-lang/rust/issues/27730#issuecomment-316411459 [0]

Re: [PATCH] rust: arc: remove unused PhantomData

[Tamir Duberstein]

On Wed, Nov 6, 2024 at 11:39 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Nov 6, 2024 at 2:45 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > In that case, should we reconsider this patch?
>
> Either that [*] or we could at least add a comment explaining it is
> not required for drop check purposes but that we have it anyway for
> clarity.
>
> Starting to use `may_dangle` is a third option, but I agree we should
> avoid it unless we got at least an indication from upstream Rust that
> they want to stabilize it soon in that form (and probably only if we
> feel an actual need for it, since it is yet another `unsafe` use).
>
> [*] Well, not this patch exactly -- the commit message should be fixed
> to explain things properly (and likely the "Fixes" tag removed) and it
> should also mention it double-checked the effect on variance and auto
> traits.

The upstream changes to dropck predate the PR I linked up-thread which
landed in 2017. Since this Arc code was written in 2022, it never had
any effect. Isn't it proper to keep the "Fixes" tag?

Re: [PATCH] rust: arc: remove unused PhantomData

[Miguel Ojeda]

On Wed, Nov 6, 2024 at 5:33 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> I would be happy to add the relevant details to the commit message but
> this is one citation that I haven't been able to locate. The closest
> mention I could find[0] only vaguely mentions that this change was
> made, but does not reference a commit (and certainly not an RFC).

In Boqun's first link, there is a reference to the nomicon with
details, and the section:

    https://doc.rust-lang.org/nomicon/phantom-data.html#generic-parameters-and-drop-checking

explains the change, including:

    "But ever since RFC 1238, this is no longer true nor necessary."

There was another RFC (1327) after that, for a finer-grained approach
(`may_dangle`). The name of the feature gate was also changed.

Anyway, I don't think we need to add any of that to the commit message
though. Perhaps linking the latest RFC is good for context, so if you
think it is a good idea, of course please go for it -- but in case you
are referring to what I said, I didn't say that we should add the RFC
bits into the commit message.

Cheers,
Miguel

Re: [PATCH] rust: arc: remove unused PhantomData

[Miguel Ojeda]

On Wed, Nov 6, 2024 at 5:41 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> The upstream changes to dropck predate the PR I linked up-thread which
> landed in 2017. Since this Arc code was written in 2022, it never had
> any effect. Isn't it proper to keep the "Fixes" tag?

If there is a bug, definitely yes, but I don't think that applies is
-- this is more of a cleanup, no?

Sometimes things are marked as "fixes" that are perhaps a stretch
(e.g. a typo in a comment). It depends a bit on the maintainer and how
we define "bug" (e.g. does it count in docs, or just actual end
users). But here it just seems something is superfluous, at worst, and
it does not need to be backported either. Even if we kept the tag for
some reason, I think this belongs in `rust-next`.

But if I am missing something, and this does indeed fix something that
we should prioritize, please let me know!

What looks more important, to me, is to clarify/document why (or why
not) we have it, regardless of whether we keep it or not, i.e. having
thought about it, I think it wouldn't hurt having a line/comment even
if we remove it.

Thanks!

Cheers,
Miguel

Re: [PATCH] rust: arc: remove unused PhantomData

[Tamir Duberstein]

On Wed, Nov 6, 2024 at 2:30 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Nov 6, 2024 at 5:41 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > The upstream changes to dropck predate the PR I linked up-thread which
> > landed in 2017. Since this Arc code was written in 2022, it never had
> > any effect. Isn't it proper to keep the "Fixes" tag?
>
> If there is a bug, definitely yes, but I don't think that applies is
> -- this is more of a cleanup, no?
>
> Sometimes things are marked as "fixes" that are perhaps a stretch
> (e.g. a typo in a comment). It depends a bit on the maintainer and how
> we define "bug" (e.g. does it count in docs, or just actual end
> users). But here it just seems something is superfluous, at worst, and
> it does not need to be backported either. Even if we kept the tag for
> some reason, I think this belongs in `rust-next`.

Thanks, I forgot that "Fixes" changes are backported. This makes sense!

> But if I am missing something, and this does indeed fix something that
> we should prioritize, please let me know!
>
> What looks more important, to me, is to clarify/document why (or why
> not) we have it, regardless of whether we keep it or not, i.e. having
> thought about it, I think it wouldn't hurt having a line/comment even
> if we remove it.

Will do.

> Thanks!
>
> Cheers,
> Miguel

Re: [PATCH] rust: arc: remove unused PhantomData

[Tamir Duberstein]

On Wed, Nov 6, 2024 at 2:20 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Nov 6, 2024 at 5:33 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > I would be happy to add the relevant details to the commit message but
> > this is one citation that I haven't been able to locate. The closest
> > mention I could find[0] only vaguely mentions that this change was
> > made, but does not reference a commit (and certainly not an RFC).
>
> In Boqun's first link, there is a reference to the nomicon with
> details, and the section:
>
>     https://doc.rust-lang.org/nomicon/phantom-data.html#generic-parameters-and-drop-checking
>
> explains the change, including:
>
>     "But ever since RFC 1238, this is no longer true nor necessary."
>
> There was another RFC (1327) after that, for a finer-grained approach
> (`may_dangle`). The name of the feature gate was also changed.
>
> Anyway, I don't think we need to add any of that to the commit message
> though. Perhaps linking the latest RFC is good for context, so if you
> think it is a good idea, of course please go for it -- but in case you
> are referring to what I said, I didn't say that we should add the RFC
> bits into the commit message.
>
> Cheers,
> Miguel

Thanks a lot for all the hand-holding here. I think the sensible thing
to do here is to add a comment rather than remove the PhantomData.
I'll send that as v2 shortly if no-one objects.

Re: [PATCH] rust: arc: remove unused PhantomData

[Alice Ryhl]

On Wed, Nov 6, 2024 at 10:13 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Wed, Nov 6, 2024 at 2:20 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > On Wed, Nov 6, 2024 at 5:33 PM Tamir Duberstein <tamird@gmail.com> wrote:
> > >
> > > I would be happy to add the relevant details to the commit message but
> > > this is one citation that I haven't been able to locate. The closest
> > > mention I could find[0] only vaguely mentions that this change was
> > > made, but does not reference a commit (and certainly not an RFC).
> >
> > In Boqun's first link, there is a reference to the nomicon with
> > details, and the section:
> >
> >     https://doc.rust-lang.org/nomicon/phantom-data.html#generic-parameters-and-drop-checking
> >
> > explains the change, including:
> >
> >     "But ever since RFC 1238, this is no longer true nor necessary."
> >
> > There was another RFC (1327) after that, for a finer-grained approach
> > (`may_dangle`). The name of the feature gate was also changed.
> >
> > Anyway, I don't think we need to add any of that to the commit message
> > though. Perhaps linking the latest RFC is good for context, so if you
> > think it is a good idea, of course please go for it -- but in case you
> > are referring to what I said, I didn't say that we should add the RFC
> > bits into the commit message.
> >
> > Cheers,
> > Miguel
>
> Thanks a lot for all the hand-holding here. I think the sensible thing
> to do here is to add a comment rather than remove the PhantomData.
> I'll send that as v2 shortly if no-one objects.

Adding a comment and keeping PhantomData SGTM.

Alice

Re: [PATCH] rust: arc: remove unused PhantomData

[Miguel Ojeda]

On Wed, Nov 6, 2024 at 8:09 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Thanks, I forgot that "Fixes" changes are backported. This makes sense!

Sometimes they are, yeah, but things that should go into stable should
be marked explicitly nevertheless.

Cheers,
Miguel