-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
Add a cycle handler for check_representability
#153470
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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}; | ||
|
|
@@ -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; | ||
| } | ||
|
Contributor
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 is this new check needed?
Contributor
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 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 This is a very crude check to rely on IMO. To remove
Contributor
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. 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.
Contributor
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. 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 ( |
||
|
|
||
| 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<'_>> { | ||
|
|
||
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.
So we are hard coding
representabilityquery cycle handling into query system. This code should be more general.