Crash when trying to assign a floating point to an integer in a C# property since 3.0.0-beta-2047

[christianrondeau]

Problem:

Given this class:

public class MyObject {
  public int MyProperty { get; set; }
}

And this code:

var engine = new Engine();
engine.SetValue("obj", new MyObject());
engine.Execute("obj.MyProperty = 100.2;");

The code now crashes with this exception:

Jint.Runtime.JavaScriptException : Cannot convert floating point number 100.2 with decimals to integral type System.Int32

The problem came from commit Disallow unsafe floating point conversion caching (#1456) (hash c4ff8051676a780a5ea9f7cf53eeb48d6b21837a) which did this intentionally.

While it's true that it's unsafe, it's also a breaking change that may cause more problems than it's trying to solve. The nature of JavaScript makes integers and floating point values interchangeable, meaning less experienced users may end up assigning values that have been divided and not rounded manually.

I think that if the integrator decided to create an integer field and map it to JavaScript code, they are aware that it will do flooring (that's what I would expect), same as when in c# I do var x = (int)0.5. If I want to detect that, I would do a double or a float property and check myself. Otherwise, I'll need to re-map every field to a double, and do the rounding myself, which doesn't sound fun :)

If you believe this unsafe rounding is a problem, I would suggest adding an option to turn it off.

What do you think?

[lahma]

Idea was to make this safe by default (not losing precision accidentally), even if it means getting an error - as JS has little means to cast data types to show intent / "I know what I'm doing".

Should it be separate option or go into value coercion options? Error should probably hint about the option? Would you like to create a PR? 😉

[lahma]

Also if you don't see value in this, check can also be removed 🤷🏻‍♂️

[christianrondeau]

Personally I'm torn between dropping the check and adding an option. I don't feel like I can speak for the community, though I guess if no one asked for it before, maybe it's not that desirable ;) I'm fine with both, really. And yes, I can try and suggest a PR but I'll let you make the hard choice :D

[lahma]

Maybe we should remove the check altogether and add it back if anyone has the need. Felt like a good idea back then, but as it's interop, let's consider responsibility and reaction to possible surprises be on library consuming side.

[christianrondeau]

I see where you come from (I always prefer crashing to silently massaging data) but in this case, I think it makes sense. I'll also add a test that verifies that the coertion works. Thanks for your quick answers, as usual :)

[christianrondeau]

Thank you very much!