rustc: fixed regionck for owned trait casts. #13129

dmski · 2014-03-25T11:15:32Z

Regionck didn't do any checks for casts/coercions to an owned trait,
which resulted in lifetimes of the source pointer
to be ignored in the result of such cast.

This fix constraints all regions of the source type of the cast/coercion to be superregions
of each region of the target type (if target trait definition has some lifetime params),
or of 'static lifetime (if there're no lifetime params in target trait's definition).

Closes #5723
Closes #9745
Closes #11971

huonw · 2014-03-25T11:30:19Z

Looks like something for @nikomatsakis to review.

flaper87 · 2014-03-25T12:44:00Z

Does this fix #10751 too? If not, I guess it probably could.

dmski · 2014-03-25T13:11:39Z

It doesn't - from the look of it, #10751 seems similar to #12470, probably happens for the same reason: trait stores not being considered when inferring variance, which leads to respective regions not being related in a cast (please see #12828 for details).
I'll be able to take a closer look at #10751 later today to confirm it.
If i'm right and #10751 is similar to #12470, then i'd say that it and what i've tried to fix here are different bugs (although ones with similar symptoms).

dmski · 2014-03-25T20:14:34Z

Hm, #10751 isn't #12470, as it still compiles with variance inference fix from #12828 applied. I'll investigate it further.

dmski · 2014-03-26T21:39:44Z

I've had a more thorough look at #10751 and i think it's happening because type bounds declared in trait impl just aren't taken into account when matching the type being cast to a trait and the type that the trait is implemented for (vtable.rs:361-372), so i think that is also not a bug i tried to fix here.

pnkfelix · 2014-03-26T22:13:44Z

cc me

flaper87 · 2014-03-26T22:24:26Z

src/librustc/middle/typeck/check/regionck.rs

-                let source_ty = rcx.fcx.expr_ty(expr);
-                constrain_regions_in_type(rcx, trait_region,
-                                            infer::RelateObjectBound(expr.span), source_ty);
+            ty::AutoObject(ast::BorrowedSigil, Some(trait_region), _, _, _, ref substs) => {


nit: could you use just maybe_region to keep it consistent in style with the next arm?

Done - i've squashed those two arms together actually, as they both do the same thing.

dmski · 2014-03-27T19:05:45Z

Travis failures are because of #13079 and should go away with the next snapshot.

dmski · 2014-04-01T20:35:17Z

@bors: retry

alexcrichton · 2014-04-01T20:39:45Z

The tests need to be fixed for windows. The run-pass tests added need to have pub fn main instead of just fn main.

alexcrichton · 2014-04-01T20:39:56Z

Sorry, not for windows, but for the check-fast target in the makefiles.

dmski · 2014-04-01T20:52:18Z

@alexcrichton ah, thanks! I somehow only saw "interrupted" everywhere and thought it was spurious.

alexcrichton · 2014-04-01T20:54:10Z

Sadly the check-fast failures are quite obscure and difficult to understand, but hopefully it'll be gone soon!

dmski · 2014-04-01T21:46:05Z

Have pushed an update - this passes both fast-check and check locally.

dmski · 2014-04-04T16:31:40Z

r? @cmr, @alexcrichton ?

alexcrichton · 2014-04-04T16:34:39Z

I don't consider myself qualified enough to review this portion of the compiler, I think @pnkfelix or @nikomatsakis would be more appropriate. There's some introductions of lifetimes which don't quite make sense to me, but perhaps it's more sound now...

dmski · 2014-04-04T16:42:30Z

@alexcrichton Ah, sorry then. It's just that this was r+'ed by @cmr earlier and failed in fast-check on windows due to non-pub fn main and i thought that it won't require another full-blown review.

Regionck didn't do any checks for casts/coercions to an owned trait, which resulted in lifetimes of the source pointer to be ignored in the result of such cast. This fix constraints all regions of the source type of the cast/coercion to be superregions of each region of the target type (if target trait definition has some lifetime params), or of 'static lifetime (if there're no lifetime params in target trait's definition). Closes rust-lang#5723 Closes rust-lang#9745 Closes rust-lang#11971

nikomatsakis · 2014-04-22T03:26:17Z

This code is headed in the right direction, but the details are off. There is really a general notion of a type being bounded by a lifetime L (meaning: all borrowed data reachable by the type has lifetime L or longer). The code doesn't really handle these cases correctly, leading to the bugs. I've been wanting to go in and refactor this for a while and your patch is definitely in the correct direction.

What is perhaps surprising is that this bound has nothing to do with the lifetime parameters of the trait type. So, for example, if you have: x as ~Trait<'a>, there is not necessarily any relationship between the data reachable via x and 'a, and certainly not the bounding relationship you are instituting. The lifetime 'a is just part of the interface, and if the type x can implement Trait<'a>, then all is good.

To see what I mean, consider this example:

trait GetRef<'a> {
  fn get(&self) -> &'a int;
  fn sum(&self) -> int;
}

struct Foo<'b> {
    data: &'b ~[int]
}

static SOME_INT: int = 3;

impl<'b> GetRef<'static> for Foo<'b> {
    fn get(&self) -> &'static int { &SOME_INT }
    fn sum(&self) -> int {
        let mut sum = 0;
        for &i in self.data.iter() {
            sum += i;
        }
        sum
    }
}

Here the type Foo<'b> implements GetRef<'static> just fine, and we certainly don't want to force 'b to be >= 'static. However, that doesn't mean we can just forget about the 'b. This is why object types have a bound (maybe_region in your patch). We do need to check that this bound is a sublifetime of 'b.

In any case, I think limiting the check to the trait bound is in the right direction and would be enough for this patch. But when I tried to modify your branch locally I got some other compilation errors for the owned-ptr-static-bound test that I don't fully understand. I'll investigate tomorrow.

dmski · 2014-04-22T21:16:06Z

Thanks for the thorough explanation!

As i understand, ~Trait type doesn't carry a region bound at the moment (unlike &Trait, for which region in trait store serves as region bound), so it would seem to me that the only safe thing to do when casting to ~Trait (without touching the rest of type system) is bounding source type with 'static lifetime.

The solution with constraining casts to ~Trait with 'static breaks some code in libsyntax (so that stage2 fails to compile) which is why i originally tried to implement a workaround with using region parameter as a bound. Concretely, https://github.com/mozilla/rust/blob/master/src/libsyntax/ext/tt/macro_rules.rs#L166 There, trncbr contains a reference to SpanHandler with lifetime 'a and is cast to ~Reader on the next line. Currently the cast succeeds and 'a is lost for trncbr. With 'static bound cast will fail.

I guess the one possible 'correct' solution would be to actually allow one to write ~Trait:'a and extend UniqTraitStore to carry a bound (similarly to RegionTraitStore). As i understand, this would require (on the high level) fixing parser to allow arbitrary lifetimes in types (right now only 'static is allowed there), probably modifying typeck type collection to only allow arbitrary lifetimes for ~Trait (so that unrelated things don't break) and modifying regionck to enforce the bound. Does this idea make sense?

alexcrichton · 2014-05-02T21:29:18Z

Closing due to inactivity, but feel free to reopen with a rebase!

Also, perhaps some of this work will be enabled by #13898?

dmski · 2014-05-03T09:15:45Z

I've actually started moving in the general direction of my previous comment, having changed libsyntax to allow arbitrary lifetimes in bounds (i.e. roughly the same thing #13898 does). Got sidetracked by another project, but i'm still interested in implementing a proper fix for this. Does it make sense for me to continue here (rebasing my changes atop #13898), or is it better to find something else to work on, now that @nikomatsakis is working on this?

alexcrichton · 2014-05-03T17:24:34Z

I know that @nikomatsakis is deep in a number of large changes right now, so I would coordinate with him if you'd like to work on this area.

nikomatsakis · 2014-05-05T14:27:07Z

@dmski sorry for being incommunicative, I am about ... oh, I don't know, 90% of the way through enabling a more general notion of bounds. I agree this is the proper fix.

dmski · 2014-05-05T17:10:02Z

@nikomatsakis Thanks for letting me know. I'll keep an eye on PRs for this then.

…xFrednet Add xFrednet back to Clippy's reviewer roulette What the title says. --- cc: rust-lang/rust-clippy#12947 changelog: none r? `@ghost`

flaper87 reviewed Mar 26, 2014
View reviewed changes

alexcrichton closed this May 2, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: fixed regionck for owned trait casts. #13129

rustc: fixed regionck for owned trait casts. #13129

dmski commented Mar 25, 2014

huonw commented Mar 25, 2014

flaper87 commented Mar 25, 2014

dmski commented Mar 25, 2014

dmski commented Mar 25, 2014

dmski commented Mar 26, 2014

pnkfelix commented Mar 26, 2014

flaper87 Mar 26, 2014

dmski Mar 27, 2014

dmski commented Mar 27, 2014

dmski commented Apr 1, 2014

alexcrichton commented Apr 1, 2014

alexcrichton commented Apr 1, 2014

dmski commented Apr 1, 2014

alexcrichton commented Apr 1, 2014

dmski commented Apr 1, 2014

dmski commented Apr 4, 2014

alexcrichton commented Apr 4, 2014

dmski commented Apr 4, 2014

nikomatsakis commented Apr 22, 2014

dmski commented Apr 22, 2014

alexcrichton commented May 2, 2014

dmski commented May 3, 2014

alexcrichton commented May 3, 2014

nikomatsakis commented May 5, 2014

dmski commented May 5, 2014

rustc: fixed regionck for owned trait casts. #13129

rustc: fixed regionck for owned trait casts. #13129

Conversation

dmski commented Mar 25, 2014

huonw commented Mar 25, 2014

flaper87 commented Mar 25, 2014

dmski commented Mar 25, 2014

dmski commented Mar 25, 2014

dmski commented Mar 26, 2014

pnkfelix commented Mar 26, 2014

flaper87 Mar 26, 2014

Choose a reason for hiding this comment

dmski Mar 27, 2014

Choose a reason for hiding this comment

dmski commented Mar 27, 2014

dmski commented Apr 1, 2014

alexcrichton commented Apr 1, 2014

alexcrichton commented Apr 1, 2014

dmski commented Apr 1, 2014

alexcrichton commented Apr 1, 2014

dmski commented Apr 1, 2014

dmski commented Apr 4, 2014

alexcrichton commented Apr 4, 2014

dmski commented Apr 4, 2014

nikomatsakis commented Apr 22, 2014

dmski commented Apr 22, 2014

alexcrichton commented May 2, 2014

dmski commented May 3, 2014

alexcrichton commented May 3, 2014

nikomatsakis commented May 5, 2014

dmski commented May 5, 2014