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

Decide on semantics for enum -> integer casts on non-Copy enums. #35941

Closed
eddyb opened this issue Aug 23, 2016 · 7 comments
Closed

Decide on semantics for enum -> integer casts on non-Copy enums. #35941

eddyb opened this issue Aug 23, 2016 · 7 comments
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Aug 23, 2016

Casting an enum which does not derive Copy to an integer moves atm:

enum E {
    A = 0,
    B = 1,
    C = 2
}

fn main() {
    let e = E::C;
    assert_eq!(e as u32, 2);
    assert_eq!(e as u32, 2); // error: use of moved value: `e`
}

This matches the behavior of other coercions/casts, specifically unsizing ones (Box<T> -> Box<Trait>).

However, one difference arises between old trans and MIR trans, wrt drops:

enum E {
    A = 0,
    B = 1,
    C = 2
}

impl Drop for E {
    fn drop(&mut self) {
        println!("drop");
    }
}

fn main() {
    let e = E::C;
    assert_eq!(e as u32, 2);
}

On stable, old trans will run the destructor exactly once. After #35764, MIR trans doesn't run it at all.
The reason is that the move that borrowck knows about causes the drops in MIR to be statically elided, whereas old trans forgot(?) to drop-fill e, resulting in the variable being dropped at end of scope.

Should the drop execute or not? IMO it's good to have uniformity between all moving operations.

@eddyb eddyb added I-nominated I-needs-decision Issue: In need of a decision. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels Aug 23, 2016
@Aatch
Copy link
Contributor

Aatch commented Aug 24, 2016

The safe option is, of course, to execute the drop. However I think that not running makes more sense to me. For one, it would match the semantics of transmute, which is the only other way to get this kind of situation (converting from a type that impls to one that doesn't).

Another option, though it would be more breaking than either already mentioned, would be to disallow enum -> int casts where the enum implements Drop.

@arielb1
Copy link
Contributor

arielb1 commented Aug 25, 2016

My original intent when I wrote the conversion code was to execute the drop. That was before mem::forget was made safe, so it was the only option.

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 25, 2016
@nikomatsakis
Copy link
Contributor

We discussed this in the @rust-lang/lang meeting. We like the idea of making it an error to coerce an enum to an integer if that enum type supports Drop -- presuming it doesn't have impact in practice (which seems likely).

@nikomatsakis nikomatsakis removed I-needs-decision Issue: In need of a decision. I-nominated labels Aug 25, 2016
@nikomatsakis
Copy link
Contributor

triage: P-medium

@rust-highfive rust-highfive added the P-medium Medium priority label Aug 25, 2016
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@oli-obk
Copy link
Contributor

oli-obk commented Jan 25, 2020

Some mentoring instructions of how I think this could be implemented:

  1. Turn the match arm body
    (Int(CEnum), Int(_)) => Ok(CastKind::EnumCast),
    into a function call similar to all the check_X_Y_cast functions.
  2. Add check whether the self.expr_ty has a destructor (you can use the has_dtor meethod for this, but that method takes a TyCtxt so you need to pass an fcx to your new function just like some of the other check_X_Y_cast functions do.
  3. If the type has a destructor, create a new error variant in
    enum CastError {
    and report that. This will also require you to actually emit an error in
    fn report_cast_error(&self, fcx: &FnCtxt<'a, 'tcx>, e: CastError) {
    (the compiler will tell you to handle the new error variant). Create a structured error similar to how https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/cast.rs#L358-L366 works.

@oli-obk oli-obk added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Jan 25, 2020
@oddg
Copy link
Contributor

oddg commented May 10, 2020

Thanks @oli-obk for the detailed approach. I'll take a stab at it if no one is working on it already.

Manishearth added a commit to Manishearth/rust that referenced this issue Jun 16, 2020
…ing-drop, r=matthewjasper,nikomatsakis

Report error when casting an C-like enum implementing Drop

Following approach described in rust-lang#35941
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 16, 2020
…ing-drop, r=matthewjasper,nikomatsakis

Report error when casting an C-like enum implementing Drop

Following approach described in rust-lang#35941
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 17, 2020
…ing-drop, r=matthewjasper,nikomatsakis

Report error when casting an C-like enum implementing Drop

Following approach described in rust-lang#35941
tmandry added a commit to tmandry/rust that referenced this issue Jun 17, 2020
…ing-drop, r=matthewjasper,nikomatsakis

Report error when casting an C-like enum implementing Drop

Following approach described in rust-lang#35941
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 19, 2020
…ing-drop, r=matthewjasper,nikomatsakis

Report error when casting an C-like enum implementing Drop

Following approach described in rust-lang#35941
@JohnTitor
Copy link
Member

I'm going to close this in favor of #72331 which implemented cenum_impl_drop_cast and #73333 which is tracking it. Feel free to re-open if I'm missing something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants