http://d.puremagic.com/issues/show_bug.cgi?id=8143

           Summary: Safe std.conv.to enum conversion
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: Phobos
        AssignedTo: nobody@puremagic.com
        ReportedBy: bearophile_hugs@eml.cc


--- Comment #0 from bearophile_hugs@eml.cc 2012-05-24 14:46:34 PDT ---
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
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8143


Andrej Mitrovic <andrej.mitrovich@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrej.mitrovich@gmail.com


--- Comment #1 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2012-10-23 16:44:27 PDT ---
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?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8143



--- Comment #2 from bearophile_hugs@eml.cc 2012-10-23 18:59:14 PDT ---
(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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8143


monarchdodra@gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |monarchdodra@gmail.com


--- Comment #3 from monarchdodra@gmail.com 2012-10-24 07:58:55 PDT ---
(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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8143



--- Comment #4 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2012-10-24 08:02:31 PDT ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8143


Andrej Mitrovic <andrej.mitrovich@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|nobody@puremagic.com        |andrej.mitrovich@gmail.com


--- Comment #5 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2012-10-24 10:13:51 PDT ---
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();
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8143


bearophile_hugs@eml.cc changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|andrej.mitrovich@gmail.com  |nobody@puremagic.com


--- Comment #6 from bearophile_hugs@eml.cc 2012-10-24 10:19:48 PDT ---
(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).

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8143



--- Comment #7 from bearophile_hugs@eml.cc 2012-10-24 10:26:36 PDT ---
(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

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8143



--- Comment #8 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2012-10-24 10:28:51 PDT ---
(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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8143



--- Comment #9 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2012-10-24 10:31:14 PDT ---
(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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------