google/elemental2

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, an int/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 handle java.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.

PR in Closure created

@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?