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

Transmute trait #1891

Closed
wants to merge 3 commits into from
Closed

Transmute trait #1891

wants to merge 3 commits into from

Conversation

canndrew
Copy link
Contributor

@canndrew canndrew commented Feb 9, 2017

Add a Transmute<T> trait for representing types that can be transmuted to T.

Rendered

@strega-nil
Copy link

strega-nil commented Feb 9, 2017

I don't get why you want to make transmute more useable in Rust. We should be trying to get rid of transmute, not making it better. Transmute is one of the worst unsafeties in Rust, and new users (and cough old users) use it far too much. Giving more support to it would make it even more popular, while we want it to be less popular.

@glaebhoerl
Copy link
Contributor

"The point", as I take it, is to clean up a wart in the language definition. Why have this kind of magical special-cased condition in the type system for transmute, when it could be expressed as a trait, like other things are?

@petrochenkov
Copy link
Contributor

@glaebhoerl
There are quite a few such conditions (starting with operator as and coercions).
Fixing them all may require a whole zoo of magic traits, similar to <type_traits>.

@canndrew
Copy link
Contributor Author

canndrew commented Feb 9, 2017

@ubsan There's no reason why we couldn't do this then deprecate transmute in the future. In the mean time, I don't see the point in keeping the type system broken.

@mark-i-m
Copy link
Member

mark-i-m commented Feb 9, 2017

The way I see it, using transmute is a wart whether or not it is covered in a trait. I might be missing something, but it seems that there is no way in Rust currently to say impl Transmute<U> for T where sizeof::<U>() == sizeof::<T>(), so we would just be moving the hack, right?

@canndrew
Copy link
Contributor Author

canndrew commented Feb 9, 2017

@petrochenkov

There are quite a few such conditions (starting with operator as and coercions).

Well as could be From if we promoted it to a lang item. If we're not comfortable with that and want to keep as meaning a trivial type conversion then, sure, why not add an As trait too?

I don't see how one for coercions would be useful, you can always use as explicitly.

Fixing them all may require a whole zoo of magic traits, similar to <type_traits>.

What other ones are there? You could argue for any operation like this having a trait but transmute in particular since it's a function which currently has magic rules attached to it.

@canndrew
Copy link
Contributor Author

canndrew commented Feb 9, 2017

@mark-i-m

so we would just be moving the hack, right?

Well the transmutability check would just move into trait selection. But it would make rust kind-safe like it's supposed to be.

Edit: Also, where sizeof::<U>() == sizeof::<T>() isn't the complete definition because we also need to ensure that U is inhabited if T is.

@mark-i-m
Copy link
Member

mark-i-m commented Feb 9, 2017

@canndrew I must confess my ignorance of what kind-safeness is and why we want it. Could you expound please?

@strega-nil
Copy link

strega-nil commented Feb 9, 2017

@canndrew It's not broken now. It's just not an operator that should be used in a generic context. In fact, I would argue that all such uses, excepting implementation of slice::from_raw_parts, are broken. transmute is a special case of bad unsafe code, that's only not deprecated because you're forced to use it to convert data pointers to function pointers, and to lifetime cast inner lifetimes.

@Ixrec
Copy link
Contributor

Ixrec commented Feb 9, 2017

I don't understand the motivation here. Today, std::mem::transmute<T, U>() has some compiler magic associated with it that ensures a compilation error if T and U are different sizes. Under this RFC, the Transmute trait would have that magic instead. But it's still magic. If we had--or were anywhere close to getting--trait bounds like where sizeof::<U>() == sizeof::<T>() and is_inhabited::<T>() and is_inhabited::<U>() then I could get behind "de-magicking" this. But if all that's being proposed is moving the magic of transmute() slightly farther away from the transmute() function...what's the benefit?

Suppose I want to make a generic function which can be called with any type that can be transmuted to a usize.

I don't think you'd ever want to do that. Many types can be transmuted to usize, but without even knowing which type you're transmuting it seems virtually guaranteed to invoke some sort of undefined behavior.

Well the transmutability check would just move into trait selection. But it would make rust kind-safe like it's supposed to be.

What does "kind-safe" mean?

@aturon aturon added the T-lang Relevant to the language team, which will review and decide on the RFC. label Feb 9, 2017
@burdges
Copy link

burdges commented Feb 9, 2017

If you want to change transmute, then you should probably be asking questions like: Can we make it safer? Can we separate transmute for types from transmute for lifetimes? etc. Not: Can I write generic bounds over it?

Also, I suspect the warnings against transmute should include better explanations about how conversions should be done. I'm thinking this issue, fix, etc to the arrayref crate. At present, the transmute section of the nomicon has a link to the unbounded lifetimes section and a note that "you can get most of the functionality of these functions using pointer casts", but says relatively little. And some examples in the docs for std::mem::transmute look risky.

@whitequark
Copy link
Member

@Ericson2314: would this help your usize-stuffing implementation of threads?

@scottmcm
Copy link
Member

Part of what makes the Transmute trait unappetizing to me is that I can't think of a scenario where implementing it in code would be possible, let alone useful. And I agree that as a generic bound it feels unhelpful, since it's essentially just HasSameSizeAs (under the mental model of uninhabited types have -∞ size).

But what if it was more restricted? (And thus not actually related to transmute.)

Today, as can--in safe code--turn -1:i32 into 4294967295:u32. But it can't turn &'a i32 into &'a u32, nor [i32; 4] into [u32; 4], neither of which is less safe. I don't think as should be expanded to do that, but imagine a new safe method that could.

Having a built-in trait for that would be nice, as it could understand alignment and recurse through appropriate-repr structs. But the library could also implement it where it works, say unsafe impl<T,U> ReinterpretableAs<Vec<U>> for Vec<T> where T:ReinterpretableAs<U> {}.

Maybe it'd be easier to discourage transmute if there was a simple alternative for the weird-but-not-actually-unsafe stuff...

@strega-nil
Copy link

strega-nil commented Feb 10, 2017

@scottmcm I've argued in the past for (unsafe) ptr_cast, which removes all pointer uses of transmute, (unsafe) lifetime_cast which removes all lifetime use of transmute, and for inherent methods on f32 and f64 to convert to/from u32/u64.

@nrc
Copy link
Member

nrc commented Feb 10, 2017

We discussed this RFC at this mornings lang team meeting. We decided while it does have some advantages, this RFC would be effectively impossible to implement and so we should not keep it open.

To summarise our feelings:

  • we agree that making the signature of transmute more explicit is a reasonable goal,
  • however, we do not think that using Transmute as a bound on generics is a good idea - we could not imagine that such a function would ever be desirable,
  • this would be a backwards-incompatible change (that could be worked around by adding a new function and deprecating transmute),
  • the only practical advantage we could imagine is that in some polymorphic contexts, the compiler could report errors earlier and/or more frequently (before monomorphisation).

However, the above is basically moot because of the implementation issues: the size of types is not known until the 'trans' phase of the compiler (in fact in the past this required using LLVM, and there is some chance we might again require input from backends like LLVM in some circumstances). It would be a major change to the compiler to move this work earlier - consider, for example, the case of enums where the size depends on the optimisations we apply to the layout of the enum. It may even be impossible to compute sizes in all cases before trans. Since we would need this information to determine if Transmute is implemented, this must be done before type checking (or during type checking, at added complexity cost).

In summary, the implementation burden here seems to vastly outweigh the usefulness of the feature, if it is possible to implement at all.

@nrc nrc closed this Feb 10, 2017
@eddyb
Copy link
Member

eddyb commented Feb 10, 2017

  • you're wrong though, we check transmutes like any other type property now
  • the new rule is either "can compute constant size_of and it matches" or "both types are effectively a pointer where the metadata comes from the same potentially unsized type parameter or projection"
  • see Compute LLVM-agnostic type layouts in rustc. rust#32939

@aturon
Copy link
Member

aturon commented Feb 10, 2017

Thanks @eddyb. The (rare) decision to close out of hand was based on assumptions about the implementation that appear to be incorrect, so I'm going to reopen for the time being.

Personally, while I'm open to cleaning up the definition of transmute in this way, I don't see much practical benefit in doing so -- as @nrc said, allowing transmute to be used in a generic context seems actively undesirable. I don't strongly object to the change, but I think it's very low priority at best.

@aturon aturon reopened this Feb 10, 2017
@eddyb
Copy link
Member

eddyb commented Feb 10, 2017

For the record, I'm not too comfortable with this, and even if I care about expressing the requirements of transmute within libcore instead of the compiler, it's probably not that great for error messages.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 10, 2017

My opinion is that we ought to close for now -- not because of technical infeasibility, but because of low priority and unclear motivation. Also I'd prefer to make more progress on the refinements to trait system impl before adding more complexity to it; this will inevitably involve some special cases (until such time as we support something like where sizeof(T) == sizeof(U), I suppose).

UPDATE: (I also think we should just avoid closing out of hand as a rule.)

@nikomatsakis
Copy link
Contributor

@rfcbot fcp close

I propose we close this RFC for the reasons given here. In particular, while adding a trait seems like the correct way to permit generic code that uses transmute, we are not sure if we would want to do that, and fairly sure we won't want to do that now. So this is not a "close forever" sort of thing, but a "close for now" sort of thing.

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 10, 2017

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

No concerns currently listed.

Once these reviewers reach consensus, 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.

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 14, 2017

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

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Feb 14, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented Feb 24, 2017

The final comment period is now complete.

@aturon
Copy link
Member

aturon commented Mar 3, 2017

The FCP has completed, with no additional comments. I'm going to close the RFC, with the stated rationale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.