Skip to content

Ensure that THIR unsafety check is done before stealing it #115012

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.

Sign up for GitHub

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 1 commit into from
Aug 24, 2023
Merged

Ensure that THIR unsafety check is done before stealing it #115012

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion compiler/rustc_mir_build/src/build/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ pub(crate) fn closure_saved_names_of_captured_variables<'tcx>(
/// Construct the MIR for a given `DefId`.
fn mir_build(tcx: TyCtxt<'_>, def: LocalDefId) -> Body<'_> {
// Ensure unsafeck and abstract const building is ran before we steal the THIR.
tcx.ensure_with_value().thir_check_unsafety(def);
tcx.ensure_with_value()
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this PR, but why is this ensure_with_value 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

ensure verifies that the dependency is green, without computing anything if not required.
This means that a query that is ensured may need to be computed later if some code needs the result value.

ensure_with_value verifies that the result is available, computed or loadable from on-disk cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm.. this means that query side effects can be emitted twice? Once when marking a query green and once when executing the query later to recover the value. I guess we need to make sure that's benign.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is ok for diagnostics, as they are deduplicated. But this is a blocker for replaying any other side-effect. I've been trying to fix this to re-create DefId, but I'm not satisfied by my design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an option to turn it off though. There's also extra logic for the parallel compiler to avoid emitting side effects multiple times, which is kind of unnecessary then.

How does DefId relate to this?

Representing side effects as forcible queries would avoid this issue, but does require work to support forcing arbitrary keys.

.thir_check_unsafety(tcx.typeck_root_def_id(def.to_def_id()).expect_local());
tcx.ensure_with_value().thir_abstract_const(def);
if let Err(e) = tcx.check_match(def) {
return construct_error(tcx, def, e);
Expand Down