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.

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

Properly find owner of closure in THIR unsafeck #87645

Merged
merged 1 commit into from
Aug 3, 2021
Merged

Properly find owner of closure in THIR unsafeck #87645

Show file tree
Hide file tree
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
11 changes: 4 additions & 7 deletions compiler/rustc_mir_build/src/check_unsafety.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,13 +599,10 @@ pub fn check_unsafety<'tcx>(tcx: TyCtxt<'tcx>, def: ty::WithOptConstParam<LocalD

// Closures are handled by their owner, if it has a body
if tcx.is_closure(def.did.to_def_id()) {
let owner = tcx.hir().local_def_id_to_hir_id(def.did).owner;
Copy link
Contributor

Choose a reason for hiding this comment

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

just to be clear I understand, because I understood something different from your text. owner here is == def.did, and thus invoking thir_check_unsafety on that will obviously cycle?

Copy link
Contributor Author

@LeSeulArtichaut LeSeulArtichaut Jul 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No, that's not the problem. Let me illustrate with an example:

fn function() {
    let closure = || {};
}

We're trying to compute unsafeck for closure, which should invoke unsafeck for the parent body function. So def.did is the DefId of the closure, and the HIR is constructed in such a way that the corresponding HirId has function as its owner, so in the code owner is the DefId of function which id correct.

Enter consts:

fn function() -> [(); { || {}; 4 }] {}

Here, if I try the same "hack", i.e. get the HirId for the closure and take its owner, I get owner to be the DefId of function whereas I want the DefId of the anonymous constant { || {}; 4 }.

To be fair I don't entirely understand which query invocations cycle and which don't when using ensure, and I've also not dived into the internals of HIR to check that enclosing_body_owner always does the right thing, but this PR doesn't cycle on the examples provided in #87414 and passed the rest of the test suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your explanation makes sense, thanks! I do think we should get a more solid strategy around all this, but I think it kind of overlaps with various other things like generics in anonymous constants. So let's merge this PR now

let owner_hir_id = tcx.hir().local_def_id_to_hir_id(owner);

if tcx.hir().maybe_body_owned_by(owner_hir_id).is_some() {
tcx.ensure().thir_check_unsafety(owner);
return;
}
let hir = tcx.hir();
let owner = hir.enclosing_body_owner(hir.local_def_id_to_hir_id(def.did));
tcx.ensure().thir_check_unsafety(hir.local_def_id(owner));
return;
}

let (thir, expr) = tcx.thir_body(def);
Expand Down
15 changes: 15 additions & 0 deletions src/test/ui/unsafe/issue-87414-query-cycle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Regression test for #87414.

// check-pass
// compile-flags: -Zthir-unsafeck

fn bad<T>() -> Box<dyn Iterator<Item = [(); { |x: u32| { x }; 4 }]>> { todo!() }

fn foo() -> [(); { |x: u32| { x }; 4 }] { todo!() }
fn bar() { let _: [(); { |x: u32| { x }; 4 }]; }

// This one should not cause any errors either:
unsafe fn unsf() {}
fn bad2<T>() -> Box<dyn Iterator<Item = [(); { unsafe { || { unsf() } }; 4 }]>> { todo!() }

fn main() {}