Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: #[derive(SmartPointer)] #3621

Merged
merged 16 commits into from
Jul 11, 2024

Conversation

Darksonn
Copy link
Contributor

@Darksonn Darksonn commented May 2, 2024

Use custom smart pointers with trait objects.

Rendered

Tracking issue: rust-lang/rust#123430


Co-authored by @Darksonn and @Veykril
Thank you to @compiler-errors for the original idea

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>
@Lokathor
Copy link
Contributor

Lokathor commented May 2, 2024

My main thought is that maybe this should just require repr(transparent), which simplifies a lot of the explanation of the requirements part.

text/3621-derive-smart-pointer.md Show resolved Hide resolved
text/3621-derive-smart-pointer.md Outdated Show resolved Hide resolved
text/3621-derive-smart-pointer.md Outdated Show resolved Hide resolved
@joshtriplett
Copy link
Member

My main thought is that maybe this should just require repr(transparent), which simplifies a lot of the explanation of the requirements part.

I don't think there should be a hard requirement that smart pointers be transparent. But since the requirements are very similar, it might make sense to incorporate the requirements by reference, for simplicity of documentation.

@ehuss ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label May 2, 2024
@clarfonthey
Copy link
Contributor

clarfonthey commented May 3, 2024

After doing a bit more research to get myself acquainted with the CoerceUnsized and DispatchFromDyn traits, I really don't think that it's a good idea to just bundle these both together as "smart pointer." Yes, all smart pointers will have these, but it's a false equivalence; having them does not make you a smart pointer.

What these traits really are about is ensuring proper variance, which is especially weird since variance is usually only exposed to programmers in Rust through weird mechanisms like PhantomData. Ultimately:

  • CoerceUnsized lets you declare a type as covariant, and varying the type this way makes it more general. For example, this lets you vary a mutable reference into an immutable reference or an array into a slice.
  • DispatchFromDyn lets you declare a type as contravariant, and varying the type this way makes it more specific. This would be the opposite, like varying a slice into an array or a trait object into a concrete type.

Smart pointers require both of these just by the nature of how references in Rust work in general: because autoderef lets us borrow things mostly automatically, we expect this automatic borrowing to happen even if a smart pointer is between us and our data. However, it's important to realise that this is just about variance and we only tie it specifically to smart pointers because unsized types in Rust today have to be used behind pointers.

If we allow unsized locals, whose RFC was accepted, now these types become even more just about variance. You should always be able to pass arrays into functions which accept unsized slices, return unsized trait objects by returning a concrete object, etc., etc.

Honestly, thinking of this more specifically as a way of managing variance, I do question whether the current method of manually implementing these traits is the right approach, and whether we should rely more on something like repr(transparent), as mentioned by @Lokathor. This falls closer to what currently is done for managing variance in other ways, via PhantomData.

Of course, we can't just use repr(transparent), for the following reasons:

  1. Reference-counting necessitates adding extra stuff alongside your original data, and requiring repr(transparent) would forbid this.
  2. The allocator API also requires extra stuff, assuming the allocator is nonempty, and that also messes with things.

Actually, the allocator API runs amok with this current proposal, too, since it would require that an allocator be zero-sized. If we reframe the proposal from the perspective of unsized types (treating smart pointers as implicitly always sized), this "single field" instead turns into "last field," like the current unsized requirements. I believe that zero-sized types are also not allowed to be after the unsized field of a struct, but we could probably amend this pretty easily.

Maybe we could have some kind of repr(variant) for smart pointers and repr(contravariant) for cells and those could be used instead, requiring that either:

  1. The last field of these structs is something that is also repr(same-variant) in the same type parameter.
  2. The last field of these structs is a value of that type parameter.

Basically ensuring that everything works as you'd expect.


A few footnotes:

  • I am aware that CoerceUnsized is specifically for references/pointers and that a pure impl<T: Unsize<U>> CoerceUnsized<U> for T does not exist, but I'm going to pretend that it does for the sake of simplicity here.
  • Unsized locals are still unstable, so, no one knows what their final form will be. I'm mostly operating on assumptions here.
  • DispatchFromDyn is also very specifically limited to dyn dyspatch (unsurprisingly) and other cases, like being able to pass arrays into unsized slice arguments are completely unaccounted for in the compiler at the moment.
  • I was right about the "last field" requirement for unsized types; adding zero-sized fields after totally break this. (playground link)

@compiler-errors
Copy link
Member

compiler-errors commented May 3, 2024

@clarfonthey: A few things regarding your last comment--

