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

Implement RFC 1210: impl specialization #30652

Merged
merged 37 commits into from Mar 15, 2016

Conversation

Projects
None yet
@aturon
Copy link
Member

aturon commented Dec 30, 2015

This PR implements impl specialization,
carefully following the proposal laid out in the RFC.

The implementation covers the bulk of the RFC. The remaining gaps I know of are:

  • no checking for lifetime-dependent specialization (a soundness hole);
  • no default impl yet;
  • no support for default with associated consts;

I plan to cover these gaps in follow-up PRs, as per @nikomatsakis's preference.

The basic strategy is to build up a specialization graph during
coherence checking. Insertion into the graph locates the right place
to put an impl in the specialization hierarchy; if there is no right
place (due to partial overlap but no containment), you get an overlap
error. Specialization is consulted when selecting an impl (of course),
and the graph is consulted when propagating defaults down the
specialization hierarchy.

You might expect that the specialization graph would be used during
selection -- i.e., when actually performing specialization. This is
not done for two reasons:

  • It's merely an optimization: given a set of candidates that apply,
    we can determine the most specialized one by comparing them directly
    for specialization, rather than consulting the graph. Given that we
    also cache the results of selection, the benefit of this
    optimization is questionable.
  • To build the specialization graph in the first place, we need to use
    selection (because we need to determine whether one impl specializes
    another). Dealing with this reentrancy would require some additional
    mode switch for selection. Given that there seems to be no strong
    reason to use the graph anyway, we stick with a simpler approach in
    selection, and use the graph only for propagating default
    implementations.

Trait impl selection can succeed even when multiple impls can apply,
as long as they are part of the same specialization family. In that
case, it returns a single impl on success -- this is the most
specialized impl known to apply. However, if there are any inference
variables in play, the returned impl may not be the actual impl we
will use at trans time. Thus, we take special care to avoid projecting
associated types unless either (1) the associated type does not use
default and thus cannot be overridden or (2) all input types are
known concretely.

r? @nikomatsakis

@cristicbz

This comment has been minimized.

Copy link
Contributor

cristicbz commented Dec 31, 2015

🎊 🎈 🎊

Regardless which way the RFC goes, this is tremendous work @aturon, big props!

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Dec 31, 2015

Obviously, needs a rebase :)

Also, I forgot to mention, but this passes the full existing test suite as well as a pile of new tests, so it should be in a pretty good state.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Dec 31, 2015

Regarding the gaps:

no partial impl yet;

This should be pretty easy to add in a follow-up, given the infrastructure laid by this PR.

no specialization on inherent impls;

I'm really not sure about this one; right now, inherent impls work pretty independently from our trait infrastructure, so this is potentially a major refactor. It's also one of the more subtle bits of the design. I think we should probably sit on this for a while.

no support for default with associated consts;

Should be easy to add; can probably sneak it into this PR.

no support for specializing based on trait hierarchies (super traits).

I'm not actually sure why this isn't working yet; somehow the selection infrastructure is failing to elaborate supertrait relationships. Probably an easy fix?

@aturon aturon force-pushed the aturon:specialization branch 2 times, most recently from 342a9d9 to 99e78f7 Dec 31, 2015

@dylanede

This comment has been minimized.

Copy link
Contributor

dylanede commented Jan 3, 2016

Here's a concise example of a problem I've found.

This code compiles fine:

use std::marker::PhantomData;

trait Id_ {
    type Out;
}

type Id<T> = <T as Id_>::Out;

impl<T> Id_ for T {
    type Out = T;
}

fn coerce<A>(_: PhantomData<A>, x: A) -> A { x }

#[test]
fn test() {
    type Bool = Id<bool>;
    let x = coerce(PhantomData::<Bool>, true);
}

But this, with the only difference being the "default" keyword, does not:

use std::marker::PhantomData;

trait Id_ {
    type Out;
}

type Id<T> = <T as Id_>::Out;

impl<T> Id_ for T {
    default type Out = T;
}

fn coerce<A>(_: PhantomData<A>, x: A) -> A { x }

#[test]
fn test() {
    type Bool = Id<bool>;
    let x = coerce(PhantomData::<Bool>, true);
}

The error reported is "associated type expected, found bool" in the second argument to the call to coerce.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 6, 2016

@dylanede Yep, looks like a bug. I'm a bit slammed with other things but I'll try to add this as an additional test for this PR :)

@dylanede

This comment has been minimized.

Copy link
Contributor

dylanede commented Jan 7, 2016

Another thing I've noticed. In some cases if an associated type specialization of an impl appears earlier in the file than the impl it specializes, I get an ICE with "unwrap on a None".

EDIT: actually, this appears to be every time.

@dylanede

This comment has been minimized.

Copy link
Contributor

dylanede commented Jan 7, 2016

For example, this code gets the ICE:

trait Foo {
    type Out;
}

impl Foo for bool {
    type Out = ();
}

impl<T> Foo for T {
    default type Out = bool;
}
@dylanede

This comment has been minimized.

Copy link
Contributor

dylanede commented Jan 7, 2016

Another, different ICE:

trait Id_ {
    type Out;
}

type Id<T> = <T as Id_>::Out;

impl<T> Id_ for T {
    default type Out = T;
}

fn main() {
    let x: Id<bool> = panic!();
}

Error and backtrace:

error: internal compiler error: type_of with TyProjection
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
thread 'rustc' panicked at 'Box<Any>', src/libsyntax/errors/mod.rs:230
stack backtrace:
   1:     0x7fd80e171460 - sys::backtrace::tracing::imp::write::h51a4398742da0623K0t
   2:     0x7fd80e177fa5 - panicking::log_panic::_<closure>::closure.41506
   3:     0x7fd80e177a67 - panicking::log_panic::hf3690f0da42f7f25Y7x
   4:     0x7fd80e141043 - sys_common::unwind::begin_unwind_inner::h53054637973a059djTs
   5:     0x7fd80a1d4657 - sys_common::unwind::begin_unwind::begin_unwind::h16847477020784951041
   6:     0x7fd80a1d4a14 - errors::Handler::bug::h623de609723e663bAZc
   7:     0x7fd80b5fd4ec - session::Session::bug::hf2ec81b68b650bda9es
   8:     0x7fd80cae9c7d - trans::type_of::in_memory_type_of::hb00ac4f71a5d2dc2bdQ
   9:     0x7fd80caed9cb - trans::base::alloc_ty::h4a46066a01d64342yhi
  10:     0x7fd80cc08f54 - trans::_match::mk_binding_alloca::h16584621283196570395
  11:     0x7fd80cb0986e - trans::base::init_local::h3cc56120bc50056al2h
  12:     0x7fd80cb23e12 - trans::controlflow::trans_block::h14e00c38eb84fe83VAw
  13:     0x7fd80cb204f7 - trans::base::trans_closure::hd70f9863156f5359bQi
  14:     0x7fd80cb2478c - trans::base::trans_fn::hf204ab03c9603db1XZi
  15:     0x7fd80cb28caf - trans::base::trans_item::hc86e02775fa6daedXnj
  16:     0x7fd80cb34c64 - trans::base::trans_crate::h1366c98d49e7fc59K7j
  17:     0x7fd80e6e50a1 - driver::phase_4_translate_to_llvm::h72f03170468523ffuTa
  18:     0x7fd80e6def36 - driver::phase_3_run_analysis_passes::_<closure>::closure.25890
  19:     0x7fd80e6bcc05 - middle::ty::context::ctxt<'tcx>::create_and_enter::create_and_enter::h12957577696868225661
  20:     0x7fd80e6b8521 - driver::phase_3_run_analysis_passes::h14675146087569771906
  21:     0x7fd80e68ceb9 - driver::compile_input::hcea038a856e2e417jca
  22:     0x7fd80e67f0cb - run_compiler::h6bc599602c13f1ce5wc
  23:     0x7fd80e67be86 - sys_common::unwind::try::try_fn::try_fn::h12460656089999964736
  24:     0x7fd80e16f0f8 - __rust_try
  25:     0x7fd80e1668eb - sys_common::unwind::try::inner_try::h193a98f8a6ded06bRPs
  26:     0x7fd80e67c1e0 - boxed::F.FnBox<A>::call_box::call_box::h4438575142861133636
  27:     0x7fd80e1764b3 - sys::thread::Thread::new::thread_start::he0b127204317aff8bax
  28:     0x7fd807a430a3 - start_thread
  29:     0x7fd80ddf504c - clone
  30:                0x0 - <unknown>

If the default keyword is removed it compiles fine.

@dylanede

This comment has been minimized.

Copy link
Contributor

dylanede commented Jan 7, 2016

Similarly, if I try std::instrinsics::type_name on any type retrieved from an associated type of a specialized trait impl, I get an ICE complaining that the type is unexpected, where the type shown is the unnormalized associated type.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 11, 2016

⛵️

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 12, 2016

@dylanede BTW, are you following the thread on the RFC? We are moving toward removing specialization of associated types.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 12, 2016

@dylanede (In particular, I'm now in the process of rebasing and addressing the initial review, and plan to remove the code for associated type specialization.)

@dylanede

This comment has been minimized.

Copy link
Contributor

dylanede commented Jan 12, 2016

@aturon Ah, I hadn't been. Bear in mind that if you remove specialisation of associated types, the metaprogramming capabilities are pretty much completely removed. As it is I don't think Rust's type system is Turing Complete at the moment (ever since the changes that added the rule that impl type parameters must appear in the parameters for the trait being impl'ed or the type being impl'ed for) - I have repeatedly tried and failed to implement the untyped lambda calculus in it. With proper associated type specialisation, the implementation of ULC is almost trivial. I currently have prototype libraries waiting on better metaprogramming support, as the type system as it is doesn't seem capable enough - an Earley-based CFG parsing library and an expression-template style linear algebra library with similar aims to Eigen for C++.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 12, 2016

@dylanede So, part of the debate on the RFC is whether, given the constraints we have to put on projections, specializable associated types are all that useful. If you could leave a comment on thread giving some concrete details of your use case, that'd be very helpful!

@DemiMarie

This comment has been minimized.

Copy link
Contributor

DemiMarie commented Jan 27, 2016

As I understand it, @dylanede's use case is type-level metaprogramming. This allows for design patterns currently not possible in Rust, such as expression templates. Expression templates can achieve massive performance improvements (they are analogous to stream fusion in Haskell).

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Jan 27, 2016

Expression templates can achieve massive performance improvements (they are analogous to stream fusion in Haskell).

Keep in mind that stream fusion is unnecessary in Rust, as lazy lists (which can be inadvertently fully evaluated) were replaced by "strictly lazy" streams (Iterator), effectively "fused" at monomorphization time.
LLVM only really needs to remove abstraction boundaries to get the exact same performance as the equivalent loop.

So I'd take "massive" with a grain of salt, but that's just me.

@aturon aturon force-pushed the aturon:specialization branch 4 times, most recently from 1e9ab7d to 4b09f0c Jan 30, 2016

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 30, 2016

@nikomatsakis

I've now rebased and pushed a commit that addresses the more basic nits you brought up.

There are several more things that need to happen before landing:

  • I want to resolve the nits around walking up the specialization graph by exploring an iterator-based approach, as you suggested; I'm hopeful this can also reduce a bit of code duplication.
  • I have not had success trying to follow better hygiene with "orphaned" substs and so on (that are floating around independently of any InferCtxt, but might have inference variables within). The code is carefully written so that it doesn't matter, but I agree with @nikomatsakis that this creates opportunities for future bugs.
    • The main problem I'm hitting is an inability to either (1) update the parameter environment in a reasonable way or (2) clone an inference context. That's forcing me to gin up new contexts out of thin air, which happens to work for my purposes here but is error prone.
  • There are a handful of TODOs. Most of them are just me checking in with @nikomatsakis about things I'm not sure about. But we also need to e.g. fully work out the story with "auto" traits and negative impls.
  • I want to track down the ICEs and weird limitations that @dylanede discovered.

I'm hoping to have all these addressed before the RFC is accepted, but @nikomatsakis I need your help on several of these points.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 30, 2016

Oh, also, my branch is currently failing some dependency graph tests, and I don't fully understand why.

@aturon aturon force-pushed the aturon:specialization branch from 4b09f0c to efd700d Feb 2, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 11, 2016

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

@aturon aturon force-pushed the aturon:specialization branch from 654e199 to 6562eeb Mar 14, 2016

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Mar 14, 2016

@bors: r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 14, 2016

📌 Commit 6562eeb has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 15, 2016

⌛️ Testing commit 6562eeb with merge 9ca7561...

bors added a commit that referenced this pull request Mar 15, 2016

Auto merge of #30652 - aturon:specialization, r=nikomatsakis
Implement RFC 1210: impl specialization

This PR implements [impl specialization](rust-lang/rfcs#1210),
carefully following the proposal laid out in the RFC.

The implementation covers the bulk of the RFC. The remaining gaps I know of are:

- no checking for lifetime-dependent specialization (a soundness hole);
- no `default impl` yet;
- no support for `default` with associated consts;

I plan to cover these gaps in follow-up PRs, as per @nikomatsakis's preference.

The basic strategy is to build up a *specialization graph* during
coherence checking. Insertion into the graph locates the right place
to put an impl in the specialization hierarchy; if there is no right
place (due to partial overlap but no containment), you get an overlap
error. Specialization is consulted when selecting an impl (of course),
and the graph is consulted when propagating defaults down the
specialization hierarchy.

You might expect that the specialization graph would be used during
selection -- i.e., when actually performing specialization. This is
not done for two reasons:

- It's merely an optimization: given a set of candidates that apply,
  we can determine the most specialized one by comparing them directly
  for specialization, rather than consulting the graph. Given that we
  also cache the results of selection, the benefit of this
  optimization is questionable.

- To build the specialization graph in the first place, we need to use
  selection (because we need to determine whether one impl specializes
  another). Dealing with this reentrancy would require some additional
  mode switch for selection. Given that there seems to be no strong
  reason to use the graph anyway, we stick with a simpler approach in
  selection, and use the graph only for propagating default
  implementations.

Trait impl selection can succeed even when multiple impls can apply,
as long as they are part of the same specialization family. In that
case, it returns a *single* impl on success -- this is the most
specialized impl *known* to apply. However, if there are any inference
variables in play, the returned impl may not be the actual impl we
will use at trans time. Thus, we take special care to avoid projecting
associated types unless either (1) the associated type does not use
`default` and thus cannot be overridden or (2) all input types are
known concretely.

r? @nikomatsakis

@bors bors merged commit 6562eeb into rust-lang:master Mar 15, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 15, 2016

🎋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment