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

erickt · 2013-09-17T04:18:51Z

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 · 2013-09-17T08:24:17Z

src/libstd/num/num.rs

+    #[inline]
+    fn to_i64(&self) -> Option<i64> {
+        // XXX: Check for range.
+        self.to_int().and_then(|x| Some(x as i64))


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

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 · 2013-09-17T08:35:30Z

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 · 2013-09-17T08:37:54Z

src/libsyntax/ext/deriving/primitive.rs

+    return match *substr.fields {
+        StaticEnum(enum_def, _) => {
+            if enum_def.variants.is_empty() {
+                cx.span_fatal(span, "`FromPrimitive` cannot be derived for enums with no variants");


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.

huonw · 2013-09-19T09:17:49Z

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

tikue · 2013-09-19T23:13:31Z

src/libstd/num/num.rs

+impl_to_primitive!(f64)
+impl_to_primitive!(float)
+
+/// A generic trait for converting an number to a value.


typo: s/an/a

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

erickt · 2013-09-23T04:23:16Z

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

alexcrichton · 2013-09-23T16:24:30Z

src/test/compile-fail/deriving-primitive.rs

+#[deriving(FromPrimitive)]
+struct A { x: int }
+//~^^ ERROR `FromPrimitive` cannot be derived for structs
+//~^^^ ERROR `FromPrimitive` cannot be derived for structs


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

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 · 2013-09-23T16:41:09Z

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 · 2013-09-24T14:09:30Z

@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 · 2013-09-24T17:30:25Z

@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.

emberian · 2013-09-25T20:43:53Z

This needs a rebase.

huonw · 2013-09-27T05:27:01Z

r=me if this gets consensus.

erickt · 2013-09-27T05:51:41Z

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 · 2013-09-28T04:58:59Z

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 · 2013-09-29T05:32:07Z

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

Right now this only works for c-style enums.

…rsion One downside with this current implementation is that since BigInt's default is now 64 bit, we can convert larger BigInt's to a primitive, however the current implementation on 32 bit architectures does not take advantage of this fact.

This allows the default methods to be properly range checked.

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.

chris-morgan · 2013-10-07T02:42:40Z

src/libstd/num/num.rs

+    /// Converts the value of `self` to an `u64`.
+    #[inline]
+    fn to_u64(&self) -> Option<u64> {
+        self.to_u64().and_then(|x| x.to_u64())


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

Good catch! I'll fix it.

huonw reviewed Sep 17, 2013
View reviewed changes

tikue reviewed Sep 19, 2013
View reviewed changes

alexcrichton reviewed Sep 23, 2013
View reviewed changes

erickt added 5 commits October 2, 2013 07:55

std: Replace num::IntConvertible with {To,From}Primitive

d9d1dfc

syntax: Add #[deriving(FromPrimitive)] syntax extension

0feaccf

Right now this only works for c-style enums.

syntax: swap from .span_fatal to .span_err in #[deriving(FromPrimitive)]

8f8cc06

test: add compile-fail test for #[deriving(FromPrimitive)]

5a64e1a

extra: Add ToBigInt and ToBigUint traits

01be9e9

erickt added 7 commits October 5, 2013 13:59

std: check bounds for ints/uints in {To,From}Primitive

cb24019

extra: Don't truncate {u64,i64} when converting to BigInts

e323749

std: fix some warnings

da145b2

std: ToPrimitive's default impls should use .to_*()

50fde8c

This allows the default methods to be properly range checked.

std: add Primitive.is_signed

41f9deb

test: fix the tests for windows

efae73d

extra: fix BigInt on 32bit machines

0e8ad4d

bors closed this Oct 5, 2013

bors merged commit 0e8ad4d into rust-lang:master Oct 5, 2013

eerden mentioned this pull request Oct 7, 2013

Fix missing IntConvertible. chris-morgan/rust-http#24

Closed

chris-morgan reviewed Oct 7, 2013
View reviewed changes

lifthrasiir mentioned this pull request Apr 16, 2014

ToBigInt and ToBigUint should not return an error #13555

Closed

Kerollmops mentioned this pull request Jan 27, 2018

RFC: Make the as keyword consider Into Trait implementations rust-lang/rfcs#2308

Closed

cuviper mentioned this pull request Aug 24, 2020

Infinite not being used rust-num/num-bigint#161

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

erickt commented Sep 17, 2013

huonw Sep 17, 2013

erickt Sep 17, 2013

huonw commented Sep 17, 2013

huonw Sep 17, 2013

huonw commented Sep 19, 2013

tikue Sep 19, 2013

erickt Sep 22, 2013

erickt commented Sep 23, 2013

alexcrichton Sep 23, 2013

erickt Sep 24, 2013

alexcrichton commented Sep 23, 2013

huonw commented Sep 24, 2013

erickt commented Sep 24, 2013

emberian commented Sep 25, 2013

huonw commented Sep 27, 2013

erickt commented Sep 27, 2013

brson commented Sep 28, 2013

huonw commented Sep 29, 2013

chris-morgan Oct 7, 2013

erickt Oct 7, 2013

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

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

Conversation

erickt commented Sep 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw commented Sep 17, 2013

Choose a reason for hiding this comment

huonw commented Sep 19, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erickt commented Sep 23, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Sep 23, 2013

huonw commented Sep 24, 2013

erickt commented Sep 24, 2013

emberian commented Sep 25, 2013

huonw commented Sep 27, 2013

erickt commented Sep 27, 2013

brson commented Sep 28, 2013

huonw commented Sep 29, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment