Skip to content

Commit

Permalink
Moved bindings for taken and life_giver to suit new lifetime rules.
Browse files Browse the repository at this point in the history
  • Loading branch information
@pnkfelix
pnkfelix committed Jan 12, 2015
1 parent 56c39ae commit 8ee3c94
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/librustc/middle/infer/error_reporting.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -818,6 +818,8 @@ impl<'a, 'tcx> ErrorReporting<'tcx> for InferCtxt<'a, 'tcx> {
let scope_id = same_regions[0].scope_id;
let parent = self.tcx.map.get_parent(scope_id);
let parent_node = self.tcx.map.find(parent);
let taken = lifetimes_in_scope(self.tcx, scope_id);
let life_giver = LifeGiver::with_taken(&taken[]);
let node_inner = match parent_node {
Some(ref node) => match *node {
ast_map::NodeItem(ref item) => {
Expand Down Expand Up @@ -860,8 +862,6 @@ impl<'a, 'tcx> ErrorReporting<'tcx> for InferCtxt<'a, 'tcx> {
};
let (fn_decl, generics, unsafety, ident, expl_self, span)
= node_inner.expect("expect item fn");
let taken = lifetimes_in_scope(self.tcx, scope_id);
let life_giver = LifeGiver::with_taken(&taken[]);
let rebuilder = Rebuilder::new(self.tcx, fn_decl, expl_self,
generics, same_regions, &life_giver);
let (fn_decl, expl_self, generics) = rebuilder.rebuild();
Expand Down

6 comments on commit 8ee3c94

@nikomatsakis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's happening here? it's not obvious to me why the old pos was not good?

@pnkfelix
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, so I'll use this as my first example to do a deep dive into the changes to region inference.

First off, here is a gist of the error message that you currently get from rustc: https://gist.github.com/pnkfelix/162eaab4433a434758b8

(it is definitely not a good message. We have others that are worse, but the main problem I have with this one is that it says "reference must be valid for the block at 817:60 ... but borrowed value is only valid for the block at 817:60." At the very least, it would be better if the error message actually described two distinct blocks (or the destruction-scope versus user-scope for a block).

@pnkfelix
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. The region inference constraint graph from the original source has about 73K nodes. Luckily for everyone it also has "only" about 73K edges. (oh wait, I may not have been looking at the correct ast node when I gathered that data ... i might have been somehow generating a region-graph for the whole impl...) Or something else is wrong with how -Z print-region-graph is selecting the method data to print.

But still, I suspect showing the graph of the original code is not going to be illuminating. (And plus, graphviz seems to be locking up attempting to process it.)

I will try to reduce it down to a smaller test case.

(I used my new -xpretty everybody_loops pretty printer to generate my starting point for this experiment, cut-and-pasting in the body of ErrorReporting for InferCtxt::give_suggestion method. Here is an abuse of gist.github.com with that text: https://gist.github.com/pnkfelix/e734baaa17109dadf427 )

@pnkfelix
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so I narrowed the problem down to the following example:

#![crate_type="lib"]

struct LifeGiver;

fn constraints<'a> (expl_self_opt: Option<&'a u8>, life_giver: &'a LifeGiver)
{
    loop  { }
}

fn give_suggestion() {
    let expl_self = None.expect("expect item fn");
    let life_giver = LifeGiver;
    constraints(expl_self, &life_giver);
}

Again, if you move let life_giver = ...; up above the let expl_self = ...; then it compiles.

The thing I noted is that expl_self is crucially different from the other arguments to the original Rebuilder::new:

impl <'a, 'tcx> Rebuilder<'a, 'tcx> {
    fn new(tcx: &'a ty::ctxt<'tcx>, fn_decl: &'a ast::FnDecl,
           expl_self_opt: Option<&'a ast::ExplicitSelf_>,
           generics: &'a ast::Generics,
           same_regions: &'a [SameRegions],
           life_giver: &'a LifeGiver) -> Rebuilder<'a, 'tcx>
    {
        loop  { }
    }

The other arguments are all of the form &'a TYPE, while expl_self is of the form Option<&'a TYPE>. I noticed that if I remove expl_self from the arguments, then the whole reduced example compiles again.

I also noticed that in the reduced example, if I change things to try to emulate variance, like so:

fn constraints<'a, 'b: 'a> (expl_self_opt: Option<&'b u8>, life_giver: &'a LifeGiver)
{
    loop  { }
}

then that also fixes things. I may actually try adopting that fix for this example, rather than continuing to play the game of "guess what spot will make all the inferred regions work out."

The question is: Should I attempt to fix this as part of this work? It seems like this is related to variance in some fashion, no? And thus is not really within scope for this PR...

@pnkfelix
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote:

if I change things to try to emulate variance, ... I may actually try adopting that fix for this example ...

This is probably not workable. My reduced example has loop { } as the body, so it misses the crucial detail that the expr_self_opt must end up as part of the returned Rebuilder<'a, 'tcx> structure. So introducing an additional 'b: 'a is not really possible.

@pnkfelix
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3@nikomatsakis and I discussed offline. @nikomatsakis points out that in an example like this (which is a further simplified version of the problem encountered above):

fn main() {
    let x = 3u8;
    let p = Option::Some(&x);
    let y = 4u8;
    constraints(p, &y);
}

fn constraints<'a> (px: Option<&'a u8>, py: &'a u8)
{
    loop  { }
}

the constraint-set cannot be solved under the new regime. The constraints in question are: the implicit lifetime in &x needs to outlive the scope of let p = ..., the implicit lifetime in &y needs to be contained within the scope following let y = ...;, and both lifetimes need to be compatible with the 'a in fn constraints.

The current solution adopted in the PR (effectively moving let y = ...; up above the let p = ...;) is fine.

A long-term option that we should investigate is: if we ever put in better variance handling for type parameters, then presumably the p binding can use one liftetime, and then when we pass it into the call to constraints, it can be restricted to the narrower sublifetime.

(See also rust-lang#21198 )

Please sign in to comment.