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

Redact CoercePointee target type #136796

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
13 changes: 13 additions & 0 deletions compiler/rustc_hir_analysis/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,19 @@ hir_analysis_cmse_output_stack_spill =
.note1 = functions with the `{$abi}` 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_cannot_coerce_unsize = `{$ty}` cannot be coerced to an unsized type
.note = `derive(CoercePointee)` demands that `{$ty}` can be coerced to an unsized type
Copy link
Member

Choose a reason for hiding this comment

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

This note is not exercised by any test, I think?

Copy link
Contributor Author

@dingxiangfei2009 dingxiangfei2009 Feb 18, 2025

Choose a reason for hiding this comment

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

I just realised that the note cannot be exercised because the span falls within the macro expansion.

Instead it is overwritten by this message.

note: this error originates in the derive macro `CoercePointee` (in Nightly builds, run with -Z macro-backtrace for more info)

This happens still after adding the missing #[note] and #[help] attributes to the diagnostics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I need to switch to the span of the ADT definition.

.help = the standard pointers such as `Arc`, `Rc`, `Box`, and other types with `derive(CoercePointee)` can be coerced to their corresponding unsized types

hir_analysis_coerce_pointee_cannot_unsize = `{$ty}` cannot be coerced to any unsized value
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
hir_analysis_coerce_pointee_cannot_unsize = `{$ty}` cannot be coerced to any unsized value
hir_analysis_coerce_pointee_cannot_unsize = `{$ty}` cannot be coerced to an unsized value

.note = `derive(CoercePointee)` demands that `{$ty}` can be coerced to an unsized type
Copy link
Member

Choose a reason for hiding this comment

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

Similar, this note is not exercised by any test?

.help = `derive(CoercePointee)` requires exactly one copy of `#[pointee]` type at the end of the `struct` definition, without any further pointer or reference indirection

hir_analysis_coerce_pointee_multiple_targets = `derive(CoercePointee)` only admits exactly one data field, {$diag_trait ->
[DispatchFromDyn] to which `dyn` methods shall be dispatched
*[CoerceUnsized] on which unsize coercion shall be performed
}

hir_analysis_coerce_pointee_no_field = `CoercePointee` can only be derived on `struct`s with at least one field

hir_analysis_coerce_pointee_no_user_validity_assertion = asserting applicability of `derive(CoercePointee)` on a target data is forbidden
Expand Down
137 changes: 110 additions & 27 deletions compiler/rustc_hir_analysis/src/coherence/builtin.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
//! Check properties that are required by built-in traits and set
//! up data structures required by type-checking/codegen.

mod diagnostics;

use std::assert_matches::assert_matches;
use std::collections::BTreeMap;

use diagnostics::{extract_coerce_pointee_data, redact_fulfillment_err_for_coerce_pointee};
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::{ErrorGuaranteed, MultiSpan};
use rustc_hir as hir;
Expand All @@ -12,6 +15,7 @@ use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::lang_items::LangItem;
use rustc_infer::infer::{self, RegionResolutionError, TyCtxtInferExt};
use rustc_infer::traits::Obligation;
use rustc_middle::bug;
use rustc_middle::ty::adjustment::CoerceUnsizedInfo;
use rustc_middle::ty::print::PrintTraitRefExt as _;
use rustc_middle::ty::{
Expand Down Expand Up @@ -307,29 +311,45 @@ fn visit_implementation_of_dispatch_from_dyn(checker: &Checker<'_>) -> Result<()
.collect::<Vec<_>>();

if coerced_fields.is_empty() {
res = Err(tcx.dcx().emit_err(errors::DispatchFromDynSingle {
span,
trait_name: "DispatchFromDyn",
note: true,
}));
if extract_coerce_pointee_data(tcx, def_a.did()).is_some() {
Copy link
Member

Choose a reason for hiding this comment

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

Is this exercised by a test?

Copy link
Member

Choose a reason for hiding this comment

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

Unless there is a good reason to have this, please revert it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

res = Err(tcx.dcx().span_delayed_bug(
span,
"a specialised message for CoercePointee is expected",
));
} else {
res = Err(tcx.dcx().emit_err(errors::DispatchFromDynSingle {
span,
trait_name: "DispatchFromDyn",
note: true,
}));
}
} else if coerced_fields.len() > 1 {
res = Err(tcx.dcx().emit_err(errors::DispatchFromDynMulti {
span,
coercions_note: true,
number: coerced_fields.len(),
coercions: coerced_fields
.iter()
.map(|field| {
format!(
"`{}` (`{}` to `{}`)",
field.name,
field.ty(tcx, args_a),
field.ty(tcx, args_b),
)
})
.collect::<Vec<_>>()
.join(", "),
}));
if extract_coerce_pointee_data(tcx, def_a.did()).is_some() {
Copy link
Member

Choose a reason for hiding this comment

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

Is this exercised by a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let spans =
coerced_fields.iter().map(|field| tcx.def_span(field.did)).collect();
res = Err(tcx.dcx().emit_err(errors::CoercePointeeMultipleTargets {
spans,
diag_trait: "DispatchFromDyn",
}));
} else {
res = Err(tcx.dcx().emit_err(errors::DispatchFromDynMulti {
span,
coercions_note: true,
number: coerced_fields.len(),
coercions: coerced_fields
.iter()
.map(|field| {
format!(
"`{}` (`{}` to `{}`)",
field.name,
field.ty(tcx, args_a),
field.ty(tcx, args_b),
)
})
.collect::<Vec<_>>()
.join(", "),
}));
}
} else {
let ocx = ObligationCtxt::new_with_diagnostics(&infcx);
for field in coerced_fields {
Expand All @@ -344,9 +364,31 @@ fn visit_implementation_of_dispatch_from_dyn(checker: &Checker<'_>) -> Result<()
),
));
}
let errors = ocx.select_all_or_error();
let mut errors = ocx.select_all_or_error();
if !errors.is_empty() {
res = Err(infcx.err_ctxt().report_fulfillment_errors(errors));
if let Some((pointee_idx, _)) = extract_coerce_pointee_data(tcx, def_a.did()) {
let target_pointee = args_b.type_at(pointee_idx);

errors = errors
.into_iter()
.filter_map(|err| {
redact_fulfillment_err_for_coerce_pointee(
tcx,
err,
target_pointee,
span,
)
})
.collect();
}
if errors.is_empty() {
res = Err(tcx.dcx().span_delayed_bug(
span,
"a specialised CoercePointee error is expected",
));
} else {
res = Err(infcx.err_ctxt().report_fulfillment_errors(errors));
}
}

// Finally, resolve all regions.
Expand All @@ -372,9 +414,11 @@ pub(crate) fn coerce_unsized_info<'tcx>(
let unsize_trait = tcx.require_lang_item(LangItem::Unsize, Some(span));

let source = tcx.type_of(impl_did).instantiate_identity();
let self_ty = source;
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this. It's not useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

source binding is shadowed later. However, the diagnostics depends on this Self type.

Copy link
Member

Choose a reason for hiding this comment

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

Why not just rename the variable? The reassignment feels a bit awkward.

let trait_ref = tcx.impl_trait_ref(impl_did).unwrap().instantiate_identity();
assert_eq!(trait_ref.def_id, coerce_unsized_trait);
let target = trait_ref.args.type_at(1);
let coerced_ty = target;
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

debug!("visit_implementation_of_coerce_unsized: {:?} -> {:?} (bound)", source, target);

let param_env = tcx.param_env(impl_did);
Expand Down Expand Up @@ -509,12 +553,28 @@ pub(crate) fn coerce_unsized_info<'tcx>(
.collect::<Vec<_>>();

if diff_fields.is_empty() {
if extract_coerce_pointee_data(tcx, def_a.did()).is_some() {
Copy link
Member

Choose a reason for hiding this comment

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

Is this exercised by a test?

return Err(tcx.dcx().span_delayed_bug(
span,
"a specialised message for CoercePointee is expected",
));
}
return Err(tcx.dcx().emit_err(errors::CoerceUnsizedOneField {
span,
trait_name: "CoerceUnsized",
note: true,
}));
} else if diff_fields.len() > 1 {
if extract_coerce_pointee_data(tcx, def_a.did()).is_some() {
Copy link
Member

Choose a reason for hiding this comment

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

Is this exercised by a test?

let spans = diff_fields
.iter()
.map(|&(idx, _, _)| tcx.def_span(fields[idx].did))
.collect();
return Err(tcx.dcx().emit_err(errors::CoercePointeeMultipleTargets {
spans,
diag_trait: "CoerceUnsized",
}));
}
let item = tcx.hir().expect_item(impl_did);
let span = if let ItemKind::Impl(hir::Impl { of_trait: Some(t), .. }) = &item.kind {
t.path.span
Expand Down Expand Up @@ -547,18 +607,41 @@ pub(crate) fn coerce_unsized_info<'tcx>(
};

// Register an obligation for `A: Trait<B>`.
let coerce_pointee_data = if let ty::Adt(def, _) = self_ty.kind() {
extract_coerce_pointee_data(tcx, def.did())
} else {
None
};
let ocx = ObligationCtxt::new_with_diagnostics(&infcx);
let cause = traits::ObligationCause::misc(span, impl_did);
let cause = traits::ObligationCause::misc(
span,
coerce_pointee_data.map_or(impl_did, |(_, impl_did)| impl_did.expect_local()),
Copy link
Member

@compiler-errors compiler-errors Feb 18, 2025

Choose a reason for hiding this comment

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

Why did you change this body id/LocalDefId? It's not changing anything in the test output, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the final diagnostic text there is no difference. I will revert.

);
let obligation = Obligation::new(
tcx,
cause,
param_env,
ty::TraitRef::new(tcx, trait_def_id, [source, target]),
);
ocx.register_obligation(obligation);
let errors = ocx.select_all_or_error();
let mut errors = ocx.select_all_or_error();
if !errors.is_empty() {
infcx.err_ctxt().report_fulfillment_errors(errors);
if let Some((pointee, _)) = coerce_pointee_data {
let ty::Adt(_def, args) = coerced_ty.kind() else { bug!() };
let target_pointee = args.type_at(pointee);
let ty::Adt(_def, _) = self_ty.kind() else { bug!() };
errors = errors
.into_iter()
.filter_map(|err| {
redact_fulfillment_err_for_coerce_pointee(tcx, err, target_pointee, span)
})
.collect();
}
if errors.is_empty() {
tcx.dcx().span_delayed_bug(span, "a specialised CoercePointee error is expected");
} else {
infcx.err_ctxt().report_fulfillment_errors(errors);
}
}

// Finally, resolve all regions.
Expand Down
136 changes: 136 additions & 0 deletions compiler/rustc_hir_analysis/src/coherence/builtin/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
use rustc_hir::LangItem;
use rustc_hir::def_id::DefId;
use rustc_middle::ty::fast_reject::SimplifiedType;
use rustc_middle::ty::{
self, GenericParamDefKind, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor,
};
use rustc_span::{Span, sym};
use rustc_trait_selection::traits::FulfillmentError;
use tracing::instrument;

use crate::errors;

#[instrument(level = "debug", skip(tcx), ret)]
pub(super) fn extract_coerce_pointee_data<'tcx>(
tcx: TyCtxt<'tcx>,
adt_did: DefId,
) -> Option<(usize, DefId)> {
Copy link
Member

Choose a reason for hiding this comment

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

We never read the DefId from this, do we?

Copy link
Member

Choose a reason for hiding this comment

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

Other than to change the LocalDefId of the obligation, which I don't think matters?

// It is decided that a query to cache these results is not necessary
// for error reporting.
// We can afford to recompute it on-demand.
if let Some(impls) = tcx.lang_items().get(LangItem::CoercePointeeValidated).and_then(|did| {
tcx.trait_impls_of(did).non_blanket_impls().get(&SimplifiedType::Adt(adt_did))
}) && let [impl_did, ..] = impls[..]
{
// Search for the `#[pointee]`
let mut first_type = None;
for (idx, param) in tcx.generics_of(adt_did).own_params.iter().enumerate() {
if let GenericParamDefKind::Type { .. } = param.kind {
first_type = if first_type.is_some() { None } else { Some(idx) };
}
if tcx.has_attr(param.def_id, sym::pointee) {
return Some((idx, impl_did));
}
}
if let Some(idx) = first_type {
return Some((idx, impl_did));
}
}
None
}

fn contains_coerce_pointee_target_pointee<'tcx>(ty: Ty<'tcx>, target_pointee_ty: Ty<'tcx>) -> bool {
struct Search<'tcx> {
pointee: Ty<'tcx>,
found: bool,
}
impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for Search<'tcx> {
fn visit_binder<T: ty::TypeVisitable<TyCtxt<'tcx>>>(
&mut self,
t: &rustc_type_ir::Binder<TyCtxt<'tcx>, T>,
) {
if self.found {
return;
}
t.super_visit_with(self)
}

fn visit_ty(&mut self, t: Ty<'tcx>) {
if self.found {
return;
}
if t == self.pointee {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this implementation "micro-optimized"? It shouldn't matter to override all these functions, and it makes the visitor much larger than it needs to be.

self.found = true;
} else {
t.super_visit_with(self)
}
}

fn visit_region(&mut self, r: <TyCtxt<'tcx> as ty::Interner>::Region) {
if self.found {
return;
}
if let rustc_type_ir::ReError(guar) = r.kind() {
self.visit_error(guar)
}
}

fn visit_const(&mut self, c: <TyCtxt<'tcx> as ty::Interner>::Const) {
if self.found {
return;
}
c.super_visit_with(self)
}

fn visit_predicate(&mut self, p: <TyCtxt<'tcx> as ty::Interner>::Predicate) {
if self.found {
return;
}
p.super_visit_with(self)
}

fn visit_clauses(&mut self, p: <TyCtxt<'tcx> as ty::Interner>::Clauses) {
if self.found {
return;
}
p.super_visit_with(self)
}
}
let mut search = Search { pointee: target_pointee_ty, found: false };
ty.visit_with(&mut search);
search.found
}

#[instrument(level = "debug", skip(tcx))]
pub(super) fn redact_fulfillment_err_for_coerce_pointee<'tcx>(
tcx: TyCtxt<'tcx>,
err: FulfillmentError<'tcx>,
target_pointee_ty: Ty<'tcx>,
span: Span,
) -> Option<FulfillmentError<'tcx>> {
if let ty::PredicateKind::Clause(ty::ClauseKind::Trait(pred)) =
err.obligation.predicate.kind().skip_binder()
{
let mentions_pointee = || {
contains_coerce_pointee_target_pointee(
pred.trait_ref.args.type_at(1),
target_pointee_ty,
)
};
let source = pred.trait_ref.self_ty();
if tcx.is_lang_item(pred.def_id(), LangItem::Unsize) {
if mentions_pointee() {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this two nested if statements when the next one is &&? Can you choose one?

// We should redact it
tcx.dcx()
.emit_err(errors::CoercePointeeCannotUnsize { ty: source.to_string(), span });
return None;
}
} else if tcx.is_lang_item(pred.def_id(), LangItem::CoerceUnsized) && mentions_pointee() {
// We should redact it
tcx.dcx()
.emit_err(errors::CoercePointeeCannotCoerceUnsize { ty: source.to_string(), span });
return None;
}
}
Some(err)
}
24 changes: 24 additions & 0 deletions compiler/rustc_hir_analysis/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1219,6 +1219,30 @@ pub(crate) struct CoercePointeeNoField {
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_coerce_pointee_multiple_targets, code = E0802)]
pub(crate) struct CoercePointeeMultipleTargets {
#[primary_span]
pub spans: Vec<Span>,
pub diag_trait: &'static str,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_coerce_pointee_cannot_unsize, code = E0802)]
pub(crate) struct CoercePointeeCannotUnsize {
#[primary_span]
pub span: Span,
pub ty: String,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_coerce_pointee_cannot_coerce_unsize, code = E0802)]
pub(crate) struct CoercePointeeCannotCoerceUnsize {
#[primary_span]
pub span: Span,
pub ty: String,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_inherent_ty_outside_relevant, code = E0390)]
#[help]
Expand Down
Loading
Loading