Minor tweaks to refcell logging

[nealsid]

• Fix the logging when debug_refcell is enabled so the location is output as a string rather than a sequence of bytes
• Avoid logging an empty struct when debug_refcell is disabled
• Make the panic error message consistent with the compiler borrow-checking error messages

[rustbot]

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[tgross35]

Thank you for the PR!

The wording isn't quite accurate because panic_already_borrowed now says "...because it is also borrowed as immutable", but borrow_mut will also fail if there is an existing mutable borrow. The current messages aren't ideal either because

let cell = RefCell::new(0u32);
let b = (cell.borrow_mut(), cell.borrow_mut());
drop(b);

also prints "already borrowed: BorrowMutError" rather than the more accurate "already mutably borrowed: BorrowMutError", but it still works.

So I think the original wording of "already (mutably) borrowed (here: {err:?})" is probably fine to keep. We could alternatively add track mutably/immutably in the error type, but I don't expect that to be worth it since the # Panics API docs are pretty easy to locate (by comparison, with the rustc error it might not be as obvious that you are mutably/immutably borrowing because e.g. it's in a function signature).

Fix the logging when debug_refcell is enabled so the location is output as a string rather than a sequence of bytes

Mind commenting the before and after here? Location's debug should just print the file, line, and column.

[tgross35]

Small nit, this can be _err -> err since it is used

[nealsid]

_err is used in one code path (when debug_refcell is enabled) but not the other, the reason being is that when debug_refcell is disabled, BorrowError/BorrowMutError are empty structs.

[tgross35]

Right, but whether or not something gets used on all branches doesn't affect liveliness - one is enough. It would be different if the comptime #[cfg(...)] was used rather than the runtime cfg!() (semantically runtime at least, even though it's gone with the most basic optimizations) but there won't be any dead_code warnings here.

[tgross35]

@rustbot author for the above

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[nealsid]

Fix the logging when debug_refcell is enabled so the location is output as a string rather than a sequence of bytes

Mind commenting the before and after here? Location's debug should just print the file, line, and column.

Location only implements Display, not Debug (link). Below is the output if you enable debug_refcell and cause a panic today:

     Running `target\debug\refcell-test.exe`
Hello, world! RefCell { value: 5 }

thread 'main' panicked at src\main.rs:11:15:
already borrowed: BorrowMutError { location: Location { file_bytes_with_nul: [115, 114, 99, 92, 109, 97, 105, 110, 46, 114, 115, 0], line: 8, col: 15 } }
stack backtrace:
   0: std::panicking::begin_panic_handler
             at C:\Users\neals\rust\library\std\src\panicking.rs:697
   1: core::panicking::panic_fmt
             at C:\Users\neals\rust\library\core\src\panicking.rs:75
   2: core::cell::panic_already_borrowed
             at C:\Users\neals\rust\library\core\src\cell.rs:791
   3: core::cell::RefCell<i32>::borrow_mut<i32>
             at C:\Users\neals\rust\library\core\src\cell.rs:1083
   4: refcell_test::main
             at .\src\main.rs:11
   5: core::ops::function::FnOnce::call_once<void (*)(),tuple$<> >
             at C:\Users\neals\rust\library\core\src\ops\function.rs:250
   6: core::hint::black_box
             at C:\Users\neals\rust\library\core\src\hint.rs:482
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
error: process didn't exit successfully: `target\debug\refcell-test.exe` (exit code: 101)
PS C:\Users\neals\refcell-test>

[nealsid]

The panic output after my proposed change (I haven't reverted the first logging change yet, this is just meant to show the change in Location's output):

     Running `target\debug\refcell-test.exe`
Hello, world! RefCell { value: 5 }

thread 'main' panicked at src\main.rs:11:15:
cannot borrow as mutable because it is also borrowed as immutable here BorrowMutError { location: src\main.rs:8:15 }
stack backtrace:
   0: std::panicking::begin_panic_handler
             at C:\Users\neals\rust\library\std\src\panicking.rs:697
   1: core::panicking::panic_fmt
             at C:\Users\neals\rust\library\core\src\panicking.rs:75
   2: core::cell::panic_already_borrowed
             at C:\Users\neals\rust\library\core\src\cell.rs:792
   3: core::cell::RefCell<i32>::borrow_mut<i32>
             at C:\Users\neals\rust\library\core\src\cell.rs:1091
   4: refcell_test::main
             at .\src\main.rs:11
   5: core::ops::function::FnOnce::call_once<void (*)(),tuple$<> >
             at C:\Users\neals\rust\library\core\src\ops\function.rs:250
   6: core::hint::black_box
             at C:\Users\neals\rust\library\core\src\hint.rs:482
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
error: process didn't exit successfully: `target\debug\refcell-test.exe` (exit code: 101)
PS C:\Users\neals\refcell-test>

