-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Introduce CoercePointeeValidated for coherence checks at typeck stage #136107
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
Changes from 1 commit
de405dc
c067324
b943505
1848343
aea5595
17026e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
The target of `derive(CoercePointee)` macro has inadmissible specification for | ||
a meaningful use. | ||
|
||
Erroneous code examples: | ||
|
||
The target data is not a `struct`. | ||
|
||
```compile_fail,E0802 | ||
#[derive(CoercePointee)] | ||
enum NotStruct<'a, T: ?Sized> { | ||
Variant(&'a T), | ||
} | ||
``` | ||
|
||
The target data has a layout that is not transparent, or `repr(transparent)` | ||
in other words. | ||
|
||
```compile_fail,E0802 | ||
#[derive(CoercePointee)] | ||
struct NotTransparent<'a, #[pointee] T: ?Sized> { | ||
ptr: &'a T, | ||
} | ||
``` | ||
|
||
The target data has no data field. | ||
|
||
```compile_fail,E0802 | ||
#[derive(CoercePointee)] | ||
#[repr(transparent)] | ||
struct NoField<'a, #[pointee] T: ?Sized> {} | ||
``` | ||
|
||
The target data is not generic over any data, or has no generic type parameter. | ||
|
||
```compile_fail,E0802 | ||
#[derive(CoercePointee)] | ||
#[repr(transparent)] | ||
struct NoGeneric<'a>(&'a u8); | ||
``` | ||
|
||
The target data has multiple generic type parameters, but none is designated as | ||
a pointee for coercion. | ||
|
||
```compile_fail,E0802 | ||
#[derive(CoercePointee)] | ||
#[repr(transparent)] | ||
struct AmbiguousPointee<'a, T1: ?Sized, T2: ?Sized> { | ||
a: (&'a T1, &'a T2), | ||
} | ||
``` | ||
|
||
The target data has multiple generic type parameters that are designated as | ||
pointees for coercion. | ||
|
||
```compile_fail,E0802 | ||
#[derive(CoercePointee)] | ||
#[repr(transparent)] | ||
struct TooManyPointees< | ||
'a, | ||
#[pointee] A: ?Sized, | ||
#[pointee] B: ?Sized> | ||
((&'a A, &'a B)); | ||
``` | ||
|
||
The type parameter that is designated as a pointee is not marked `?Sized`. | ||
|
||
```compile_fail,E0802 | ||
#[derive(CoercePointee)] | ||
#[repr(transparent)] | ||
struct NoMaybeSized<'a, #[pointee] T> { | ||
ptr: &'a T, | ||
} | ||
``` | ||
|
||
In summary, the `CoercePointee` macro demands the type to be a `struct` that is | ||
generic over at least one type or over more types, one of which is marked with | ||
`#[pointee]`, and has at least one data field and adopts a `repr(transparent)` | ||
layout. | ||
The only generic type or the type marked with `#[pointee]` has to be also | ||
marked as `?Sized`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -545,6 +545,7 @@ E0798: 0798, | |
E0799: 0799, | ||
E0800: 0800, | ||
E0801: 0801, | ||
E0802: 0802, | ||
); | ||
) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,10 @@ pub(super) fn check_trait<'tcx>( | |
checker | ||
.check(lang_items.dispatch_from_dyn_trait(), visit_implementation_of_dispatch_from_dyn)?; | ||
checker.check(lang_items.pointer_like(), visit_implementation_of_pointer_like)?; | ||
checker.check( | ||
lang_items.coerce_pointee_wellformed_trait(), | ||
visit_implementation_of_coerce_pointee_wellformed, | ||
)?; | ||
Ok(()) | ||
} | ||
|
||
|
@@ -782,3 +786,27 @@ fn visit_implementation_of_pointer_like(checker: &Checker<'_>) -> Result<(), Err | |
.with_note(why_disqualified) | ||
.emit()) | ||
} | ||
|
||
fn visit_implementation_of_coerce_pointee_wellformed( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we still doing validation inside of the proc macro? Shouldn't we remove the validation from the proc macro since we are doing it here now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Speaking from another perspective, rejecting the derivation earlier as well means that we can refuse to generate the unnecessary ASTs that later compilation passes needs to process. Given the current state of the unstable features involved, it seems to me that it could probably be better for us to intercept inadmissible uses of the macro so that we would not inadvertently expose the behaviour of those unstable features that we had not committed to yet. I am honestly not sure. This can go either way to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not think we should keep both validation. Please remove it :) |
||
checker: &Checker<'_>, | ||
) -> Result<(), ErrorGuaranteed> { | ||
let tcx = checker.tcx; | ||
let self_ty = tcx.impl_trait_ref(checker.impl_def_id).unwrap().instantiate_identity().self_ty(); | ||
let ty::Adt(def, _args) = self_ty.kind() else { | ||
return Err(tcx.dcx().emit_err(errors::CoercePointeeNotConcreteType { | ||
span: tcx.def_span(checker.impl_def_id), | ||
})); | ||
}; | ||
let did = def.did(); | ||
let span = | ||
if let Some(local) = did.as_local() { tcx.source_span(local) } else { tcx.def_span(did) }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this The def id always should be local, right? if it's not, then it's an incoherent impl and we should probably just delay a bug. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not use
|
||
if !def.is_struct() { | ||
return Err(tcx | ||
.dcx() | ||
.emit_err(errors::CoercePointeeNotStruct { span, kind: def.descr().into() })); | ||
} | ||
if !def.repr().transparent() { | ||
return Err(tcx.dcx().emit_err(errors::CoercePointeeNotTransparent { span })); | ||
} | ||
Ok(()) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1289,3 +1289,9 @@ pub trait FnPtr: Copy + Clone { | |
pub macro CoercePointee($item:item) { | ||
/* compiler built-in */ | ||
} | ||
|
||
#[cfg(not(bootstrap))] | ||
#[lang = "coerce_pointee_wellformed"] | ||
#[unstable(feature = "derive_coerce_pointee", issue = "123430")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be gated behind a different feature gate, since users should never be able to implement |
||
#[doc(hidden)] | ||
pub trait CoercePointeeWellformed {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please elaborate what well-formedness means.
I think you may also want to strongly consider using a different term than "well-formed". We already use the term within the type system for a different meaning. I think it's more correct to call this something like "DeriveValidity" or something, to make it clear that we use this impl to validate the correcness of the derive.