# Scope issues 2021 edition

## Motivation

I think the borrow checker/destructor behavior currently makes the compiler less empathetic than it could be. This is an issue because:

* Workarounds can be ugly or non-obvious, requiring extra effort to add comments to explain the limitations. For example #2 below, I had to adjust many tests in my code base in a way which I felt should be unnecessary -- just adding boilerplate, "manual memory [resource] management" that we usually don't need.
* For people just learning Rust, the compiler raising errors for things that are conceptually safe can be a reason to turn away.
* For experienced Rustaceans, it can be very surprising when the otherwise empathetic/advanced compiler throws up a roadblock. Personally, I usually look for ways to convince the compiler what I'm doing it makes sense (asking for help if necessary), which can definitely slow me down (I probably spent a few hours on example #1 below).

I think we need to adjust some of this logic to use the control flow graph rather than lexical scope to guide dropping. I also wonder about memory usage dropping if we free allocations more eagerly.

## Examples

### 1. Tail expressions

Here's the issue I ran into a few weeks ago:

use std::collections::BinaryHeap;
use std::collections::binary_heap::PeekMut;

struct Foo {
    inner: BinaryHeap<usize>,
}

impl Foo {
    fn next(&mut self) -> Option<PeekMut<'_, usize>> {
        loop {
            let num = match self.inner.peek_mut() {
                Some(num) => num,
                None => return None,
            };
            
            if *num < 5 {
                continue;
            }
            
            break Some(num)
        }
    }
}

This results in an error:

error[E0499]: cannot borrow `self.inner` as mutable more than once at a time
  --> src/lib.rs:11:29
   |
9  |     fn next(&mut self) -> Option<PeekMut<'_, usize>> {
   |             - let's call the lifetime of this reference `'1`
10 |         loop {
11 |             let num = match self.inner.peek_mut() {
   |                             ^^^^^^^^^^ mutable borrow starts here in previous iteration of loop
...
20 |             break Some(num)
   |                   --------- returning this value requires that `self.inner` is borrowed for `'1`

(This is a simplified version of Quinn's [`Assembler`](https://github.com/quinn-rs/quinn/blob/main/quinn-proto/src/connection/assembler.rs).)

It seems obvious to me that the return value will live as long as the self argument. It was explained to me as "any borrow that ends up in the return value ends up being considered live for code that lexically follows the return statement, even though that's obviously nonsense", pointing at [rust-lang/rust#51132](https://github.com/rust-lang/rust/issues/51132).

I ended up [splitting the method](https://github.com/rust-lang/rust/issues/51132) out the type and (in the real version) take 4 separate arguments that are part of `Foo`'s state that were necessary to fulfill its goals, before discarding this approach because it ended up too complex (not only due to this issue).

### 2. impl Drop invalidates code

In the same bout of refactoring, I ran into this issue:

fn simple() {
    let mut foo = Foo;
    let mut adapter = foo.simple();
    adapter.next();
    adapter.next();
    let mut adapter = foo.simple();
    adapter.next();
}

struct FooAdapter<'a>(&'a mut Foo);

impl<'a> FooAdapter<'a> {
    fn next(&mut self) -> Option<usize> {
        Some(3)
    }
}

fn sophisticated() {
    let mut foo = Foo;
    let mut adapter = foo.sophisticated();
    adapter.next();
    adapter.next();
    let mut adapter = foo.sophisticated();
    adapter.next();
}

struct FooAdapterWithDrop<'a>(&'a mut Foo);

impl<'a> FooAdapterWithDrop<'a> {
    fn next(&mut self) -> Option<usize> {
        Some(5)
    }
}

impl<'a> Drop for FooAdapterWithDrop<'a> {
    fn drop(&mut self) {
        println!("droppy");
    }
}

struct Foo;

impl Foo {
    fn simple(&mut self) -> FooAdapter<'_> {
        FooAdapter(self)
    }
    
    fn sophisticated(&mut self) -> FooAdapterWithDrop<'_> {
        FooAdapterWithDrop(self)
    }
}

While `simple()` compiles, `sophisticated()` does not:

error[E0499]: cannot borrow `foo` as mutable more than once at a time
  --> src/lib.rs:24:23
   |
21 |     let mut adapter = foo.sophisticated();
   |                       --- first mutable borrow occurs here
...
24 |     let mut adapter = foo.sophisticated();
   |                       ^^^ second mutable borrow occurs here
25 |     adapter.next();
26 | }
   | - first borrow might be used here, when `adapter` is dropped and runs the `Drop` code for type `FooAdapterWithDrop`

I found it surprising that just implementing `Drop` for a type can cause downstream code to trivially fail to compile, for what I find to be no good reason: the first `adapter` is dead where the second one is instantiated, so I feel the compiler should allow this.

### 3. Dropping locks before await points

Here's a piece of code [from bb8](https://github.com/djc/bb8/blob/main/bb8/src/inner.rs#L112):

let (tx, rx) = oneshot::channel();
{
    let mut locked = self.inner.internals.lock();
    let approvals = locked.push_waiter(tx, &self.inner.statics);
    self.spawn_replenishing_approvals(approvals);
};

match timeout(self.inner.statics.connection_timeout, rx).await {
    Ok(Ok(mut guard)) => Ok(PooledConnection::new(self, guard.extract())),
    _ => Err(RunError::TimedOut),
}

I think the inner lexical scope there should not be necessary. The compiler should know to drop `locked` before the `timeout().await`.