'Dropping temporaries' prevents borrow check to succeed in match arm without shared ownership

[Byron]

I tried this code (playground link):

struct Owner {
    to_be_shared: String,
}

impl Owner {
    fn share(&mut self) -> Result<Box<dyn ToString + '_>, ()> {
        Ok(Box::new(&self.to_be_shared) as Box<dyn ToString>)
    }
    fn do_something_else(&mut self) {}
}

fn use_owner(mut owner: Owner) {
    let _v = match owner.share() {
        Ok(v) => v,
        Err(()) => {
            owner.do_something_else();
            return;
        }
    };
}

fn main() {
    let owner = Owner {
        to_be_shared: String::new(),
    };
    use_owner(owner);
}

I expected to see this happen:

The code compiles. Especially it's possible to call owner.do_something_else() in the match branch dealing with the error, which does not borrow from owner.

Instead, this happened:

rror[E0499]: cannot borrow `owner` as mutable more than once at a time
  --> src/main.rs:16:13
   |
13 |     let _v = match owner.share() {
   |                    -------------
   |                    |
   |                    first mutable borrow occurs here
   |                    a temporary with access to the first borrow is created here ...
...
16 |             owner.do_something_else();
   |             ^^^^^ second mutable borrow occurs here
...
19 |     };
   |      - ... and the first borrow might be used here, when that temporary is dropped and runs the destructor for type `std::result::Result<std::boxed::Box<dyn std::string::ToString>, ()>`

According to The Rust Reference:Destructors I believe the drop scope should be relative to each match arm, not according to the outer scope. It appears that Rust still wants to drop the Result<_, _> which is already dissolved in the match arm.

Also, if no destructor is involved, the code compiles as expected.

The issue first occurred in gitoxide, and here is theworkaround.

Meta

rustc --version --verbose:

rustc 1.46.0 (04488afe3 2020-08-24)
binary: rustc
commit-hash: 04488afe34512aa4c33566eb16d8c912a3ae04f9
commit-date: 2020-08-24
host: x86_64-apple-darwin
release: 1.46.0
LLVM version: 10.0

rustc +nightly --version --verbose:

rustc 1.48.0-nightly (397b390cc 2020-08-27)
binary: rustc
commit-hash: 397b390cc76ba1d98f80b2a24a371f708dcc9169
commit-date: 2020-08-27
host: x86_64-apple-darwin
release: 1.48.0-nightly
LLVM version: 11.0

[csmoe]

As you might notice in the reference, the scrutinee of match isn't a temporary scope, temporary inside owner.share() will live until the end of match { ... }, so we have two mutable references here created: a) by share(); b) by do_something_else()

[Byron]

Thanks for taking the time. I suspected that this might actually be documented behaviour, but hope there is a shared understanding that this behaviour is surprising and undesirable. To the 'user' of the match statement, it does look like it's clear that in the match arm there is nothing with a constructor anymore, it seems like the Result has already been 'destructured'. Also there is no obvious way this can be achieved, as a workaround.

Do you see a way to improve the user experience around this issue? Maybe with a compiler error that suggests a workaround in these situations? Maybe there is even a way to change the behaviour of Rustc, very much in the same vein as what happened when NLL was introduced. Match statements suddenly became very intuitive to use.

Thanks again for your continued support.

[Byron]

I am now working around this issue by disabling the drop code of the box by leaking it temporarily. This being possible, to my mind, hints at the current handling of destructors in match being a technical shortcoming rather than a necessity.

For the example provided here in this issue, here is the corresponding workaround (playground):

struct Owner {
    to_be_shared: String,
}

impl Owner {
    fn share(&mut self) -> Result<Box<dyn ToString + '_>, ()> {
        Ok(Box::new(&self.to_be_shared) as Box<dyn ToString>)
    }
    fn do_something_else(&mut self) {}
}

fn use_owner(mut owner: Owner) {
    // disable drop code by leaking
    let _v = match owner.share().map(Box::leak) {
        Ok(v) => Ok::<_, ()>(v),
        Err(()) => {
            owner.do_something_else();
            return;
        }
    }
    .map(|leaked| unsafe { Box::from_raw(leaked as *mut _) }); // undo the leak right away, so the drop code runs after the match
}

fn main() {
    let owner = Owner {
        to_be_shared: String::new(),
    };
    use_owner(owner);
}

[csmoe]

Not sure the scrutinee temporary case can be improved, but the error message should be more informative at least, reopen for better suggestion.

[Byron]

Having received some pain from this issue, I should be able to come up with a message that would have helped me.
It's a tough one, as to me it made (and still makes) no sense to have the whole match expression as drop scope, which is quite clearly indicated by the message already.

