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

Tracking issue for type_id stabilization #27745

Open
aturon opened this Issue Aug 12, 2015 · 38 comments

Comments

Projects
None yet
@aturon
Member

aturon commented Aug 12, 2015

The method get_type_id in Any is intended to support reflection. It's currently unstable in favor of using an associated constant instead.

@bluss

This comment has been minimized.

Contributor

bluss commented Sep 8, 2015

It was pointed out it's one of the very few methods to be named with get_ prefix, so it could be renamed to just type_id().

@aturon

This comment has been minimized.

Member

aturon commented Sep 8, 2015

I suspect this is just some very old convention that wasn't updated during stabilization (since we didn't stabilize this API).

@flosse

This comment has been minimized.

flosse commented Jul 25, 2016

Any progress here?

@Zaerei

This comment has been minimized.

Zaerei commented Aug 11, 2016

I tried to tackle it, but it looks like the main problem is that TypeId::of isn't constant and can't be constant. It calls intrinsics::type_id which, on any attempt to make it a const function yields the cryptic error "unmatched visibility pub". This would be a simple change if TypeId::of was a const fn.

(Though making an extern function const is probably dicey from a compiler perspective anyway, I'm not an expert on that).

@aturon aturon added the I-nominated label Aug 29, 2016

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Sep 12, 2016

Removing nomination as this was an accidental mistake about what this issue was about.

@fschutt

This comment has been minimized.

Contributor

fschutt commented Mar 17, 2017

Any update on this? Is is discouraged to use it in current code?

@jweinst1

This comment has been minimized.

jweinst1 commented Apr 9, 2017

Is there any other way to do reflection or dynamic typing that will work?

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Apr 9, 2017

@jweinst1 the rest of the std::any::Any trait is stable:

fn is<T>(&self) -> bool where T: Any;
fn downcast_ref<T>(&self) -> Option<&T> where T: Any;
fn downcast_mut<T>(&mut self) -> Option<&mut T> where T: Any;
fn downcast<T>(Box<self>) -> Result<Box<T>, Box<Any>> where T: Any ;

In each of these the call site is specifying a specific T. (Internally the implementations of these method compare Self::type_id() and T::type_id().)

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Apr 9, 2017

I’ve just realized that TypeId and TypeId::of are also stable, so you could copy the Any trait into your own code.

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jan 16, 2018

I propose we make TypeId::of a constant function. This should be a rather trivial step, primarily just adding it to the list of constant intrinsics as far as I can tell. (@eddyb Can you confirm?).

Once that's done, we can add a constant to Any and Error that'll replace the currently unstable methods associated with both, and stabilize those.


@SimonSapin I don't think it's that easy since there exist methods in the wild that pass around trait objects (&Any and &Error) and since we don't have trait object casts today (if we ever will, not sure if it's possible, seems like perhaps not) without these constants/methods there's not a way to retrieve the underlying type of a trait object.

@sfackler

This comment has been minimized.

Member

sfackler commented Jan 16, 2018

It might be better off as an associated constant if possible.

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jan 16, 2018

I might not be understanding, but I think that's what I suggested? That we have something like const TYPE_ID: TypeId = TypeId::of::<Self>(); in Any and Error?

@sfackler

This comment has been minimized.

Member

sfackler commented Jan 16, 2018

Oh right derp, there isn't anywhere to put the constant on TypeId itself since it isn't parameterized.

@Badel2

This comment has been minimized.

Contributor

Badel2 commented Jan 30, 2018

Alright, I'm working on this and I think I managed to turn type_id into a constant intrinsic, meaning I can do const A: TypeId = TypeId::of::<T>();, however when I try to add it to the Any trait I get an error (E0038) that says "the trait cannot contain associated consts". Anyway, I'll add a test and submit a PR.

Edit: I solved the mistery, the const needs to be inserted in the impl Any block, not in trait Any. This can be done after the next stage0.

@glaebhoerl

This comment has been minimized.

Contributor

glaebhoerl commented Jan 31, 2018

I wonder if #10389 is relevant here

kennytm added a commit to kennytm/rust that referenced this issue Feb 4, 2018

Rollup merge of rust-lang#47892 - Badel2:const_type_id_of, r=oli-obk
Turn `type_id` into a constant intrinsic

rust-lang#27745

The method `get_type_id` in `Any` is intended to support reflection. It's currently unstable in favor of using an associated constant instead. This PR makes the `type_id` intrinsic a constant intrinsic, the same as `size_of` and `align_of`, allowing `TypeId::of` to be a `const fn`, which will allow using an associated constant in `Any`.
@Badel2

This comment has been minimized.

Contributor

Badel2 commented Mar 3, 2018

It turns out that associated constants cannot be used in trait objects, because the constants aren't stored in the vtable. So this won't work:

trait Any {
    const TYPE_ID: TypeId = TypeId::of::<Self>();
}

And this also won't work, because it just stores the TypeId of the Any trait object, not the TypeId of the type that implements Any:

trait Any {}
impl Any {
    const TYPE_ID: TypeId = TypeId::of::<Self>();
}

So assuming we still want to make TYPE_ID an associated constant, we need to add support for associated constants in trait objects, or make this a special case and add a dedicated field to the vtable which stores the TypeId.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 16, 2018

we need to add support for associated constants in trait objects

I think this wouldn’t work? This code:

fn foo(object: &Any) {
    let type_id = object.get_type_id();
}

… can call a trait method from a trait object. Accessing an associated constant would be <T as Any>::TYPE_ID, but this code does not know what T is.

Other code that does have a concrete T type can use TypeId::of::<T>(), which is now a const fn.

So I propose keeping this method as a method, renaming to type_id to remove the get_ prefix, and stabilizing.

@sfackler

This comment has been minimized.

Member

sfackler commented Mar 16, 2018

That seems reasonable.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 17, 2018

I propose keeping this method as a method, renaming to type_id to remove the get_ prefix, and stabilizing.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

rfcbot commented Mar 17, 2018

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@sfackler

This comment has been minimized.

Member

sfackler commented Mar 17, 2018

Seems like we might as well make it a const method while we're stabilizing it. It should be pretty trivial since TypeId::of is already const.

@rfcbot

This comment has been minimized.

rfcbot commented Mar 18, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@Badel2

This comment has been minimized.

Contributor

Badel2 commented Mar 18, 2018

@sfackler get_type_id cannot be const because it is a trait method:

trait Any {
    const fn get_type_id(&self) -> TypeId;
}
error[E0379]: trait fns cannot be declared const
 --> src/main.rs:5:5
5 |     const fn get_type_id(&self) -> TypeId;
  |     ^^^^^ trait fns cannot be const
@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Mar 19, 2018

@rfcbot concern performance

Are we sure we want to stabilize this as a function? I figured we'd hold off on stabilizing this as it requires some form of rustc storing constants in vtables (which is currently unimplemented). As a result the proposed type_id can have not-so-great perf as it's a virtual function call rather than a simple load.

Do we have good rationale for stabilizing the known slower approach? Are there use cases today that are critically waiting on type_id() to stabilize to get enabled?

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 19, 2018

I don’t mind waiting for another solution, but in addition to implementing support for constants in vtables we’d need syntax to access them. As far as I know no such syntax exists today, and I don’t know have a clear idea of what it could look like.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Mar 19, 2018

Yes it's true that it's quite aways away, but is the rationale for stabilizing these today with known drawbacks?

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 19, 2018

It doesn’t have to be today, but having a realistic path to some resolution one way or another within the next few months would be nice.

This issue is two and a half years old. This method has been unstable for as long as the stability mechanism has existed. In my opinion features being unstable should be a temporary state while we figure out unresolved questions (and ideally get some usage experience).

At some point we should decide: if this feature turns out not to be useful, let’s deprecate and later remove it. If it is useful, then there is a need that we’re failing to serve if after several years we’re still waiting for an hypothetical language feature when we don’t even have a sketch of what that would look like.

Our choice for the foreseeable future is not slower vs faster, it’s slower vs nothing. If we stabilize, we always have the option to deprecate later when/if something better comes along.

@nox

This comment has been minimized.

Contributor

nox commented Mar 19, 2018

I have a way out, I think:

  1. keep Any::get_type_id as unstable;
  2. introduce an inherent method type_id(&self) -> TypeId on Any calling get_type_id;
  3. change type_id to use vtable constants whenever those are introduced into the language.

Edit: s/type_of/type_id/

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Mar 19, 2018

@SimonSapin I agree that we shouldn't leave issues in limbo forever, but I don't think it sounds like it's urgent we do anything on this in the near future? We have a theoretical path to stabilization and if necessary to stabilize in the near future we can do so with the understanding that it's not as good as it may be one day and plan appropriately.

To that end I'd still like to see an argument in favor of stabilizing this today. I think it's fine to do so, but it'd be best to have a reason more than "this has been unstable for quite some time". Lots of implementation details in the standard library are pema-unstable, and this is currently an implementation detail of the is method.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Mar 19, 2018

I'm not certain that there's a benefit to stabilizing this. If I want to create a downcastable trait object, I have to define my own get_type_id in that trait anyway (see the mopa crate for example).

But I think any better solution would not benefit from this being unstable either. Probably, a better solution would involve deprecating the Any trait entirely, since it works so poorly for this feature (again, see mopa). We'd want to extend the language support beyond the type-id intrinsic probably, to include something like the ability to get the type id from any 'static trait object's vtable, or the ability to downcast any 'static trait object with first class syntax, or something along those lines. We want to revisit trait objects in general soon after the 2018 edition stuff is done.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 19, 2018

Lots of implementation details in the standard library are pema-unstable

It’s fine that those implementation details exist, but we should be clear about what items are in that category as opposed to features that are not stable yet but have some path to stabilization. Perma-unstable implementation details have #[doc(hidden)] and no tracking issue.

The existence of this open tracking issue indicates that there is something to be tracked. Closing it and deciding that get_type_id should be a hidden implementation detail is one possible resolution.

@ExpHP

This comment has been minimized.

Contributor

ExpHP commented Mar 20, 2018

Hm? Did rfcbot not register the concern here? I thought it normally edits those into its post.

@leodasvacas

This comment has been minimized.

Contributor

leodasvacas commented Mar 23, 2018

We want to revisit trait objects in general soon after the 2018 edition stuff is done.

Unfortunate timing, considering a new syntax will be introduced for them. This would otherwise be a good opportunity to introduce small breaking changes to behavior. But I guess that's off topic here.

@rfcbot

This comment has been minimized.

rfcbot commented Mar 28, 2018

The final comment period is now complete.

tmccombs added a commit to tmccombs/rust that referenced this issue Apr 3, 2018

@tmccombs

This comment has been minimized.

Contributor

tmccombs commented Apr 3, 2018

I figured we'd hold off on stabilizing this as it requires some form of rustc storing constants in vtables (which is currently unimplemented).

Are there any plans to implement this, or RFCs that specify that?

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Apr 6, 2018

@withoutboats

I'm not certain that there's a benefit to stabilizing this. If I want to create a downcastable trait object, I have to define my own get_type_id in that trait anyway (see the mopa crate for example).

That's the exact motivation for stabilising this: as a trait method it is directly callable on trait objects extending Any. That means that mopa and the like can get rid of their custom Any traits, which I view as a significant benefit.

This issue is even directly linked from comments in the mopa source code: https://github.com/chris-morgan/mopa/blob/master/src/lib.rs#L163

We should stabilize this as-is - I don't see how performance is a concern since we already have TypeId::of for the static case, and we're still leaving open the possibility of adding it as an associated constant if trait objects gain support for them.

leodasvacas added a commit to leodasvacas/rust that referenced this issue May 22, 2018

`Self` in where clauses may not be object safe
This is virtually certain to cause regressions, needs crater.

In rust-lang#27745 it was discovered that our object safety rules are not sound because we allow `Self` in where clauses without restrain. This PR is a direct fix to the rules so that we disallow methods with unsound where clauses.

This currently uses hard error to measure impact, but we will want to downgrade it to a future compat error.

Fixes rust-lang#27745.

r? @nikomatsakis
@tmccombs

This comment has been minimized.

Contributor

tmccombs commented Jun 3, 2018

Unfortunately I think there may be a bug in fcp where I registered a blocking concern to block stabilizing this method but it didn't halt the FCP process.

From #49613 (comment)

@Centril should this have the finished-final-comment-period label?

See also, #27745 (comment)

@Centril

This comment has been minimized.

Contributor

Centril commented Jun 3, 2018

@tmccombs unclear; I defer to @rust-lang/libs

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