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

"as uint" works on bare constructors of non-C-like enums #18154

Closed
kmcallister opened this issue Oct 19, 2014 · 18 comments
Closed

"as uint" works on bare constructors of non-C-like enums #18154

kmcallister opened this issue Oct 19, 2014 · 18 comments

Comments

@kmcallister
Copy link
Contributor

@kmcallister kmcallister commented Oct 19, 2014

rustc 0.13.0-dev (222ae8b 2014-10-18 00:47:22 +0000)

enum Tag {
    Dynamic,
    Inline(u8),
    Static,
}

#[test]
fn f() {
    assert_eq!(Inline as uint, 1);
}

compiles and produces

task 'f' failed at 'assertion failed: `(left == right) && (right == left)` (left: `140041300628448`, right: `1`)', foo.rs:9

I think it's casting the constructor function itself as a function pointer? But this is super confusing behavior (caused a segfault in unsafe code) when I just refactored an enum to be not C-like and I want to get rid of remaining occurrences of as uint.

Also it doesn't warn for as u8, even though it's too small for a function pointer.

@kmcallister
Copy link
Contributor Author

@kmcallister kmcallister commented Nov 23, 2014

Nominating.

@kmcallister
Copy link
Contributor Author

@kmcallister kmcallister commented Nov 23, 2014

Casting the nullary constructors is already forbidden.

@kmcallister
Copy link
Contributor Author

@kmcallister kmcallister commented Nov 24, 2014

Hm, I now feel like this should be a lint, because it's a kind of heuristic syntactic thing rather than a typechecking rule. We would reasonably expect this code to work:

let f: fn(u8) -> Tag = Inline;
// ...
let n = f as uint;

Only when a literal enum constructor appears in the cast can we reasonably guess that the behavior is unintended.

kmcallister added a commit to kmcallister/rust that referenced this issue Nov 24, 2014
kmcallister added a commit to kmcallister/rust that referenced this issue Nov 26, 2014
kmcallister added a commit to kmcallister/rust that referenced this issue Nov 26, 2014
@brson brson added P-medium and removed I-nominated labels Dec 11, 2014
@brson
Copy link
Contributor

@brson brson commented Dec 11, 2014

P-high, not 1.0. Ugly wart, and we'd love someone to fix.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Dec 11, 2014

One possible solution is restricting casts from function pointers to uint. We could just say that is not legal but it is legal to cast to a function pointer to an unsafe pointer, and then (continue to) permit unsafe pointers to cast to uints.

One of the main reasons we allow these wacky casts with as, for reference, is constant expressions, where transmute is not possible.

@bombless
Copy link
Contributor

@bombless bombless commented Apr 29, 2015

One possible solution is restricting casts from function pointers to uint. We could just say that is not legal but it is legal to cast to a function pointer to an unsafe pointer, and then (continue to) permit unsafe pointers to cast to uints.

+1 for this, but I guess it's too late for 1.0.x

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jun 29, 2015

The segfault is gone BTW

@arielb1 arielb1 removed the I-wrong label Jun 29, 2015
@steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Oct 27, 2015

Updated code:

enum Tag {
    Dynamic,
    Inline(u8),
    Static,
}

#[test]
fn f() {
    assert_eq!(Tag::Inline as usize, 1);
}

fn main() {}

Still produces the same error.

@brson
Copy link
Contributor

@brson brson commented Jul 14, 2016

Triage: probably impossible to fix this in a satisfying way now, but there may be mitigations. Can we decide the fate here? If we make it a lint, can somebody mentor?

@brson brson added the P-low label Jul 14, 2016
@steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Jul 14, 2016

If we make it a lint, can somebody mentor?

New lints need RFCs nowadays.

bors added a commit that referenced this issue Jul 14, 2016
test: Remove NOTE assertions from trace_macros-gate

If no NOTE assertions are present I believe they aren't asserted at all, and it
looks like the number of NOTEs differs on distcheck vs `make check`, so let's
just remove them all.

Closes #18154
@bors bors closed this in b2d1f7e Jul 14, 2016
@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jul 14, 2016

Closed erroneously! (sorry)

@alexcrichton alexcrichton reopened this Jul 14, 2016
@arielb1 arielb1 added the A-lint label Jul 21, 2016
@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Jul 21, 2016

triage: P-low

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Jul 21, 2016

triage: P-low

@rust-highfive rust-highfive added P-low and removed P-low labels Jul 21, 2016
@nrc
Copy link
Member

@nrc nrc commented Jul 21, 2016

cc @Manishearth in case he wants to add it to Clippy

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 21, 2016

I do think a lint is the only fix here.

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Jul 21, 2016

(consensus of compiler team is that this should be some form of lint)

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jul 22, 2016

closing.

@arielb1 arielb1 closed this Jul 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.