8000 Introduce CoercePointeeValidated for coherence checks at typeck stage by dingxiangfei2009 · Pull Request #136107 · rust-lang/rust · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 6 commits into from
Feb 11, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
introduce CoercePointeeWellformed for coherence checks at typeck stage
  • Loading branch information
dingxiangfei2009 committed Feb 9, 2025
commit de405dcb8fbcd0add1e60c75800dac9b8fbe4884
64 changes: 55 additions & 9 deletions compiler/rustc_builtin_macros/src/deriving/coerce_pointee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ use ast::ptr::P;
use rustc_ast::mut_visit::MutVisitor;
use rustc_ast::visit::BoundKind;
use rustc_ast::{
self as ast, GenericArg, GenericBound, GenericParamKind, ItemKind, MetaItem,
self as ast, GenericArg, GenericBound, GenericParamKind, Generics, ItemKind, MetaItem,
TraitBoundModifiers, VariantData, WherePredicate,
};
use rustc_attr_parsing as attr;
use rustc_data_structures::flat_map_in_place::FlatMapInPlace;
use rustc_errors::E0802;
use rustc_expand::base::{Annotatable, ExtCtxt};
use rustc_macros::Diagnostic;
use rustc_span::{Ident, Span, Symbol, sym};
Expand Down Expand Up @@ -88,8 +89,7 @@ pub(crate) fn expand_deriving_coerce_pointee(
} else {
let mut pointees = type_params
.iter()
.filter_map(|&(idx, span, is_pointee)| is_pointee.then_some((idx, span)))
.fuse();
.filter_map(|&(idx, span, is_pointee)| is_pointee.then_some((idx, span)));
match (pointees.next(), pointees.next()) {
(Some((idx, _span)), None) => idx,
(None, _) => {
Expand All @@ -110,6 +110,52 @@ pub(crate) fn expand_deriving_coerce_pointee(
// Declare helper function that adds implementation blocks.
// FIXME(dingxiangfei2009): Investigate the set of attributes on target struct to be propagated to impls
let attrs = thin_vec![cx.attr_word(sym::automatically_derived, span),];
// # Wellformed-ness assertion
Copy link
Member

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.

{
let trait_path =
cx.path_all(span, true, path!(span, core::marker::CoercePointeeWellformed), vec![]);
let trait_ref = cx.trait_ref(trait_path);
push(Annotatable::Item(
cx.item(
span,
Ident::empty(),
attrs.clone(),
ast::ItemKind::Impl(Box::new(ast::Impl {
safety: ast::Safety::Default,
polarity: ast::ImplPolarity::Positive,
defaultness: ast::Defaultness::Final,
constness: ast::Const::No,
generics: Generics {
params: generics
.params
.iter()
.map(|p| match &p.kind {
GenericParamKind::Lifetime => {
cx.lifetime_param(p.span(), p.ident, p.bounds.clone())
}
GenericParamKind::Type { default: _ } => {
cx.typaram(p.span(), p.ident, p.bounds.clone(), None)
}
GenericParamKind::Const { ty, kw_span: _, default: _ } => cx
.const_param(
p.span(),
p.ident,
p.bounds.clone(),
ty.clone(),
None,
),
})
.collect(),
where_clause: generics.where_clause.clone(),
span: generics.span,
},
of_trait: Some(trait_ref),
self_ty: self_type.clone(),
items: ThinVec::new(),
})),
),
));
}
let mut add_impl_block = |generics, trait_symbol, trait_args| {
let mut parts = path!(span, core::ops);
parts.push(Ident::new(trait_symbol, span));
Expand Down Expand Up @@ -430,35 +476,35 @@ impl<'a, 'b> rustc_ast::visit::Visitor<'a> for AlwaysErrorOnGenericParam<'a, 'b>
}

#[derive(Diagnostic)]
#[diag(builtin_macros_coerce_pointee_requires_transparent)]
#[diag(builtin_macros_coerce_pointee_requires_transparent, code = E0802)]
struct RequireTransparent {
#[primary_span]
span: Span,
}

#[derive(Diagnostic)]
#[diag(builtin_macros_coerce_pointee_requires_one_field)]
#[diag(builtin_macros_coerce_pointee_requires_one_field, code = E0802)]
struct RequireOneField {
#[primary_span]
span: Span,
}

#[derive(Diagnostic)]
#[diag(builtin_macros_coerce_pointee_requires_one_generic)]
#[diag(builtin_macros_coerce_pointee_requires_one_generic, code = E0802)]
struct RequireOneGeneric {
#[primary_span]
span: Span,
}

#[derive(Diagnostic)]
#[diag(builtin_macros_coerce_pointee_requires_one_pointee)]
#[diag(builtin_macros_coerce_pointee_requires_one_pointee, code = E0802)]
struct RequireOnePointee {
#[primary_span]
8000 span: Span,
}

#[derive(Diagnostic)]
#[diag(builtin_macros_coerce_pointee_too_many_pointees)]
#[diag(builtin_macros_coerce_pointee_too_many_pointees, code = E0802)]
struct TooManyPointees {
#[primary_span]
one: Span,
Expand All @@ -467,7 +513,7 @@ struct TooManyPointees {
}

#[derive(Diagnostic)]
#[diag(builtin_macros_coerce_pointee_requires_maybe_sized)]
#[diag(builtin_macros_coerce_pointee_requires_maybe_sized, code = E0802)]
struct RequiresMaybeSized {
#[primary_span]
span: Span,
Expand Down
80 changes: 80 additions & 0 deletions compiler/rustc_error_codes/src/error_codes/E0802.md
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`.
1 change: 1 addition & 0 deletions compiler/rustc_error_codes/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ E0798: 0798,
E0799: 0799,
E0800: 0800,
E0801: 0801,
E0802: 0802,
);
)
}
Expand Down
39 changes: 38 additions & 1 deletion compiler/rustc_expand/src/build.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use rustc_ast::ptr::P;
use rustc_ast::util::literal;
use rustc_ast::{
self as ast, AttrVec, BlockCheckMode, Expr, LocalKind, MatchKind, PatKind, UnOp, attr, token,
self as ast, AnonConst, AttrVec, BlockCheckMode, Expr, LocalKind, MatchKind, PatKind, UnOp,
attr, token,
};
use rustc_span::source_map::Spanned;
use rustc_span::{DUMMY_SP, Ident, Span, Symbol, kw, sym};
Expand Down Expand Up @@ -138,6 +139,42 @@ impl<'a> ExtCtxt<'a> {
}
}

pub fn lifetime_param(
&self,
span: Span,
ident: Ident,
bounds: ast::GenericBounds,
) -> ast::GenericParam {
ast::GenericParam {
id: ast::DUMMY_NODE_ID,
ident: ident.with_span_pos(span),
attrs: AttrVec::new(),
bounds,
is_placeholder: false,
kind: ast::GenericParamKind::Lifetime,
colon_span: None,
}
}

pub fn const_param(
&self,
span: Span,
ident: Ident,
bounds: ast::GenericBounds,
ty: P<ast::Ty>,
default: Option<AnonConst>,
) -> ast::GenericParam {
ast::GenericParam {
id: ast::DUMMY_NODE_ID,
ident: ident.with_span_pos(span),
attrs: AttrVec::new(),
bounds,
is_placeholder: false,
kind: ast::GenericParamKind::Const { ty, kw_span: DUMMY_SP, default },
colon_span: None,
}
}

pub fn trait_ref(&self, path: ast::Path) -> ast::TraitRef {
ast::TraitRef { path, ref_id: ast::DUMMY_NODE_ID }
}
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_hir/src/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,8 @@ language_item_table! {

PointerLike, sym::pointer_like, pointer_like, Target::Trait, GenericRequirement::Exact(0);

CoercePointeeWellformed, sym::coerce_pointee_wellformed, coerce_pointee_wellformed_trait, Target::Trait, GenericRequirement::Exact(0);

ConstParamTy, sym::const_param_ty, const_param_ty_trait, Target::Trait, GenericRequirement::Exact(0);
UnsizedConstParamTy, sym::unsized_const_param_ty, unsized_const_param_ty_trait, Target::Trait, GenericRequirement::Exact(0);

Expand Down Expand Up @@ -429,9 +431,13 @@ language_item_table! {
ContractCheckRequires, sym::contract_check_requires, contract_check_requires_fn, Target::Fn, GenericRequirement::None;
}

/// The requirement imposed on the generics of a lang item
pub enum GenericRequirement {
/// No restriction on the generics
None,
/// A minimum number of generics that is demanded on a lang item
Minimum(usize),
/// The number of generics must match precisely as stipulated
Exact(usize),
}

Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_hir_analysis/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,12 @@ hir_analysis_cmse_output_stack_spill =
.note1 = functions with the `"{$abi_name}"` ABI must pass their result via the available return registers
.note2 = the result must either be a (transparently wrapped) i64, u64 or f64, or be at most 4 bytes in size

hir_analysis_coerce_pointee_not_concrete_ty = `derive(CoercePointee)` is only applicable to `struct`

hir_analysis_coerce_pointee_not_struct = `derive(CoercePointee)` is only applicable to `struct`, instead of `{$kind}`

hir_analysis_coerce_pointee_not_transparent = `derive(CoercePointee)` is only applicable to `struct` with `repr(transparent)` layout

hir_analysis_coerce_unsized_may = the trait `{$trait_name}` may only be implemented for a coercion between structures

hir_analysis_coerce_unsized_multi = implementing the trait `CoerceUnsized` requires multiple coercions
Expand Down
28 changes: 28 additions & 0 deletions compiler/rustc_hir_analysis/src/coherence/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}

Expand Down Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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) };
Copy link
Member

Choose a reason for hiding this comment

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

What is this as_local check?

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.

Copy link
Member

Choose a reason for hiding this comment

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

Do not use source_span -- the query says:

. This span is meant for dep-tracking rather than diagnostics. It should not be used outside of rustc_middle::hir::source_map.

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(())
}
22 changes: 22 additions & 0 deletions compiler/rustc_hir_analysis/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1180,6 +1180,28 @@ pub(crate) struct DispatchFromDynRepr {
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_coerce_pointee_not_struct, code = E0802)]
pub(crate) struct CoercePointeeNotStruct {
#[primary_span]
pub span: Span,
pub kind: String,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_coerce_pointee_not_concrete_ty, code = E0802)]
pub(crate) struct CoercePointeeNotConcreteType {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_coerce_pointee_not_transparent, code = E0802)]
pub(crate) struct CoercePointeeNotTransparent {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_inherent_ty_outside_relevant, code = E0390)]
#[help]
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ symbols! {
Cleanup,
Clone,
CoercePointee,
CoercePointeeWellformed,
CoerceUnsized,
Command,
ConstParamTy,
Expand Down Expand Up @@ -619,6 +620,7 @@ symbols! {
cmp_partialord_lt,
cmpxchg16b_target_feature,
cmse_nonsecure_entry,
coerce_pointee_wellformed,
coerce_unsized,
cold,
cold_path,
Expand Down
6 changes: 6 additions & 0 deletions library/core/src/marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Copy link
Member

Choose a reason for hiding this comment

The 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 CoercePointeeWellFormed manually. You can follow StructuralPartialEq to see how to gate an internal derive-only trait properly.

#[doc(hidden)]
pub trait CoercePointeeWellformed {}
Loading
0