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

Stabilize transmute in constants and statics but not const fn #64011

Closed
wants to merge 1 commit into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Aug 30, 2019

r? @Centril

We already have unions inside constants and it's strictly better to use transmute for transmutation purposes since you also get a size check.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2019
@jonas-schievink jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 30, 2019
@jonas-schievink jonas-schievink added this to the 1.39 milestone Aug 30, 2019
@est31
Copy link
Member

est31 commented Aug 30, 2019

Why not also stabilize them in const fn?

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 30, 2019

If we permit transmute in const fn, we need to talk about "const soundness" first. Right now there is no way one can write a const fn whose behaviour differs between runtime and compile-time. Once you can do really unsafe things in const fn, we need to make sure everyone is aware that if you make a const unsound const fn, then you can cause UB, even if the thing isn't UB at runtime.

@bors
Copy link
Contributor

bors commented Aug 31, 2019

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

@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Aug 31, 2019
@Centril
Copy link
Contributor

Centril commented Aug 31, 2019

Hmm... I'm skeptical about diverging const items and const fn more than we have to. The size check may be nice but I'm personally not a big fan of it since it's a type system complication right now and if you actually cause UB with a union then CTFE will throw a fit and error...

@RalfJung
Copy link
Member

RalfJung commented Sep 1, 2019

if you actually cause UB with a union then CTFE will throw a fit and error...

It might not be UB though, just reading the wrong data, if e.g. a union U { f1: u32, f2: u16 } starts to right-align the fields.

Given that const already have this capability, I think it only makes sense to also expose it ergonomically. However, union only works on Copy types; we should make sure transmute working on all types does not cause new problems.

I'm skeptical about diverging const items and const fn more than we have to.

That's fair. OTOH, I feel like const items provide a nice testing ground for us to stabilize CTFE features in a very controlled way -- since we do a validity check on the result, nothing "way too weird" should be able to happen.

@RalfJung
Copy link
Member

RalfJung commented Sep 1, 2019

I also think stabilization should be blocked on #61495 being resolved. We should make sure our intrinsic const checks actually check what we want them to check, before stabilizing direct access to an intrinsic in CTFE.

@JohnCSimon
Copy link
Member

Ping from triage
@oli-obk Can you please fix the merge conflicts?
Thanks.

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 10, 2019

I'll close this until #61495 is resolved.

@oli-obk oli-obk closed this Sep 10, 2019
@Centril Centril removed this from the 1.39 milestone Sep 26, 2019
@jplatte
Copy link
Contributor

jplatte commented Feb 19, 2020

@oli-obk #61495 is resolved now. Will you re-open this PR / create a new one?

@oli-obk oli-obk reopened this Feb 26, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 26, 2020

Uh, I just realized this scheme isn't possible anymore. We either stabilize const_transmute everywhere or nowhere.

@oli-obk oli-obk closed this Feb 26, 2020
@RalfJung
Copy link
Member

@oli-obk could you explain why?

@RalfJung
Copy link
Member

RalfJung commented Jun 2, 2020

@oli-obk should we give this another try? Not much progress has been made on the unconst side of things, but allowing transmute where we already allow union could help get rid of a lot of nasty union-type-punning code (probably half of which is wrong because it forgets to add repr(C) to the union).

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 2, 2020

Before we do that we should probably get rid of the extra min_const_fn logic, and instead integrating its checks into the new const checking infrastructure.

Oh well, we could just do it in min_const_fn :/ yea no need to block it. I'll rebase this and fix it up

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 2, 2020

Ah, trying that I now remember what the problem is:

we declare intrinsics to be stable by adding the rustc_const_stable attribute to them... There's no concept for undoing that just for const fn. I can add a specific check just for const fn, but if we plan on doing more of these, we'll need a new attribute.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 2, 2020

let's continue any further discussion in #53605

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 10, 2020
Stabilize `transmute` in constants and statics but not const fn

cc rust-lang#53605 (leaving issue open so we can add `transmute` to `const fn` later)

Previous attempt: rust-lang#64011

r? @RalfJung

cc @rust-lang/wg-const-eval
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 11, 2020
Stabilize `transmute` in constants and statics but not const fn

cc rust-lang#53605 (leaving issue open so we can add `transmute` to `const fn` later)

Previous attempt: rust-lang#64011

r? @RalfJung

cc @rust-lang/wg-const-eval
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 11, 2020
Stabilize `transmute` in constants and statics but not const fn

cc rust-lang#53605 (leaving issue open so we can add `transmute` to `const fn` later)

Previous attempt: rust-lang#64011

r? @RalfJung

cc @rust-lang/wg-const-eval
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 11, 2020
Stabilize `transmute` in constants and statics but not const fn

cc rust-lang#53605 (leaving issue open so we can add `transmute` to `const fn` later)

Previous attempt: rust-lang#64011

r? @RalfJung

cc @rust-lang/wg-const-eval
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 11, 2020
Stabilize `transmute` in constants and statics but not const fn

cc rust-lang#53605 (leaving issue open so we can add `transmute` to `const fn` later)

Previous attempt: rust-lang#64011

r? @RalfJung

cc @rust-lang/wg-const-eval
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 14, 2020
Stabilize `transmute` in constants and statics but not const fn

cc rust-lang#53605 (leaving issue open so we can add `transmute` to `const fn` later)

Previous attempt: rust-lang#64011

r? @RalfJung

cc @rust-lang/wg-const-eval
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants