Bug 8143 – Safe std.conv.to enum conversion

import std.conv: to; enum Foo : int { A = 10, B = 20 } void main() { int x = 10; Foo f1 = to!Foo(x); assert(f1 == Foo.A); x = 5; Foo f2 = to!Foo(x); // needs to throw an exception } DMD 2.060alpha gives: ...\dmd2\src\phobos\std\conv.d(267): Error: template std.conv.toImpl does not match any function template declaration ...\dmd2\src\phobos\std\conv.d(298): Error: template std.conv.toImpl cannot deduce template function from argument types !(Foo)(int) ...\dmd2\src\phobos\std\conv.d(267): Error: template instance toImpl!(Foo) errors instantiating template temp.d(5): Error: template instance std.conv.to!(Foo).to!(int) error instantiating This is handy to *safely* convert run-time values to enum items. Using a cast(Foo) is useful in other cases, because cast(Foo) doesn't raise a run-time exceptions. The same difference is visible in this program: import std.conv: to; void main() { int x = -10; uint y1 = cast(uint)x; // no errors here uint y2 = to!uint(x); // throws std.conv.ConvOverflowException }

One problem with this is: enum EF : float { C = 4.9 } float f = 4.9; EF en2 = to!EF(f); This will fail internally if conv.to compares members via "==", because of floating point comparison semantics. So the question is, is this going to be a problem? If yes, should we use approxEqual for floating point comparisons? Or maybe we should simply ban using std.conv on enums that have a floating point base type?

(In reply to comment #1) > This will fail internally if conv.to compares members via "==", because of > floating point comparison semantics. > > So the question is, is this going to be a problem? If yes, should we use > approxEqual for floating point comparisons? By far the main purpose of enums is with integral values (ints, uint, chars, etc), to be used to enumerate something or as bitfields. Using float/double/real enums is supported in D, but it's not common. Using approxEqual is suboptimal, using std.math.feqrel is better. but all approximate floating point comparisons have their quirks and limits. Backing-in one solution is not a good idea. > Or maybe we should simply ban using > std.conv on enums that have a floating point base type? What about user-defined floating point types, or a double wrapped in a struct with an alias this? I think refusing conv on built-in floating point types is an acceptable solution to avoid most troubles. Other cases like wrapped doubles are left at the care of the programmer.

(In reply to comment #2) > (In reply to comment #1) > > This will fail internally if conv.to compares members via "==", because of > > floating point comparison semantics. > > > > So the question is, is this going to be a problem? If yes, should we use > > approxEqual for floating point comparisons? > By far the main purpose of enums is with integral values (ints, uint, chars, > etc), to be used to enumerate something or as bitfields. Using > float/double/real enums is supported in D, but it's not common. > Using approxEqual is suboptimal, using std.math.feqrel is better. but all > approximate floating point comparisons have their quirks and limits. Backing-in > one solution is not a good idea. > > Or maybe we should simply ban using > > std.conv on enums that have a floating point base type? > What about user-defined floating point types, or a double wrapped in a struct > with an alias this? > I think refusing conv on built-in floating point types is an acceptable > solution to avoid most troubles. Other cases like wrapped doubles are left at > the care of the programmer. I'd say there is nothing wrong with using floats as enums. It's rare because it's new (C++ only supported integral up to now). In C++, I've seen integral based enums used to index arrays which contained the actual payload (floats, strigns, others). Now we don't have to do this anymore. The *real* issue (IMO) is only when converting *back* from float to enum, which (IMO again), is plain too dangerous to realisticly assume we can support. I'd rather have float-to-enum always fail, personally.

Or alternatively require an additional alias parameter for the comparison function in case of floats? E.g. enum EF : float { C = 4.9 } float f = 4.9; static bool compFunc(float lhs, float rhs) { ... } EF en2 = to!(EF, compFunc)(f); Since conv.to will already do the work necessary to 1) find the matching member, 2) ensure there's only 1 matching member, I think customization like this might be friendlier than just rejecting conversion.

Ok here's a first implementation, let me know if it can be improved before a pull request is made (the docs will be improved too): External: http://dpaste.dzfl.pl/ee99ce99 And copied here: import std.traits; import std.conv : ConvException, assertThrown; import std.string; import std.math; import std.stdio; /** Convert a value that is implicitly convertible to the enum base type into an Enum value. If the value does not match any enum member values, or if it matches more than one member value throw a ConvException. */ T toImpl(T, S)(S value) if (is(T == enum) && is(S : OriginalType!T) && !isFloatingPoint!(OriginalType!T)) { T result; size_t matches; foreach (Member; EnumMembers!T) { if (Member == value) { result = Member; if (++matches > 1) throw new ConvException(format("Value (%s) matches more than one member value of enum '%s'", value, fullyQualifiedName!T)); } } if (!matches) throw new ConvException(format("Value (%s) does not match any member value of enum '%s'", value, fullyQualifiedName!T)); return result; } /** Ditto: Specialization for Enums that have a floating point base type. @equal must be a function which takes the enum base type as its first parameter, the type of @value as its second parameter, and return true if the two compare equal. */ T toImpl(T, alias equal, S)(S value) if (is(T == enum) && is(S : OriginalType!T) && isFloatingPoint!(OriginalType!T)) { T result; size_t matches; foreach (Member; EnumMembers!T) { if (equal(Member, value)) { result = Member; if (++matches > 1) throw new ConvException(format("Value (%s) matches more than one member value of enum '%s'", value, fullyQualifiedName!T)); } } if (!matches) throw new ConvException(format("Value (%s) does not match any member value of enum '%s'", value, fullyQualifiedName!T)); return result; } alias toImpl to; void test() { enum En : int { A = 10, B = 20, C = 30, D = 20 } En en1 = to!En(10); assert(en1 == En.A); assertThrown!ConvException(to!En(5)); // matches more than one assertThrown!ConvException(to!En(20)); static bool equal(float a, float b) { return feqrel(a, b) >= 24; } enum EF : float { C = 4.9 } float f = 4.9; EF enf = to!(EF, equal)(f); enum EF2 : float { A = 4.9, B = 1.0, C = 4.9 } float f2 = 4.9; // matches more than one assertThrown!ConvException(to!(EF2, equal)(f2)); } void main() { test(); }

(In reply to comment #4) > I think customization like > this might be friendlier than just rejecting conversion. It also makes the implementation and usage a bit more complex. Ask to other people (like Andrei) to see what they think. (Here my preference goes to a simple solution).

(In reply to comment #5) > Ok here's a first implementation, let me know if it can be improved before a > pull request is made (the docs will be improved too): One more test case: En[][] m1 = to!(En[][])([[10, 30], [30, 10]]); Have you compiled your code with "-property -w"? I am seeing some errors: ...\dmd2\src\phobos\std\traits.d(221): Error: not a property test ...\dmd2\src\phobos\std\traits.d(225): Error: not a property test ...\dmd2\src\phobos\std\traits.d(229): Error: not a property test ...\dmd2\src\phobos\std\traits.d(234): Error: not a property test ...\dmd2\src\phobos\std\traits.d(231): Error: template instance std.traits.fullyQualifiedName!(test) error instantiating test.d(27): instantiated from here: fullyQualifiedName!(En) test.d(76): instantiated from here: toImpl!(En,int) test.d(27): Error: template instance std.traits.fullyQualifiedName!(En) error instantiating test.d(76): instantiated from here: toImpl!(En,int) test.d(26): Error: constructor std.conv.ConvException.this (string s, string fn = __FILE__, uint ln = cast(uint)__LINE__) is not callable using argument types (_error_) test.d(26): Error: constructor std.conv.ConvException.this (string s, string fn = __FILE__, uint ln = cast(uint)__LINE__) is not callable using argument types (_error_) test.d(26): Error: constructor std.conv.ConvException.this (string s, string fn = __FILE__, uint ln = cast(uint)__LINE__) is not callable using argument types (_error_) test.d(26): Error: constructor std.conv.ConvException.this (string s, string fn = __FILE__, uint ln = cast(uint)__LINE__) is not callable using argument types (_error_) test.d(32): Error: constructor std.conv.ConvException.this (string s, string fn = __FILE__, uint ln = cast(uint)__LINE__) is not callable using argument types (_error_) test.d(76): Error: template instance test.toImpl!(En,int) error instantiating

(In reply to comment #6) > (In reply to comment #4) > > > I think customization like > > this might be friendlier than just rejecting conversion. > > It also makes the implementation and usage a bit more complex. Ask to other > people (like Andrei) to see what they think. (Here my preference goes to a > simple solution). It's only complex for the case of floating point conversion. We could by default set the alias to be a safe floating-point comparison function by default so the user doesn't have to pass one if he doesn't want to. There are other to!() implementations that take special arguments, e.g. in radix conversions an extra argument is passed.

(In reply to comment #7) > One more test case: > En[][] m1 = to!(En[][])([[10, 30], [30, 10]]); Ah, haven't thought about arrays. Will fix.. > Have you compiled your code with "-property -w"? > I am seeing some errors: Those seem to be Phobos errors, unrelated to my code.

One more test case: enum En[][] m1 = to!(En[][])([[10, 30], [30, 10]]);

http://dpaste.dzfl.pl/f4e35e84 More tests welcome!

(In reply to comment #11) > http://dpaste.dzfl.pl/f4e35e84 Please attach this in Bugzilla :-) > More tests welcome! Is this supported? enum En[][] m1 = to!(En[][])([[10, 30], [30, 10]]);

Created attachment 1153 d (In reply to comment #12) > Is this supported? > enum En[][] m1 = to!(En[][])([[10, 30], [30, 10]]); Yes.

(In reply to comment #13) > > Is this supported? > > enum En[][] m1 = to!(En[][])([[10, 30], [30, 10]]); > > Yes. Then I suggest to add it to your tests, because it's not present. Your tests contain code like: En[][] m1 = to!(En[][])([[10, 30], [30, 10]]);

Created attachment 1154 code (In reply to comment #14) > (In reply to comment #13) > > > > Is this supported? > > > enum En[][] m1 = to!(En[][])([[10, 30], [30, 10]]); > > > > Yes. > > Then I suggest to add it to your tests, because it's not present. > > Your tests contain code like: > > En[][] m1 = to!(En[][])([[10, 30], [30, 10]]); Ah it took me a while to realize you were looking for CTFE ability.

(In reply to comment #15) > Ah it took me a while to realize you were looking for CTFE ability. Sorry for not being more clear. Another significant test case, this is useful in many simulations and games, to write safe tables and start conditions: enum Code : char { A='A', B='B', C='C' } void test() { ... const code = to!(Code[][])(["ABCCBA", "BBAA"]); with (Code) assert(code == [[A, B, C, C, B, A], [B, B, A, A]]); } The main problem with this is that you receive conversion errors only at run-time, and in D the problem with array compile-time constants is that the compiler re-creates them every time where you use them. So they are not so useful. ------------------ Unfortunately this is not yet allowed because to!() is not pure: immutable code = to!(Code[][])(["ABCCBA", "BBAA"]);

Created attachment 1155 new Without purity it works ok though.

(In reply to comment #17) > Created an attachment (id=1155) [details] > new > > Without purity it works ok though. Not much (any) response in D NG. I could try and make a pull and see what folks have to say.

(In reply to comment #18) > Not much (any) response in D NG. I could try and make a pull and see what folks > have to say. OK.

(In reply to comment #16) > Another significant test case, this is useful in many simulations and games, to > write safe tables and start conditions: > > enum Code : char { A='A', B='B', C='C' } > > void test() > { > ... > const code = to!(Code[][])(["ABCCBA", "BBAA"]); > with (Code) > assert(code == [[A, B, C, C, B, A], [B, B, A, A]]); > } I won't be able to do this, the problem is there is already a toImpl overload which takes a string and turns it into an enum. It has different semantics, it searches for matching member names rather than member values.

(In reply to comment #20) > I won't be able to do this, the problem is there is already a toImpl overload > which takes a string and turns it into an enum. It has different semantics, it > searches for matching member names rather than member values. I see. See also Issue 5515

Commits pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/72a09960890aedda14173304263c21ff3671262b Fixes Issue 8143 - Safe std.conv.to enum conversion https://github.com/D-Programming-Language/phobos/commit/f110604b593799f74af4cdedd6bd61c8275926e3 Merge pull request #897 from AndrejMitrovic/Fix_8143 Fix Issue 8143 - Safe std.conv.to enum conversion

Commit pushed to staging at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/e22fef3c36362aebd2c2fd60e344c32ec06dd0b3 Fixes Issue 8143 - Safe std.conv.to enum conversion

*** Issue 5515 has been marked as a duplicate of this issue. ***