Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NLL / MIR-borrowck regression in serde #48179

Closed
SimonSapin opened this issue Feb 13, 2018 · 16 comments
Closed

NLL / MIR-borrowck regression in serde #48179

SimonSapin opened this issue Feb 13, 2018 · 16 comments
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug.

Comments

@SimonSapin
Copy link
Contributor

On nightly-2018-02-13 16362c7, RUSTFLAGS=-Zborrowck=mir cargo build causes the first error below and RUSTFLAGS=-Znll cargo build --all-features causes the second one. I don’t know if they’re related.

   Compiling serde v1.0.27 (file:///home/simon/projects/servo-deps/serde/serde)
error[E0505]: cannot move out of `self` because it is borrowed
    --> serde/src/private/de.rs:1152:22
     |
1151 |                 Content::Map(ref v) if v.is_empty() => visitor.visit_unit(),
     |                              ----- borrow of `self.content.0` occurs here
1152 |                 _ => self.deserialize_any(visitor),
     |                      ^^^^ move out of `self` occurs here

error: aborting due to previous error

error: Could not compile `serde`.
error: internal compiler error: unresolved type in dtorck

error: aborting due to previous error

error: Could not compile `serde`.

CC @nikomatsakis

@pietroalbini pietroalbini added A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. WG-compiler-nll labels Feb 13, 2018
@sapphire-arches sapphire-arches added this to the NLL: Valid code works milestone Feb 13, 2018
@Aaron1011
Copy link
Member

@SimonSapin: While I can reproduce the second error (unresolved type in dtorck), I can't reproduce the first. Is it fixed on the latest nightly for you as well?

@SimonSapin
Copy link
Contributor Author

No, I can still reproduce both on rustc 1.25.0-nightly (3ec5a99 2018-02-14).

@Aaron1011
Copy link
Member

@SimonSapin: Oops, I misread your first comment. I didn't see that it took two sets of flags to produce the errors.

My understanding is that the combination of -Z nll -Z borrowck=mir -Z nll is what will eventually be made default (and is what is currently set by #![feature(nll)]. Since the first error goes away when adding -Z nll, I think it should be considered 'fixed'.

unresolved type in dtorck occurs with all flags, however, so it's definitely a bug.

@SimonSapin
Copy link
Contributor Author

Doesn’t NLL imply MIR borrow-checking?

Isn’t -Znll the same as feature(nll)? (Except that RUSTFLAGS applies to dependencies too.)

I’m not sure what you mean by "two sets of flags". The two full, independent commands I run in the serde repo are:

  • RUSTFLAGS=-Zborrowck=mir cargo build
  • RUSTFLAGS=-Znll cargo build --all-features

@Aaron1011
Copy link
Member

@SimonSapin: #![feature(nll)] and -Z nll are two different things. #![feature(nll)] is equivalent to -Z borrowck=mir -Z nll -Z two-phase-borrows. Using -Z nll implies -Z borrowck=mir, but not -Z two-phase-borrows.

For more details, see the code that handles it:

pub fn nll(&self) -> bool {
self.features.borrow().nll || self.opts.debugging_opts.nll
}
/// If true, we should use the MIR-based borrowck (we may *also* use
/// the AST-based borrowck).
pub fn use_mir(&self) -> bool {
self.borrowck_mode().use_mir()
}
/// If true, we should gather causal information during NLL
/// checking. This will eventually be the normal thing, but right
/// now it is too unoptimized.
pub fn nll_dump_cause(&self) -> bool {
self.opts.debugging_opts.nll_dump_cause
}
/// If true, we should enable two-phase borrows checks. This is
/// done with either `-Ztwo-phase-borrows` or with
/// `#![feature(nll)]`.
pub fn two_phase_borrows(&self) -> bool {
self.features.borrow().nll || self.opts.debugging_opts.two_phase_borrows
}
/// What mode(s) of borrowck should we run? AST? MIR? both?
/// (Also considers the `#![feature(nll)]` setting.)
pub fn borrowck_mode(&self) -> BorrowckMode {
match self.opts.borrowck_mode {
mode @ BorrowckMode::Mir |
mode @ BorrowckMode::Compare => mode,
mode @ BorrowckMode::Ast => {
if self.nll() {
BorrowckMode::Mir
} else {
mode
}
}
}
}

as well as this comment: #48070 (comment)

@Aaron1011
Copy link
Member

I'd like to work on this.

@SimonSapin
Copy link
Contributor Author

@Aaron1011 Good to know, thanks. If I only test one set of flags, which one should that be?

@Aaron1011
Copy link
Member

I suspect that it should be all of them: -Z borrowck=mir -Z nll -Z two-phase-borrows.

@nikomatsakis: Can you confirm this?

@Aaron1011
Copy link
Member

Aaron1011 commented Feb 17, 2018

Here's an initial minimization (can probably be improved on):

pub struct Container<T: Iterator> {
    value: Option<T::Item>,
}

impl<T: Iterator> Container<T> {
    pub fn new(iter: T) -> Self {
        panic!()
    }
}


pub struct Wrapper<'a> {
    content: &'a Content,
}

impl<'a, 'de> Wrapper<'a> {
    pub fn new(content: &'a Content) -> Self {
        Wrapper {
            content: content,
        }
    }
}


