Make some MIR ref types closer to the source they are lowered from#150171
Make some MIR ref types closer to the source they are lowered from#150171ShoyuVanilla wants to merge 2 commits intorust-lang:mainfrom
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
rustbot
added
S-waiting-on-review
| let local_decl = LocalDecl::new(temp.ty, temp.source_info.span); | ||
| if temp.mutability == Mutability::Not { local_decl.immutable() } else { local_decl } | ||
| }; | ||
| let new_temp = self.promoted.local_decls.push(new_local_decl); |
There was a problem hiding this comment.
This wasn’t discussed in
#149973 (comment),
but I think it would be better to syntactically track promoted consts.
For example, the promoted const in the following code:
static FOO: () = {
let _ = &&FOO;
};is currently lowered to:
const FOO::promoted[0]: &&() = {
let mut _0: &&();
let mut _1: &();
let mut _2: &();
bb0: {
_2 = const {alloc1: &()};
_1 = &(*_2);
_0 = &_1;
return;
}
}
I think it would make more sense for _2 to be immutable. This would also make it clearer when detecting &STATIC from:
_2 = const {alloc1: &()};
_1 = &(*_2);
It seems very unlikely that _2 would ever be truly mutable (for example, being reassigned with a different value), but anyway _2 being immutable gives some more assurance.
There was a problem hiding this comment.
Mutability of local variables is pretty much entirely irrelevant in MIR. I don't think it's worth changing this, and it definitely should be separated from the other change which is about the actual types we see in MIR.
There was a problem hiding this comment.
None of these tests actually demonstrate the type of these temporaries changing, right? This just removes some mutability, which is largely irrelevant on the MIR level.
Could you add a test that shows the new logic in action? Ideally add the test in a first commit before the other changes, and then in the 2nd commit just have the diff of what that commit does to the test, so one can see it in action.
| let ty = if let hir::Node::Expr(&hir::Expr { | ||
| kind: hir::ExprKind::AddrOf(hir::BorrowKind::Raw, mutbl, _), | ||
| .. | ||
| }) = self.tcx.parent_hir_node(expr.hir_id) |
There was a problem hiding this comment.
This will only find &raw one level up, but will fail to detect &raw static.field1.field2 or similar expressions.
I am not very familiar with this code -- isn't there some existing infrastructure to figure out the "place context" we are in?
As usual I am not sure whom to ping for MIR building things...
Cc @oli-obk @matthewjasper @saethlin
There was a problem hiding this comment.
This will only find
&rawone level up, but will fail to detect&raw static.field1.field2or similar expressions.
Oh, exactly
There was a problem hiding this comment.
Should we also handle cases like &raw const STATIC[idx] or &raw mut STATIC[start..end]?
Coming back to the original issue: since std::ops::Index::index takes &Self as its receiver, the technically correct form would be unsafe { &*&raw const STATIC }[idx]. That said, unsafe { &*&raw const STATIC[idx] } might already be sufficient to convey the user’s intent—that they are explicitly acknowledging and accepting the potential UB. 🤔
|
@rustbot author |
rustbot
added
S-waiting-on-author
3 participants
cc #149973 (comment)
r? RalfJung
Sorry for bothering you, but I think it might be better to get the required MIR changes approved first, and then move on to implementing the actual lints for recursive static refs 😅