Working around overzealous drop check

cole-miller · July 19, 2022, 10:33pm

This playground shows the shape of the code I'm writing. It compiles fine when the _nontrivial_drop field of Encoder is commented out. However, when that field is added, we get some errors from the drop check (I think):

error[E0499]: cannot borrow `*stuff` as mutable more than once at a time
  --> src/lib.rs:58:57
   |
51 |         stuff,
   |         ----- first mutable borrow occurs here
...
56 |             encoder = match encoder.block() {
   |             ------- first borrow might be used here, when `encoder` is dropped and runs the destructor for type `Encoder<'_>`
57 |                 Either::Left(enc) => break enc,
58 |                 Either::Right(save) => save.resume(grow(stuff)),
   |                                                         ^^^^^ second mutable borrow occurs here

error[E0499]: cannot borrow `*stuff` as mutable more than once at a time
  --> src/lib.rs:64:57
   |
51 |         stuff,
   |         ----- first mutable borrow occurs here
...
61 |         encoder = loop {
   |         ------- first borrow might be used here, when `encoder` is dropped and runs the destructor for type `Encoder<'_>`
...
64 |                 Either::Right(save) => save.resume(grow(stuff)),
   |                                                         ^^^^^ second mutable borrow occurs here

For more information about this error, try `rustc --explain E0499`.

Is there actually a soundness issue here? I wouldn't think so, since the drop glue for Encoder<'a> won't touch the &'a mut [u8] field. And if my code is sound, how can I (safely or unsafely) work around the drop check in this case? (I don't think the eyepatch helps.)

jonh · July 19, 2022, 11:40pm

You write functions to return the item you intend to still use. e.g.
fn block(self) -> Either<BlockEncoder<'a>, (EncoderSave, &'a mut Vec<u8>)> {

steffahn · July 19, 2022, 11:51pm

I’ve done some reduction work to get a smaller example; here’s what I got so far:

#![allow(dead_code)]

enum Either<L, R> {
    Left(L),
    Right(R),
}

struct Encoder<'a, T> {
    stuff: &'a mut [u8],
    _nontrivial_drop: T,
}

struct EncoderSave<T>(T);

impl<T> EncoderSave<T> {
    fn resume<'a>(self, _stuff: &'a mut [u8]) -> Encoder<'a, T> {
        unimplemented!()
    }
}

impl<'a, T> Encoder<'a, T> {
    fn block(self) -> Either<(), EncoderSave<T>> {
        unimplemented!()
    }
}

fn grow(_: &mut Vec<u8>) -> &mut [u8] {
    unimplemented!()
}

// fails without the `: Copy`

// but then, works again, if the out-of-loop `let mut encoder`
// becomes an ordinary `let encoder =` inside of the loop
fn branch1<T: Copy>(mut save: EncoderSave<T>, stuff: &mut Vec<u8>) -> () {
    let mut encoder;
    loop {
        encoder = save.resume(grow(stuff));
        save = match encoder.block() {
            Either::Left(enc) => break,
            Either::Right(save) => save,
        };
    };
}

quinedot · July 19, 2022, 11:52pm

One workaround is to create loop-local Encoders.

quinedot · July 20, 2022, 12:22am

Maybe this?

github.com/rust-lang/rust

spurious "borrow might be used .. when [variable] is dropped and runs the destructor"

opened 07:26AM - 08 Apr 20 UTC

comex

C-enhancement A-destructors A-borrow-checker T-lang T-compiler

[Playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&…gist=ca0e9f5171bf484d9b744d08057203ec) ```rust use std::rc::Rc; use std::cell::{RefCell, RefMut}; use std::mem::drop; fn huh(x: Rc<RefCell<i32>>) { let mut rc: Rc<RefCell<i32>> = x.clone(); let mut inner: RefMut<'_, i32> = rc.borrow_mut(); loop { drop(inner); drop(rc); rc = x.clone(); inner = rc.borrow_mut(); } } ``` produces: ``` error[E0505]: cannot move out of `rc` because it is borrowed --> src/lib.rs:9:14 | 6 | let mut inner: RefMut<'_, i32> = rc.borrow_mut(); | -- borrow of `rc` occurs here ... 9 | drop(rc); | ^^ move out of `rc` occurs here ... 12 | inner = rc.borrow_mut(); | ----- borrow might be used here, when `inner` is dropped and runs the destructor for type `std::cell::RefMut<'_, i32>` error[E0506]: cannot assign to `rc` because it is borrowed --> src/lib.rs:11:9 | 6 | let mut inner: RefMut<'_, i32> = rc.borrow_mut(); | -- borrow of `rc` occurs here ... 11 | rc = x.clone(); | ^^ assignment to borrowed `rc` occurs here 12 | inner = rc.borrow_mut(); | ----- borrow might be used here, when `inner` is dropped and runs the destructor for type `std::cell::RefMut<'_, i32>` ``` (Same error with `-Zpolonius`.) The error messages are wrong: the assignment to `inner` can never run a destructor, since the previous value of `inner` was dropped earlier on. And AFAICT this code is sound and ought to be accepted.

steffahn · July 20, 2022, 1:12am

It’s not pretty or particularly DRY, but this compiles even though the control flow is essentially “the same”; probably because it successfully avoids mutating encoder or block_encoder in any loop that also involves repeated .resume(grow(stuff)) calls:

#![allow(dead_code)]

enum Either<L, R> {
    Left(L),
    Right(R),
}

struct Encoder<'a> {
    stuff: &'a mut [u8],
    _nontrivial_drop: Vec<u8>,
}

struct EncoderSave;

impl EncoderSave {
    fn resume(self, _stuff: &mut [u8]) -> Encoder<'_> {
        unimplemented!()
    }
}

impl<'a> Encoder<'a> {
    fn block(self) -> Either<BlockEncoder<'a>, EncoderSave> {
        unimplemented!()
    }
}

struct BlockEncoder<'a> {
    stuff: &'a mut [u8],
    _nontrivial_drop: Vec<u8>,
}

struct BlockEncoderSave;

impl BlockEncoderSave {
    fn resume(self, _stuff: &mut [u8]) -> BlockEncoder<'_> {
        unimplemented!()
    }
}

impl<'a> BlockEncoder<'a> {
    fn encode(self) -> Either<Encoder<'a>, BlockEncoderSave> {
        unimplemented!()
    }
}

fn grow(_: &mut Vec<u8>) -> &mut [u8] {
    unimplemented!()
}

fn go(stuff: &mut Vec<u8>) {
    let mut encoder = Encoder {
        stuff,
        _nontrivial_drop: Vec::new(),
    };
    let mut block_encoder;
    loop {
        match encoder.block() {
            Either::Left(enc) => {
                block_encoder = enc;
            }
            Either::Right(mut save) => {
                let mut bsave;
                loop {
                    'x: loop {
                        let mut encoder = save.resume(grow(stuff));
                        let mut block_encoder;
                        loop {
                            match encoder.block() {
                                Either::Left(enc) => {
                                    block_encoder = enc;
                                }
                                Either::Right(save_) => {
                                    save = save_;
                                    continue 'x;
                                }
                            }
                            match block_encoder.encode() {
                                Either::Left(enc) => {
                                    encoder = enc;
                                }
                                Either::Right(bsave_) => {
                                    bsave = bsave_;
                                    break 'x;
                                }
                            }
                        }
                    }
                    'y: loop {
                        let mut encoder;
                        let mut block_encoder = bsave.resume(grow(stuff));
                        loop {
                            match block_encoder.encode() {
                                Either::Left(enc) => {
                                    encoder = enc;
                                }
                                Either::Right(bsave_) => {
                                    bsave = bsave_;
                                    continue 'y;
                                }
                            }
                            match encoder.block() {
                                Either::Left(enc) => {
                                    block_encoder = enc;
                                }
                                Either::Right(save_) => {
                                    save = save_;
                                    break 'y;
                                }
                            }
                        }
                    }
                }
            }
        }
        match block_encoder.encode() {
            Either::Left(enc) => {
                encoder = enc;
            }
            Either::Right(mut bsave) => {
                let mut save;
                loop {
                    'y: loop {
                        let mut encoder;
                        let mut block_encoder = bsave.resume(grow(stuff));
                        loop {
                            match block_encoder.encode() {
                                Either::Left(enc) => {
                                    encoder = enc;
                                }
                                Either::Right(bsave_) => {
                                    bsave = bsave_;
                                    continue 'y;
                                }
                            }
                            match encoder.block() {
                                Either::Left(enc) => {
                                    block_encoder = enc;
                                }
                                Either::Right(save_) => {
                                    save = save_;
                                    break 'y;
                                }
                            }
                        }
                    }
                    'x: loop {
                        let mut encoder = save.resume(grow(stuff));
                        let mut block_encoder;
                        loop {
                            match encoder.block() {
                                Either::Left(enc) => {
                                    block_encoder = enc;
                                }
                                Either::Right(save_) => {
                                    save = save_;
                                    continue 'x;
                                }
                            }
                            match block_encoder.encode() {
                                Either::Left(enc) => {
                                    encoder = enc;
                                }
                                Either::Right(bsave_) => {
                                    bsave = bsave_;
                                    break 'x;
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

steffahn · July 20, 2022, 2:26am

In terms of practical workarounds: this seems to work nicely and without overhead:

fn go(stuff: &mut Vec<u8>) {
    let mut encoder = ManuallyDrop::new(Encoder {
        stuff,
        _nontrivial_drop: Vec::new(),
    });
    loop {
        let mut block_encoder = loop {
            encoder = ManuallyDrop::new(match ManuallyDrop::into_inner(encoder).block() {
                Either::Left(enc) => break enc,
                Either::Right(save) => save.resume(grow(stuff)),
            });
        };
        encoder = loop {
            block_encoder = match block_encoder.encode() {
                Either::Left(enc) => break ManuallyDrop::new(enc),
                Either::Right(save) => save.resume(grow(stuff)),
            };
        };
    }
}

Might need block_encoder wrapped similarly if that one has nontrivial drop, too.

There isn’t even any downsides such as leak on panic, since the encoder is always immediately consumed with …::into_inner without anything that can panic in-between. I think. It's probably easy to restructure the code in order to make this even more obvious.

cole-miller · July 20, 2022, 3:09am

(Looking over my code again I'm struck by the bad/confusing type and variable names. Sorry about that.)

Thanks, this seems promising. I'll report back whether I'm able to apply it to my real code.

I need to study this a bit more before I can wrap my head around it. It seems like the key is to temporarily split the nontrivial-Drop part of Encoder from the &mut part, so the compiler can't see problematic interaction between them.

Looks familiar. I guess I misidentified the issue in my OP—the destructor of Encoder<'a> never runs at all, because the value is consumed by Encoder::block instead, but the compiler doesn't see that for whatever reason.

Neat! I'd love to see whether the core logic here can be packaged up into a utility method on Encoder or something, because ultimately I want consumers of my library to be able to write this kind of loop in their own code.