Skip to content

Make transmuting inhabited to uninhabited types an error #36489

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

Closed
wants to merge 1 commit into from

Conversation

canndrew
Copy link
Contributor

This changes the behaviour of the transmute intrinsic so that, for any
uninhabited type Void and any inhabited type NonVoid:

  • transmute::<Void, T> is okay
  • transmute::<NonVoid, Void> is an error

This changes the behaviour of the transmute intrinsic so that, for any
uninhabited type `Void` and any inhabited type `NonVoid`:

 * transmute::<Void, T> is okay
 * transmute::<NonVoid, Void> is an error
Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

transmute::<Void, T> is okay

This can’t be possibly called, can it? Good thing you cannot get function pointer of an intrinsic, as making one up for this particular function would be kinda tricky :)

Overall this patch LGTM.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 15, 2016

the issue code should probably be used as a regression test.

@durka
Copy link
Contributor

durka commented Sep 15, 2016

@nagisa you can still construct a void by using transmute_copy or pointer casts (both seem to correctly invoke UB), so it could still be called.

@arielb1
Copy link
Contributor

arielb1 commented Sep 16, 2016

I think we should rather be changing the code in trans::block to make transmuting to a ! result in an unreachable, rather than hackily adding errors. At the ty::layout level, I would imagine we want to propagate this further.

Why are you making transmute::<Void, NonVoid> legal? That is an odd exception to the same-size rule.

@canndrew
Copy link
Contributor Author

canndrew commented Sep 16, 2016

At the ty::layout level, I would imagine we want to propagate this further.

The hackyness begins at whatever level we start pretending that uninhabited types are ZSTs, so I agree that the lower we can push that the better. What exactly do you have in mind though?

Why are you making transmute::<Void, NonVoid> legal?

Why not? It's always safe to transmute from an uninhabited type to anything else (every element of Void has the same size as T for any T). Plus, transmute::<A, B> isn't a symmetrical operation w.r.t. the types A and B so it's not necessarily more elegant to make it symmetrical w.r.t. their sizes inhabitedness. I wouldn't feel strongly about changing it though.

@alexcrichton
Copy link
Member

ping, what's the status of this?

@canndrew
Copy link
Contributor Author

canndrew commented Nov 2, 2016

@alexcrichton It prolly needs the lang team to have a look at it and see if they agree with it. Other than that though it should be good to merge.

@steveklabnik
Copy link
Member

/cc @rust-lang/lang

Also, there's no reviewer for this? I'm not sure who should be assigned.

@aturon aturon added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Nov 7, 2016
@aturon
Copy link
Member

aturon commented Nov 7, 2016

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Nov 7, 2016

The patch is alright, I suppose, but I don't understand the motivation between this change.
AFAICT the main effect is making transmute::<ZST, Void> an error (which requires at least a crater run).

@canndrew
Copy link
Contributor Author

canndrew commented Nov 9, 2016

AFAICT the main effect is making transmute::<ZST, Void> an error

Correct. Because it's an insane thing to do and it has the same problems as uninitialized::<Void>. That said, I'm starting to feel we should settle the questions around uninitialized::<Void> first before merging anything like this :/

@eddyb
Copy link
Member

eddyb commented Nov 9, 2016

I definitely want a bunch of warnings in this area, not so sure about outright errors.
Deny-by-default lints are quite good at this sort of bad-unsafe-code sort of thing IMO.

@alexcrichton
Copy link
Member

If we're thinking of holding off on this until we've settled more questions about uninitialized types, perhaps we should close in the meantime? (to help clear out the queue)

@canndrew
Copy link
Contributor Author

@alexcrichton Righto. Closed for now.

@canndrew canndrew closed this Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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