[nll] optimize + fix check_if_reassignment_to_immutable_state

[nikomatsakis]

check_if_reassignment_to_immutable_state has the of handling assignments to immutable local variables. These are a bit tricky: you are allowed to assign to an immutable local variable, but only if it has never been assigned to before. Actually, the logic implemented in the NLL borrow checker is kind of wrong (or at least inconsistent with the AST checker). It's also just slow.

Here is an example of the sort of assignments that should be accepted (playground):

#![feature(nll)]

fn main() {
    let x: (u32, u32);
    
    if true {
        x = (1, 2);
    } else {
        x = (3, 4);
    }
    
    drop(x);
}

This example is rightly rejected (but we should check to ensure we have a test) (playground):

#![feature(nll)]

fn main() {
    let x: (u32, u32);
    x = (1, 2);
    x = (3, 4);
    drop(x);
}

This example should be rejected but currently is not (playground):

#![feature(nll)]

fn main() {
    let x: (u32, u32);
    
    x.0 = 22;
}

In addition to being slightly wrong, the current implementation is also slow. It iterates over all initialized paths and checks that the path being assigned to (e.g., x.0 in the third example) is disjoint from all of them:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 1612 to 1619 in 52c785b

	for i in flow_state.ever_inits.iter_incoming() {
	let init = self.move_data.inits[i];
	let init_place = &self.move_data.move_paths[init.path].place;
	if places_conflict::places_conflict(self.tcx, self.mir, &init_place, place, Deep) {
	self.report_illegal_reassignment(context, (place, span), init.span, err_place);
	break;
	}
	}

I will write below how I think it should work.

[nikomatsakis]

I think what we should do:

• Distinguish the case of assignment directly to an immutable local (x = ...) from any other place.
  • For assignments to an immutable local, we find the move-path-index and just check the "ever initialized" bit directly.
  • For assignments to any other place, we use the self.access_place method with LocalMutationIsAllowed::No (unlike now, where we use ExceptUpvars).

[nikomatsakis]

As a bonus, I think we could remove ExceptUpvars entirely. It is only used in two places -- the one above, and here:

rust/src/librustc_mir/borrow_check/nll/invalidation.rs

Lines 358 to 372 in 52c785b

	/// Simulates mutation of a place
	fn mutate_place(
	&mut self,
	context: Context,
	place: &Place<'tcx>,
	kind: ShallowOrDeep,
	_mode: MutateMode,
	) {
	self.access_place(
	context,
	place,
	(kind, Write(WriteKind::Mutate)),
	LocalMutationIsAllowed::ExceptUpvars,
	);
	}

and both should now be just No.

[nikomatsakis]

@spastorino would like to take this on.

[pnkfelix]

At some point I thought we had considered allowing a structs fields to be initialized independently .... don’t remember how far that went at the moment ....

[nikomatsakis]

@pnkfelix

We were considering it, but we don't presently allow it. Even with NLL this does not compile, for example:

#![feature(nll)]

fn main() {
    let x: (u32, u32);
    x.0 = 1;
    x.1 = 2;
    println!("{:?}", x);
}

[nikomatsakis]

It's true though that the existing borrow checker is sort of inconsistent here -- if you do let mut x, then it permits x.0 = 1 and x.1 = 2 but rejects using x as a whole (just like NLL does).

[nikomatsakis]

I was suggesting that we match the existing behavior because it's simple to do — I am not opposed to being smarter later on though.

I guess to do this the "right" way we would do something like:

• Find the move path corresponding to the place being assigned
  • If there is no precise match, that is an error (e.g., assigning to a single field of a drop struct)
• If there is a precise match, check its bit

Sound about right @pnkfelix ?

[memoryruins]

Following a similar table format from #53114:

Sample	Current	NLL	Example
let x: (u32, u32); x.0 = 1; x.1 = 2;	❌	✔️	playground
let mut x: (u32, u32); x.0 = 1; x.1 = 2;	✔️	⚠️	playground
println!("{:?}", x);	❌	❌	playground

[nikomatsakis]

@memoryruins I guess that the ⚠️ in that table refers to the fact that you get a warning about the mut not being needed? That would be false if we take the simplified version, I guess.

In any case, I can go either way here. I'd be fine with matching the AST checker, because it's easy to do, and then trying to revisit this whole question more carefully (in particular, I would want later references to x to work).

But it's also not that hard to make the current system more efficient, and I think it is more internally consistent than what the AST checker does.

[memoryruins]

Correct, the ⚠️ refers to the following warning:

4 |     let mut x: (u32, u32);
  |         ----^
  |         |
  |         help: remove this `mut`