pub struct Content;

fn crash_it(content: Content) {
    let items = vec![content];
    let map = items.iter().map(|ref o| Wrapper::new(o));

    let mut map_visitor = Container::new(map);

}

fn main() {}

The dtorck error occurs as a result of calling dtor_ck_constraint_for_ty on Container<std::iter::Map<std::slice::Iter<'_, Content>, [closure@min_dtor.rs:29:32: 29:55]>>. From what I've been able to discover, the issue involves trying to resolve the projection for Iterator::Item when storing map in Container.

@Aaron1011
Copy link
Member

Looking at this further, this issue seems to be similar to the problem in #47920.

With NLL, TypeChecker#fully_perform_op is used to wrap many inference-related operations, including projection normalization. However, fully_perform_op clears out any generated lifetime constraints from the infcx when it returns. This prevents the projection cache from working properly, since a fresh set of inference variables will be generated every time a given projection is normalized.

Here's how I believe this causes the current issue:

Both ast-borrowck and NLL call dtorck_constraint_for_ty in a loop - normalizing the returned types, and feeding them back to dtorck_constraint_for_ty. ast-borrowck ends up storing an inference variable in the projection cache, which at some point is equated to the proper type (here, Wrapper).

With NLL, the projection cache is unable to work properly, since inference variables will never be properly re-used. Every time a particular projection is resolved, a new inference variable will be generated, which will never be equated with the proper type. This means that add_drop_live_constraint has no choice but to pass an inference variable to dtorck_constraint_for_ty,

I'm not sure how best to resolve this - from what I understand, clearing out the constraints from the infcx (at some point in time) is important to the correctness of the implementation of NLL. One idea I have is to apply the 'type freshener' used for the selection cache to the projection cache - but only when using NLL. However, I'm not certain if this would be sound.

@nikomatsakis
Copy link
Contributor

@Aaron1011 I'll take a look. I've got a PR that is making some progress towards refactoring how the dtorck_constraint_for_ty works.

@SimonSapin
Copy link
Contributor Author

This might be the same as #48132?

@Aaron1011
Copy link
Member

@SimonSapin: I think it is - @FraGag's explanation of their repro is almost identical to mine.

@nikomatsakis
Copy link
Contributor

This works with #48411

@nikomatsakis nikomatsakis self-assigned this Feb 27, 2018
@SimonSapin
Copy link
Contributor Author

@nikomatsakis That seems to contradict what you wrote there:

this entire branch ought to be a "pure refactoring", I believe, not changing anything about external behavior

@nikomatsakis
Copy link
Contributor

@SimonSapin true. That was realy meant to apply to code that does not have NLL enabled. With NLL, it fixes (obviously) some bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

5 participants