Open Bug 1846606 Opened 1 year ago Updated 20 days ago

parseFloat() and console.log("%f") disagree

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

People

(Reporter: domfarolino, Unassigned, Mentored)

References

(Depends on 1 open bug)

Details

(Keywords: good-first-bug, parity-chrome, parity-safari)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/113.0.0.0 Safari/537.36

Steps to reproduce:

Per the Console Standard, console.log("%f", ...) should delegate to ECMAScript's %parseFloat()%. All browsers agree that parseFloat(23) prints "23", however in Firefox, console.log("%f", 23) prints "23.000000" (i.e., including an unnecessary number of zeros). If console.log(%f) were truly delegating to parseFloat(), then their outputs should be the same.

Additionally, parseFloat(null) appropriately prints "NaN", which all browsers agree on. However, console.log("%f", null) prints 0.000000 ONLY in Firefox, which appears to be another spec deviation.

The Bugbug bot thinks this bug should belong to the 'DevTools::Console' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → Console
Product: Firefox → DevTools
Status: UNCONFIRMED → NEW
Component: Console → DOM: Core & HTML
Ever confirmed: true
Keywords: parity-chrome, parity-safari
Product: DevTools → Core
Severity: -- → S4
Keywords: good-first-bug

Hi, I'm totally new around here and would love to give this bug a shot if that's ok! Could it be assigned to me?

I still see the issue present in the latest Nightly (128.0a1 2024-05-27). In fact, the issue is a bit more broad than what is reported, if we are following the Console Standard here: https://console.spec.whatwg.org/#formatter steps 6.2 and 6.3. (Please let me know if this isn't right/I should be looking at a different version.)

We don't do anything special for Symbol (6.2.1 and 6.3.1); e.g. console.log("%f", Symbol.match) or console.log("%d", Symbol.unscopables) prints nothing in Firefox while both Chrome and Safari agree that NaN should be printed.

We also don't defer to parseInt or parseFloat (per 6.2.2 and 6.3.2) but instead use to my understanding a totally different code path to handle the conversion to string, which causes the disagreements such as printing 0 instead of NaN noted by Dominic as well as several other issues, for example (everything below checked with Chrome/Safari):

  • console.log("%d", "xyz") should print NaN, but instead prints 0
  • console.log("%f", "3.14 is tasty") should print 3.14, but instead prints NaN
  • console.log("%f", ["123", "45"]) should print 123 (since parseFloat(["123", "45"]) yields 123), but instead it prints NaN. (Interestingly so does Safari in this case)

Relevant code for this issue: https://searchfox.org/mozilla-central/source/dom/console/Console.cpp#1983-2031

Hello :gong!
Thank you for your interests in helping this bug. I just assigned this to you.

Assignee: nobody → brandon.gong

Hi Brandon,

if you have any questions about this bug, or general engineering workflow questions, feel free to ask questions and needinfo me, happy to help out. :)

Thank you for taking a look into this bug!

Thank you Jan :)

Mentor: jjaschke

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Assignee: brandon → nobody

Hello,

Am I correct in understanding that this bug is now actively being worked on?
 
Regardless, I would like to share a few points that I believe could be useful, even if someone is currently addressing the issue.

Based on the previous comment by :gong and referencing the whatwg console specification, when we execute something like console.log("%f", f), it should be interpreted as console.log("%f", Number.parseFloat(f)). This suggests that the following outcomes should occur when calling console.log:

  • console.log("%f", "3.14 is tasty") -> console.log("%f", Number.parseFloat("3.14 is tasty")) -> console.log("%f", 3.14) -> 3.14
  • console.log("%f", ["123", "45"]) -> console.log("%f", Number.parseFloat(["123", "45"])) -> console.log("%f", 123) -> 123

However, what actually happens right now is:

  • console.log("%f", "3.14 is tasty") -> console.log("%f", ToNumber("3.14 is tasty")) -> console.log("%f", NaN) -> NaN
     
    While JS::ToNumber functions correctly, it seems we should not be invoking it in this case. Instead, we should be calling a parseFloat equivalent.
    if (NS_WARN_IF(!JS::ToNumber(aCx, value, &v))) {
        return false;
    }

/* ES6 draft 20141224, 7.1.3. */
MOZ_ALWAYS_INLINE bool ToNumber(JSContext* cx, HandleValue v, double* out) {
    detail::AssertArgumentsAreSane(cx, v);

    if (v.isNumber()) {
        *out = v.toNumber();
        return true;
    }
    return js::ToNumberSlow(cx, v, out);
}

 
At present, there is no JS::ParseFloat implementation. From a quick glance, adding JS::ParseFloat would not be a trivial task. We already have a js::NumberParseInt implementation, and while adding a JS::ParseInt function would not be too complicated, simply copying and pasting js::NumberParseInt to create js::NumberParseFloat would not be straightforward. However, the existing js::number_parseFloat function can help in correctly implementing js::NumberParseFloat, similar to how js::NumberParseInt is implemented.

// Parse the input string as if Number.parseInt had been called.
extern bool NumberParseInt(JSContext* cx, JS::HandleString str, int32_t radix,
                           JS::MutableHandleValue result);

 
