CLR type decimal not working correctly if thread CultureInfo has "," as a decimal separator

[timoi]

This code seems to fail:

Thread.CurrentThread.CurrentCulture = CultureInfo.GetCultureInfo("fi-FI");
var ctx = new IronJS.Hosting.CSharp.Context();
ctx.SetGlobal("decimalVal", 1.5m);
Assert.True(ctx.Execute("decimalVal < 3"), "1.5 should be smaller");

Works ok if the CurrentCulture is something like "en-US" where "." is used as a decimal separator.

[gboya]

The same happens with Single precision floats btw.

AFAIK, only Integer and Double CLR numeric types are trated as first-class "Numbers" when they get boxed to BoxedValue thru the SetGlobal call. (see the BoxedValue.Box(object) method)
This test fails :

            var ctx = new IronJS.Hosting.CSharp.Context();
            const float f = 1.5F;

            ctx.SetGlobal("myFloat", f);
            var result = ctx.GetGlobal("myFloat"); // Result is a "BoxedValue"

            Assert.IsTrue(result.IsNumber); // This fails

What's even more funny is that for Single precision floats, this could be resolved by avoiding the boxing from float to object that happens somewhere (I don't know exactly where...) when invoking SetGlobal, which eventually prevents the implicit conversion float --> double to happen. (Yet this wouldn't fix the "GetGlobal" test, see below).

Anyway, that would not solve the problem for decimal values.
After some digging, what seems to happen under the hood when comparison operators are involved with non-treated-as-numbers (which is the case for Decimal), is :

1. Since they are not both numbers and not both string, an attempt is made to convert them to Primitive Types supported by IronJS runtime.
2. In your example, the value "3.0" remains as a boxed number, but the decimal value gets converted to string using the Object.ToString() method . That's where your CurrentCulture enters...
3. Finally, an attempt to a Number comparison is done by converting the converted-to-string decimal back to a Number. And as you can see in the TypeConverters.ToNumber(string) method, it is based an InvariantCulture based conversion (with a few tricks and cleaning though).

So some possible workarounds to this issue :

• Convert all non-integral numeric types to Double when boxing. That's fine for Single, but for decimal, it implies 1) potential loss of precision 2) We would make such a test throw a nasty Exception, because we would be loosing the CLR Type information. Note that this test passes now.

            var ctx = new IronJS.Hosting.CSharp.Context();
            const decimal d = 1.5m;
            const string globalVariableName = "decimalVal";

            ctx.SetGlobal(globalVariableName, d);
            var result = ctx.GetGlobalAs<decimal>(globalVariableName);

            Assert.AreEqual(d, result);

• Add some logic in TypeConverters.ToNumber() to give a last chance to parse strings using the CurrentCulture. That would fix this issue, but couldn't we get unexpected results with awkward number formats ?
• When converting boxed decimal/float to String (in BoxedValue.ToPrimitive() ), add an extra check for hidden numeric types in the underlying CLR object, and in that case, use an InvariantCulture conversion. That would be a safer fix, I assume, and quite simple to implement.
• Avoid completely the String conversion part (which is the root cause of the problem), by either 1) implementing the extra type checkings in Operators implementation or 2) implementing a proper number conversion in the TypeConverters.ToPrimitive method.

I'll try to push a fix on my fork within days/weeks. Actually this also concerns my "fr-FR" CurrentCulture :-)

[fholm]

There is no support in JavaScript for some sort of "decimal" type, which is
why there is no support for the .NET decimal. But you are right in that one
could just convert floats to doubles to solve the issue with float values.
*

Regards,
Fredrik Holmström*

On Mon, Mar 4, 2013 at 3:46 PM, Gabriel Boya notifications@github.comwrote:

The same happens with Single precision floats btw.

AFAIK, only Integer and Double CLR numeric types are trated as
first-class "Numbers" when they get boxed to BoxedValue thru the SetGlobal
call. (see the BoxedValue.Box(object) method)
This test fails :

        var ctx = new IronJS.Hosting.CSharp.Context();


        const float f = 1.5F;

        ctx.SetGlobal("myFloat", f);
        var result = ctx.GetGlobal("myFloat"); // Result is a "BoxedValue"

        Assert.IsTrue(result.IsNumber); // This fails

What's even more funny is that for Single precision floats, this _could_be resolved by avoiding the boxing from
float to object that happens somewhere (I don't know exactly
where...) when invoking SetGlobal, which eventually prevents the implicit
conversion float --> double to happen. (Yet this wouldn't fix the
"GetGlobal" test, see below).

Anyway, that would not solve the problem for decimal values.
After some digging, what seems to happen under the hood when comparison
operators are involved with non-treated-as-numbers (which is the case for
Decimal), is :

1. Since they are not both numbers and not both string, an attempt is made
   to convert them to Primitive Types supported by IronJS runtime.
2. In your example, the value "3.0" remains as a boxed number, but the
   decimal value gets converted to string using the Object.ToString() method. That's where your CurrentCulture enters...
3. Finally, an attempt to a Number comparison is done by converting the
   converted-to-string decimal back to a Number. And as you can see in the
   TypeConverters.ToNumber(string) method, it is based an InvariantCulture
   based conversion (with a few tricks and cleaning though).

So some possible workarounds to this issue :

Convert all non-integral numeric types to Double when boxing. That's
fine for Single, but for decimal, it implies 1) potential loss of precision
2) We would make such a test throw a nasty Exception, because we would be
loosing the CLR Type information. Note that this test passes now.

       var ctx = new IronJS.Hosting.CSharp.Context();


       const decimal d = 1.5m;
       const string globalVariableName = "decimalVal";

       ctx.SetGlobal(globalVariableName, d);
       var result = ctx.GetGlobalAs<decimal>(globalVariableName);

       Assert.AreEqual(d, result);

-

Add some logic in TypeConverters.ToNumber() to give a last chance to
parse strings using the CurrentCulture. That would fix this issue, but
couldn't we get unexpected results with awkward number formats ?
-

When converting boxed decimal/float to String (in
BoxedValue.ToPrimitive() ), add an extra check for hidden numeric
types in the underlying CLR object, and in that case, use an
InvariantCulture conversion. That would be a safer fix, I assume,
and quite simple to implement.
-

Avoid completely the String conversion part (which is the root cause
of the problem), by either 1) implementing the extra type checkings in
Operators implementation or 2) implementing a proper number conversion in
the TypeConverters.ToPrimitive method.

I'll try to push a fix on my fork within days/weeks. Actually this also
concerns my "fr-FR" CurrentCulture :-)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/97#issuecomment-14383841
.

[gboya]

Yep, I'm aware of that. Yet we should try to keep a consistent behaviour across different CultureInfo.
But some kind of "support" of operators for such CLR types (decimal, float, etc...) could be implemented at the ToPrimitives level, just to make these conversions CurrentCulture independent.

[fholm]

I really do not want to intermix Decimal into the JS runtime, floats fine
as they can be up-converted to double. But I would prefer to keep the
runtime incompatible with Decimal.
*

Regards,
Fredrik Holmström*

On Mon, Mar 4, 2013 at 4:05 PM, Gabriel Boya notifications@github.comwrote:

Yep, I'm aware of that. Yet should we try to keep a consistent behaviour
across different CultureInfo.
But some kind of "support" of operators for such CLR types (decimal,
float, etc...) could be implemented at the ToPrimitives level, just to
make these conversions CurrentCulture independent.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/97#issuecomment-14384825
.