Don't require unsafe blocks for Share mut items #13348
Conversation
| let ety = ty::node_id_to_type(self.tcx, expr.id); | ||
| if !ty::type_is_sharable(self.tcx, ety) { | ||
| self.require_unsafe(expr.span, "use of mutable static") | ||
| } |
There was a problem hiding this comment.
I'm not sure that this is quite right, this code should be disallowed:
static mut FOO: AtomicUint = INIT_ATOMIC_UINT;
let foo: &mut AtomicUint = &mut FOO;Could you add a test for this to make sure it's disallowed? Only &-pointers to shareable static muts should be allowed.
There was a problem hiding this comment.
I'm sorry, I forgot about disallowing &mut MY_STATIC. Good catch!
|
Based on the comments, the PR definitely needs more work. Sorry for the poor implementation here and not considering the other possible corner cases. |
|
@flaper87, no worries! This is tricky stuff! |
|
@alexcrichton @huonw this likely needs some cleanup but I think it already covers all the rules we talked about. Pushed it in case you guys want to take a look, I'll do the cleanup tomorrow 😴 |
|
|
||
| fn expr_is_mut_static(&mut self, e: &ast::Expr) -> bool { | ||
| match self.tcx.def_map.borrow().find(&e.id) { | ||
| Some(&def) => { |
There was a problem hiding this comment.
How about
match self.tcx... {
Some(ast::DefStatic(_, true)) => true,
_ => false
}?
There was a problem hiding this comment.
indeed, way better! thanks!
|
Does this handle something like &((((STATIC))))(which should be perfectly valid)? Also what about something like What's the safety of: static SHARE: ...;
static NON_SHARE: ...;
fn main() {
x = SHARE;
y = NON_SHARE:
}? I'm quite concerned about the fragility of this approach, where it is basically hardcoding the two types of uses of a |
|
For the sake of discussion. I agree with @huonw on the fragility of this approach. I struggled to find an implementation that barely convinced me. I'm not happy with it, still. I'd like to try another approach, which is 'disable everything but some specific cases' by reusing the |
Instead of having an `unsafe_block` field in the `EffectVisitor`, use an env to hold this information. Envs are clone before walking a sub-expression, this helps keeping flags isolated and per-expression.
In general, it's not allowed to access mutable static items outside an unsafe block. However, taking an immutable address of a mutable static items whose type implements `Share` is considered a safe operation. This is enforced by `rustc::middle::effect` Closes rust-lang#13232
| pub fn fn_mut(_: &mut Sharable) {} | ||
|
|
||
|
|
||
| pub fn main() { |
There was a problem hiding this comment.
What about method calls on NonSharable?
There was a problem hiding this comment.
I thought I had added them.
|
@flaper87 Overall, this looks pretty good to me, but I also see why you say you are not fully satisfied. I think that ultimately the AST visitor is not quite the right tool here. That is, I don't think we necessarily want to descend all the way down to variables, passing in extra context, etc. I think the problem is that we want to treat "lvalues" as a unit here: that is, we want to identify something like &a.b.c and handle the lvalue Something similar arises in the borrow checker. I've been thinking about a refactoring that would help to address this (as part of fixing #5016 and #12624). Let me play with this a bit right now and get back to you, hopefully in next day or two. (Seems like this code is not especially prone to bitrot) |
|
I need to focus on the opt-in trait work so, I'll close this PR until I've got time to get back to it. The work here requires some more iterations and a better implementation. |
Closes #13232