Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regression: cannot borrow ... as immutable because it is also borrowed as mutable #135671

Open
BoxyUwU opened this issue Jan 18, 2025 · 9 comments · Fixed by #135709
Open

regression: cannot borrow ... as immutable because it is also borrowed as mutable #135671

BoxyUwU opened this issue Jan 18, 2025 · 9 comments · Fixed by #135709
Assignees
Labels
P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 18, 2025

[INFO] [stdout] error[E0502]: cannot borrow `*inputs` as immutable because it is also borrowed as mutable
[INFO] [stdout]    --> examples/basic.rs:267:27
[INFO] [stdout]     |
[INFO] [stdout] 263 |           if let Some(grd) = inputs[0].grad.as_mut() {
[INFO] [stdout]     |                              -------------- mutable borrow occurs here
[INFO] [stdout] 264 | /             *grd += output_grad
[INFO] [stdout] 265 | |                 * if inputs[0].val > 0.0 {
[INFO] [stdout] 266 | |                     1.0
[INFO] [stdout] 267 | |                 } else if inputs[0].val == 0.0 {
[INFO] [stdout]     | |                           ^^^^^^^^^ immutable borrow occurs here
[INFO] [stdout] ...   |
[INFO] [stdout] 270 | |                     -1.0
[INFO] [stdout] 271 | |                 };
[INFO] [stdout]     | |_________________- mutable borrow later used here
@BoxyUwU BoxyUwU added regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue. labels Jan 18, 2025
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 18, 2025
@compiler-errors
Copy link
Member

This regressed in #133734 cc @scottmcm

instead -- with some asterisks for arrays and &mut that need it to be done slightly differently.

I assume that maybe there was something missing here 🤔

@compiler-errors
Copy link
Member

compiler-errors commented Jan 18, 2025

Minimal:

struct Test {
    a: i32,
    b: i32,
}

fn main() {
    let inputs: &mut [_] = &mut [Test { a: 0, b: 0 }];
    let a = &mut inputs[0].a;
    let b = &mut inputs[0].b;

    *a = 0;
    *b = 1;
}

@compiler-errors
Copy link
Member

I think we should probably revert #133734 (and the follow-up PR that fully removed the Len operand), and spend some more time on a solution that is both resilient to borrowck and also to miri.

Specifically, the problem here is that the borrow-checker treated the Len operand specially (as a kind of fake borrow) which allowed it to be interleaved with mutable live mutable references pointing into the slice. The same does not apply to the RawPtr operand (&raw const) that we emit as part of the new lowering (all of this happens because we must read the slice length to emit a bounds check before slice accesses). Instead, in MIR borrowck, we currently treat RawPtr operands like borrows, so the metadata access for let b = ... is treated like an access conflict with the mutable reference of let a = ....

After this revert, we could either think about:

  • Changing the semantics of &raw (const|mut) operand in borrowck to not act like an access
  • Emitting a new kind of special copy operand (much like CopyForDeref) that allows us to treat the access of an array for length access as disjoint.
  • Some other solution...

However, in the mean time, I'd rather we not crunch trying to find and more importantly validate the soundness of a solution 🤔

@steffahn
Copy link
Member

steffahn commented Jan 18, 2025

Oh wow, that minimal repro is … just normal code very deliberately supported by borrow checking for a long time!? There was no UI test for that? I’ve even shared code examples like this in the forums before … multiple times o.O

(One example is here, e.g. the very first code block is broken on beta. And here’s another one, the example bar2 in the playground behind the last paragraph’s link.)

I suppose, this means I should write down some of this stuff as UI tests, right?


By the way, tuples could be used, too… e.g. for a minimal repro without defining a struct:

fn main() {
    let slice = &mut [(0, 0)][..];
    std::mem::swap(&mut slice[0].0, &mut slice[0].1);
}

And here’s an example similar to the latter one of my forum-examples linked above

fn foo(a: &mut [(i32, i32)], i: usize, j: usize) -> (&mut i32, &mut i32) {
    (&mut a[i].0, &mut a[j].1)
}

@compiler-errors
Copy link
Member

I suppose, this means I should write down some of this stuff as UI tests, right?

@steffahn: If you want to contribute some tests that exercise disjoint borrows, feel free to. I'll review them.

@lqd
Copy link
Member

lqd commented Jan 19, 2025

Fixed on nightly. Reopening to track beta backport.

@lqd lqd reopened this Jan 19, 2025
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jan 20, 2025
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 23, 2025
@apiraino
Copy link
Contributor

closing since #135709 was merged

@lqd
Copy link
Member

lqd commented Jan 29, 2025

Reopening as #135709 wasn't merged.

@lqd lqd reopened this Jan 29, 2025
@saethlin saethlin self-assigned this Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants