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

Want -Z borrowck=migrate to test planning migration to non-lexical lifetimes #46908

Closed
4 tasks done
pnkfelix opened this issue Dec 21, 2017 · 11 comments · Fixed by #52681
Closed
4 tasks done

Want -Z borrowck=migrate to test planning migration to non-lexical lifetimes #46908

pnkfelix opened this issue Dec 21, 2017 · 11 comments · Fixed by #52681
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. NLL-diagnostics Working towards the "diagnostic parity" goal P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Dec 21, 2017

Task list:

  • Change MIR-borrowck (aka NLL) to so that it first buffers all errors and then emits them in a post-pass over the buffer at the end.
  • Add a pure mode to AST-borrowck where it does not have any side-effects (namely no I/O related to emitting errors), but rather just returns a boolean based on whether any errors occurred or not.
  • Add -Z borrowck=migrate; it first runs NLL. If the code passes, then we're done. If there were errors buffered, then before emitting its buffered errors, it runs AST-borrowck in the aforementioned pure mode. If AST-borrowck signals any error, then emit the NLL errors. If the AST-borrowck accepts the code, then downgrade all the NLL errors to warnings and then emit them.
  • Make sure the docs and/or diagnostics indicate that the NLL errors-downgraded-to-warnings represent future-compatibility hazards in the developer's code and will become hard errors in the future.

Original bug report follows:

We are currently planning on an initial deployment of non-lexical lifetimes (NLL) via an opt-in feature flag #![feature(nll)] (which actually just landed last night, woo hoo)!

However, our plan is not to just choose some arbitrary point in the future and making #![feature(nll)] the default with no warning.

In particular, there is some code that is accepted today under AST borrowck that NLL rejects. So we need a migration path.

Luckily, we already have the pieces in place to run both AST-borrowck and MIR-borrowck (aka NLL). So we can make that part of the migration path.

In particular, lets add a -Z borrowck=migrate flag. When its turned on, we run both borrow checkers (much the same way that -Z borrowck=compare does today). But now we consider both results when reporting errors:

  • If both AST- and MIR-borrowck accept the code, then accept (of course),
  • If both AST- and MIR-borrowck reject the code, then reject (ibid),
  • If AST-borrowck rejects the code and MIR-borrowck accepts, then accept. (Indeed, this is the point of NLL, to start accepting a broader swath of code.)
  • If AST-borrowck accepts the code and MIR-borrowck rejects, then warn, and point out that the warning will become a hard error in the future.

Of course the devil is in the details here. In particular, the above breakdown acts like one can always meaningfully relate the errors generated by AST- and MIR-borrowck, but the reality is that the two will differ in their reports.

To avoid presenting duplicated sets of errors from both AST- and MIR-borrowck in the cases where they both reject, my plan is to use the error code numbers as an approximation to whether an error was signaled or not from AST-borrowck before deciding whether to report it or treat it as a warning from MIR-borrowck.

@pnkfelix pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 21, 2017
@pnkfelix
Copy link
Member Author

Actually I may have been overthinking this; that is may be no need to track error numbers.

if we just track whether AST-borrowck signals any error at all (a mere boolean), then we can use this logic, I think:

  • Run AST-borrowck. Cancel any errors, but track if they occur.
  • If AST-borrowck signalled any errors (which were all cancelled above), then just run MIR-borrowck (and let it signal errors).
  • If AST-borrowck did not signal any errors, then run MIR-borrowck, but force any signalling it does to be a warning.

@nikomatsakis nikomatsakis added this to the NLL Future Compat Warnings milestone Jan 4, 2018
@nikomatsakis nikomatsakis removed this from the NLL run-pass without ICEs milestone Jan 19, 2018
@nikomatsakis nikomatsakis added the NLL-diagnostics Working towards the "diagnostic parity" goal label Mar 14, 2018
@jkordish jkordish added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Mar 21, 2018
@pnkfelix
Copy link
Member Author

@nikomatsakis and @pnkfelix recently discovered that we hadn't quite ironed out all the details here as much as @pnkfelix had thought.

You can see discussion at: https://rust-lang.zulipchat.com/login/#narrow/stream/122657-wg-nll/subject/weekly.20meeting.20May.2029 (though I suppose you'll need to log into zulip to see it... maybe I'll transcribe the relevant portion to a gist or something publicly accessible...)

@nikomatsakis
Copy link
Contributor

I'm marking this as I-nominated for us to discuss in the meeting tomorrow. I think if we are going to get the Edition Preview out the door, we really need to see this get fixed. Here are some of the steps I think we need to take:

Currently, when NLL is enabled, if we get "regionck errors", we supress them, but we issue a warning:

// But with nll, it's nice to have some note for later.
for error in errors {
match *error {
RegionResolutionError::ConcreteFailure(ref origin, ..)
| RegionResolutionError::GenericBoundFailure(ref origin, ..) => {
self.tcx
.sess
.span_warn(origin.span(), "not reporting region error due to nll");
}
RegionResolutionError::SubSupConflict(ref rvo, ..) => {
self.tcx
.sess
.span_warn(rvo.span(), "not reporting region error due to nll");
}
}

This was written before we trusted NLL. Now that we do, we should remove this warning.

Next, we need to add a Migrate variant to the BorrowckMode enum:

#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub enum BorrowckMode {
Ast,
Mir,
Compare,
}

and parse it:

let borrowck_mode = match debugging_opts.borrowck.as_ref().map(|s| &s[..]) {
None | Some("ast") => BorrowckMode::Ast,
Some("mir") => BorrowckMode::Mir,
Some("compare") => BorrowckMode::Compare,
Some(m) => early_error(error_format, &format!("unknown borrowck mode `{}`", m)),
};

The idea then is to make this mode act like borrowck=mir except that we have to add the "funky behavior" when errors are reported. In this behavior, we decide between issuing MIR errors as errors or warnings. We use the result of AST borrowck to do that:

  • If AST borrowck would also have had errors, we can issue errors, but otherwise we have to issue future-compatibility warnings.

To make this work, we need to re-engineer the AST borrowck query so that it returns some kind of boolean indicating whether errors were reported. This is the query in question; it currently returns what mut declarations were used (for the purposes of the lint):

fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId)
-> Lrc<BorrowCheckResult>

Presently, we are actually always executing AST borrowck, as well, which will need to be tweaked:

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/mod.rs#L225-L228

@nikomatsakis nikomatsakis added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Jul 3, 2018
@spastorino spastorino self-assigned this Jul 3, 2018
@nnethercote
Copy link
Contributor

Let me play devil's advocate for minute: is the transition necessary? Why not just switch to the new borrow checker and immediately issue errors for the invalid code? Correct me if I'm wrong, but errors missed by the old borrow checker are presumably soundness issues, and my understanding is that soundness issues have been fixed ASAP in the past without concerns for backward compatibility.

It would be a more abrupt approach, but would simplify things greatly, and avoid performance problems caused by running two borrow checkers.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jul 6, 2018

@nnethercote note that -- once #52083 lands -- we will only run the AST borrow checker in the case of error, which is less important to overall performance. There are still some performance wins that we can get once we rip out the old borrow check though.

Regardless, it's really not an option to stop a non-trivial portion of the existing crates from compiling -- particularly since some of those new NLL errors may be bugs.

@pnkfelix
Copy link
Member Author

triage: P-high

@pnkfelix pnkfelix added P-high High priority and removed I-nominated labels Jul 12, 2018
@pnkfelix
Copy link
Member Author

(also, I am scheduled to discuss this with @spastorino tonight)

@pnkfelix
Copy link
Member Author

visiting for triage. @spastorino and I have made progress. I hope to have a PR up this week.

@lqd
Copy link
Member

lqd commented Jul 20, 2018

For unit testing purposes, there are a some minimized repros in the crater run reports: pieces of code accepted by AST borrowck and rejected by MIR borrowck. Some of those are definitely MIR borrowck bugs, so I'll just list some but they may need to be filtered :)

Report one
Report two

Example one
Example two
Example three
Example four
Example five
Example six

bors added a commit that referenced this issue Jul 23, 2018
…ate, r=nikomatsakis

Buffer NLL errors

Buffer the errors generated during MIR-borrowck (aka NLL).

This is the first big step towards resolving issue #46908.
@spastorino spastorino assigned pnkfelix and unassigned spastorino Jul 23, 2018
@spastorino
Copy link
Member

Assigning to @pnkfelix, he took this over. Ping me if I can help in some way :).

@pnkfelix
Copy link
Member Author

visited for triage. I've got a branch that is almost ready to be made into a PR; I just need to resolve one issue with the new MIR-codegen enabled by NLL causing an ICE downstream when it is given (unsound) code that was previously accepted by the AST-borrowck.

bors added a commit that referenced this issue Jul 27, 2018
Add `-Z borrowck=migrate`

This adds `-Z borrowck=migrate`, which represents the way we want to migrate to NLL under Rust versions to come. It also hooks this new mode into `--edition 2018`, which means we're officially turning NLL on in the 2018 edition.

The basic idea of `-Z borrowck=migrate` that there are cases where NLL is fixing old soundness bugs in the borrow-checker, but in order to avoid just breaking code by immediately rejecting the programs that hit those soundness bugs, we instead use the following strategy:

If your code is accepted by NLL, then we accept it.
If your code is rejected by both NLL and the old AST-borrowck, then we reject it.
If your code is rejected by NLL but accepted by the old AST-borrowck, then we emit the new NLL errors as **warnings**.

These warnings will be turned into hard errors in the future, and they say so in these diagnostics.

Fix #46908
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-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. NLL-diagnostics Working towards the "diagnostic parity" goal P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants