search

rust-lang / rfcs

RFCs for changes to Rust
https://rust-lang.github.io/rfcs/
Apache License 2.0
5.9k stars 1.56k forks source link

RFC: #[derive(SmartPointer)] #3621

Closed Darksonn closed 2 months ago

Darksonn commented 5 months ago

Use custom smart pointers with trait objects.

Rendered

Tracking issue: https://github.com/rust-lang/rust/issues/123430

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

Lokathor commented 5 months ago

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

joshtriplett commented 5 months ago

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.

clarfonthey commented 5 months ago

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:

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:

compiler-errors commented 5 months ago

@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)

[^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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

kennytm commented 4 months ago

@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 https://github.com/rust-lang/rfcs/pull/3621#issuecomment-2092479531 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).

RalfJung commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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.

davidhewitt commented 4 months ago

I see, I think you might also be able to do something like trait MyTraitRefCounted: MyTrait + AlwaysRefCounted when it's not possible to modify MyTrait directly.

For Py<T> in PyO3 we don't need a trait on T so we don't suffer the same problem. However the pointer we store is always NonNull<PyObject> hence my wondering about where to fit the metadata.

programmerjake commented 4 months ago

For Py<T> in PyO3 we don't need a trait on T so we don't suffer the same problem. However the pointer we store is always NonNull<PyObject> hence my wondering about where to fit the metadata.

I expect you can just store NonNull<T> but it's really always *mut PyObject in disguise, you'd always cast pointer types when creating or accessing the pointer.

clarfonthey commented 4 months ago

I tried typing this up before on my phone but GitHub ate my message. -_-

Basically translating what I was proposing into a more concrete proposal. The tl;dr is that I think that instead of derive(SmartPointer), this should be repr(smart_pointer). The #[pointee] attribute remains the same. Going over a few of the details:

This also converts my nebulous and partially wrong analysis into an actionable proposal.

nikomatsakis commented 4 months ago

I went through the RFC and comments and I have a few thoughts.

On soundness with Pin

It seems to me that...

...the only reason not to do this is if we want derive(SmartPointer) to require that you meet the pin contract -- i.e., we don't want to allow people in the future to have a "coercible pointer" that is not also pinnable. I generally think that we eventually want some simpler way to say "this is a true smart pointer" -- i.e., it is backed by a pointer, it is dynamic dispatch safe, supports pure deref, etc, all in one convenient package, but I think we don't know how to do that yet. So maybe it's fine to start with just the pieces and come back to it.

Zulip discussion here

On naming

On the topic of starting with just the pieces, I don't love the name derive(SmartPointer). It promises quite a lot but delivers quite a bit less. It seems to me that what this is really doing is saying "this is an unsizeable pointer". Perhaps a name like one of the following would be better

...or perhaps in some module other than std::ptr (maybe even not from std, but just core? seems a bit weird, but it would let us keep this "quieter".)

Overall

But the above are really nits. The concept of "let's stabilize SOME way to get unsizing and work out the underlying traits later" I think is very solid. Therefore I am going to proposed FCP...

@rustbot fcp merge

...and get wider @rust-lang/lang attention!

nikomatsakis commented 4 months ago

I went through the RFC and comments and I have a few thoughts.

On soundness with Pin

It seems to me that...

...the only reason not to do this is if we want derive(SmartPointer) to require that you meet the pin contract -- i.e., we don't want to allow people in the future to have a "coercible pointer" that is not also pinnable. I generally think that we eventually want some simpler way to say "this is a true smart pointer" -- i.e., it is backed by a pointer, it is dynamic dispatch safe, supports pure deref, etc, all in one convenient package, but I think we don't know how to do that yet. So maybe it's fine to start with just the pieces and come back to it.

Zulip discussion here

On naming

On the topic of starting with just the pieces, I don't love the name derive(SmartPointer). It promises quite a lot but delivers quite a bit less. It seems to me that what this is really doing is saying "this is an unsizeable pointer". Perhaps a name like one of the following would be better

...or perhaps in some module other than std::ptr (maybe even not from std, but just core? seems a bit weird, but it would let us keep this "quieter".)

Overall

But the above are really nits. The concept of "let's stabilize SOME way to get unsizing and work out the underlying traits later" I think is very solid. Therefore I am going to proposed FCP...

@rfcbot fcp merge

...and get wider @rust-lang/lang attention!

PS, I posted this once before with the wrong bot name. =)

