JsNumber.toFixed either needs more overloads or needs @DoNotAutobox
niloc132 opened this issue · 3 comments
Discovered via https://stackoverflow.com/questions/57244451/bug-jsnumber-tofixed-returns-different-values-in-superdev-and-js
When calling JsNumber.toFixed with an int
value (as would seem to be the sanest option), the int
is boxed to Integer
, which isn't treated as a number by the underlying ToInteger -> ToNumber -> ToPrimitive (see https://www.ecma-international.org/ecma-262/5.1/#sec-15.7.4.5 etc).
Adding elemental2.core.JsNumber.toFixed.digits
to core's integer-entities.txt only results in this logged message at build time:
SEVERE: Unused integer entities: [elemental2.core.JsNumber.toFixed.digits]
Some proposed solutions that seem to make sense on face value (though I haven't tried to fully implement any of them yet):
- In the same way as integer-entities.txt, provide a way to mark some parameter as
@DoNotAutobox
, so that if truly any value is allowed, anint
/float
/etc is passed directly to JS without boxing. It might be reasonable to have this be the default, since native methods aren't likely to specially handlejava.lang.Integer
, etc. - In the extern diffs, specify that this method might take Object, or might take Number, allowing the regular
integer-entities.txt
functionality to work here. - Permit
jsinterop.generator.visitor.IntegerEntitiesConverter
in google/jsinterop-generator to act on any supertype of Double/double, so that overloads are generated automatically.
I think we should fix the extern file for this method. It should accept a Number and not an Object.
Then we can use interger-entities.txt to turn the parameter to int.
@niloc132 with the Closure PR merged, the signature in next Elemental release should be toFixed(double)
, which is consistent with existing toPrecision(double)
and toExponential(double)
. Both of those methods work OK with integer argument both in compiled and super-dev mode. Since (1/3).toFixed(5.5)
is technically valid JS, maybe declaring digits
to be an integer is not needed and this issue can be closed?