unsoundness caused by hidden borrow in nightly match ergonomics

[bvinc]

Playground

#[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub enum Foo {
    ReallyReallyReallyLongName,
    Foo2,
    Foo3,
}

struct Iter {
    done: bool
}

impl Iterator for Iter {
    type Item = Result<Foo, String>;

    fn next(&mut self) -> Option<Self::Item> {
        if self.done { return None; }
        self.done = true;
        Some(Ok(Foo::ReallyReallyReallyLongName))
    }
}

fn main() {
    let iter = Iter{done: false};
    let mut iter = iter.peekable();
    
    // peek() returns type Option<&Result<Foo, String>>
    // pattern Some(Ok(foo)) is used instead of Some(&Ok(foo))
    // This causes foo to be a hidden borrow of type &Foo
    while let Some(Ok(foo)) = iter.peek() {
        iter.next();  // destroys what foo is pointing to
        println!("foo={:?}", *foo);
    }
}

Output:

Illegal instruction (core dumped)

The above code should fail the borrow checker. They key line seems to be the pattern match. It creates a borrow that seems to be invisible to the borrow checker. This appears to only be possible in nightly due to the match_default_bindings feature.

[varkor]

Note that this is not a problem when using NLL: playground link.

[nikomatsakis]

Sigh. It works with an explicit ref, as well, which suggests that the problem is the ExprUseVisitor.

[nikomatsakis]

OK, so I found the bug. Not sure yet what is best fix. Let me write out what is going on:

• ExprUseVisitor winds up aborting because it invokes the mem-categorization code on a pattern gets back an Err result. It assumes (incorrectly) that this indicates a compilation error was reported in typeck.
• mem-categorization is correctly looking at the desugared pattern Some(&Ok(ref foo))
  • however, it is incorrectly tracking the type of the value being matched
  • that type starts out correct, as Option<&Result<..>>
  • but when we pass through the Some pattern to the subpatterns:
    • we incorrectly adjust the type to Result, not &Result
    • this is because we get the type from pat_ty:

rust/src/librustc/middle/mem_categorization.rs

Line 1265 in 56714ac

let subpat_ty = self.pat_ty(&subpat)?; // see (*2)

So we need to stop doing that. Ah, I guess the pat_adjustments array stores the information we need:

rust/src/librustc/ty/context.rs

Lines 368 to 381 in 56714ac

	/// Stores the types which were implicitly dereferenced in pattern binding modes
	/// for later usage in HAIR lowering. For example,
	///
	/// ```
	/// match &&Some(5i32) {
	/// Some(n) => {},
	/// _ => {},
	/// }
	/// ```
	/// leads to a `vec![&&Option<i32>, &Option<i32>]`. Empty vectors are not stored.
	///
	/// See:
	/// https://github.com/rust-lang/rfcs/blob/master/text/2005-match-ergonomics.md#definitions
	pat_adjustments: ItemLocalMap<Vec<Ty<'tcx>>>,

[leoyvens]

I think we should not stabilize language features right before beta is branched. I don't know if this issue in particular is big deal to backport or not, but stabilizing things at the begging/middle of the cycle avoids rushing to stabilize and then rushing to fix and backport regressions.

[nikomatsakis]

( Fix in #49714 )

[nikomatsakis]

@leodasvacas

I don't know if this issue in particular is big deal to backport or not, but stabilizing things at the begging/middle of the cycle avoids rushing to stabilize and then rushing to fix and backport regressions.

In this case, the fix is trivial -- you can see #49714.

In general, we thought about trying to time landings into the middle of the cycle back in the early days, but we wound up concluding that there is no "right time" to land something. Any change can break stuff, of course, and wherever we draw the line, we've got a shot at getting something breaking.

So I don't know. Maybe it's worth putting rules against stabilizing, but I kinda don't think so. At worst we can "unstabilize" (which I fear we will have to do for !...)