large_assignments: Unactionable diagnostics with -Zinline-mir

[Enselic]

Tracking issue #83518.

Doing cargo build --release will activate -Zinline-mir under the hood (through -Copt-level=3). But it makes large_assignments diagnostics unhelpful, because it can make diagnostics point to library code that the user can't change.

How to reproduce

src/main.rs

#![feature(large_assignments)]
#![deny(large_assignments)]
#![move_size_limit = "1000"]

pub fn main() {
    let data = [10u8; 9999];
    let cell = std::cell::UnsafeCell::new(data);
    std::hint::black_box(cell);
}

# One of:
cargo +nightly build --release
cargo +nightly rustc -- -Zmir-opt-level=1 -Zinline-mir

Actual

   Compiling minimal v0.1.0 (/home/martin/src/bin)
error: moving 9999 bytes
    --> /home/martin/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/cell.rs:2054:9
     |
2054 |         UnsafeCell { value }
     |         ^^^^^^^^^^^^^^^^^^^^ value moved from here
     |

Expected

   Compiling minimal v0.1.0 (/home/martin/src/bin)
error: moving 9999 bytes
 --> src/main.rs:7:43
  |
7 |     let cell = std::cell::UnsafeCell::new(data);
  |                                           ^^^^ value moved from here
  |

Remarks

The expected diagnostics is given with these commands. Note how -Zinline-mir is deactivated in both cases:

# One of:
cargo +nightly build
cargo +nightly rustc -- -Zmir-opt-level=1

The above test case is ui-testified here.

CC E-mentor @oli-obk who maybe have an idea on how to fix this? (I currently don't.)

[oli-obk]

heh. we used to have a marker in spans that signaled that something got inlined. That was a bit funky though, so we removed it (#111950).

You can check the source scopes nowadays to see if https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.SourceScopeData.html#structfield.inlined has been set. If it is set, don't emit the lint on that span, but instead emit it on the span within that option

[jogru0]

@rustbot claim

[jogru0]

#139551 made progress on this issue, it now shows

LL |     let cell = std::cell::UnsafeCell::new(data);
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ value moved from here

but still not the expected

LL |     let cell = std::cell::UnsafeCell::new(data);
   |                                           ^^^^ value moved from here

@oli-obk You mentioned here that we might want to adjust something so we don't loose the argument span when inlining happens. If that's something that might make sense for me to look into and you could give me some directions/mentoring for that, I'm happy to try it. However, that's certainly not something I know how to do on my own, so if this topic would be too involved for that, I can also just unassign this issue from me and let someone more experienced take over.

[oli-obk]

In https://github.com/rust-lang/rust/blob/master/compiler/rustc_mir_transform/src/inline.rs#L1081 we are ignoring the a.span and not passing it to create_temp_if_necessary. I do not believe this is the cause of the issue here, but it is related, as it would be the same issue if the call would not just use a variable, but something like UnsafeCell::new(foo.data), where it would first create a let _tmp15 = foo.data;, but with the call site span instead of the foo.data span.

You could try this on its own to get your hands dirty with the mir-opt test suite, which any changes here will have to touch. It should be as simple as running x t mir-opt --bless to update that test suite, and should likely not require manual changes.

To fix the actual issue you were encountering we'll have to go to where the args from make_call_args are processed. We'd first need to return the Spans somehow, too, and I'm not sure what the best approach for this is. Most likely just turning the Box<[Local]> into a Box<[(Local, Span)]>

But looking at that now, I don't think this is actually resolvable here. The spans and locals should all already be correct, but the later mir-optimizations remove those locals without adjusting debug information, so the local with the right span is erased and only the locals that were inlined are left.

I do not know where this happens, but if you want to do some exploration, you can use -Zdump-mir=all on your test (with all the inlining flags and other opt flags that cause the issues with diagnostics), and go through the optimizations after the inlining pass and see if something obvious shows up

[jogru0]

Thanks for the detailed explanation, really appreciated it. I tried to look into it a bit, but unfortunately, I think this is a bit too advanced for me. So I'll unassign myself for now so someone else can take over who is able to work on this more effectively.

@rustbot release-assignment