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

Stabilize derive(CoercePointee) #133820

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dingxiangfei2009
Copy link
Contributor

@dingxiangfei2009 dingxiangfei2009 commented Dec 3, 2024

What is being proposed for stabilization

This stabilization report concerns the macro specified by RFC3621, now called CoercePointee.

This macro enables the derivation of trait implementation of Unsize, CoerceUnsize and DispatchFromDyn traits, both of which are unstable at the moment, under a well-defined and structured schema, so that the users who would like to allow their custom type to admit unsizing coercion and use of dyn DST in one of its generic type parameter in a safe way. The greatest use case of this macro is to allow users to equip their types with this behaviour without dependence on alloc crate, or when alloc is not available and a custom implementation of pointer-like data structure in the likes of Rc and Arc is highly desirable. The most prominent example is the kernel crate by Rust-for-Linux, which would greatly benefit from this macro due to reduction of boilerplate, and reduce the project's dependence on unstable features behind Unsize, CoerceUnsize and DispatchFromDyn.

In a nutshell, derive(CoercePointee) derives the implementation of CoerceUnsize and DispatchFromDyn traits on the target struct definition with all the necessary trait bounds. It identifies one generic type variable in the generic list of the target struct, that is either uniquely annotated with the #[pointee] attribute or is the only type variable among the generics. This identified generic, which is called source type in this document, will be treated as the target of unsizing operation and dyn trait object dispatch. Correspondingly, the resultant type after the unsizing coercion will be called target type in this document. In case additional trait bounds applies on the source type, the said trait bounds will be migrated to both the source type and the target type in the trait implementation.

Out of necessity of unsizing coercion, the macro requires the struct to adopt the repr(transparent) layout. The only data field in the struct definition, which is required by this layout, should also implement CoerceUnsize and DispatchFromDyn at the moment, or at least conform to requirement of the unsizing coercion and trait object dispatching protocol.

As an example, here is how the generated code would look like after expanding the macro. The following is a user source that invokes this macro.

pub trait MyTrait<T: ?Sized> {}

#[derive(core::marker::CoercePointee)]
#[repr(transparent)]
pub struct MyPointer2<'a, Y, Z: MyTrait<T>, #[pointee] T: ?Sized + MyTrait<T>, X: MyTrait<T> = ()>
where
    Y: MyTrait<T>,
{
    data: &'a mut T,
    x: core::marker::PhantomData<X>,
}

This is the expansion result. The source type T after the unsizing coercion is assumed to be __S and the trait bounds requested by the original definition are migrated for __S as well throughtout.

#[repr(transparent)]
pub struct MyPointer2<
    'a, Y, Z: MyTrait<T>,
    #[pointee] T: ?Sized + MyTrait<T>,
    X: MyTrait<T> = ()
>
where Y: MyTrait<T>
{
    data: &'a mut T,
    x: core::marker::PhantomData<X>,
}

#[automatically_derived]
impl<
    'a, Y,
    Z: MyTrait<T> + MyTrait<__S>,
    T: ?Sized + MyTrait<T> + ::core::marker::Unsize<__S>,
    __S: ?Sized + MyTrait<__S>,
    X: MyTrait<T> + MyTrait<__S>
>
::core::ops::DispatchFromDyn<MyPointer2<'a, Y, Z, __S, X>> for MyPointer2<'a, Y, Z, T, X>
where Y: MyTrait<T>, Y: MyTrait<__S> {
}

#[automatically_derived]
impl<
    'a, Y,
    Z: MyTrait<T> + MyTrait<__S>,
    T: ?Sized + MyTrait<T> + ::core::marker::Unsize<__S>,
    __S: ?Sized + MyTrait<__S>,
    X: MyTrait<T> + MyTrait<__S>
>
::core::ops::CoerceUnsized<MyPointer2<'a, Y, Z, __S, X>> for MyPointer2<'a, Y, Z, T, X>
where Y: MyTrait<T>, Y: MyTrait<__S> {
}

Future interaction

This stabilization of this macro does not require the stabilization of any of the unstable traits mentioned. This macro aims at stabilising a subset of features thereof, which has widely proliferated in the ecosystem, without blocking further development of DSTs and unsizing coercion. In case of change in the design of these language features, as long as basic provision of the coercion operation remains the same, the macro should be updated to use the new facility inside its implementation when necessary, to preserve only the semantics and capabilities that this macro exposes users to.

Tracking:

cc @rust-lang/lang

@rustbot
Copy link
Collaborator

