Do not ICE on missing access place description during mutability error reporting

[estebank]

Fix #61187.

[rust-highfive]

r? @varkor

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

[estebank]

Cc @rust-lang/wg-diagnostics @cramertj for wording

[davidtwco]

I've only had a cursory glance, but would we be able to give the name of the place if we allowed CompilerDesugaringKind::Async here or would that break some cases?

rust/src/librustc_mir/borrow_check/error_reporting.rs

Line 283 in 4680580

Some(name) if !local.from_compiler_desugaring() => {

[estebank]

We could probably detect the Async desugaring and say something like "cannot borrow async argument as mutable, as it is not declared as mutable"

[estebank]

@davidtwco I think it is a cool idea to use the span desugaring mark for this, but sadly at the moment we don't mark the argument's spans with it and it is taking me some time to change it to do so.

Disregard, just managed to get it working (was looking at the wrong span).

[oli-obk]

I don't see any output actually producing this. Should we bug! or at least delay_span_bug here?

[oli-obk]

@bors r+ p=5 async await priorization

my nit may be fixed in a separate PR

[bors]

📌 Commit d72f97d has been approved by oli-obk

[Centril]

@bors p=0 rollup

That was slightly too over-prioritized 😂 -- including it in a rollup will do.

r? @oli-obk

[oli-obk]

"just following orders" 😛

[matthewjasper]

I don't think this is really the correct fix, the variable being mutated here shouldn't have it's Span marked. I guess this is fine for now.

[oli-obk]

The problem isn't that the argument has its Span marked, what we're seeing is a temp local that is part of the expansion of async fn causing the error. That temp local is directly connected to the argument where the Span points to.

EDIT: Nevermind, I misremembered, and didn't read the following method carefully enough during review:

rust/src/librustc_mir/borrow_check/error_reporting.rs

Lines 280 to 289 in 8197085

	fn append_local_to_string(&self, local_index: Local, buf: &mut String) -> Result<(), ()> {
	let local = &self.mir.local_decls[local_index];
	match local.name {
	Some(name) if !local.from_compiler_desugaring() => {
	buf.push_str(name.as_str().get());
	Ok(())
	}
	_ => Err(()),
	}
	}

yea, we should probably not mark the local's span during HIR lowering.

[oli-obk]

@bors r-

[estebank]

we should probably not mark the local's span during HIR lowering.

@oli-obk why shouldn't we? I believe there are a couple of places where we mark spans that aren't strictly part of the desugared output, but that not marking would make it impossible to provide good diagnostics in some cases. I believe this might just be one of these cases :-/

[davidtwco]

In this instance, the span is marked to avoid unused_mut lint (since the desugaring will just pull the argument directly into the closure). We can’t remove the mut as that would affect the input that macros get. I’m hoping to tidy up the logic around that desugaring soon (because it’s unfortunately a mess) but I’m not sure if this span being marked will change with that.

[oli-obk]

Hmm... in that case, would it make sense to special case append_local_to_string instead of special casing on the output of it?

[matthewjasper]

The problems is that we are marking more than we should be. The lowered code here looks like:

async fn response(
    data: Vec<u8> // This is an internal variable, it should have its Span marked
) -> ::std::future::from_generator(move || {
    let data // This variable is effectively the user's parameter, it should not have it's Span marked
        = data; 
    data.reverse();
}) 

This is causing false negatives in cases like:

#![deny(unused_mut)]
pub async fn response(mut data: Vec<u8>) {
    let _ = data;
}

[davidtwco]

@estebank, I have a drive-by fix for this in #61413 which re-implements the async fn drop order lowering (sorry!).