[nealsid]

Thank you for the PR!

The wording isn't quite accurate because panic_already_borrowed now says "...because it is also borrowed as immutable", but borrow_mut will also fail if there is an existing mutable borrow. The current messages aren't ideal either because

let cell = RefCell::new(0u32);
let b = (cell.borrow_mut(), cell.borrow_mut());
drop(b);

also prints "already borrowed: BorrowMutError" rather than the more accurate "already mutably borrowed: BorrowMutError", but it still works.

So I think the original wording of "already (mutably) borrowed (here: {err:?})" is probably fine to keep. We could alternatively add track mutably/immutably in the error type, but I don't expect that to be worth it since the # Panics API docs are pretty easy to locate (by comparison, with the rustc error it might not be as obvious that you are mutably/immutably borrowing because e.g. it's in a function signature).

Sounds good, thanks! I'll revert that part of the logging.

Your example code made me think of another potential issue:

    let v = RefCell::new(5);
    let w = v.borrow();
    let y = v.borrow();
    drop(w);
    let x = v.borrow_mut();

In this case, the RefCell will track the location of the borrow for w (since it tracks the first borrow: link) and log that in the panic, although y is the conflicting borrow when borrow_mut() is called on the last line. Is that something that should be fixed?

[tgross35]

Location only implements Display, not Debug (link). Below is the output if you enable debug_refcell and cause a panic today:

It does implement Debug via the derive, but you're right that it just prints the bag of bytes. Looks like this is a very recent regression #135054. We should fix the Debug implementation instead since that affects more than this, so you can actually drop the format_args!("{}", self.location) here.

Opened #142279 to track that.

In this case, the RefCell will track the location of the borrow for w (since it tracks the first borrow: link) and log that in the panic, although y is the conflicting borrow when borrow_mut() is called on the last line. Is that something that should be fixed?

It would be nice, but there isn't an easy way to do this is there? We would need to store multiple locations since we can't tell which will get released last, which would mean keeping some kind of location collection rather than just incrementing/decrementing the borrow count.

[nealsid]

I seem to have hosed my repo, sorry about that! Please ignore.

[nealsid]

@rustbot label +S-waiting-on-author -S-waiting-on-review

[nealsid]

The wording isn't quite accurate because panic_already_borrowed now says "...because it is also borrowed as immutable", but borrow_mut will also fail if there is an existing mutable borrow. The current messages aren't ideal either because

Ah, I missed that, thanks!

let cell = RefCell::new(0u32);
let b = (cell.borrow_mut(), cell.borrow_mut());
drop(b);

also prints "already borrowed: BorrowMutError" rather than the more accurate "already mutably borrowed: BorrowMutError", but it still works.

So I think the original wording of "already (mutably) borrowed (here: {err:?})" is probably fine to keep. We could alternatively add track mutably/immutably in the error type, but I don't expect that to be worth it since the # Panics API docs are pretty easy to locate (by comparison, with the rustc error it might not be as obvious that you are mutably/immutably borrowing because e.g. it's in a function signature).

Agreed on it not being worth it to track mutably/immutably in the error type. However, I still think the error messages are a bit cryptic, even if the audience is developers (or, in the case of enabling debug_refcell, developers who have rebuilt std). They require some spelunking into the code to understand, especially to understand the location field (because which previous borrow it picked to log isn't entirely clear).

How about the following?

Without debug_refcell enabled:

thread 'main' panicked at src\main.rs:9:15:
RefCell already mutably borrowed

With debug_refcell enabled:

thread 'main' panicked at src\main.rs:9:15:
RefCell already mutably borrowed, a previous borrow was at this location: src\main.rs:8:15

[nealsid]

@tgross35 In my testing I've noticed that the Debug impl for BorrowError/BorrowMutError don't output anything different from having BorrowError/BorrowMutError having #[derive(Debug)] added to them, unless I'm missing something. What do you think about removing them? Should be less code to maintain while retaining backward-compatibility.

[nealsid]

In this case, the RefCell will track the location of the borrow for w (since it tracks the first borrow: link) and log that in the panic, although y is the conflicting borrow when borrow_mut() is called on the last line. Is that something that should be fixed?

It would be nice, but there isn't an easy way to do this is there? We would need to store multiple locations since we can't tell which will get released last, which would mean keeping some kind of location collection rather than just incrementing/decrementing the borrow count.

Yeah, I don't see an easy way to do that, and even if we did store a location collection, it would be out of date after borrows were dropped.