I experimented with using js::NumberParseInt in a debug build, and it works as expected for integer types (we would also need to adjust the logic for %d or %i arguments). However, this seems like a complex fix for a "good-first-bug" and might require more significant code changes.

  • console.log("%d", [23, 45]) prints 23 in Google Chrome, in Mozilla 0, in my debug build it also prints 23.

 
Therefore, my suggestion would be to split this bug into several smaller tasks:

  1. Add JS::ParseInt and JS::ParseFloat implementations
  2. Replace JS::ToNumber with JS::ParseInt or JS::ParseFloat where appropriate
  3. (If needed) Further split the task of adding JS::ParseInt and JS::ParseFloat
Flags: needinfo?(jjaschke)

Hi Nikolai,

thank you for your input! It might make sense to split this up as you proposed. Are you interested in helping out with the implementation here as well?

:gong, are you still actively working on this bug?

Flags: needinfo?(jjaschke) → needinfo?(brandon)

Hi Nikolai and Jan,

I have not worked on the bug in a long while as I have been swept up with other things. I regret not updating you all sooner -- I keep thinking I'll come back to it soon, but now it's been 5 months of silence from me. My apologies for this.

In the time that I did have to look at it, I was not able to make much headway; it's heavily due to my lack of familiarity with the codebase, but looking at Nikolai's investigation it seems that the issue is also more complex than I had imagined.

That being said, I foresee my schedule becoming a bit lighter than it was in the past few months. If you all would be willing, I would still be interested in helping. The task as a whole is probably more difficult than I can handle as a good-first-bug, but I would be happy to take a smaller piece to chew on. Though I very much understand if it would be more productive for someone else to handle this bug!

Thanks all for your time!

Flags: needinfo?(brandon)

Hello Jan and Brandon,

I’d like to participate in the implementation if possible. As Jan agreed, we should split this bug into at least two parts. One approach could be to file a new bug for implementing JS::ParseInt and JS::ParseFloat, and then make the current bug dependent on that new one.

However, after some research, I found that the JS namespace falls under the JavaScript Engine (SpiderMonkey) component. This suggests that adding new methods to this namespace won’t be a quick task, as it will involve other developers from a different component. We’ll also need to provide a clear explanation for why we are modifying code in their component.

That said, I’m not in a rush. I’m happy to take on the task of adding JS::ParseInt and JS::ParseFloat and leave the current bug for future work. This way, Brandon (:gong) or other developers can find time to contribute when they're able.

Regarding filing a new bug, I’m not familiar with the procedure but I’m willing to give it a try if possible.

Flags: needinfo?(jjaschke)

Hey folks, thanks for coming to help out! Feel free to open bugs against the JS engine. If you run into any issues feel free to ping.

Reading through this issue, you are almost there. The core problem is we are missing a C++ facing interface. You don't need to reimplement parseFloat, you can split it. It is currently setting the value of something called rval, but that is actually just a HandleValue under the hood (same type as v in ToNumber). You can share quite a bit from there, split the arg handling from the value setting. The c++ interface doesn't need to bother with CallArgs args = CallArgsFromVp(argc, vp); or checking the length of args, because it can pass the HandleValue directly.

Some context: the engine can be called in different ways. From JS, from the JITs or from C++ that lives in the browser engine. num_parseFloat is called by JS, same for num_parseInt. js::NumberParseInt is called by both JS (via num_parseInt) and the JITs (see the callvms in there) but both of these are engine internal. It isn't called by the browser, though it could be. I can't remember if it needs to be a JS_PUBLIC_API or not, but it seems like you already got it working so maybe not. It might need a public api wrapper or to be a public api itself in order to be called by external C++.

This is a bit of refactoring in order to make it work, but we are happy for the help :). Feel free to stop by the SpiderMonkey channel on matrix for help also.

Depends on: 1928966

(removing my ni? because I could see that :gaporf is active on Matrix discussing the matter with folks that are far more involved than me. Feel free to ping me again if you need anything. Thank you all for working on this and for supporting!)

Flags: needinfo?(jjaschke)

Working on the blocking bug 1928966 I discovered an interesting edge case:

 

In both Mozilla Firefox and Google Chrome, the following commands behave consistently as expected:

> Number.parseInt(-0)
< 0
> Number.parseInt("-0")
< -0

 

However, the behavior of console.log differs in Google Chrome:

> console.log("%d", -0)
0
< undefined

> console.log("%d", "-0") // We expect -0 due to Number.parseInt("-0") = -0
0
< undefined

Safari behaves similarly to Google Chrome:

> console.log("%d", -0)
[Log] 0
< undefined
> console.log("%d", "-0")
[Log] 0
< undefined

> Number.parseInt("-0")
< -0
> Number.parseInt(-0)
< 0

 

I suppose we should add test cases for the following scenarios:

  • console.log("%d", -0), we should expect 0 output;
  • console.log("%d", "-0"), we should expect -0 output.

These test cases should validate whether the output distinguishes between -0 and 0 in %d formatting, ensuring consistency with the behavior of Number.parseInt.

You need to log in before you can comment on or make changes to this bug.