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

Reflect trait #23712

Merged
merged 5 commits into from
Mar 28, 2015
Merged

Reflect trait #23712

merged 5 commits into from
Mar 28, 2015

Conversation

nikomatsakis
Copy link
Contributor

This PR introduces a Reflect marker trait which is a supertrait of Any. The idea is that Reflect is defined for all concrete types, but is not defined for type parameters unless there is a T:Reflect bound. This is intended to preserve the parametricity property. This allows the Any interface to be stabilized without committing us to unbounded reflection that is not easily detectable by the caller.

The implementation of Reflect relies on an experimental variant of OIBIT. This variant behaves differently for objects, since it requires that all types exposed as part of the object's interface are Reflect, but isn't concerned about other types that may be closed over. In other words, you don't have to write Foo+Reflect in order for Foo: Reflect to hold (where Foo is a trait).

Given that Any is slated to stabilization and hence that we are committed to some form of reflection, the goal of this PR is to leave our options open with respect to parametricity. I see the options for full stabilization as follows (I think an RFC would be an appropriate way to confirm whichever of these three routes we take):

  1. We make Reflect a lang-item.
  2. We stabilize some version of the OIBIT variation I implemented as a general mechanism that may be appropriate for other use cases.
  3. We give up on preserving parametricity here and just have impl<T> Reflect for T instead. In that case, Reflect is a harmless but not especially useful trait going forward.

cc @aturon
cc @alexcrichton
cc @glaebhoerl (this is more-or-less your proposal, as I understood it)
cc @reem (this is more-or-less what we discussed on IRC at some point)
cc @flaper87 (vaguely pertains to OIBIT)

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@@ -82,20 +82,61 @@ use marker::Sized;
// Any trait
///////////////////////////////////////////////////////////////////////////////

/// A marker trait indicates a type that can be reflected over. This
/// trait is implemented for all types. It's purpose is to ensure that
Copy link
Contributor

Choose a reason for hiding this comment

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

s/It's/Its/

@nikomatsakis
Copy link
Contributor Author

r? @flaper87

I'm not sure how is best to review this to be honest. @flaper87 knows the code well, so he's a good start, but I'd like @aturon and @glaebhoerl to look over the logic (and anyone else who would like to do so). The basic idea is that you can say that Trait<A,B,Output=C>: Reflect if A, B, and C all implement Reflect. This is a bit specific to reflect in that it assumes that any concrete types which appear in the trait interface already implement Refect (since all concrete types implement reflect, after all).

@rust-highfive rust-highfive assigned flaper87 and unassigned pnkfelix Mar 25, 2015
@alexcrichton
Copy link
Member

Does this also need to add the Reflect bound to TypeId::of? (this would transitively affect Any::downcast)

@reem
Copy link
Contributor

reem commented Mar 25, 2015

Since we want to stabilize TypeId::of it needs the Reflect bound.

/// [1]: http://en.wikipedia.org/wiki/Parametricity
#[rustc_reflect_like]
#[stable(feature = "rust1", since = "1.0.0")]
pub trait Reflect: 'static {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not need some sort of marker bound to make Self used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because where Self: 'static is itself a use of Self.

@nikomatsakis
Copy link
Contributor Author

@alexcrichton

Does this also need to add the Reflect bound to TypeId::of? (this would transitively affect Any::downcast)

Ah, yes. Thanks.

@nikomatsakis
Copy link
Contributor Author

Note: make check revealed some minor bugs, I'll mop those up tomorrow.

}

#[cfg(stage0)]
impl<T: 'static> Reflect for T { }
Copy link
Contributor

Choose a reason for hiding this comment

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

oh mmh, I thought we already had a snapshot for defaulted traits.

@glaebhoerl
Copy link
Contributor

Oh, yay! I didn't think there was still hope for improving this. :)

Just to be explicit about it, the solution I had been envisioning was that we would have something like:

trait Reflect { const TYPE_ID: TypeId; ... }

and for every type that is declared, an impl for that particular type would automatically be derived:

// you write
struct Foo { ... }
// compiler writes
impl Reflect for Foo { ... }

// you write
struct Bar<T> { ... }
// compiler writes
impl<T: Reflect> Reflect for Foo<T> { ... }

// you write
struct Baz<'a, T> { ... }
// compiler writes
impl<T: Reflect> Reflect for Baz<'static, T> { ... }

and so on. (This gets hairier with HKTs, because then you need either Reflect to be polykinded or an infinite family of Reflect traits at each kind to be able to (auto-)write impls for types with higher-kinded type parameters, but we can cross that bridge when we come to it.)

