Remove contraction from region inference #29188

nikomatsakis · 2015-10-20T14:20:23Z

This fixes #29048 (though I think adding better transactional support would be a better fix for that issue, but that is more difficult). It also simplifies region inference and changes the model to a pure data flow one, as discussed in this internals thread. I am not 100% sure though if this PR is the right thing to do -- or at least maybe not at this moment, so thoughts on that would be appreciated.

r? @pnkfelix
cc @arielb1

arielb1 · 2015-10-20T14:56:20Z

src/librustc/middle/def_id.rs

-            }
-            Ok(())
-        }));
+        if self.is_local() { // (1)


Does this fix RUST_LOG=rustc::middle::traits?

@arielb1

Does this fix RUST_LOG=rustc::middle::traits?

Yes, I expect so. I think I was having problems with RUST_LOG=rustc_typeck, but I suspect it's the same issue. And yes, a fallible version of item_path_str would be better.

nikomatsakis · 2015-10-21T13:45:27Z

Interestingly, this PR fixes a bug that @dherman was encountered in his nanny project. I need to investigate that a bit more deeply though.

nikomatsakis · 2015-10-23T11:22:50Z

I looked into @dherman's case. It comes down to a trait method defined like this:

fn foo<'root, T:Trait<'root>>(t: T) -> &i32

the point being that 'root does not appear in the signature in anyway, and hence has no incoming constraints. In some sense this PR is a reasonable way to fix that problem, though I think that the fix I really want is to guarantee that all type/lifetime parameters to a fn outlive calls to that fn. But that requires some changes to the language to achieve completely. Still, I am somewhat surprised that there aren't incoming edges to the variable from the point where the parameters are instantiated -- I think a reasonable change might be to make it so that when you create a region variable, you must provide a lower bound (which could be ty::ReEmpty). I've started a PR to that effect a few times but never quite finished it.

nikomatsakis · 2015-10-23T11:27:20Z

@arielb1

Isn't SupSupConflict dead now? You may want to remove it.

yes, done.

arielb1 · 2015-10-23T13:18:39Z

@nikomatsakis

I am unable to reproduce @dherman's case, but its not like putting a restriction on 'a would be helpful in any way, because of the "lifetime truncation" behaviour.

nikomatsakis · 2015-10-23T14:37:20Z

@arielb1 what do you mean by "lifetime truncation"?

arielb1 · 2015-10-23T17:54:54Z

@nikomatsakis

The fact that you can substitute an fn with some lifetime parameters that don't appear in the signature and then call it after these lifetime parameters are dead.

A reasonable argument for the soundness of this is that, because lifetime bounds in Rust are never strict, if you LUB all the lifetimes in a validate predicate with some lifetime you still get something valid - this is basically the reason lifetime erasure works (and therefore I call it "truncation").

For example, in your code:

trait Trait<'root> {}
impl<'a,'root> Trait<'root> for &'a i32 where 'a: 'root {}
fn foo<'root, T:Trait<'root>>(t: T) -> i32 {
    /* ... - your original signature doesn't specify the return type's lifetime */
}

fn main() {
    'v: {
        let v = &2;
        ('t: { foo::<'t, &'v i32> })(&v);
    }
}

foo is substituted with the lifetime parameter 't, which is valid as &'v i32: Trait<'t> holds (because of 'v: 't). On the other hand, 't is not live when we call foo, so for soundness we can LUB 't with the lifetime of the call to get some lifetime 't2, LUB 'v with the lifetime of the call to get 'v back, and we have that foo::<'v, &'v i32> is a valid substitution.

This, of course, breaks horribly if we have types that can't be bought fresh to some lifetime - e.g. unloadable code, but we already know that.

nikomatsakis · 2015-10-23T18:01:21Z

OK, yes. This is what I was saying I would potentially like to fix, though
it requires language changes.

On Fri, Oct 23, 2015 at 1:55 PM, arielb1 [email protected] wrote:

@nikomatsakis https://github.com/nikomatsakis

The fact that you can substitute an fn with some lifetime parameters that
don't appear in the signature and then call it after these lifetime
parameters are dead.

A reasonable argument for the soundness of this is that, because lifetime
bounds in Rust are never strict, if you LUB all the lifetimes in a validate
predicate with some lifetime you still get something valid - this is
basically the reason lifetime erasure works (and therefore I call it
"truncation").

For example, in your code:

trait Trait<'root> {}impl<'a,'root> Trait<'root> for &'a i32 where 'a: 'root {}fn foo<'root, T:Trait<'root>>(t: T) -> i32 {
/* ... - your original signature doesn't specify the return type's lifetime */
}
fn main() {
'v: {
let v = &2;
('t: { foo::<'t, &'v i32> })(&v);
}
}

