Add {To,From}Primitive conversion trait, add #[deriving(FromPrimitive)] for enums

[erickt]

This PR solves one of the pain points with c-style enums. Simplifies writing a fn to convert from an int/uint to an enum. It does this through a #[deriving(FromPrimitive)] syntax extension.

Before this is committed though, we need to discuss if ToPrimitive/FromPrimitive has the right design (cc #4819). I've changed all the .to_int() and from_int() style functions to return Option<int> so we can handle partial functions. For this PR though only enums and extra::num::bigint::* take advantage of returning None for unrepresentable values. In the long run it'd be better if i64.to_i8() returned None if the value was too large, but I'll save this for a future PR.

Closes #3868.

[huonw]

Is this correct on 32-bit platforms? (I guess it's not, and so, should this truncation be the default behaviour?)

[erickt]

I'll change the unimplemented version to be .to_i64(). I was guessing the people would reach first towards implementing .to_int() and .to_uint(), but you're right, it'd be better to avoid truncation by default.

[huonw]

As above for 32-bit platforms.

[huonw]

Could this be span_err rather than span_fatal? (We can continue from here to possibly pick up more errors later in expansion, so no particular need to stop the entire build.)

[huonw]

I think this will ICE on #[deriving(FromPrimitive)] struct Foo { .. }? Could there be an arm with a span_err for the StaticStruct case? (You can use the expr_unreachable to make a "dummy" expression to satisfy the return value.)

[huonw]

Looks pretty nifty, will certainly reduce the number of transmutes people are driven to. I think this would benefit from some negative tests also: e.g. I think the following should all fail with nice error messages.

#[deriving(FromPrimitive)]
struct Foo { x: int }

#[deriving(FromPrimitive)]
enum Bar { A(int), B(uint) }

#[deriving(FromPrimitive)]
enum Baz { S { x: int } }

so we should test to stop them regressing accidentally.

[huonw]

I think this still makes sense, and probably doesn't need special treatment at all, since the loop below will just not run?

It just returns None on every possible conversion: i.e. having 0 variants mean the numbers in the (empty) set [0, 0) are valid to convert from, and anything else (i.e. everything) gives None.

[alexcrichton]

It seems that the behavior of this is that to_int would return None if the value is out of bounds of an int's range. However when calling to_i8, silent truncation happens when something is between the ranges of i8 and int.

I'm ok with this behavior, but I think that it should be well documented. I'm also more ok with this when the methods to implement are for i64 and u64 because that's just saying that you need to be within the threshold of machine integers to return Some, which isn't so bad.

[huonw]

I'd be happy to r+ this, but it seems like a large enough change to be worth getting input from someone else.

[tikue]

typo: s/an/a

[erickt]

thanks! I've got it fixed in my upcoming rebase of this PR.

[erickt]

This has been rebased to HEAD and is ready for review again.

[alexcrichton]

Why are each of the error messages doubled in this test? I would imagine that only one should be printed each time.

[erickt]

Downside to the #[deriving(FromPrimitive)] implementation. syntax::ext::deriving::generic::* works a method at a time. Because FromPrimitive requires at least two methods to be implemented, both generating both methods ends up reporting an error. This wouldn't be that difficult to change. We would just have to modify TraitDef.expand_struct_def and TraitDef.expand_enum_def to return either an Option<@ast::item> or Result<@ast::item,~str> and use that to signal that it's not worth implementing the rest of the methods.

[alexcrichton]

I'm a little uneasy with this change in general. I think that the deriving(FromPrimitive) for enums is pretty awesome, but otherwise it doesn't quite sit well with me. It seems like a bit of a regression going from writing BigInt::new(int_value) to FromPrimitive::from_int(int_value), and there's a lot of truncation going on in both the from and the to case which isn't well documented.

That being said, this seems like it might be a brainchild of discussion in #4819, which I haven't been following that closely. Is there an intent on using {From,To}Primitive in many use-cases other than the big numbers and in deriving-situations? In general it's a bit of a pain to explicitly call static methods on traits (it's not entirely clear that the trait is implemented for the type), so I thought we were attempting to avoid them in general.

[huonw]

@alexcrichton I'd say that if we're going to be avoid static methods in traits because they're hard to use, we need to fix that, rather than just avoiding them entirely even when they are the appropriate thing to use.

(Also, I think that *Primitive as implemented here are an improvement over NumCast, e.g. they are checked for overflow.)

[erickt]

@alexcrichton: This PR only partially implements my plans, I wanted to submit it early before I invested too much into this work in case the community decided I was going in a wrong direction. There's a couple things left to do:

• Implement #[deriving(FromPrimitive)] for struct Foo(int) tuple structs.
• Implement #[deriving(ToPrimitive)] for c style enums and struct Foo(int) tuple structs.
• Fix the BigInt trunction, which should be pretty simple.
• Add ToBigInt and ToBigUint traits and add impls for the primitive numeric types.
• Actually add checks for under/overflow to .to_int(), from_uint(uint_value) and etc.

Finally, I agree it is a pain to write FromPrimitive::from_int(int_value), that's why I added some helper functions like num::from_int(int_value) that just calls the trait method.

[cmr]

This needs a rebase.

[huonw]

r=me if this gets consensus.

[erickt]

This latest version adds:

• bounds checking on ints and uints, but not yet for floats.
• Fixes the truncation issue with BigInts
• Adds ToBigInt and ToBigUint

[brson]

I r+'ed this. It's pretty big and I haven't followed it closely, but the response appears to be mostly favorable. Like all things it can be tweaked later.

[huonw]

The run-pass tests need pub fn main for the windows test runner.

[erickt]

r+

[bors]

saw approval from erickt
at erickt@0e8ad4d

[chris-morgan]

I venture to suggest this is not what was intended. self.to_i64(), perhaps? (Either that or omit the default implementation.)

[erickt]

Good catch! I'll fix it.