-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Check whether the closure's owner is an ADT in thir-unsafeck #86138
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
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
// ...unless they occur inside an ADT definition, e.g. in the | ||
// length part of an array (issue #85871) | ||
let owner = tcx.hir().local_def_id_to_hir_id(def.did).owner; | ||
if !tcx.type_of(owner).is_adt() { |
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.
I'm not sure if this is adequate. For example, what about type Foo = [...; || ...];
?
We probably want to identify the cases where we should check the owner instead and enumerate those.
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.
You are right, .is_adt()
is not comprehensive enough (although it is not a regression from the current behavior either).
I have used .maybe_body_owned_by(...).is_some()
now, which should be more robust, and expanded the test case; could you have another look?
The concept of body owner is a bit confusing, but this does seem right. Nice. |
@bors r+ |
📌 Commit 433c1ae has been approved by |
☀️ Test successful - checks-actions |
This pull request fixes #85871. The code in
rustc_mir_build/src/check_unsafety.rs
incorrectly assumes that a closure's owner always has a body, but only functions, closures, and constants have bodies, whereas a closure can also appear inside a struct or enum:This pull request fixes the resulting ICE by checking whether the closure's owner is an ADT and only deferring to
thir_check_unsafety(owner)
if it isn't.