rustbot commented Dec 3, 2024

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 3, 2024
@dingxiangfei2009 dingxiangfei2009 force-pushed the stabilize-coerce-pointee branch from 8baca16 to 7865ddb Compare December 3, 2024 21:16
@rust-log-analyzer

This comment has been minimized.

@joboet joboet added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 4, 2024
@dingxiangfei2009 dingxiangfei2009 changed the title Stabilize derive(CoercePointee) Stabilize derive(CoercePointee) Dec 4, 2024
@dingxiangfei2009 dingxiangfei2009 force-pushed the stabilize-coerce-pointee branch from 7865ddb to 208e5bb Compare December 4, 2024 15:58
@dingxiangfei2009
Copy link
Contributor Author

@rustbot ready

  • Documentation is updated to reflect the requirements as laid out by the RFC

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 4, 2024
@rust-log-analyzer

This comment has been minimized.

@dingxiangfei2009 dingxiangfei2009 force-pushed the stabilize-coerce-pointee branch from 208e5bb to d9f6c00 Compare December 4, 2024 16:52
@traviscross traviscross added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 4, 2024
@dingxiangfei2009 dingxiangfei2009 marked this pull request as ready for review December 4, 2024 18:32
@dingxiangfei2009
Copy link
Contributor Author

@rustbot label +F-derive_smart_pointer

@rustbot rustbot added the F-derive_coerce_pointee Feature: RFC 3621's oft-renamed implementation label Dec 4, 2024
#[rustc_builtin_macro(CoercePointee, attributes(pointee))]
#[allow_internal_unstable(dispatch_from_dyn, coerce_unsized, unsize)]
#[unstable(feature = "derive_coerce_pointee", issue = "123430")]
#[cfg(not(bootstrap))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question:

Do we need to condition on bootstrap? No standard library item depends on this macro yet.

@Darksonn
Copy link
Contributor

Darksonn commented Dec 5, 2024

The RFC for this feature said that it depends on the arbitrary self types feature, but it is not a full blocker to stabilizing this feature. There are two things that the macro enables:

The first thing is coercions from MySmartPtr<MyType> to MySmartPtr<dyn MyTrait>.

The second thing is making traits such as this one object safe:

trait MyTrait {
    fn func(self: MySmartPtr<Self>);
}

Without arbitrary self types, it's not legal to declare this trait because MySmartPtr is not a valid self type. However, the coercions from MySmartPtr<MyType> to MySmartPtr<dyn MyTrait> still work, and custom smart pointers are still useful, e.g. given an MyArc<dyn MyTrait> that derefs to &dyn MyTrait, you can call trait methods on MyTrait that take &self just fine.

So if we stabilize the macro now, then we gain the ability to make coercions and call trait methods via deref, but we don't gain the ability to declare traits that take the smart pointer without a deref coercion. We would gain that ability once arbitrary self types are stabilized.

@traviscross
Copy link
Contributor

@rfcbot fcp merge

Thanks @dingxiangfei2009. We talked about this in our triage call today. Let's ship it.

@rfcbot
Copy link

rfcbot commented Dec 11, 2024

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

No concerns currently listed.

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
Copy link
Contributor

@compiler-errors my preferred way to manage that is not unchecking my box but filing a concern

@compiler-errors
Copy link
Member

Yeah, at this point @lcnr has already begun to do a good job at characterizing the types-level surface of what is being stabilized here, and funny enough that they found an interesting quirk in the process :D

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 7, 2025

@rfcbot concern not properly checking repr(transparent) #135206

@workingjubilee workingjubilee added the S-waiting-on-concerns Status: Awaiting concerns to be addressed by the author label Jan 7, 2025
@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 7, 2025

DispatchFromDyn doesn't require the type parameter to only be used in one field and potentially phantomdata, instead it requires one field and arbitarily many ZSTs playground. This in theory means that dispatch from dyn impls can result in effectively arbitrary transmutes from MyZST<dyn Trait> to MyZST<Foo>.

In practice I don't think this is exploitable, regardless I don't think we should stabilize the ability for semi-user-written impls or trait bounds to be written involving this trait until the builtin checks are brought in line with CoerceUnsized.

edit: thanks to @steffahn for pointing this out

@rfcbot concern DispatchFromDyn builtin checks are weaker than CoerceUnsized

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 7, 2025

The CoerceUnsized builtin checks that the parameter being unsized is only used in PhantomDatas and one field does not handle aliases and just directly checks if the Ty is a PhantomData. This will have to be fixed before stabilizing lazy_type_alias so we may as well just handle it from the beginning. See the following two playground examples for things that don't compile but probably ought to before we stabilize any form of user written impls for the trait:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=35e9d30ae8a278601de5d8a45bc40417
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=850f363579a53b61b88c6b02a502c31b

@rfcbot concern CoerceUnsized builtin checks do not handle aliases

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 7, 2025

I've moved some of the concerns out to separate issues where people can assign themselves and Fixes #... them

@bors
Copy link
Contributor

bors commented Jan 9, 2025

☔ The latest upstream changes (presumably #135286) made this pull request unmergeable. Please resolve the merge conflicts.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 18, 2025

@rfcbot resolved DispatchFromDyn builtin checks are weaker than CoerceUnsized
@rfcbot resolved CoerceUnsized builtin checks do not handle aliases

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 20, 2025

I dont know who the "owners" of arbitrary self types are but I would like for them to sign off on this to some degree as this does stabilize additional method receivers and I can't speak to the status of the implementation of that feature.

@rfcbot concern arbitrary self types impl status

From a types POV though I've spent a while thinking over the traits involved and feel somewhat convinced that DispatchFromDyn is conceptually fine especially with the restrictions imposed by CoercePointee. I will write something more about this on the tracking issue though, or here later as other people might find the reasoning useful

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 20, 2025

cc @adetaylor from the tracking issues of arbitrary self types it seems like you've been rather with arbitrary self types? see the above comment

@adetaylor
Copy link
Contributor

Yep, I've been the one driving arbitrary self types. I'm in the early stages of drafting a stabilization report for it - hopefully I'll have written that up in about a week, but I've got a few experiments I need to run first, so it might be a wee bit longer. I'll read through here as well in the next few days and add any comments/questions I've got.

@Darksonn
Copy link
Contributor

as this does stabilize additional method receivers and I can't speak to the status of the implementation of that feature.

This feature doesn't have any effect on what method receivers are possible. Adding #[derive(CoercePointee)] to MySmartPointer changes whether traits containing self: MySmartPointer<Self> are object safe or not if it's legal to declare such a trait. However, the macro has no influence on whether it's legal to declare such a trait.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 20, 2025

Ah okay, I did not realise that dispatch from dyn was completely irrelevant to this stabilization given that it had been previously talked about and calling trait methods on these smart pointers had been thrown around in code examples (and the description of the PR explicitly showing that DispatchFromDyn had been derived)

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 20, 2025

@rfcbot resolved concern arbitrary self types impl status

@RalfJung
Copy link
Member

This is about object safety, so doesn't that mean that DispatchFromDyn is indeed relevant?

@steffahn
Copy link
Member

@RalfJung Current dyn-compatibility definitions also require all (non-Self: Sized) trait methods to have a self argument, and dispatch to happen through the self argument. Otherwise, it’s never dyn-compatible anyway.

This means that without arbitrary_self_types, the question of whether DispatchFromDyn is implemented or not is only relevant for types supported as self-types anyway:

If the type of the self parameter is specified, it is limited to types resolving to one generated by the following grammar (where 'lt denotes some arbitrary lifetime):

P = &'lt S | &'lt mut S | Box<S> | Rc<S> | Arc<S> | Pin<P>
S = Self | P

(according to the reference)

So custom smart pointers are never relevant. (The type Self becoming a custom smart pointer in trait impls is irrelevant, too, because dyn-compatibility is always determined in the abstract, so Self has the abstract meaning of “the formal Self parameter”, not a synonym of a specific type.)

@Darksonn
Copy link
Contributor

I mean, arbitrary self types are relevant because you cannot use the DispatchFromDyn part of #[derive(CoercePointee)] unless you can declare a trait with self: MySmartPtr<Self>, which you can't do without arbitrary self types. But you can still do this without arbitrary self types:

trait MyTrait {
    fn foo(&mut self);
}

fn foo(arc: MySmartPtr<dyn MyTrait>) {
    arc.foo();
}

foo(MySmartPtr::new(MyStruct::new()));

@joboet
Copy link
Member

joboet commented Jan 24, 2025

I'm not the right reviewer for this (just cleaning up my review queue, this is waiting on FCP anyway).
r? types

@joboet
Copy link
Member

joboet commented Jan 24, 2025

@rustbot ping
Does this bot dream of electronic sheep?

@rustbot

This comment was marked as off-topic.

@rustbot rustbot assigned compiler-errors and unassigned joboet Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-derive_coerce_pointee Feature: RFC 3621's oft-renamed implementation I-lang-nominated Nominated for discussion during a lang team meeting. I-types-nominated Nominated for discussion during a types team meeting. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-concerns Status: Awaiting concerns to be addressed by the author S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.