A helpful suggestion would be one that helps working around the issue, and I don't know if the 'leak and unleak' workaround is anything we would want to suggest here due to the need for unsafe.

[SkiFire13]

This could be a variation of #47680, but it doesn't compile when using -ZPolonius.

Also note that this is related to the use of + '_. If share returned a Result<&'_ dyn ToString, ()> or a Result<Box<&'_ dyn ToString>, ()> it would work.

Another way to make it work would be to manually drop the value returned from share in the Err branch. This can be done by either saving the result in a temporary variable before the match or by using a @ binding:

fn use_owner(mut owner: Owner) {
    let o = owner.share();
    let _v = match o {
        Ok(v) => v,
        Err(()) => {
            drop(o);
            owner.do_something_else();
            return;
        }
    };
}

or

fn use_owner(mut owner: Owner) {
    let _v = match owner.share() {
        Ok(v) => v,
        e @ Err(()) => {
            drop(e);
            owner.do_something_else();
            return;
        }
    };
}

[Byron]

Thanks a lot! In my case, I could remove the 'leak' workaround and replace it with version 1 of the workarounds above. Much better and certainly something to remember until it is fixed.

Maybe it's even possible to turn this into a suggestion, as it seems like the one with the least 'dependencies'. Alternatively, it this one is indeed related to some dark corners of analysis, maybe it's something that should indeed be fixed there.
However, if feasible, in a first step the suggestion going along with it could be improved to hint at a workaround.

For now it is my hope that people can find this issue when searching, and will be able to help themselves.

[Byron]

It gets more interesting than that. When trying to use 'owner' once more after the match expression, there seems to be no way to accomplish this as according to borrowcheck, there is still a borrowed value. The usual 'smartness' doesn't seem to apply.

The only way I could make this work is by using a scope - the usual explicit drop(…)s don't work at all.

playground

struct Owner {
    to_be_shared: String,
}

impl Owner {
    fn share(&mut self) -> Result<Box<dyn ToString + '_>, ()> {
        Ok(Box::new(&self.to_be_shared) as Box<dyn ToString>)
    }
    fn do_something_else(&mut self) {}
}

fn use_owner(mut owner: Owner) {
    { // this scope is now required to truly sever the link between the locals and `owner`
        let res = owner.share();
        let _v = match res {
            Ok(v) => v,
            Err(()) => {
                drop(res);
                owner.do_something_else();
                return;
            }
        };
    }
    // this is the added line, which I would expect to work without the extra scope.
    owner.do_something_else();
}

fn main() {
    let owner = Owner {
        to_be_shared: String::new(),
    };
    use_owner(owner);
}

[e00E]

I encountered what I think is another instance of this issue and a workaround:

struct HoldsReference<'a> {
    reference: &'a u32,
}

// must have manual drop impl
impl<'a> std::ops::Drop for HoldsReference<'a> {
    fn drop(&mut self) {
        unimplemented!()
    }
}

struct Error {/* ... */}

fn create<'a>(_reference: &'a u32) -> Result<HoldsReference<'a>, Error> {
    unimplemented!()
}

struct Owner {
    owned: u32,
}

impl Owner {
    // must be `mut self`
    fn on_error(&mut self, _error: Error) {
        unimplemented!()
    }

    // All of the following functions seem equivalent but only the last compiles.

    fn fails_to_compile_0(&mut self) {
        if let Err(err) = create(&self.owned) {
            self.on_error(err)
        }
    }

    fn fails_to_compile_1(&mut self) {
        match create(&self.owned) {
            Ok(_) => (),
            Err(err) => self.on_error(err),
        }
    }

    fn fails_to_compile_2(&mut self) {
        match create(&self.owned) {
            Ok(_) => return,
            Err(err) => self.on_error(err),
        }
    }

    fn compiles(&mut self) {
        let err = match create(&self.owned) {
            Ok(_) => return,
            Err(err) => err,
        };
        self.on_error(err);
    }
}

fn main() {}

The error message is

error[E0502]: cannot borrow `*self` as mutable because it is also borrowed as immutable
  --> src/main.rs:30:13
   |
29 |         if let Err(err) = create(&self.owned) {
   |                           -------------------
   |                           |      |
   |                           |      immutable borrow occurs here
   |                           a temporary with access to the immutable borrow is created here ...
30 |             self.on_error(err)
   |             ^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
31 |         }
32 |     }
   |     - ... and the immutable borrow might be used here, when that temporary is dropped and runs the destructor for type `std::result::Result<HoldsReference<'_>, Error>`
   |
help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
   |
31 |         };
   |          ^

In this case the suggestion of adding a semicolon does not help.