Disallow mutable borrow to non-mut statics

[topecongiro]

Closes #42344.

[rust-highfive]

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

[topecongiro]

Hmm, I just realized that when nll is enabled, the error message is slightly different.
Is this acceptable?

with nll

error[E0596]: cannot borrow immutable item `*TAB[..]` as mutable
  --> test.rs:16:5
   |
16 |     TAB[0].iter_mut();
   |     ^^^^^^ cannot borrow as mutable
   |
   = note: Value not mutable causing this error: `TAB`

error: aborting due to previous error

without nll

error[E0596]: cannot borrow borrowed content of mutable binding as mutable
  --> test.rs:14:5
   |
14 |     TAB[0].iter_mut();
   |     ^^^^^^ cannot borrow as mutable

error: aborting due to previous error

[eddyb]

r? @nikomatsakis

[nikomatsakis]

@topecongiro that's acceptable, it's a separate issue

[nikomatsakis]

tbh, though, I find the MIR borrowck error much easier to understand =)

[nikomatsakis]

Thanks! =)

[nikomatsakis]

Something about this check doesn't feel right. There is a specific check that is supposed to guarantee that the reference is not an "aliasable" location, and it seems to be going awry, but I feel like we should be fixing that check, and not adding a new one. This is the check I am talking about:

rust/src/librustc_borrowck/borrowck/gather_loans/mod.rs

Lines 164 to 203 in 5965b79

	/// Implements the A-* rules in README.md.
	fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
	borrow_span: Span,
	loan_cause: AliasableViolationKind,
	cmt: mc::cmt<'tcx>,
	req_kind: ty::BorrowKind)
	-> Result<(),()> {
	let aliasability = cmt.freely_aliasable();
	debug!("check_aliasability aliasability={:?} req_kind={:?}",
	aliasability, req_kind);
	match (aliasability, req_kind) {
	(mc::Aliasability::NonAliasable, _) => {
	/* Uniquely accessible path -- OK for `&` and `&mut` */
	Ok(())
	}
	(mc::Aliasability::FreelyAliasable(mc::AliasableStatic), ty::ImmBorrow) => {
	// Borrow of an immutable static item.
	Ok(())
	}
	(mc::Aliasability::FreelyAliasable(mc::AliasableStaticMut), _) => {
	// Even touching a static mut is considered unsafe. We assume the
	// user knows what they're doing in these cases.
	Ok(())
	}
	(mc::Aliasability::FreelyAliasable(alias_cause), ty::UniqueImmBorrow) |
	(mc::Aliasability::FreelyAliasable(alias_cause), ty::MutBorrow) => {
	bccx.report_aliasability_violation(
	borrow_span,
	loan_cause,
	alias_cause,
	cmt);
	Err(())
	}
	(..) => {
	Ok(())
	}
	}
	}

Can you maybe tell me what the cmt is in this case, and what the value of aliasability is?

[topecongiro]

The cmt is

cmt=cmt_ {
        id: NodeId(16),
        span: test.rs:13:5: 13:11,
        cat: Deref(cmt_ {
            id: NodeId(16),
            span: test.rs:13:5: 13:11,
            cat: Interior(cmt_ {
                id: NodeId(14),
                span: test.rs:13:5: 13:8,
                cat: StaticItem,
                mutbl: McImmutable,
                ty: [&'static mut [u8]; 0],
                note: NoteNone,
            }, []),
            mutbl: McImmutable,
            ty: &'static mut [u8],
            note: NoteNone,
        }, BorrowedPtr(MutBorrow, ReStatic)),
        mutbl: McDeclared,
        ty: [u8],
        note: NoteNone
    }

and the value of aliasability is

aliasability=FreelyAliasable(AliasableStatic)

[nikomatsakis]

Huh, so I wonder in that case why this code:

rust/src/librustc_borrowck/borrowck/gather_loans/mod.rs

Lines 190 to 198 in 5965b79

	(mc::Aliasability::FreelyAliasable(alias_cause), ty::UniqueImmBorrow) |
	(mc::Aliasability::FreelyAliasable(alias_cause), ty::MutBorrow) => {
	bccx.report_aliasability_violation(
	borrow_span,
	loan_cause,
	alias_cause,
	cmt);
	Err(())
	}

doesn't execute... req_kind is ty::MutBorrow, right?

[topecongiro]

That is because check_mutability triggers an error, and check_aliasability is no called at all.

[nikomatsakis]

Huh. So how come I don't see an error reporting in the original issue #42344? I see the ICE -- where does that ICE occur?

[topecongiro]

Oh, were you asking about the behavior of the original rustc? I was talking about how rustc bahaves when this PR was applied. I am sorry for the confusion.

Using the master branch, yes, req_kind is ty::MutBorrow, and bccx.report_aliasability_violation is executed. The following error is reported.

error: internal compiler error: aliasability violation for static `cannot borrow data mutably`
 --> test.rs:3:5
  |
3 |     TAB[0].iter_mut();
  |     ^^^^^^

error: aborting due to previous error

[nikomatsakis]

@topecongiro ah, I see -- actually, I think that the delay_span_bug is just wrong and should be removed. The logic was that a case like static X: u32 = 2; &mut X should not reach there (and indeed I think it won't) -- but it overlooked the possibility of an &mut living in a static.

[nikomatsakis]

Can you update this PR to just remove the delay_span_bug call for AliasabilityReason::Static?

[topecongiro]

Sure! Thank your for your patience with my slow updates.

[shepmaster]

Ping from triage, @topecongiro ! Will you be able to respond to the most recent set of feedback soon?

[shepmaster]

Disallow mutable borrow to non-mut statics

I got excited and thought this fixed #46557 😝

[topecongiro]

Oops, I missed the notification of review by @nikomatsakis. Sorry!

I will work on this today at some point.

[nikomatsakis]

@topecongiro left another question :)

[nikomatsakis]

@bors r+

Nice =)

[bors]

📌 Commit 3d114c7 has been approved by nikomatsakis