foo is substituted with the lifetime parameter 't, which is valid as &'v
i32: Trait<'t> holds (because of 'v: 't). On the other hand, 't is not
live when we call foo, so for soundness we can LUB 't with the lifetime
of the call to get some lifetime 't2, LUB 'v with the lifetime of the
call to get 'v back, and we have that foo::<'v, &'v i32> is a valid
substitution.

This, of course, breaks horribly if we have types that can't be bought
fresh to some lifetime - e.g. unloadable code, but we already know that.

—
Reply to this email directly or view it on GitHub
#29188 (comment).

nikomatsakis · 2015-10-26T19:33:16Z

@pnkfelix would you be able to review this sooner rather than later? It does fix ~~a regression~~ two regressions, which will be branched into beta any day now.

pnkfelix · 2015-10-28T15:20:58Z

okay it took me a little bit longer than I expected to wrap my head around this. You left in the fn contraction method, so at first I thought something more subtle was going on, but all it now does is check that the ConstrainVarSubReg constraints hold.

It seems like the code is just calling out for a simplification pass; e.g. the factoring of fn check_node within the body of fn contract_node, which made some sense when there was a separate fn adjust_node path, just seems like unnecessary abstraction.

In addition, it would be nice if the README were updated, removing the discussion of processing the Contracting nodes.

But there's no reason to hold up this PR for those kinds of changes.

pnkfelix · 2015-10-28T15:31:00Z

@nikomatsakis r=me once you've addressed the travis failure in some reasonable manner.

nikomatsakis · 2015-10-28T19:41:50Z

@pnkfelix

It seems like the code is just calling out for a simplification pass; e.g. the factoring of fn check_node within the body of fn contract_node, which made some sense when there was a separate fn adjust_node path, just seems like unnecessary abstraction.

Yes, I agree, I was just being hasty. I will try to clean it up a bit.

In addition, it would be nice if the README were updated, removing the discussion of processing the Contracting nodes.

agreed

arielb1 · 2015-10-28T22:44:05Z

region_inference/README.md needs some rework to account for unboxed closures and the new higher-rank inference system (I think).

empty region, and they complicate region inference to no particular end. They also lead in some cases to spurious errors like rust-lang#29048 (though in some cases these errors are helpful in tracking down missing constraints).

expansion already by growing the RHS to be bigger than LHS (all the way to `'static` if necessary). This is needed because contraction doesn't handle givens. Fixes rust-lang#28934.

nikomatsakis · 2015-10-29T09:13:30Z

@bors r=pnkfelix

bors · 2015-10-29T09:13:32Z

📌 Commit c2277de has been approved by pnkfelix

bors · 2015-10-29T11:14:28Z

⌛ Testing commit c2277de with merge 427140f...

@pnkfelix

This fixes #29048 (though I think adding better transactional support would be a better fix for that issue, but that is more difficult). It also simplifies region inference and changes the model to a pure data flow one, as discussed in [this internals thread](https://internals.rust-lang.org/t/rough-thoughts-on-the-impl-of-region-inference-mir-etc/2800). I am not 100% sure though if this PR is the right thing to do -- or at least maybe not at this moment, so thoughts on that would be appreciated. r? @pnkfelix cc @arielb1

bors · 2015-10-29T13:09:11Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

SimonSapin · 2015-10-30T16:35:50Z

Does the beta-nominated tag mean this PR is to be backported to 1.5?

I’d appreciate this: at the moment I have code that compiles on stable and nightly, but not 1.5.0-beta.2: https://travis-ci.org/servo/rust-cssparser/builds/88377023
… and no known work-around.

alexcrichton · 2015-10-30T16:37:46Z

@SimonSapin yeah if accepted this'll be backported to 1.5

brson · 2015-11-03T23:00:27Z

@rust-lang/lang @SimonSapin wants this backported soon. Can you review?

nikomatsakis · 2015-11-04T01:41:31Z

@brson I think this is really a @rust-lang/compiler issue. But I'm inclined to accept it this for beta backport.

pnkfelix · 2015-11-04T10:56:41Z

+1 to backport

brson · 2015-11-04T22:36:21Z

Backport PR is up when you are ready.

nikomatsakis · 2015-11-05T00:59:02Z

Accepting for beta.

rust-highfive assigned pnkfelix Oct 20, 2015

arielb1 reviewed Oct 20, 2015
View reviewed changes

arielb1 mentioned this pull request Oct 25, 2015

Rustc confuses lifetimes of different unrelated variables of type &'static str when implementing a trait for &'a str #26448

Closed

nikomatsakis mentioned this pull request Oct 26, 2015

Nightly regression around HRTBs, likely connected to wf changes #28934

Closed

nikomatsakis added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 28, 2015

nikomatsakis added 8 commits October 28, 2015 18:48

do not dump extern def-ids with path for now

60ab57e

Regression test for rust-lang#29048. Fixes rust-lang#29048.

18698c8

remove SupSupConflict, which is now dead code

6934618

Don't "double check" var-sub-var constraints, which are handled in

c81ce82

expansion already by growing the RHS to be bigger than LHS (all the way to `'static` if necessary). This is needed because contraction doesn't handle givens. Fixes rust-lang#28934.

Update docs for region inference to reflect current state better

9ef2416

Do some slight refactoring, leave the rest for rust-lang#29436

690206c

Move test file to run-fail, since it does an unwrap

c2277de

nikomatsakis force-pushed the remove-contraction branch from 1ccd800 to c2277de Compare October 28, 2015 22:51

bors merged commit c2277de into rust-lang:master Oct 29, 2015

SimonSapin mentioned this pull request Oct 30, 2015

Treat url(<string>) as a normal functions, per spec change. servo/rust-cssparser#90

Merged

dherman mentioned this pull request Oct 30, 2015

static scope-shadowing restriction neon-bindings/neon#11

Closed

pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 31, 2015

brson mentioned this pull request Nov 4, 2015

Beta next #29592

Merged

nikomatsakis added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Nov 5, 2015

dherman mentioned this pull request Dec 2, 2015

default to system rust when stable is ready neon-bindings/neon-cli#2

Closed

nikomatsakis deleted the remove-contraction branch March 30, 2016 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove contraction from region inference #29188

Remove contraction from region inference #29188

nikomatsakis commented Oct 20, 2015

arielb1 Oct 20, 2015

nikomatsakis Oct 23, 2015

nikomatsakis commented Oct 21, 2015

nikomatsakis commented Oct 23, 2015

nikomatsakis commented Oct 23, 2015

arielb1 commented Oct 23, 2015

nikomatsakis commented Oct 23, 2015

arielb1 commented Oct 23, 2015

nikomatsakis commented Oct 23, 2015

nikomatsakis commented Oct 26, 2015

pnkfelix commented Oct 28, 2015

pnkfelix commented Oct 28, 2015

nikomatsakis commented Oct 28, 2015

arielb1 commented Oct 28, 2015

nikomatsakis commented Oct 29, 2015

bors commented Oct 29, 2015

bors commented Oct 29, 2015

bors commented Oct 29, 2015

SimonSapin commented Oct 30, 2015

alexcrichton commented Oct 30, 2015

brson commented Nov 3, 2015

nikomatsakis commented Nov 4, 2015

pnkfelix commented Nov 4, 2015

brson commented Nov 4, 2015

nikomatsakis commented Nov 5, 2015

Remove contraction from region inference #29188

Remove contraction from region inference #29188

Conversation

nikomatsakis commented Oct 20, 2015

arielb1 Oct 20, 2015

Choose a reason for hiding this comment

nikomatsakis Oct 23, 2015

Choose a reason for hiding this comment

nikomatsakis commented Oct 21, 2015

nikomatsakis commented Oct 23, 2015

nikomatsakis commented Oct 23, 2015

arielb1 commented Oct 23, 2015

nikomatsakis commented Oct 23, 2015

arielb1 commented Oct 23, 2015

nikomatsakis commented Oct 23, 2015

nikomatsakis commented Oct 26, 2015

pnkfelix commented Oct 28, 2015

pnkfelix commented Oct 28, 2015

nikomatsakis commented Oct 28, 2015

arielb1 commented Oct 28, 2015

nikomatsakis commented Oct 29, 2015

bors commented Oct 29, 2015

bors commented Oct 29, 2015

bors commented Oct 29, 2015

SimonSapin commented Oct 30, 2015

alexcrichton commented Oct 30, 2015

brson commented Nov 3, 2015

nikomatsakis commented Nov 4, 2015

pnkfelix commented Nov 4, 2015

brson commented Nov 4, 2015

nikomatsakis commented Nov 5, 2015