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

CoercePointee doesn't tightly enforce the restriction to #[repr(transparent)]-only wrappers, if other proc-macros are involved #135206

Closed
steffahn opened this issue Jan 7, 2025 · 4 comments · Fixed by #136107
Assignees
Labels
C-bug Category: This is a bug. F-derive_coerce_pointee Feature: RFC 3621's oft-renamed implementation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@steffahn
Copy link
Member

steffahn commented Jan 7, 2025

Define a proc macro that strips the #[repr(transparent)].

#[proc_macro_attribute]
pub fn untransparent(_: TokenStream, item: TokenStream) -> TokenStream {
    let mut i = item.into_iter();
    drop(i.next());
    drop(i.next());
    i.collect()
}

use it

#![feature(derive_coerce_pointee)]
use std::marker::CoercePointee;
use:untransparent;

#[derive(CoercePointee)]
#[untransparent]
#[repr(transparent)]
struct Foo<T: ?Sized>(Box<T>);

compiles successfully, expands to

#![feature(prelude_import)]
#![feature(derive_coerce_pointee)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
use std::marker::CoercePointee;
use::untransparent;
struct Foo<T: ?Sized>(Box<T>);
#[automatically_derived]
impl<
    T: ?Sized + ::core::marker::Unsize<__S>,
    __S: ?Sized,
> ::core::ops::DispatchFromDyn<Foo<__S>> for Foo<T> {}
#[automatically_derived]
impl<
    T: ?Sized + ::core::marker::Unsize<__S>,
    __S: ?Sized,
> ::core::ops::CoerceUnsized<Foo<__S>> for Foo<T> {}

As far as I understand the RFC and intent, the intended stabilization should only stabilize allowing these (CoerceUnsized & DispatchFromDyn impls) to be generated for #[repr(transparent)] types.

@rustbot label F-derive_coerce_pointee

@steffahn steffahn added the C-bug Category: This is a bug. label Jan 7, 2025
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. F-derive_coerce_pointee Feature: RFC 3621's oft-renamed implementation labels Jan 7, 2025
@steffahn steffahn changed the title CoercePointee doesn't effectively enforce the restriction to #[repr(transparent)]-only wrappers CoercePointee doesn't tightly enforce the restriction to #[repr(transparent)]-only wrappers, if other proc-macros are involved Jan 7, 2025
@jieyouxu jieyouxu added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 7, 2025
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 7, 2025
@dingxiangfei2009
Copy link
Contributor

Hey @steffahn and @BoxyUwU ! Thanks for spotting this!

I was thinking the other day. Would it make sense that we enforce the repr(transparent) on the HIR typeck stage? I imagine that ...

  1. we could introduce a concrete but hidden trait CoercePointeeWellformed, like AsyncFnKindHelper. Name subject to scrutinee.
  2. in derive(CoercePointee), we add a impl<..> CoercePointeeWellformed for ...
  3. while checking coherence, we check validity of each impl<..> CoercePointeeWellformed for ... For this issue, it will check the field and shape of the Self type in perspective impls.

Apologies ahead for using wrong terms and please don't hesitate to correct me.

cc @Darksonn

@steffahn
Copy link
Member Author

steffahn commented Jan 15, 2025

My ”dumb idea“ personally here would be: could validation of the DispatchFromDyn (the code that already makes sure that there’s only a single non-ZST field – recently upgraded to single non-PhantomData) impl simply also enforce the #[repr(transparent)] (on the struct) if there’s #[automatically_derived] on it (on the impl)?

@Darksonn
Copy link
Contributor

Checking #[automatically_derived] sounds like it would take fewer lines of code, but it also sounds more hacky, so I'm not sure it's the best approach.

@compiler-errors In the fix you wanted to make for #135217, are you planning to introduce a new CoercePointeeWellFormed trait? If so, then it sounds like there's an opportunity to use the same trait for both checks.

@dingxiangfei2009
Copy link
Contributor

I am experimenting the WF trait since this week. I will report back with results.

@rustbot claim

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 10, 2025
…llformed, r=compiler-errors

Introduce CoercePointeeWellformed for coherence checks at typeck stage

Fix rust-lang#135206

This is the first PR to introduce the "wellformedness" check for `derive(CoercePointee)`.