Three inter-connected thoughts I don't know how to paragraphise together:

  • Above I have TYPE_ID as a member of Reflect, but whether it's done that way or if the compiler "natively" knows type IDs for each type, and Reflect is only used as a way to regulate access to that functionality (as I believe is done in this PR), seems like basically an implementation detail.
  • Instead of auto-deriving as above, explicit #[derive(Reflect)] could also be required, like GHC used to do with Typeable, but apart from concerns like code size (for run-time type information) I can't imagine any reason you wouldn't want to have a given type to have an impl (which is why GHC is going to auto-derive as of 7.10).
  • Ability to reason about code (parametricity) is the biggest reason to want this, but another, more down-to-earth reason might be that if we ever want to extend "don't pay for what you don't use" to RTTI, then having impl<T> Any for T seems like it would prevent us from doing that. (Or at least, my assumption is that Rust currently emits type IDs into the generated code for every declared type, but maybe this is mistaken?)

In connection to the above points GHC's new Typeable implementation might also be of interest.

All of that throat-clearing out of the way: I'm not really familiar with OIBIT so I'm not sure to what extent it matches up with the above, but I suspect that it probably does.

With respect to traits, just to make sure I'm understanding correctly, the idea is that given a non-trait type viewed as a trait object, let foo: &Display = &123: &u32, you wouldn't be able to reflect into foo and discover that it is in fact a u32, and for that, you would need to use &(Display+Reflect) or &(Display+Any)? And meanwhile, given bar: &Any = foo: &Display, we do want to be able to discover that bar is in fact a Display trait object? (So in Haskell, this corresponds to something like (using ExistentialQuantification and ConstraintKinds) data TraitObject (trait :: * -> Constraint) = forall ty. trait ty => TraitObject ty, where we have instance Typeable trait => Typeable (TraitObject trait), with Typeable, at kind Constraint, being required for the trait (class) that's used as a parameter in order for TraitObject trait itself to be Typeable.)

If that's the case, then I still have to think harder about it to be sure :), but the proposed rules feel like they probably have the right idea.

@glaebhoerl
Copy link
Contributor

Another thing that's occurred to me is that we could potentially generalize the above scheme to lift the 'static restriction, with something like:

trait Reflect<'a> { ... }

struct Foo<'a> { ... }
// auto-derived:
impl Reflect<'a> for Foo<'a> { ... }

type OldReflect = Reflect<'static>

trait Any<'a>: Reflect<'a> { ... }

(This might need the auto-generated impls to vary in accordance with the variances of the given type's parameters.)

@nikomatsakis
Copy link
Contributor Author

OK, this passes make check now. I had to cleanup some of the binder logic, there's still some questionable bits (I left a FIXME...) but good enough for now I guess.

@nikomatsakis
Copy link
Contributor Author

I also significantly revised the design of the trait. The Reflect trait is now basically a marker trait you can use to tag other reflection mechanisms, but not interesting to users on its own. Even better, it can be marked unstable. This means that functions that wish to use reflections over generic types should bound those types with Any, not Reflect.

The reason for this change (among other things):

  1. The old design requires Reflect: 'static, which was stronger than other reflection mechanisms need, and also too strong for Any. In particular, Fn(&int) failed because that would in turn require for<'a> &'a int: 'static (because Reflect requires that all types reachable impl Reflect, unlike the normal intepretation).
  2. This means that trait Any: Reflect+'static contains the full requirements for TypeId etc. The use of Reflect is there just to ensure that the blanket impl for Any is not "overeager".
  3. It seems to just read nicer in general -- fewer traits to be concerned with.

@alexcrichton
Copy link
Member

I'm quite happy with how this turned out! r=me from the library side at least

@flaper87
Copy link
Contributor

@bors: r+ dd8cf92

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 27, 2015
 This PR introduces a `Reflect` marker trait which is a supertrait of `Any`. The idea is that `Reflect` is defined for all concrete types, but is not defined for type parameters unless there is a `T:Reflect` bound. This is intended to preserve the parametricity property. This allows the `Any` interface to be stabilized without committing us to unbounded reflection that is not easily detectable by the caller.

The implementation of `Reflect` relies on an experimental variant of OIBIT. This variant behaves differently for objects, since it requires that all types exposed as part of the object's *interface* are `Reflect`, but isn't concerned about other types that may be closed over. In other words, you don't have to write `Foo+Reflect` in order for `Foo: Reflect` to hold (where `Foo` is a trait).

Given that `Any` is slated to stabilization and hence that we are committed to some form of reflection, the goal of this PR is to leave our options open with respect to parametricity. I see the options for full stabilization as follows (I think an RFC would be an appropriate way to confirm whichever of these three routes we take):

1. We make `Reflect` a lang-item.
2. We stabilize some version of the OIBIT variation I implemented as a general mechanism that may be appropriate for other use cases.
3. We give up on preserving parametricity here and just have `impl<T> Reflect for T` instead. In that case, `Reflect` is a harmless but not especially useful trait going forward.

cc @aturon
cc @alexcrichton
cc @glaebhoerl (this is more-or-less your proposal, as I understood it)
cc @reem (this is more-or-less what we discussed on IRC at some point)
cc @flaper87 (vaguely pertains to OIBIT)
@nikomatsakis
Copy link
Contributor Author

@glaebhoerl

what is implemented matches what you wrote fairly closely, but with the exception that it is automatically lifted over structural types and so forth.

In terms of objects, there is (currently) no mechanism for testing what type an object has, but yes if you wanted to do that you'd presumably need something like &(Trait+Any).

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 27, 2015
This PR introduces a `Reflect` marker trait which is a supertrait of `Any`. The idea is that `Reflect` is defined for all concrete types, but is not defined for type parameters unless there is a `T:Reflect` bound. This is intended to preserve the parametricity property. This allows the `Any` interface to be stabilized without committing us to unbounded reflection that is not easily detectable by the caller.

The implementation of `Reflect` relies on an experimental variant of OIBIT. This variant behaves differently for objects, since it requires that all types exposed as part of the object's *interface* are `Reflect`, but isn't concerned about other types that may be closed over. In other words, you don't have to write `Foo+Reflect` in order for `Foo: Reflect` to hold (where `Foo` is a trait).

Given that `Any` is slated to stabilization and hence that we are committed to some form of reflection, the goal of this PR is to leave our options open with respect to parametricity. I see the options for full stabilization as follows (I think an RFC would be an appropriate way to confirm whichever of these three routes we take):

1. We make `Reflect` a lang-item.
2. We stabilize some version of the OIBIT variation I implemented as a general mechanism that may be appropriate for other use cases.
3. We give up on preserving parametricity here and just have `impl<T> Reflect for T` instead. In that case, `Reflect` is a harmless but not especially useful trait going forward.

cc @aturon
cc @alexcrichton
cc @glaebhoerl (this is more-or-less your proposal, as I understood it)
cc @reem (this is more-or-less what we discussed on IRC at some point)
cc @flaper87 (vaguely pertains to OIBIT)
@bors bors merged commit dd8cf92 into rust-lang:master Mar 28, 2015
/// [1]: http://en.wikipedia.org/wiki/Parametricity
#[rustc_reflect_like]
#[unstable(feature = "core", reason = "requires RFC and more experience")]
pub trait Reflect : MarkerTrait {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is totally tangential and has probably been raised before, but isn't this saying trait Reflect where Self: MarkerTrait, whereas what we might actually want (and what might actually make sense) would be impl MarkerTrait for Reflect?

Copy link
Member

Choose a reason for hiding this comment

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

This exists solely because Self and type params have to be used by methods or supertraits.

@glaebhoerl
Copy link
Contributor

(Is there no interest in lifting the 'static restriction on Any, or is it not that simple?)

@eddyb
Copy link
Member

eddyb commented Mar 29, 2015

@glaebhoerl it's impossible (the set of possible lifetime instantiations is almost always unbounded at compile time), unless you have some scheme in mind for reflecting lifetimes.
Or we could go for some limited covariant scheme where TypeId is parametrized by the shortest lifetime.
Even if that would work, it would only allow immutable access, as mutable acess must be invariant.
I have to wonder whether trait objects have some lurking unsoundness because of this

@glaebhoerl
Copy link
Contributor

@eddyb I was thinking that Any itself would have a lifetime parameter (like Reflect in my previous comment), so &Any<'a> could close over things of lifetime at least 'a.

@nikomatsakis
Copy link
Contributor Author

@glaebhoerl interesting thought; also seems like a possible future extension if we permitted defaulted lifetime parameters.

blaenk added a commit to blaenk/anymap that referenced this pull request Apr 6, 2015
I believe the [new] std::marker::[Reflect] trait made it necessary to
specify a Reflect bound, but it's advised to use an Any bound instead.

Adding this bound makes it compile. I ran the tests and got an error
saying it couldn't determine the type of one of the expressions so I
introduced a scope to bind the result of the expression and annotate its
type.

I also removed the `convert` feature that was no longer needed.

[new]: rust-lang/rust#23712
[Reflect]: http://doc.rust-lang.org/std/marker/trait.Reflect.html
@nikomatsakis nikomatsakis deleted the reflect-trait branch March 30, 2016 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants