Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New inference scheme #15955

Conversation

@nikomatsakis
Copy link
Contributor

commented Jul 24, 2014

The inference scheme proposed in http://smallcultfollowing.com/babysteps/blog/2014/07/09/an-experimental-new-type-inference-scheme-for-rust/.

This is theoretically a [breaking-change]. It is possible that you may encounter type checking errors, particularly related to closures or functions with higher-ranked lifetimes or object types. Adding more explicit type annotations should help the problem. However, I have not been able to make an example that actually successfully compiles with the older scheme and fails with the newer scheme.

f? @pcwalton, @pnkfelix

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2014

There are some FIXMEs still in place concerning better tracking of "expected vs required" types for error messages. I'm not sure how important that is, shouldn't be too hard to fix properly.

cmr did a graph of performance: http://i.imgur.com/JKFv3oX.png despite the labels, this PR is the red line.

expr: &ast::Expr,
freevars: &[freevars::freevar_entry]) {
/*!
* There is currently a rule that procs that they can only

This comment has been minimized.

Copy link
@pcwalton

pcwalton Jul 24, 2014

Contributor

nit: "for procs"

// necessarily true on later iterations), we will first
// instantiate `b_vid` with a *generalized* version of
// `a_ty`. Generalization introduces other inference
// variables whereever subtyping could occur (at time of

This comment has been minimized.

Copy link
@pcwalton

pcwalton Jul 24, 2014

Contributor

typo: "wherever"

// \ B / / \ / /
// \ / * \ /
// A \ / A
// B

This comment has been minimized.

Copy link
@pcwalton

pcwalton Jul 24, 2014

Contributor

So glad to see this code gone :)

@pcwalton

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2014

@nikomatsakis Does this close any P-backcompat-lang issues (part of #5527)?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2014

@pcwalton it in and of itself doesn't close any issues I know of, but it is part of #5527

@lilyball

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2014

After re-reading your blog post, this scheme right now assumes that lifetimes are the only way to get subtyping right now (your post says it can be extended to e.g. struct subtyping if necessary). What about trait objects, does that not count as subtyping? You can't say Box<Foo+Bar> for some reason (I have no idea why not), and an instance of a sub-trait is not considered a subtype of the supertrait for some reason (also no idea why not), but it does seem that Box<Foo+Send> is considered a subtype of Box<Foo>.

trait Foo {
    fn foo(&self);
}

impl Foo for uint {
    fn foo(&self) {
        println!("foo: {}", *self);
    }
}

pub fn main() {
    let x: Box<Foo+Send> = box 42u;
    let y: &Box<Foo+Send> = &x;
    let z: &Box<Foo> = y;
    z.foo();
}

How does your new scheme handle this?

@pcwalton

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2014

Trait object "subtyping" has to be handled via coercions. You will see why if you try to sketch out how it must be handled at runtime.

@zwarich

This comment has been minimized.

Copy link

commented Jul 24, 2014

@pcwalton Not that I want true trait object subtyping as a feature, but why can't you handle it with standard single inheritance vtables? Or were you referring to the general case with multiple trait bounds?

@pcwalton

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2014

The general case with multiple trait bounds. Trait inheritance isn't single inheritance. The only solution would be something like C++'s virtual inheritance with multiple trees of vtables, which is… not a road I want to go down. :)

@lilyball

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2014

I have never looked at the implementation of trait objects in Rust. I can definitely think of schemes that would allow trait object subtyping to work without coercion, but I assume the current scheme uses a fixed index into a table of function pointers, and that's going to be faster than any subtyping-compatible solution.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2014

On Thu, Jul 24, 2014 at 12:29:16PM -0700, Kevin Ballard wrote:

After re-reading your blog post, this scheme right now assumes that
lifetimes are the only way to get subtyping right now (your post
says it can be extended to e.g. struct subtyping if necessary). What
about trait objects, does that not count as subtyping? You can't say
Box<Foo+Bar> for some reason (I have no idea why not), and an
instance of a sub-trait is not considered a subtype of the
supertrait for some reason (also no idea why not), but it does seem
that Box<Foo+Send> is considered a subtype of Box<Foo>.

Good point. I had overlooked that source of subtyping. That said, it
would be fairly straightforward to extend the scheme to cover it, if
we wanted to (which itself is unclear and should be separately
discussed). Essentially what is required is to introduce a new sort of
variable for each source of subtyping -- in this case, it'd be a
"trait set" variable. (Currently the code in this PR simply does not
infer subtyping of this form.)

@bkoropoff

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2014

I should mention that this PR appears to fix issue #15244 for me.

@eddyb

This comment has been minimized.

Copy link
Member

commented Aug 10, 2014

@pcwalton @kballard Supporting upcasting in trait objects, given multiple inheritance, is absolutely trivial.
The set of methods has a fixed form, and supertrait vtables can be nested within the subtrait's vtable.
There would be some minor duplication, w.r.t destructors, but that can be optimized to zero duplication in the single inheritance case, and LLVM could be taught to use an embedded vtable as the true vtable of the supertrait itself.

@Kimundi

This comment has been minimized.

Copy link
Member

commented Aug 10, 2014

To elaborate on what @eddyb said: Today, if you have a trait hierachy that looks somewhat like this:

trait A { ... }
trait B { ... }
trait AB: A + B { ... }

Then for the usage of a trait object the compiler emits these vtables for a given type:

AB: [destructor_of_t, ..methods_of_ab, ..methods_of_a, ..methods_of_b]
A:  [destructor_of_t, ..methods_of_a]
B:  [destructor_of_t, ..methods_of_b]

If we instead change the compiler to emit this vtable instead:

[destructor_of_t, ..methods_of_a, destructor_of_t, ..methods_of_b, ..methods_of_ab]

Then upcasting becomes a simple slicing operation of the vtable with statically known indices at the point of the upcast.

@huonw

This comment has been minimized.

Copy link
Member

commented Aug 10, 2014

It's worth noting that @eddyb and @Kimundi's scheme is a vector-backed tree (i.e. just one way of implementing what @pcwalton mentioned above).

@lilyball

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2014

@eddyb @Kimundi If the two traits both inherit from a common supertrait then you're also going to have to duplicate all of the supertrait methods. Certainly doable, but it is more bloat.

@Kimundi

This comment has been minimized.

Copy link
Member

commented Aug 11, 2014

@kballard: I actually explored such situations a bit today (raw notes are at https://gist.github.com/Kimundi/e3e292188221ee8afdf2)

While its certainly true that there is duplication, it actually often ends up as a win because you can (in theory) make all vtables of a supertrait point into vtables of a subtrait.

Besides, is a few pointer more here and there such a big deal? Or are there other considerations than just binary size?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2014

Regarding the object type relation, it is somewhat off topic, I am preparing an RFC. I see no reason that the new inference scheme can accommodate whatever subtyping relation we ultimately elect to have, though for various reasons I am inclined to have subtyping only on the object lifetime bound and prefer coercion for the rest. I will describe those reasons in the RFC.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2014

Regarding this branch in general, I am rebasing it on top of PR #16453, because that branch cleans up how we propagate constraints for 'static, making some of the more invasive refactoring on this branch unnecessary. It may still be worth reviewing the "main commit" here (which is the final one) independently just so speed things up. Will discuss with @pcwalton offline.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2014

Rebased. All tests pass locally. Note that the history includes commits that are part of PR #16453.

@pcwalton

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2014

Looks good to me modulo a couple of trivial nits.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:issue-5527-new-inference-scheme branch from 0ced472 to 3ed4a42 Aug 29, 2014
@nikomatsakis nikomatsakis force-pushed the nikomatsakis:issue-5527-new-inference-scheme branch 2 times, most recently from 61a3f43 to 8b710cf Aug 29, 2014
@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2014

Rebased and updated. Also added a new test showing clearly where the older algorithm could fall down on perfectly reasonable code. (That example was from actual code that was failing to compile; took me a while to trace down the root cause. Imagine my delight to discover that the new inference algorithm would have fixed it.)

bors added a commit that referenced this pull request Aug 29, 2014
…eme, r=pcwalton

The inference scheme proposed in <http://smallcultfollowing.com/babysteps/blog/2014/07/09/an-experimental-new-type-inference-scheme-for-rust/>.

This is theoretically a [breaking-change]. It is possible that you may encounter type checking errors, particularly related to closures or functions with higher-ranked lifetimes or object types. Adding more explicit type annotations should help the problem. However, I have not been able to make an example that *actually* successfully compiles with the older scheme and fails with the newer scheme.

f? @pcwalton, @pnkfelix
…t most one type, and region variables are introduced as needed
@nikomatsakis nikomatsakis force-pushed the nikomatsakis:issue-5527-new-inference-scheme branch from 9d72a8d to 6e27c2f Aug 29, 2014
@nikomatsakis

This comment has been minimized.

Copy link
Owner Author

commented on 6e27c2f Aug 29, 2014

r=pcwalton

This comment has been minimized.

Copy link

replied Aug 29, 2014

@bors: retry

@bors

This comment has been minimized.

Copy link
Contributor

commented on 6e27c2f Aug 29, 2014

saw approval from pcwalton
at nikomatsakis@6e27c2f

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2014

merging nikomatsakis/rust/issue-5527-new-inference-scheme = 6e27c2f into auto

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2014

nikomatsakis/rust/issue-5527-new-inference-scheme = 6e27c2f merged ok, testing candidate = ce5d33e

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2014

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2014

saw approval from pcwalton
at nikomatsakis@6e27c2f

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2014

merging nikomatsakis/rust/issue-5527-new-inference-scheme = 6e27c2f into auto

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2014

nikomatsakis/rust/issue-5527-new-inference-scheme = 6e27c2f merged ok, testing candidate = bd159d3

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2014

fast-forwarding master to auto = bd159d3

bors added a commit that referenced this pull request Aug 29, 2014
…eme, r=pcwalton

The inference scheme proposed in <http://smallcultfollowing.com/babysteps/blog/2014/07/09/an-experimental-new-type-inference-scheme-for-rust/>.

This is theoretically a [breaking-change]. It is possible that you may encounter type checking errors, particularly related to closures or functions with higher-ranked lifetimes or object types. Adding more explicit type annotations should help the problem. However, I have not been able to make an example that *actually* successfully compiles with the older scheme and fails with the newer scheme.

f? @pcwalton, @pnkfelix
bors added a commit that referenced this pull request Aug 29, 2014
…eme, r=pcwalton

The inference scheme proposed in <http://smallcultfollowing.com/babysteps/blog/2014/07/09/an-experimental-new-type-inference-scheme-for-rust/>.

This is theoretically a [breaking-change]. It is possible that you may encounter type checking errors, particularly related to closures or functions with higher-ranked lifetimes or object types. Adding more explicit type annotations should help the problem. However, I have not been able to make an example that *actually* successfully compiles with the older scheme and fails with the newer scheme.

f? @pcwalton, @pnkfelix
@bors bors closed this Aug 29, 2014
vincom2 added a commit to vincom2/racer that referenced this pull request Aug 31, 2014
Lifetime inference in some parts of racer appears to have been broken
by rust-lang/rust#15955. Adding explicit lifetime parameters
resolves racer-rust#46.
@nikomatsakis nikomatsakis deleted the nikomatsakis:issue-5527-new-inference-scheme branch Mar 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.