This patch introduces a new error code to cover all the prerequisites of the said macro. The checks that is enforced with this patch is whether the data is indeed `struct` and whether the layout is set to `repr(transparent)`.

A following series of patch will arrive later to address the following concern.
1. rust-lang#135217 so that we would only admit one single coercion on one type parameter, and leave the rest for future consideration in tandem of development of other coercion rules.
1. Enforcement of data field requirements.

**An open question** is whether there is a good schema to encode the `#[pointee]` as well, so that we could also check if the `#[pointee]` type parameter is indeed `?Sized`.

`@rustbot` label F-derive_coerce_pointee
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 10, 2025
…llformed, r=compiler-errors

Introduce CoercePointeeWellformed for coherence checks at typeck stage

Fix rust-lang#135206

This is the first PR to introduce the "wellformedness" check for `derive(CoercePointee)`.

This patch introduces a new error code to cover all the prerequisites of the said macro. The checks that is enforced with this patch is whether the data is indeed `struct` and whether the layout is set to `repr(transparent)`.

A following series of patch will arrive later to address the following concern.
1. rust-lang#135217 so that we would only admit one single coercion on one type parameter, and leave the rest for future consideration in tandem of development of other coercion rules.
1. Enforcement of data field requirements.

**An open question** is whether there is a good schema to encode the `#[pointee]` as well, so that we could also check if the `#[pointee]` type parameter is indeed `?Sized`.

`@rustbot` label F-derive_coerce_pointee
@bors bors closed this as completed in af3c51d Feb 11, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 11, 2025
Rollup merge of rust-lang#136107 - dingxiangfei2009:coerce-pointee-wellformed, r=compiler-errors

Introduce CoercePointeeWellformed for coherence checks at typeck stage

Fix rust-lang#135206

This is the first PR to introduce the "wellformedness" check for `derive(CoercePointee)`.

This patch introduces a new error code to cover all the prerequisites of the said macro. The checks that is enforced with this patch is whether the data is indeed `struct` and whether the layout is set to `repr(transparent)`.

A following series of patch will arrive later to address the following concern.
1. rust-lang#135217 so that we would only admit one single coercion on one type parameter, and leave the rest for future consideration in tandem of development of other coercion rules.
1. Enforcement of data field requirements.

**An open question** is whether there is a good schema to encode the `#[pointee]` as well, so that we could also check if the `#[pointee]` type parameter is indeed `?Sized`.

``@rustbot`` label F-derive_coerce_pointee
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this issue Mar 11, 2025
…llformed, r=compiler-errors

Introduce CoercePointeeWellformed for coherence checks at typeck stage

Fix rust-lang#135206

This is the first PR to introduce the "wellformedness" check for `derive(CoercePointee)`.

This patch introduces a new error code to cover all the prerequisites of the said macro. The checks that is enforced with this patch is whether the data is indeed `struct` and whether the layout is set to `repr(transparent)`.

A following series of patch will arrive later to address the following concern.
1. rust-lang#135217 so that we would only admit one single coercion on one type parameter, and leave the rest for future consideration in tandem of development of other coercion rules.
1. Enforcement of data field requirements.

**An open question** is whether there is a good schema to encode the `#[pointee]` as well, so that we could also check if the `#[pointee]` type parameter is indeed `?Sized`.

``@rustbot`` label F-derive_coerce_pointee
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this issue Mar 11, 2025
…llformed, r=compiler-errors

Introduce CoercePointeeWellformed for coherence checks at typeck stage

Fix rust-lang#135206

This is the first PR to introduce the "wellformedness" check for `derive(CoercePointee)`.

This patch introduces a new error code to cover all the prerequisites of the said macro. The checks that is enforced with this patch is whether the data is indeed `struct` and whether the layout is set to `repr(transparent)`.

A following series of patch will arrive later to address the following concern.
1. rust-lang#135217 so that we would only admit one single coercion on one type parameter, and leave the rest for future consideration in tandem of development of other coercion rules.
1. Enforcement of data field requirements.

**An open question** is whether there is a good schema to encode the `#[pointee]` as well, so that we could also check if the `#[pointee]` type parameter is indeed `?Sized`.

``@rustbot`` label F-derive_coerce_pointee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. F-derive_coerce_pointee Feature: RFC 3621's oft-renamed implementation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants