[NLL] Make causal tracking lazy #48682

spastorino · 2018-03-02T20:09:04Z

Close #46590

cc @nikomatsakis

rust-highfive · 2018-03-02T20:09:09Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

spastorino · 2018-03-02T20:10:28Z

r? @nikomatsakis

nikomatsakis · 2018-03-02T21:05:42Z

src/librustc_mir/borrow_check/nll/region_infer/values.rs

-            } else {
-                None
-            },
+            causes: Some(CauseMap::default()),


This seems ok but mildly surprising -- that is, I think we should adjust some comments. I guess I would like a comment on new like this:

Creates a new set of "region values" that tracks causal information. Each of the regions in num_region_variables will be initialized with an empty set of points and no causal information.

Then I would expect to remove the Clone impl and add a new method like:

// This is what we use at first... fn clone_not_tracking_causes(&self) -> Self { .. } // ...this is what `clone` is today fn clone_tracking_causes(&self) -> Self { .. }

nikomatsakis · 2018-03-02T21:08:34Z

src/test/ui/nll/closure-requirements/escape-argument-callee.stderr

+           because of an outlives relation created at `bb0[1]`
+           because of an outlives relation created at `bb0[1]`
+           because of an outlives relation created at `bb0[0]`
+           because `'_#3r` is universally quantified


didn't we remove this output? I guess this PR is independent from that one?

Yes, the other PR is not in master yet.

estebank · 2018-03-02T21:56:05Z

src/test/ui/nll/return-ref-mut-issue-46557.stderr

+   | | -
+   | | |
+   | |_temporary value only lives until here
+   |   borrow later used here


This is very odd looking.

It seems to me that 1) there's a bug in the graphing code for a multiline span ending in the same char as a single line span, and 2) we should probably only show one of these spans, as they do not add more information.

Personally, I would prefer the output to look more like this:

error[E0597]: borrowed value does not live long enough --> $DIR/return-ref-mut-issue-46557.rs:17:21 | LL | fn gimme_static_mut() -> &'static mut u32 { | ---------------- the returned value needs to live for the duration of the `'static` lifetime LL | let ref mut x = 1234543; | ^^^^^^^ value does not live long enough LL | x | - value is being returned here LL | } | - value only lives until here

@estebank @nikomatsakis I thought exactly the same thing. Waht about opening a different issue to avoid tackling a lot of different things here?.

bors · 2018-03-04T21:15:17Z

☔ The latest upstream changes (presumably #48592) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2018-03-05T15:37:48Z

src/librustc_mir/borrow_check/mod.rs

+        R: ToRegionVid,
+    {
+        let inferred_values = &self.nonlexical_cause_info.as_ref()
+            .expect("region values not yet inferred")


Nit: this should say "region cause information not yet populated" or something like that

I think I had expected to put this method onto the RegionCausalInfo, precisely so that the "unwrap/expect" question is pushed up to the caller. This way is OK too, but I mildly prefer it to be moved.

nikomatsakis · 2018-03-05T15:40:27Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

@@ -125,6 +123,10 @@ pub(crate) enum Cause {
    },
 }

+pub(crate) struct RegionCausalInfo {
+    pub(crate) inferred_values: RegionValues,


This field should not be public, I think -- RegionValues were meant to be encapsulated within the region_infer module. If we move the why_borrow_contains_point method onto the RegionCausalInfo struct, that would work out fine.

nikomatsakis · 2018-03-05T15:40:48Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

+        self.inferred_values = Some(causal_info.inferred_values);
+    }
+
+    pub(crate) fn compute_causal_info2(&self, mir: &Mir<'tcx>, track_causes: bool) -> RegionCausalInfo {


This shouldn't be to be pub(crate), right?

nikomatsakis · 2018-03-05T15:41:55Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

            let mut constraints: Vec<_> = self.constraints.iter().collect();
            constraints.sort();
            constraints
        });

        // The initial values for each region are derived from the liveness
        // constraints we have accumulated.
-        let mut inferred_values = self.liveness_constraints.clone();
+        let mut inferred_values =


Rather than pass a flag, I personally would have this take as argument the initial RegionValues, so that the caller clones either with causes or without. I just hate boolean flags. If we are going to have a flag, I'd prefer it to be struct TrackCauses(bool), so that when we see a call to it, it looks like self.compute_causal_info(..., TrackCauses(true)) and hence we know what that true/false means.

Had a thought. First off, I see an argument for keeping the flag (though I prefer the struct), because -- as I wrote on gitter -- it shows that it only makes sense for compute_causal_info to start with a fresh clone of the liveness data versus some caller that might clone the liveness data, manipulate it in some way, and then invoke compute_causal_info. But if we are going to have the flag, then let's remove clone_tracking_causal_info thing and pass the flag to RegionValues:

fn clone(&self, track_causes: TrackCauses) -> RegionValues { ... }

Maybe pick another name than clone (duplicate?)

nikomatsakis · 2018-03-05T15:42:17Z

src/librustc_mir/borrow_check/nll/region_infer/values.rs


 /// Maps between the various kinds of elements of a region value to
 /// the internal indices that w use.
-pub(super) struct RegionValueElements {
+pub(crate) struct RegionValueElements {


I'd rather that this is kept private to region_infer

nikomatsakis · 2018-03-05T20:35:43Z

src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

+                self.nonlexical_cause_info = Some(regioncx.compute_causal_info(mir));
+            }
+
+            if let Some(cause_info) = &self.nonlexical_cause_info {


can you rewrite this as:

let cause_info = self.nonlexical_cause_info.as_ref().unwrap();

after all, it had better be Some, since we just assigned it up there!

nikomatsakis · 2018-03-05T20:37:18Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

+        self.inferred_values = Some(causal_info.inferred_values);
+    }
+
+    fn compute_causal_info2(&self, mir: &Mir<'tcx>, track_causes: TrackCauses) -> RegionCausalInfo {


I think this should return a RegionValues; we should build the struct in compute_causal_info()

Let's rename this to compute_region_values or .. anything but something matching the regex .*2$ =)

nikomatsakis · 2018-03-05T20:44:22Z

src/librustc_mir/borrow_check/nll/region_infer/values.rs

@@ -218,6 +220,18 @@ impl RegionValues {
        }
    }

+    pub(super) fn duplicate(&self, track_causes: TrackCauses) -> Self {


I think a comment would be nice:

Duplicates the region values. If track_causes is false, then the resulting value will not track causal information (and any existing causal information is dropped). Otherwise, the causal information is preserved and maintained. Tracking the causal information makes region propagation significantly slower, so we prefer not to do it until an error is reported.

nikomatsakis · 2018-03-05T20:45:03Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

@@ -444,21 +439,30 @@ impl<'tcx> RegionInferenceContext<'tcx> {
        }
    }

+    pub(crate) fn compute_causal_info(&self, mir: &Mir<'tcx>) -> RegionCausalInfo {


I think a comment would be nice:

Re-execute the region inference, this time tracking causal information. This is significantly slower, so it is done only when an error is being reported.

nikomatsakis · 2018-03-05T20:47:41Z

src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

@@ -19,69 +19,75 @@ use util::liveness::{self, DefUse, LivenessMode};

 impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
    pub(in borrow_check) fn explain_why_borrow_contains_point(


Pre-existing, but needs a comment:

Adds annotations to `err` explaining *why* the borrow contains the point from `context`. This is key for the "3-point errors" [described in the NLL RFC][d]. [d]: https://rust-lang.github.io/rfcs/2094-nll.html#leveraging-intuition-framing-errors-in-terms-of-points

bors · 2018-03-05T21:44:34Z

☔ The latest upstream changes (presumably #48208) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis

lgtm!

nikomatsakis · 2018-03-06T21:10:49Z

src/librustc/infer/error_reporting/mod.rs

+            ty::ReEarlyBound(_) | ty::ReFree(_)  => {
+                self.msg_span_from_early_bound_and_free_regions(region)
+            },
+            ty::ReStatic => ("the static lifetime".to_owned(), None),


nikomatsakis · 2018-03-07T09:26:01Z

@bors r+

bors · 2018-03-07T09:26:02Z

📌 Commit 52a47d4 has been approved by nikomatsakis

@nikomatsakis

…matsakis [NLL] Make causal tracking lazy Close rust-lang#46590 cc @nikomatsakis

Rollup of 7 pull requests - Successful merges: #48292, #48682, #48699, #48738, #48752, #48789, #48808 - Failed merges:

rust-highfive assigned estebank Mar 2, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2018

rust-highfive assigned nikomatsakis and unassigned estebank Mar 2, 2018

spastorino changed the title ~~[NLL] [WIP] Remove nll-dump-cause flag and allow label_diagnostic to run if -Zverbose~~ [NLL] [WIP] Make causal tracking lazy Mar 2, 2018

nikomatsakis reviewed Mar 2, 2018

View reviewed changes

estebank reviewed Mar 2, 2018

View reviewed changes

spastorino force-pushed the make_causal_lazy branch from 6f59d5f to ca64989 Compare March 3, 2018 17:58

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2018

spastorino changed the title ~~[NLL] [WIP] Make causal tracking lazy~~ [NLL] Make causal tracking lazy Mar 5, 2018

spastorino force-pushed the make_causal_lazy branch from cba6cf3 to a8fbb11 Compare March 5, 2018 15:39

nikomatsakis requested changes Mar 5, 2018

View reviewed changes

spastorino force-pushed the make_causal_lazy branch 4 times, most recently from cf4d7fb to fd81cfd Compare March 5, 2018 19:21

nikomatsakis requested changes Mar 5, 2018

View reviewed changes

spastorino force-pushed the make_causal_lazy branch from 4fe7890 to 61c223c Compare March 5, 2018 21:31

spastorino force-pushed the make_causal_lazy branch 6 times, most recently from 8c7cffd to e97d862 Compare March 6, 2018 06:03

Remove nll-dump-cause flag and always track causes

ec76190

spastorino force-pushed the make_causal_lazy branch from e97d862 to 6c2dec0 Compare March 6, 2018 13:35

nikomatsakis mentioned this pull request Mar 6, 2018

refactor the BorrowckErrors trait to take fn(self) #48783

Closed

spastorino force-pushed the make_causal_lazy branch 3 times, most recently from 7067d1f to 746f1b3 Compare March 6, 2018 20:34

spastorino added 3 commits March 6, 2018 18:03

Make causal tracking lazy

c85c5a0

mir is already a reference

4b12ded

Run rustfmt over modified files

52a47d4

spastorino force-pushed the make_causal_lazy branch from 746f1b3 to 52a47d4 Compare March 6, 2018 21:04

nikomatsakis approved these changes Mar 6, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 7, 2018

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 8, 2018

Rollup merge of rust-lang#48682 - spastorino:make_causal_lazy, r=niko…

d7f44ac

…matsakis [NLL] Make causal tracking lazy Close rust-lang#46590 cc @nikomatsakis

Manishearth mentioned this pull request Mar 8, 2018

Rollup of 7 pull requests #48849

Merged

bors added a commit that referenced this pull request Mar 8, 2018

Auto merge of #48849 - Manishearth:rollup, r=Manishearth

604d4ce

Rollup of 7 pull requests - Successful merges: #48292, #48682, #48699, #48738, #48752, #48789, #48808 - Failed merges:

bors merged commit 52a47d4 into rust-lang:master Mar 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NLL] Make causal tracking lazy #48682

[NLL] Make causal tracking lazy #48682

spastorino commented Mar 2, 2018 •

edited

Loading

rust-highfive commented Mar 2, 2018

spastorino commented Mar 2, 2018

nikomatsakis Mar 2, 2018

nikomatsakis Mar 2, 2018

spastorino Mar 2, 2018

estebank Mar 2, 2018

spastorino Mar 2, 2018

estebank Mar 3, 2018

bors commented Mar 4, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

nikomatsakis Mar 5, 2018

bors commented Mar 5, 2018

nikomatsakis left a comment

nikomatsakis Mar 6, 2018

nikomatsakis commented Mar 7, 2018

bors commented Mar 7, 2018

		@@ -19,69 +19,75 @@ use util::liveness::{self, DefUse, LivenessMode};

		impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
		pub(in borrow_check) fn explain_why_borrow_contains_point(

[NLL] Make causal tracking lazy #48682

[NLL] Make causal tracking lazy #48682

Conversation

spastorino commented Mar 2, 2018 • edited Loading

rust-highfive commented Mar 2, 2018

spastorino commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 5, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Mar 7, 2018

bors commented Mar 7, 2018

spastorino commented Mar 2, 2018 •

edited

Loading