Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
20 changes: 11 additions & 9 deletions compiler/rustc_middle/src/queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -590,14 +590,15 @@ rustc_queries! {
separate_provide_extern
}

/// Checks whether a type is representable or infinitely sized
query check_representability(key: LocalDefId) -> rustc_middle::ty::Representability {
/// Checks whether a type is representable or infinitely sized,
/// causes a cycle error if it's infinitely sized.
query check_representability(key: LocalDefId) {
desc { "checking if `{}` is representable", tcx.def_path_str(key) }
// Infinitely sized types will cause a cycle. The custom `FromCycleError` impl for
// `Representability` will print a custom error about the infinite size and then abort
// compilation. (In the past we recovered and continued, but in practice that leads to
// For infinitely sized types a cycle handler will print
// a custom error about the infinite size and then abort compilation.
// (In the past we recovered and continued, but in practice that leads to
// confusing subsequent error messages about cycles that then abort.)
cycle_delay_bug

// We don't want recursive representability calls to be forced with
// incremental compilation because, if a cycle occurs, we need the
// entire cycle to be in memory for diagnostics. This means we can't
Expand All @@ -607,15 +608,16 @@ rustc_queries! {

/// An implementation detail for the `check_representability` query. See that query for more
/// details, particularly on the modifiers.
query check_representability_adt_ty(key: Ty<'tcx>) -> rustc_middle::ty::Representability {
query check_representability_adt_ty(key: Ty<'tcx>) {
desc { "checking if `{}` is representable", key }
cycle_delay_bug
anon
}

/// Set of param indexes for type params that are in the type's representation
///
/// An implementation detail for the `representability` query
query params_in_repr(key: DefId) -> &'tcx rustc_index::bit_set::DenseBitSet<u32> {
desc { "finding type parameters in the representation" }
desc { "finding type parameters in the representation of `{}`", tcx.def_path_str(key) }
arena_cache
no_hash
separate_provide_extern
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_middle/src/query/erase.rs
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,6 @@ impl_erasable_for_simple_types! {
rustc_middle::ty::Destructor,
rustc_middle::ty::fast_reject::SimplifiedType,
rustc_middle::ty::ImplPolarity,
rustc_middle::ty::Representability,
rustc_middle::ty::UnusedGenericParams,
rustc_middle::ty::util::AlwaysRequiresDrop,
rustc_middle::ty::Visibility<rustc_span::def_id::DefId>,
Expand Down
5 changes: 0 additions & 5 deletions compiler/rustc_middle/src/ty/adt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -740,8 +740,3 @@ impl<'tcx> AdtDef<'tcx> {
if self.is_struct() { tcx.adt_sizedness_constraint((self.did(), sizedness)) } else { None }
}
}

/// This type exists just so a `FromCycleError` impl can be made for the `check_representability`
/// query.
#[derive(Clone, Copy, Debug, HashStable)]
pub struct Representability;
6 changes: 6 additions & 0 deletions compiler/rustc_query_impl/src/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use rustc_span::{DUMMY_SP, Span};

use crate::collect_active_jobs_from_all_queries;
use crate::dep_graph::{DepNode, DepNodeIndex};
use crate::from_cycle_error::representability_cycle_handler;
use crate::job::{QueryJobInfo, QueryJobMap, find_cycle_in_stack, report_cycle};
use crate::plumbing::{current_query_job, next_job_id, start_query};

Expand Down Expand Up @@ -125,6 +126,11 @@ fn mk_cycle<'tcx, C: QueryCache>(
tcx: TyCtxt<'tcx>,
cycle_error: CycleError,
) -> C::Value {
// Try to handle the cycle error with our custom handlers.
// They will unwind if they handle the cycle.
representability_cycle_handler(tcx, &cycle_error);
Copy link
Contributor

Choose a reason for hiding this comment

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

So we are hard coding representability query cycle handling into query system. This code should be more general.


// Create the default cycle error
let error = report_cycle(tcx.sess, &cycle_error);
match query.cycle_error_handling {
CycleErrorHandling::Error => {
Expand Down
77 changes: 43 additions & 34 deletions compiler/rustc_query_impl/src/from_cycle_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use rustc_hir::def::{DefKind, Res};
use rustc_middle::dep_graph::DepKind;
use rustc_middle::query::CycleError;
use rustc_middle::query::plumbing::CyclePlaceholder;
use rustc_middle::ty::{self, Representability, Ty, TyCtxt};
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_middle::{bug, span_bug};
use rustc_span::def_id::LocalDefId;
use rustc_span::{ErrorGuaranteed, Span};
Expand Down Expand Up @@ -86,42 +86,51 @@ impl<'tcx> FromCycleError<'tcx> for ty::Binder<'_, ty::FnSig<'_>> {
}
}

impl<'tcx> FromCycleError<'tcx> for Representability {
fn from_cycle_error(
tcx: TyCtxt<'tcx>,
cycle_error: CycleError,
_guar: ErrorGuaranteed,
) -> Self {
let mut item_and_field_ids = Vec::new();
let mut representable_ids = FxHashSet::default();
for info in &cycle_error.cycle {
if info.frame.dep_kind == DepKind::check_representability
&& let Some(field_id) = info.frame.def_id
&& let Some(field_id) = field_id.as_local()
&& let Some(DefKind::Field) = info.frame.info.def_kind
{
let parent_id = tcx.parent(field_id.to_def_id());
let item_id = match tcx.def_kind(parent_id) {
DefKind::Variant => tcx.parent(parent_id),
_ => parent_id,
};
item_and_field_ids.push((item_id.expect_local(), field_id));
}
pub(super) fn representability_cycle_handler(tcx: TyCtxt<'_>, cycle_error: &CycleError) {
// We only handle cycle errors where `check_representability` is present and
// `check_representability_adt_ty` is the only potential additional query
let applies = cycle_error.cycle.iter().all(|query| {
matches!(
query.frame.dep_kind,
DepKind::check_representability | DepKind::check_representability_adt_ty
)
}) && cycle_error
.cycle
.iter()
.find(|query| query.frame.dep_kind == DepKind::check_representability)
.is_some();
if !applies {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this new check needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

This code manually checks if it handles representability query by checking if all queries in a cycle are representability. Meanwhile currently we skip any non-representability queries in a cycle. However now representability_cycle_handler is directly called from mk_cycle and doesn't use value_from_cycle_error function pointer for specializing on Representability return type.

This is a very crude check to rely on IMO. To remove FromCycleError I would've first implemented a FallbackProvider for fallback queries to directly specialize value_from_cycle_error function pointer and not through a result type's FromCycleError trait impl. I did it in #149319 and Zoxc did something similar in #153028. In my surely very unbiased opinion my #149319 gets rid of FromCycleError (Value back then) more elegantly than Zoxc have demonstrated in #153028 so far. Per Zoxc's stated motivation:

This is a step towards removing query cycle recovery as the FromCycleError trait would be removed for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also AFAIK if any query is representability then every other query in a cycle is representability too. So we could add such assertion here if I'm correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't know about #149319. (I was CC'd, but last year, long before I started looking into the query system.) From a quick look it has a vtable function pointer (execute_fallback) but also requires specifying providers. My rough design below uses a query modifier (like cache_on_disk_if) without needing providers.


let mut item_and_field_ids = Vec::new();
let mut representable_ids = FxHashSet::default();
for info in &cycle_error.cycle {
if info.frame.dep_kind == DepKind::check_representability
&& let Some(field_id) = info.frame.def_id
&& let Some(field_id) = field_id.as_local()
&& let Some(DefKind::Field) = info.frame.info.def_kind
{
let parent_id = tcx.parent(field_id.to_def_id());
let item_id = match tcx.def_kind(parent_id) {
DefKind::Variant => tcx.parent(parent_id),
_ => parent_id,
};
item_and_field_ids.push((item_id.expect_local(), field_id));
}
for info in &cycle_error.cycle {
if info.frame.dep_kind == DepKind::check_representability_adt_ty
&& let Some(def_id) = info.frame.def_id_for_ty_in_cycle
&& let Some(def_id) = def_id.as_local()
&& !item_and_field_ids.iter().any(|&(id, _)| id == def_id)
{
representable_ids.insert(def_id);
}
}
for info in &cycle_error.cycle {
if info.frame.dep_kind == DepKind::check_representability_adt_ty
&& let Some(def_id) = info.frame.def_id_for_ty_in_cycle
&& let Some(def_id) = def_id.as_local()
&& !item_and_field_ids.iter().any(|&(id, _)| id == def_id)
{
representable_ids.insert(def_id);
}
// We used to continue here, but the cycle error printed next is actually less useful than
// the error produced by `recursive_type_error`.
let guar = recursive_type_error(tcx, item_and_field_ids, &representable_ids);
guar.raise_fatal();
}
// We used to continue here, but the cycle error printed next is actually less useful than
// the error produced by `recursive_type_error`.
recursive_type_error(tcx, item_and_field_ids, &representable_ids).raise_fatal()
}

impl<'tcx> FromCycleError<'tcx> for ty::EarlyBinder<'_, Ty<'_>> {
Expand Down
8 changes: 3 additions & 5 deletions compiler/rustc_ty_utils/src/representability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use rustc_hir::def::DefKind;
use rustc_index::bit_set::DenseBitSet;
use rustc_middle::bug;
use rustc_middle::query::Providers;
use rustc_middle::ty::{self, Representability, Ty, TyCtxt};
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_span::def_id::LocalDefId;

pub(crate) fn provide(providers: &mut Providers) {
Expand All @@ -14,7 +14,7 @@ pub(crate) fn provide(providers: &mut Providers) {
};
}

fn check_representability(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Representability {
fn check_representability(tcx: TyCtxt<'_>, def_id: LocalDefId) {
match tcx.def_kind(def_id) {
DefKind::Struct | DefKind::Union | DefKind::Enum => {
for variant in tcx.adt_def(def_id).variants() {
Expand All @@ -28,7 +28,6 @@ fn check_representability(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Representabili
}
def_kind => bug!("unexpected {def_kind:?}"),
}
Representability
}

fn check_representability_ty<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) {
Expand Down Expand Up @@ -67,7 +66,7 @@ fn check_representability_ty<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) {
// Looking at the query cycle above, we know that `Bar` is representable
// because `check_representability_adt_ty(Bar<..>)` is in the cycle and
// `check_representability(Bar)` is *not* in the cycle.
fn check_representability_adt_ty<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Representability {
fn check_representability_adt_ty<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) {
let ty::Adt(adt, args) = ty.kind() else { bug!("expected adt") };
if let Some(def_id) = adt.did().as_local() {
let _ = tcx.check_representability(def_id);
Expand All @@ -82,7 +81,6 @@ fn check_representability_adt_ty<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Repre
}
}
}
Representability
}

fn params_in_repr(tcx: TyCtxt<'_>, def_id: LocalDefId) -> DenseBitSet<u32> {
Expand Down
Loading