rfcbot commented 4 months ago

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. See this document for info about what commands tagged team members can give me.

nikomatsakis commented 4 months ago

@rfcbot concern name

I propose we rename the derive to something like "unsize pointee". Not sure what. I think this concern can be resolved by just adding an "unresolved concern" to the RFC that says we'll settle on the ultimate name prior to stabilization.

nikomatsakis commented 4 months ago

@rfcbot concern name

I propose we rename the derive to something like "unsize pointee". Not sure what. I think this concern can be resolved by just adding an "unresolved concern" to the RFC that says we'll settle on the ultimate name prior to stabilization.

nikomatsakis commented 4 months ago

@rfcbot resolve name

scottmcm commented 3 months ago

@rfcbot concern require-repr-transparent

The reference section spells out a bunch of requirements that I think should be deduped to referencing an existing feature.

Rather than

  1. The struct must not be #[repr(packed)] or #[repr(C)].
  2. Other than one-aligned, zero-sized fields, the struct must have exactly one field.

I think the requirements should just be

  1. The struct is #[repr(transparent)]
  2. The struct must have at least one field.

(Where that latter one is already implied by the requirement that there's a pointer in it somewhere.)

For layout reasons it sounds like these ought to always be repr(transparent) anyway (so you can assume the layout of MySmartPointer<SizedType>), so might as well insist on it.

Otherwise, 👍

@rfcbot reviewed

pnkfelix commented 3 months ago

i'm with scottmcm regarding repr(transparent).

The RFC itself says it is going to rely on "something similar to a transmute"; that, combined with the requirements that scottmcm already noted (the repr constraints and the single non-trivial field constraints) to me entails that this is already just as constraining as repr(transparent) would be.

If we find a reason later to generalize this construction to work in other contexts, then we can do that then. But right now, trying to avoid mention of repr(transparent) seems like trying to appear like this is a more general-purpose feature than it actually is.

pnkfelix commented 3 months ago

I've posted my opinion regarding what I see as a slight hiccup between #[smart_pointer:...] and #[derive(SmartPointer)].

But I don't feel that opinion strongly enough to frame it as a formal concern.

@rfcbot reviewed

kennytm commented 3 months ago

@scottmcm #[repr(transparent)] won't work for types which are allocator-aware such as Box<T, A> / Rc<T, A> / Arc<T, A> etc. Granted, Box<T, A> does not implement DispatchFromDyn (only Box<T, Global> does), plus allocator_api is still unstable, but you can't conditionally apply #[repr(transparent)] to A = Global or size_of::<A>() == 0 only either.

For this RFC, it means #[derive(SmartPointer)] and allocator support will become mutually exclusive, and unlike other derivable traits you can't manually impl DispatchFromDyn (The underlying traits used by its expansion will remain unstable for now.) so a custom smart pointer can never support allocators?? Removing #[repr(transparent)] is also considered breaking change btw.

RalfJung commented 3 months ago

You can't conditionally derive SmartPointer either though, so I don't think derive(SmartPointer) as proposed can handle Box<T, A>? Custom allocators and DispatchFromDyn are incompatible right now, that's just the reality. As you say, only Box<T, Global> is DispatchFromDyn.

kennytm commented 3 months ago

The issue with forcing #[repr(transparent)] is that the type MyBox<T, A> can't exist at all, even if we only want impl<T> DispatchFromDyn for MyBox<T, Global>.

And if you don't #[repr(transparent)] then dispatching MyBox<dyn Trait, (Global)> to self: MyBox<T, (Global)> can't ever happen, as long as there is no plan to stabilize DispatchFromDyn and #[derive(SmartPointer)] being the only way to implement the trait.

I also think that if the door to impl<T, A> DispatchFromDyn for MyBox<T, A> is not entirely shut let's not jump ahead and enforce a stricter-than-necessary requirement.

RalfJung commented 3 months ago

This RFC already can't handle your MyBox. So no doors are being shut by requiring repr(transparent) in this first version of the derive. If later a new RFC is written that enables some form of conditional SmartPointer, or that allows DispatchFromDyn with non-default allocators, then the requirement can always be lifted.

kennytm commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

Darksonn commented 3 months ago

@scottmcm and @pnkfelix please review the latest commit.

pnkfelix commented 3 months ago

@rfcbot reviewed

scottmcm commented 3 months ago

@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 commented 3 months ago

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

tmccombs commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

Darksonn commented 2 months ago

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