NLL: regressions due to weakness in two-phase borrows

[pnkfelix]

Look at rust-lang/rust-clippy#2982

It seems like some of the changes NLL is requiring were expected to be handled silently by two phase borrows

[pnkfelix]

(its also possible that rust-lang/rust-clippy#2982 was in fact due to this oversight in the 2018 migration mode: #52967 )

[nikomatsakis]

Based on the contents of rust-lang/rust-clippy#2982, it appears @pnkfelix that you now believe these to be legitimate regressions (i.e., NLL-fixed-by-NLL)? I'm inclined therefore to close this issue? Or am I wrong...

[nikomatsakis]

Leaving untriaged for now.

[ghost]

This is fixed. The borrow checker warnings aren't emitted with the current nightly compiler. See rust-lang/rust-clippy#3085.

[nikomatsakis]

Nominating for discussion — probably we can just close this? I put it on the milestone too just to help us keep things tidy

[nikomatsakis]

Hmm, I think I would like to have some minimized reproductions of the cases in question prior to closing. I'd like to look at just what was making them fail before and convince myself that the new behavior is not going to have problems like #38899 (that particular problem does not reoccur, I tested :)

[pnkfelix]

@nikomatsakis its not that I thought they were legitimate regressions.

It that I believe(d) rust-lang/rust-clippy#2982 was mis-identified (by me) as arising due to a fault or weakness in 2PB, when in fact it was a much simpler problem that was subsequently fixed.

Its quite possible that we do need an issue tracking all the cases where 2PB could be improved. But I don't think it would be useful for this issue to serve that purpose.

[pnkfelix]

Having said that, I think I agree that some minimized tests that model what was going on in rust-lang/rust-clippy#2982 may indeed be a good idea, assuming that those cases really do significantly differ from any tests that were part of PR #52967

[pnkfelix]

(I'll do the minimization, since I already had dipped my toe in this a month ago.)

[pnkfelix]

Okay here is a somewhat minimized test. (I say "somewhat" because there are trivial ways to further reduce it that would obscure its origins within clippy and librustc. I think think this minimization represents a sweet spot in terms of inferring the relationship with the original source.)

playground link

#![allow(dead_code)]

#[derive(Copy, Clone)]
struct BodyId;
struct AnonConst { body: BodyId }
enum ExprKind { Closure(BodyId), Repeat(AnonConst), Others }
struct Expr { node: ExprKind, }

struct Body { value: Expr }

struct Map;
impl Map { fn body(&self, _: BodyId) -> &Body { loop { } } }

struct SpanlessHash<'a> { cx: &'a Map, }

impl <'a> SpanlessHash<'a> {
    fn hash_expr(&mut self, e: &Expr) {
        match e.node {
            ExprKind::Closure(eid) => {
                self.hash_expr(&self.cx.body(eid).value);
            },
            ExprKind::Repeat(ref l_id) => {
                self.hash_expr(&self.cx.body(l_id.body).value);
            },
            _ => {}
        }
    }
}

struct Config;
trait LateLintPass<'a> { }
type LateLintPassObject = Box<dyn for<'a> LateLintPass<'a>>;

struct TriviallyCopyPassByRef { }
impl TriviallyCopyPassByRef { fn new(_: &Config) -> Self { loop { } } }
impl<'a> LateLintPass<'a> for TriviallyCopyPassByRef { }

struct Session { target: Config }
struct Registry<'a> { sess: &'a Session }
impl<'a> Registry<'a> {
    fn register_late_lint_pass(&mut self, _: LateLintPassObject) { loop { } }
}

fn register_plugins(reg: &mut Registry<'_>) {
    reg.register_late_lint_pass(Box::new(TriviallyCopyPassByRef::new(&reg.sess.target)));
}

fn main() { }

[pnkfelix]

Now, having done the minimization, there are two cases to consider:

1. The self.hash_expr(&self.cx.body(..).value) invocations on lines 20 and 23,
2. The reg.register_late_lint_pass(Box::new(Thing::new(&reg.sess.target))) invocation on line 45

I think niko has already expressed a little concern about case 1 in a comment here on the clippy repo.

(I want to look a little more carefully at both cases before I add more notes...)

[pnkfelix]

Result of investigating case 1: The reason that the call to self.hash_expr is allowed there, AFAICT, is that self has an immutable borrow somewhere along the path from it to the AST map it borrows, and therefore a &mut self still cannot do anything to interfere with the result of the &self.cx.body(..).value.

For example, this works: https://play.rust-lang.org/?gist=bd9009010499a92e0cfb9985d7bf1330&version=nightly&mode=debug&edition=2018

But if I change the latter to mutably borrow the AST map from SpanlessHash, it stops working: https://play.rust-lang.org/?gist=b3e05fdc13eff13093541487df2bfa06&version=nightly&mode=debug&edition=2018

This is enough to convince me that we aren't seeing evidence of unsoundness from case 1 in my previous comment.

(I'm going to dive into case 2 next.)

[pnkfelix]

I think case 2 is another instance of the same phenomenon: the Registry has only an immutable reference to the Session, and so the reg.register_late_lint_pass does not interfere with the &reg.sess.target borrow.

[pnkfelix]

Hmm I may have spoken too soon for case 2; unlike the previous analysis, changing to struct Registry<'a> { sess: &'a mut Session } does not cause the compiler to start rejecting the example. Investigating now.

[pnkfelix]

Oh, I think this explains it: in both the original source and in my minimization, struct TriviallyCopyPassByRef carries no references (and has no lifetimes on its type). That might explain why changing the registry to have a &mut Session did not cause any errors to trigger.

But if you change to a &mut Session and switch back to the 2015 edition, it will: https://play.rust-lang.org/?gist=30fae44818a42cd3f931493180199743&version=nightly&mode=debug&edition=2015

[pnkfelix]

One might reasonably wonder: "Wait, what do those examples (case 1 and 2 from my above comment) have to do with two-phase borrows."

I think the best answer to might be for me to just add two tests using revisions to illustrate the cases here, with a revision set like { borrowck=ast, borrowck=mir, borrowck=mir+two-phase-borrows }, and then that should show how the errors arise when you leave out support for two-phase-borrows.

[pnkfelix]

(I have a test written up, will post a PR soon.)