Properly find owner of closure in THIR unsafeck #87645
Conversation
Co-authored-by: FabianWolff <[email protected]>
rust-highfive
added
the
S-waiting-on-review
| @@ -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; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
@bors r+ |
|
📌 Commit 1280423 has been approved by |
bors
added
S-waiting-on-bors
|
I confirm that this PR fixes the ICE I had in #87114, where I implemented essentially the same logic. |
Properly find owner of closure in THIR unsafeck Previously, when encountering a closure in a constant, the THIR unsafeck gets invoked on the owner of the constant instead of the constant itself, producing cycles. Supersedes rust-lang#87492. `@FabianWolff` thanks for your work on that PR, I copied your test file and added you as a co-author. Fixes rust-lang#87414. r? `@oli-obk`
Properly find owner of closure in THIR unsafeck Previously, when encountering a closure in a constant, the THIR unsafeck gets invoked on the owner of the constant instead of the constant itself, producing cycles. Supersedes rust-lang#87492. ``@FabianWolff`` thanks for your work on that PR, I copied your test file and added you as a co-author. Fixes rust-lang#87414. r? ``@oli-obk``
Rollup of 8 pull requests Successful merges: - rust-lang#87645 (Properly find owner of closure in THIR unsafeck) - rust-lang#87646 (Fix a parser ICE on invalid `fn` body) - rust-lang#87652 (Validate that naked functions are never inlined) - rust-lang#87685 (Write docs for SyncOnceCell From and Default impl) - rust-lang#87693 (Add `aarch64-apple-ios-sim` as a possible target to the manifest) - rust-lang#87708 (Add convenience method for handling ipv4-mapped addresses by canonicalizing them) - rust-lang#87711 (Correct typo) - rust-lang#87716 (Allow generic SIMD array element type) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Previously, when encountering a closure in a constant, the THIR unsafeck gets invoked on the owner of the constant instead of the constant itself, producing cycles.
Supersedes #87492. @FabianWolff thanks for your work on that PR, I copied your test file and added you as a co-author.
Fixes #87414.
r? @oli-obk