]> git.lizzy.rs Git - rust.git/commitdiff
Auto merge of #67016 - lqd:placeholder_loans, r=matthewjasper
authorbors <bors@rust-lang.org>
Mon, 9 Dec 2019 10:50:41 +0000 (10:50 +0000)
committerbors <bors@rust-lang.org>
Mon, 9 Dec 2019 10:50:41 +0000 (10:50 +0000)
In which we implement illegal subset relations errors using Polonius

This PR is the rustc side of implementing subset errors using Polonius. That is, in
```rust
fn foo<'a, 'b>(x: &'a u32, y: &'b u32) -> &'a u32 {
    y
}
```
returning `y` requires that `'b: 'a` but we have no evidence of that, so this is an error. (Evidence that the relation holds could come from explicit bounds, or via implied bounds).

Polonius outputs one such error per CFG point where the free region's placeholder loan unexpectedly flowed into another free region. While all these CFG locations could be useful in diagnostics in the future, rustc does not do that (and the duplication is only partially handled in the rest of the errors/diagnostics infrastructure, e.g. duplicate suggestions will be shown by the "outlives suggestions" or some of the `#[rustc_*]` NLL/MIR debug dumps), so I deduplicated the errors.

(The ordering also matters, otherwise some of the elided lifetime naming would change behaviour).

I've blessed a couple of tests, where the output is currently suboptimal:
- the `hrtb-perfect-forwarding` tests mix subset errors with higher-ranked subtyping, however the plan is for chalk to eventually take care of some of this to generate polonius constraints (i.e. it's not polonius' job). Until that happens, polonius will not see the error that NLL sees.
- some other tests have errors and diagnostics specific to `'static`, I _believe_ this to be because of it being treated as more "special" than in polonius. I believe the output is not wrong, but could be better, and appears elsewhere (I feel we'll need to look at polonius' handling of `'static` at some point in the future, maybe to match a bit more what NLL does when it produces errors)

I'll create a tracking issue in the polonius repo to record these 2 points (and a general "we'll need to go over the blessed output" issue, much like we did for NLLs)

The last blessed test is because it's an improvement: in this case, more errors/suggestions were computed, instead of the existing code path where this case apparently stops at the first error.

The `Naive` variant in Polonius computes those errors, so this PR also switches the default variant to that, as we're also in the process of temporarily deactivating all other variants (which exist mostly for performance considerations) until we have completed more work on completeness and correctness, before focusing on efficiency once again.

While most of the correctness in this PR is hidden in the polonius compare-mode (which of course passes locally), I've added a couple of smoke-tests to the existing ones, so that we have some confidence that it works (and keeps working) until we're in a position where we can run them on CI.

As mentioned during yesterday's wg-polonius meeting, @nikomatsakis has already read through most of this PR (and which is matching  what they thought needed to be done [during the recent Polonius sprint](https://hackmd.io/CGMNjt1hR_qYtsR9hgdGmw#Compiler-notes-on-generating-the-placeholder-loans-support)), but Matthew was hopefully going to review (again, not urgent), so:

r? @matthewjasper

(This updates to the latest `polonius-engine` release, and I'm not sure whether `Cargo.lock` updates can easily be rolled up, but apart from that: this changes little that's tested on CI, so seems safe-ish to rollup ?)

1  2 
Cargo.lock
src/librustc/Cargo.toml
src/librustc_mir/borrow_check/nll/mod.rs
src/librustc_mir/borrow_check/nll/region_infer/mod.rs

diff --cc Cargo.lock
Simple merge
Simple merge
index 49a03ce1ed232b32be9a80894867138b4fd1c479,0c2e2320bf984807271840caea2c16f532ee66fd..bbcb823c8f91c003551c3f1b7d5bb19b46d13dd4
@@@ -170,10 -171,14 +171,10 @@@ pub(in crate::borrow_check) fn compute_
      errors_buffer: &mut Vec<Diagnostic>,
  ) -> (
      RegionInferenceContext<'tcx>,
-     Option<Rc<Output<RegionVid, BorrowIndex, LocationIndex, Local, MovePathIndex>>>,
+     Option<Rc<PoloniusOutput>>,
      Option<ClosureRegionRequirements<'tcx>>,
  ) {
 -    let mut all_facts = if AllFacts::enabled(infcx.tcx) {
 -        Some(AllFacts::default())
 -    } else {
 -        None
 -    };
 +    let mut all_facts = AllFacts::enabled(infcx.tcx).then_some(AllFacts::default());
  
      let universal_regions = Rc::new(universal_regions);