It's not correct to call the relationship between [T; N] and [T] subtyping (which I know you're not doing explicitly, but you are kinda implicitly doing so by mentioning variance), and saying that CoerceUnsize and DispatchFromDyn are about variance is a bit misleading.

Their relationship is called unsizing in Rust, which is an explicit coercion operation inserted into the MIR and involves actually changing the type layout of pointer.

Also, Box<T> is covariant in its T parameter, but implements DispatchFromDyn, so I'm not certain what you mean by it enforcing contravariance in its argument.

DispatchFromDyn really is just a trait that instructs typecheck what's valid so that later codegen can peel off layers of a type to look for the "data" part to pass into the vtable method for, e.g., Rc<Self> or Box<Self>; it has slightly different requirements that try to (somewhat generally) capture what needs to happen when codegenning a dynamic call.1

Also the last field requirement you mention is a limitation of data being Unsized, but not pointers being CoerceUnsized, which are kinda two different operations. That is to say, SmartPointer<T> doesn't need its NonNull<T> field to be its last field, but Struct<[i32; 1]> needs that array located at the end (or as we call in the compiler, the tail) of the struct for it to be unsized. But this is mostly related to the limitation that the only struct field that is not required to be SIzed is the last one.

(disclaimer that i've been too busy to read the actual rfc yet, just wanted to clarify some stuff before people started also responding and a whole thread gets generated based off of false premises)

Footnotes

  1. the way that codegen actually works is why DispatchFromDyn for Box and Rc don't support custom allocators -- this could work, but it would need to piece back together the allocator on the receiver end.2

  2. though, wow, the codegen for DispatchFromDyn is kind... interesting. I actually wonder why it doesn't just rip off the metadata from the ScalarPair and pass just the "sized" version of the data.... 🤔

@clarfonthey
Copy link
Contributor

clarfonthey commented May 3, 2024

Oh, huh, I was under the impression that DispatchFromDyn effectively had to be able to do more than just remove the metadata from a pointer, since like mentioned the presence of extra data on Arc and Rc effectively mandates this, but I guess that theoretically "could" work?

I just assumed that allocators were kind of in the same boat, where they could be present like the counts for references but would be peeled away somehow, also like those.

In terms of my mention of contravariance: I definitely am jumbling up terms here since generally what people mean by variance is about subtyping and supertyping, whereas I'm using it to mean literally varying the type via the unsize and dynamic dispatch operations you describe. It's a sloppy analogy for sure, but the main reason I used it is to point out that it shouldn't be explicitly derived and instead inferred somehow like variance currently is.

I guess in that sense, it's closer to the mathematical definitions of stuff like homomorphisms, where instead of stating that two things have identical properties, you have the existence of operations which can be applied while retaining certain properties. Subtle differences but similar analogies.

@kennytm
Copy link
Member

kennytm commented May 3, 2024

The last-field limitation actually means nothing ABI-wise since the Rust layout allows reordering the fields

use std::{ptr::NonNull, mem::offset_of};

struct MySmartPointer<T: ?Sized> {
    extra: u16,
    interior: NonNull<T>,
}

fn main() {
    type P = MySmartPointer<usize>;
    assert_eq!(offset_of!(P, interior), 0);
    assert_eq!(offset_of!(P, extra), 8);
}

So, to call fn method(self: MySmartPointer<Self>, ...), the DispatchFromDyn needs to strip the pointer-metadata in the middle of the struct anyway.

MySmartPointer<T>
    +---+---+---+---+---+---+---+---+---+---+
    | interior                      | extra |
    +---+---+---+---+---+---+---+---+---+---+

            |                       ^
            |                       |
        CoerceUnsized       DispatchFromDyn
            |                       |
            v                       |

    +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
    | interior as *const ()         | ptr::metadata(interior)       | extra |
    +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
MySmartPointer<dyn Trait>

However, since CoerceUnsized can perfectly insert the metadata into the middle of struct, I don't see any theoretical reason why the reverse operation in DispatchFromDyn can't be done. IMO the pointer-shape requirement is just an artificial limitation of the current rustc implementation, and once lifted the two traits DispatchFromDyn and CoerceUnsized can be made equivalent.

@programmerjake
Copy link
Member

The last-field limitation actually means nothing ABI-wise since the Rust layout allows reordering the fields

the Rust compiler is what chooses what order the fields are in, which also means the Rust compiler can always leave the unsizable field at the end, as I'm guessing it currently does. This means conversion from/to dyn Trait doesn't need to insert/remove the vtable in the middle and move everything after, and that conversion from dyn Trait back to the underlying type can just use a prefix of the struct, which in some cases may mean just changing pointer types.

@Darksonn
Copy link
Contributor Author

Darksonn commented May 3, 2024

What these traits really are about is ensuring proper variance, which is especially weird since variance is usually only exposed to programmers in Rust through weird mechanisms like PhantomData. Ultimately:

  • CoerceUnsized lets you declare a type as covariant, and varying the type this way makes it more general. For example, this lets you vary a mutable reference into an immutable reference or an array into a slice.
  • DispatchFromDyn lets you declare a type as contravariant, and varying the type this way makes it more specific. This would be the opposite, like varying a slice into an array or a trait object into a concrete type.

I can see how CoerceUnsized is a form of covariance, though as @compiler-errors mentions, it is not the same subtyping relationship as the one with which we usually use the word "covariance" with in Rust.

But I disagree with calling DispatchFromDyn contravariance. When something is contravariant, you must be able to coerce it to any subtype. When you have a fn(&'short T), you can cast that to fn(&'long T) for any lifetime 'long that is longer than 'short. But in the case of DispatchFromDyn, we can't do that. It's not up to you whether you want to cast your Box<dyn MyTrait> to Box<MyFirstStruct> or Box<MySecondStruct>. Only one of those conversions can be valid, but contravariance means that both must be okay.

Copy link
Contributor

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to use the fully qualified path of Sized for U? For T the prelude is used.

text/3621-derive-smart-pointer.md Outdated Show resolved Hide resolved
text/3621-derive-smart-pointer.md Outdated Show resolved Hide resolved
@kennytm
Copy link
Member

kennytm commented May 3, 2024

@programmerjake

the Rust compiler is what chooses what order the fields are in, which also means the Rust compiler can always leave the unsizable field at the end, as I'm guessing it currently does.

My example code in #3621 (comment) already showed it does not. We are not talking about maybe-unsized struct tail here, but a pointer to a maybe-unsized type which can appear anywhere.

struct X<T: ?Sized> {
    extra: u16,
    tail: T,  // yes, this field is guaranteed to be at the end of X<T>
}

/* layout of X<T>:

    +---------------+
0   | extra         |
    +---------------+
2   |x(padding)x x x|
    | x x x x x x x |
4   |x x x x x x x x|
    | x x x x x x x |
6   |x x x x x x x x|
    +---------------+
8   | tail          |
    | (*actual      |
A   |   offset      |
    |   depends on  |
C   |   align_of T) |
    |               |
    :               :

*/

struct P<T: ?Sized> {
    extra: u16,
    ptr: *const T, // note that it is a pointer here, there is no guarantee about offset of `ptr` in P<T>
}

/* layout of P<T>:

    +---------------+
0   | ptr           |
    |             ~~~~~~~~~~~>  +---------------+
2   |               |           | *ptr          |
    |               |           |               |
4   |               |           :               :
    |               |
6   |               |
    +---------------+
8   | extra         |
    +---------------+
A   |x(padding)x x x|
    | x x x x x x x |
C   |x x x x x x x x|
    | x x x x x x x |
E   |x x x x x x x x|
    +---------------+

*/

This means conversion from/to dyn Trait doesn't need to insert/remove the vtable in the middle and move everything after

Coercion from P<T> to P<dyn Trait> is already inserting a vtable in the middle of the struct (next to the ptr).


Whenever a `self: MySmartPointer<Self>` method is called on a trait object, the
compiler will convert from `MySmartPointer<dyn MyTrait>` to
`MySmartPointer<MyStruct>` using something similar to a transmute. Because of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually worse than a transmute of these values, we're effectively transmuting the function pointer. This means the two types do not just have to be layout compatible, they have to be function call ABI compatible. That's also why allowing extra fields in these types is pretty difficult.

@RalfJung
Copy link
Member

RalfJung commented May 3, 2024

I don't think there should be a hard requirement that smart pointers be transparent.

We actually currently rely on them being the equivalent of transparent. The only reason DispatchFromDyn does not just check the repr is that we want it to be implemented on Box, and Box cannot be repr(transparent) because of the allocator -- but DispatchFromDyn is only implemented for Box<_, Global> which is effectively repr(transparent).

The way we call arbitrary-self receiver functions via vtables relies on repr(transparent) -- specifically, it relies on ABI compatibility. There's a subtle invariant here that all instances of repr(Rust) types that follow the repr(transparent) rules are indeed ABI-compatible to their one non-1-ZST field even if the repr(transparent) attribute is not present. I hope all our ABI adjustment logic ensures this invariant (and we have tests covering the basics), but this is easy to get wrong. This would be much easier if we could rely on repr(transparent)...

@clarfonthey
Copy link
Contributor

I don't think there should be a hard requirement that smart pointers be transparent.

We actually currently rely on them being the equivalent of transparent. The only reason DispatchFromDyn does not just check the repr is that we want it to be implemented on Box, and Box cannot be repr(transparent) because of the allocator -- but DispatchFromDyn is only implemented for Box<_, Global> which is effectively repr(transparent).

The way we call arbitrary-self receiver functions via vtables relies on repr(transparent) -- specifically, it relies on ABI compatibility. There's a subtle invariant here that all instances of repr(Rust) types that follow the repr(transparent) rules are indeed ABI-compatible to their one non-1-ZST field even if the repr(transparent) attribute is not present. I hope all our ABI adjustment logic ensures this invariant (and we have tests covering the basics), but this is easy to get wrong. This would be much easier if we could rely on repr(transparent)...

Is there any plan to make this work with allocators at all, or are we basically stuck with this design for the foreseeable future? Since as I mentioned, the current RFC proposal would also be unable to do anything with non-zero-sized allocators for reasons mentioned. I'd guess that kernel development would at best be dealing with ZSTs anyway since storing an extra pointer or something like it would be undesirable for performance reasons, but it's still a question that feels worth asking.

What I'd imagine would happen here is the fields for the allocator would always be after the pointee to ensure that you can still cast the pointer and have it Just Work, but that requires a bit of compiler help.

@RalfJung
Copy link
Member

RalfJung commented May 3, 2024

Extending DispatchFromDyn to be able to cover more cases (i.e., to handle types that are not just newtyped pointers) is a non-trivial topic on its own, I'd rather not derail this RFC by going there.

@davidhewitt
Copy link
Contributor

One question I have is about FFI smart pointers, which were one of the key use cases for arbitrary self types.

If I understand correctly, these smart pointers cannot easily be e.g. CppRef<dyn MyTrait> or Py<dyn MyTrait> because the pointee is always a thin pointer from FFI so there is no space for the dyn metadata.

It seems a little awkward to call this derive SmartPointer if an important category of smart pointers cannot implement it.

Is there perhaps a way where the FFI smart pointers could be made compatible with this? By having dyn metadata (manually?) placed in a second field, perhaps?

@Darksonn
Copy link
Contributor Author

Darksonn commented May 4, 2024

@davidhewitt I think you underestimate the extent to which something like CppRef could support this today. I'm not so familiar with the CppRef type, but the kernel also has a smart pointer for FFI. It's called ARef<T>, where T is some C struct with refcounting built in.

pub struct ARef<T: AlwaysRefCounted> {
    ptr: NonNull<T>,
    _p: PhantomData<T>,
}

pub unsafe trait AlwaysRefCounted {
    fn inc_ref(&self);
    unsafe fn dec_ref(obj: NonNull<Self>);
}

Making this work with ARef<dyn MyTrait> probably requires MyTrait to be a sub-trait of AlwaysRefCounted, so AlwaysRefCounted must be object safe. It isn't right now because of the raw pointer argument, but it could be made object safe if we wrap NonNull in a #[derive(SmartPointer)] struct.

All that said, mixing FFI and dyn Trait isn't something the Linux Kernel needs right now. But I think it would work.

@kennytm
Copy link
Member

kennytm commented Jun 18, 2024

Sure, but adding #[repr(transparent)] to an existing type, adding a defaulted trait parameter (A = Global), and implementing a trait (#[derive(SmartPointer)]) are all minor changes; removing #[repr(transparent)] after it is decided "the requirement can always be lifted" is a breaking change.

@tmandry
Copy link
Member

tmandry commented Jun 19, 2024

This RFC is clearly the result of a lot of thorough discussion and iteration. It anticipated many of my questions with better answers than I expected. Thank you for pushing it forward, @Darksonn, and to the many others who contributed.

After reading it I agree that SmartPointer is a name that promises too much for a type that isn't required to implement either Deref or Receiver.

On the #[repr(transparent)] question I'm not sure. I think we should either defer to the safety requirements of DispatchFromDyn or update those accordingly.

@rfcbot reviewed

@Darksonn
Copy link
Contributor Author

I do not mind making it require repr(transparent) but the actual implementation of that is probably somewhat cumbersome. The existing requirements about repr aren't verified syntactically, but by the compiler in it's built in checks for implementations of DispatchFromDyn. However, we can't add the repr(transparent) check there because that wouldn't work for some standard library types, so we would probably need to add a new unstable AssertTransparent trait with a similar verification and emit an implementation of that.

@safinaskar
Copy link

Until now #[derive(Foo)] derived trait named Foo and only that. So this feature should not be named #[derive(SmartPointer)]. This will cause confusion. Users will expect that trait SmartPointer will be derived. So, please name language construct as #[smart_pointer] or something similar

@tmandry
Copy link
Member

tmandry commented Jun 21, 2024

I agree that the RFC should mention the point about the derive not matching any trait name (either as a drawback, or in the unresolved questions by expanding the name bikeshed to include the possibility of a regular attribute).

While this would be a first for the standard library, it matches the way user-defined derives work (matching the derive with a trait name is by convention only) and it's only meant to be used by advanced users. So I don't think we have to be too careful about violating their expectations.

@Darksonn
Copy link
Contributor Author

That's already mentioned in the drawbacks section of the rfc.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 24, 2024
… r=davidtwco

SmartPointer derive-macro

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->

Possibly replacing rust-lang#123472 for continued upkeep of the proposal rust-lang/rfcs#3621 and implementation of the tracking issue rust-lang#123430.

cc `@Darksonn` `@wedsonaf`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 24, 2024
Rollup merge of rust-lang#125575 - dingxiangfei2009:derive-smart-ptr, r=davidtwco

SmartPointer derive-macro

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->

Possibly replacing rust-lang#123472 for continued upkeep of the proposal rust-lang/rfcs#3621 and implementation of the tracking issue rust-lang#123430.

cc `@Darksonn` `@wedsonaf`
@Darksonn
Copy link
Contributor Author

@scottmcm and @pnkfelix please review the latest commit.

@pnkfelix
Copy link
Member

@rfcbot reviewed

@scottmcm
Copy link
Member

@rfcbot resolve require-repr-transparent

I don't have strong feelings regarding whether the macro should check for it syntactically or use an internal marker trait. I'm happy to leave how the requirement is checked up to compiler folks in the implementation.

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jun 27, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 27, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jun 27, 2024
@tmccombs
Copy link

tmccombs commented Jul 5, 2024

The current state of this RFC is a little confusing on the requirement of #[repr(transparent)].

The "Input Requirements" section lists that it must be transparent as a requirement, but non of the examples include a repr attribute on the structs. And that requirement isn't mentioned in the "Requirements for using the macro" section in the guide.

It also isn't very clear why the AssertReprTransparent trait is needed.

@matthieu-m
Copy link

CustomPointer

I would recommend naming the derive CustomPointer instead of SmartPointer, since it seems it could be used with any custom pointers, not only smart pointers.

CustomPointer(T)

Mayhaps the syntax to specify the pointee type should be #[derive(CustomPointer(T))] or #[derive(CustomPointer = "T")] rather than requiring any extra attribute?

It would avoid collision with any other derive/macro.

Truly Custom Pointers

I apologize if I missed it in the RFC, is an inner pointer (somewhere) necessary for this trait to work?

It sometimes is useful to create "pointer-like" objects which internally use an offset, either for compression (u32) or to be able to shared them with other processes (shared memory, mapped files, etc...).

It's not essentially -- especially if we consider the trait a stop gap measure -- but may be worth mentioning.

@bjorn3
Copy link
Member

bjorn3 commented Jul 5, 2024

I apologize if I missed it in the RFC, is an inner pointer (somewhere) necessary for this trait to work?

Yes, both CoerceUnsized and DispatchFromDyn directly modify the inner pointer without going through Deref. The former attaches metadata to a thin pointer, turning it into a fat pointer. The latter reads the metadata for a fat pointer to get the method to call and then strips the metadata to turn it back into a thin pointer.

@jdahlstrom
Copy link

jdahlstrom commented Jul 5, 2024

Jumping onto the bikeshed train, I'd like to note that the spelling "ptr" is far more common in std than "pointer". In particular, all items of the form "foo pointer" are named FooPtr.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Jul 7, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Jul 7, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

And add #[repr(transparent)] to the examples.
@Darksonn
Copy link
Contributor Author

Darksonn commented Jul 9, 2024

@tmccombs Thanks. I removed the unnecessary details about how #[repr(transparent)] will be enforced.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huzzah! The @rust-lang/lang team has decided to accept this RFC. Please follow along on rust-lang/rust#123430.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.