Last Comment Bug 896264 - Math.hypot returns NaN when only one argument is passed
: Math.hypot returns NaN when only one argument is passed
Status: RESOLVED FIXED
[qa-]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 25 Branch
: All All
: -- normal (vote)
: mozilla27
Assigned To: David Caabeiro [:dcaabeiro]
:
:
Mentors:
Depends on: 995673
Blocks: 717379
  Show dependency treegraph
 
Reported: 2013-07-21 07:14 PDT by daniel
Modified: 2014-04-12 16:48 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
disabled
disabled
fixed
27+


Attachments
math_hypot.patch (3.53 KB, patch)
2013-07-23 09:13 PDT, André Bargull
no flags Details | Diff | Splinter Review
disable Math.hypot (5.60 KB, patch)
2013-08-21 10:37 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
disable Math.hypot (4.82 KB, patch)
2013-08-21 10:44 PDT, :Benjamin Peterson
jorendorff: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
bug-896264-fix.patch (5.93 KB, patch)
2013-10-01 15:35 PDT, David Caabeiro [:dcaabeiro]
no flags Details | Diff | Splinter Review
bug-896264-fix.patch (7.24 KB, patch)
2013-10-02 08:35 PDT, David Caabeiro [:dcaabeiro]
jorendorff: review+
Details | Diff | Splinter Review

Description daniel 2013-07-21 07:14:14 PDT
When Math.hypot is called passing only one argument it returns NaN.

The [spec](http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.8.2.29) says:
If any argument is +∞, the result is +∞.
If any argument is −∞, the result is +∞.
If no argument is +∞ or −∞, and any argument is NaN, the result is NaN.
If all arguments are either +0 or -0, the result is +0.

Example:
According to the spec, Math.hypot(Infinity) should return Infinity, in FF it returns NaN.
Comment 1 Tiziana Sellitto [:tiziana] 2013-07-22 08:55:20 PDT
Thanks for reporting this!
Could you provide a minimal testcase to better reproduce this?
Comment 2 daniel 2013-07-22 13:01:38 PDT
Here's a testcase:
http://jsbin.com/obeyit/2/edit?html,javascript,live

Support for Math.hypot hasn't propogated to FF stable yet. I've been testing in FF nightly.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2013-07-22 13:28:29 PDT
Erm.  The spec doesn't actually define the behavior of this function... in particular, it doesn't define little things like what order the arguments are converted to numbers in!

It's hard to tell whether our behavior is following the spec or not if there is no processing model in the spec.  :(
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-22 13:40:55 PDT
The order of evaluation of arguments isn't defined in these methods, it's defined in some sort of prefatory text -- 15.8.2 in the 20130715 (?) ES6 draft:

"""
Each of the following Math object functions applies the ToNumber abstract operation to each of its arguments (in left-to-right order if there is more than one). If ToNumber returns an abrupt completion, that completion record is immediately returned. Otherwise, functction performs a computation on the resulting Number value(s).
"""

Pretty sure the same stuff is in ES5 as well.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-22 16:07:45 PDT
The semantics currently implemented are just wrong, at least as far as the latest ES6 draft goes.  Math.hypot computes the sum of the squares of the first N arguments, for N up to 3 -- there's no requirement that at least two or three arguments be passed for the result to not always be NaN, but that's what the code has now.

This is easily fixed.  We should fix this, ideally before uplift, but it's not the end of the world if we have to fix it after uplift.  I don't think we should ship a release with wrong behavior here, but we're not in danger of that yet.
Comment 6 André Bargull 2013-07-23 09:13:00 PDT
Created attachment 779815 [details] [diff] [review]
math_hypot.patch

I'm not sure whether `args.hasDefined()` or an additional `args.length() == 1` check is better in this case. I'm going with `args.hasDefined()` for now to reduce the amount of changes.
Comment 7 Jason Orendorff [:jorendorff] 2013-07-31 10:32:29 PDT
> Math.hypot computes the sum of the squares of the first N arguments, for N up to 3

The spec begins: "Given two or three arguments...". We have to look elsewhere to see how it should behave when given 0 arguments or 1 argument.

The fourth paragraph of Clause 15 says:

> Unless otherwise specified in the description of a particular function, if a
> function or constructor described in this clause is given fewer arguments than the
> function is specified to require, the function or constructor shall behave exactly
> as if it had been given sufficient additional arguments, each such argument being
> the undefined value.

So Math.hypot(1) is the same as Math.hypot(1, undefined), and according to the text quoted in comment 4, that's the same as Math.hypot(1, NaN), which is NaN.

But Math.hypot(Infinity) should be Infinity. I think that is the only bug though. Checking with es-discuss.
Comment 8 Lukas Blakk [:lsblakk] use ?needinfo 2013-07-31 12:53:02 PDT
It's not a regression and doesn't appear to have critical user impact, please nominate a fix for uplift when ready if low-risk enough.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2013-08-01 09:25:44 PDT
It's a "regression" to the extent that it's a bug in newly in newly introduced functionality.
Comment 10 Jason Orendorff [:jorendorff] 2013-08-05 09:17:27 PDT
Allen (who edits the spec) claims that the analysis in comment 7 is incorrect and Math.hypot(1, undefined) should return 1.

Discussion on es-discuss has petered out.

http://esdiscuss.org/topic/math-hypot-1

Taking an arbitrary number of arguments is the only thing that makes a lick of sense. Let's hope sanity prevails.

bz, should we patch to disable Math.hypot in FF25 (and tip for the time being)? I think we should. It's not baked.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2013-08-05 09:30:40 PDT
I think disabling it if people don't agree on the behavior and we're not in a huge rush to use it makes sense...
Comment 12 :Benjamin Peterson 2013-08-21 10:37:49 PDT
Created attachment 793572 [details] [diff] [review]
disable Math.hypot
Comment 13 :Benjamin Peterson 2013-08-21 10:44:37 PDT
Created attachment 793579 [details] [diff] [review]
disable Math.hypot

remove unrelated change
Comment 14 Jason Orendorff [:jorendorff] 2013-08-21 12:42:38 PDT
Comment on attachment 793579 [details] [diff] [review]
disable Math.hypot

Thanks. r=me.

In jsmath.cpp:
> js::math_atanh(JSContext *cx, unsigned argc, Value *vp)
> {
>     return math_function<math_atanh_impl>(cx, argc, vp);
> }
> 
>+
>+// Math.hypot is disabled pending the resolution of spec issues (bug 896264).

House style is to avoid multiple blank lines together like this.
Comment 15 :Benjamin Peterson 2013-08-21 13:09:47 PDT
Whitespace adjusted and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfbc5978b5f5

Leaving this bug open to actually adjust the implementation to spec when that's figured out.
Comment 16 :Benjamin Peterson 2013-08-21 13:11:26 PDT
Comment on attachment 793579 [details] [diff] [review]
disable Math.hypot

[Approval Request Comment]
We're simply disabling this feature until the ES6 spec improves its specification of it.
Comment 17 Jason Orendorff [:jorendorff] 2013-08-21 16:24:47 PDT
Comment on attachment 779815 [details] [diff] [review]
math_hypot.patch

Clearing review flag. I'm still waiting for word from TC39 on what the spec will ultimately say.
Comment 18 Jason Orendorff [:jorendorff] 2013-08-21 16:26:38 PDT
Setting needinfo?dherman with the understanding that an answer may not be immediately forthcoming.
Comment 19 :Ms2ger (⌚ UTC+1/+2) 2013-08-22 04:33:52 PDT
https://hg.mozilla.org/mozilla-central/rev/cfbc5978b5f5
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2013-08-23 13:24:25 PDT
Comment on attachment 793579 [details] [diff] [review]
disable Math.hypot

Approving disabling patch since this was implemented in 25 there shouldn't be dependencies on it and this is therefore perceived as low-risk.
Comment 22 AWAY Tom Schuster [:evilpie] 2013-09-21 10:26:06 PDT
Do we have any news on this?
Comment 23 Jason Orendorff [:jorendorff] 2013-09-23 11:09:31 PDT
Not yet. I asked Dave to ask at the TC39 meeting, but I haven't heard from him.
Comment 24 Jason Orendorff [:jorendorff] 2013-09-23 16:24:57 PDT
TC39 says Math.hypot is n-ary. Not sure about Math.hypot.length yet. I'll wait for rwaldron to post the meeting notes.
Comment 25 Jason Orendorff [:jorendorff] 2013-09-24 08:40:51 PDT
I'm deeply moved by the following direct quote from the meeting notes. dherman makes a moral argument and carries the day.

> ## 4.3 Math.hypot
>
> DH: The idea is that it returns the square root of the sum of its
> arguments, but we have 2 and 3 argument special casing.
>
> http://wiki.ecmascript.org/doku.php?id=harmony:more_math_functions
>
> It's an offense to mathematics if it's not: "The square root of the
> sum of the squares of all the arguments"
>
> Defend the pristine purity of math. If it's only a 2-argument
> function, that's OK, but having it a 2- and 3-argument function is not
> good, just make it an n-argument function.
>
> BE: Jason Orendorff outlined these problematic cases... (see
> es-discuss)
>
> DH: this is no good man.
>
> LH: Should be variadic
>
> DC: Programming in general is an insult to mathematics. Because what
> we call functions are not what they call functions. One convention we
> have in JS is that calling a function with three parameters is the
> same as calling it with two plus undefined.
>
> DH: but not for variadic functions. Since we want it for arity 2 and
> arity 3, cleanest way is to make it variadic. If you have 2 cases, yo
> have n cases.
>
> #### Consensus/Resolution
>
> - variadic
> - call ToNumber on all actual arguments
> - if one is NaN, no special behaviour, allow to fall out.

We can implement this now. Anyone want to take?
Comment 26 David Caabeiro [:dcaabeiro] 2013-09-24 08:59:44 PDT
> We can implement this now. Anyone want to take?

I've been doing some work on this module some time ago (and specifically, wrote the current hypot() implementation in jsmath). Wanted to apologise for being silent over the last weeks, but been very busy (moving, etc) to be able to set some time apart. In any case, I wouldn't mind volunteering to complete this work, together with improving / fixing any other potential issues that may pop up.
Comment 27 Till Schneidereit [:till] 2013-09-24 11:24:50 PDT
David, thanks for offering to take this on. I've assigned the bug to you.
Comment 28 David Caabeiro [:dcaabeiro] 2013-10-01 15:35:24 PDT
Created attachment 812871 [details] [diff] [review]
bug-896264-fix.patch

Please find a proposed patch. The implementation is an extension of the same concept (this time applied to N arguments) in order to avoid possible underflow or overflow cases.

I chose to use two flags to determine if any of the arguments is Infinite or NaN, due to the new draft requirement. Even though we could return as soon as an Infinite value is detected, I thought the code this way would look cleaner. Any better suggestion would be appreciated.

I also chose to implement the code without using a separate function (*_impl, etc) given that this function is rather unique to ES6 (not found in std C++, etc) so I bet there wouldn't be a "fallback to native" case. BTW, I'm sure someone already considered this, but wouldn't "norm" be a more suitable name for this?

I also added a few cases to the corresponding units, mostly to take into account the new ES6 draft.

Any comment or feedback will be appreciated.
Comment 29 David Caabeiro [:dcaabeiro] 2013-10-02 08:35:07 PDT
Created attachment 813141 [details] [diff] [review]
bug-896264-fix.patch

Small update over last patch: added check of return value in ToNumber() and also removed old (#ifdef'ed) hypot() implementation code.
Comment 30 Jason Orendorff [:jorendorff] 2013-10-18 12:34:10 PDT
Comment on attachment 813141 [details] [diff] [review]
bug-896264-fix.patch

Review of attachment 813141 [details] [diff] [review]:
-----------------------------------------------------------------

I made the changes described below and sent this off to the try server.

https://tbpl.mozilla.org/?tree=Try&rev=2d857fe36b86

::: js/src/jsmath.cpp
@@ +1283,5 @@
> +
> +    double scale = 0;
> +    double sumsq = 1;
> +
> +    for (unsigned int i = 0; i < args.length(); ++i) {

The house style is:

    for (unsigned i = 0; i < args.length(); i++) {

@@ +1291,5 @@
> +
> +        isInfinite |= mozilla::IsInfinite(x);
> +        isNaN |= mozilla::IsNaN(x);
> +
> +        if(x) {

Space after 'if'.

It's a little subtle what's going on here (really the problem is that we can end up with a divide by zero if *scale* is 0) so I rearranged the code a bit, like this:

>-if (x) {
>     double xabs = mozilla::Abs(x);
> 
>     if(scale < xabs) {
>         sumsq = 1 + sumsq * (scale / xabs) * (scale / xabs);
>         scale = xabs;
>-    }
>-    else {
>+    } else if (scale != 0) {
>         sumsq += (xabs / scale) * (xabs / scale);
>     }
>-}

(This also lets us avoid a branch about half the time, but I didn't measure whether it observably affects the speed.)

@@ +1306,3 @@
>      }
>  
> +    double result = isInfinite ? PositiveInfinity() : 

removed trailing whitespace from this line
Comment 31 Jason Orendorff [:jorendorff] 2013-10-23 09:54:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/eafe6b0acd33
Comment 32 David Caabeiro [:dcaabeiro] 2013-10-23 10:04:09 PDT
Thanks for reviewing Jason.

> It's a little subtle what's going on here (really the problem is that we can
> end up with a divide by zero if *scale* is 0) so I rearranged the code a
> bit, like this:

Not sure if I'm following you here. xabs should be >= 0 but the previous "if (x)" discards the case of zero, so inside the branch xabs is > 0.

Assuming scale was zero, then it will always follow the "if(scale < xabs)" branch, so no divide by zero issue there.

> >-if (x) {
> >     double xabs = mozilla::Abs(x);
> > 
> >     if(scale < xabs) {
> >         sumsq = 1 + sumsq * (scale / xabs) * (scale / xabs);
> >         scale = xabs;
> >-    }
> >-    else {
> >+    } else if (scale != 0) {
> >         sumsq += (xabs / scale) * (xabs / scale);
> >     }
> >-}
> 
> (This also lets us avoid a branch about half the time, but I didn't measure
> whether it observably affects the speed.)

Let me know if I'm missing something. If that's not the case, perhaps this proves we should add some proper comment to make it look less subtle? Let me know if you want me to add that.
Comment 33 Jason Orendorff [:jorendorff] 2013-10-23 14:55:19 PDT
(In reply to David Caabeiro [:dcaabeiro] from comment #32)
> Thanks for reviewing Jason.
> 
> > It's a little subtle what's going on here (really the problem is that we can
> > end up with a divide by zero if *scale* is 0) so I rearranged the code a
> > bit, like this:
> 
> Not sure if I'm following you here. xabs should be >= 0 but the previous "if
> (x)" discards the case of zero, so inside the branch xabs is > 0.
> 
> Assuming scale was zero, then it will always follow the "if(scale < xabs)"
> branch, so no divide by zero issue there.

I didn't mean that there was a bug in your code, I just meant that the reason the `if (x)` in your code is necessary has surprisingly little to do with x!

So I changed the code a little, without changing the behavior.

I pushed this to mozilla-inbound (see comment 31). It might go into tonight's Nightly.
Comment 34 Carsten Book [:Tomcat] 2013-10-24 00:26:01 PDT
https://hg.mozilla.org/mozilla-central/rev/eafe6b0acd33
Comment 36 :Ms2ger (⌚ UTC+1/+2) 2013-10-26 12:05:55 PDT
Fwiw, I think an explanation about *how* this computes the sum of squares would have been helpful as a comment.
Comment 37 Jason Orendorff [:jorendorff] 2013-10-26 22:45:20 PDT
Then file a followup bug with the patch!
Comment 38 :Ms2ger (⌚ UTC+1/+2) 2013-10-27 00:32:55 PDT
I don't understand it myself, so I can't write the comment :)
Comment 39 Jason Orendorff [:jorendorff] 2013-10-27 08:07:04 PDT
Then file a followup bug without the patch.

There's a conventional formula for the two-argument case; this code generalizes that to many arguments.
Comment 40 Florian Bender 2013-11-24 12:38:34 PST
Nom'ing for Fx 27 relnote.
Comment 41 Jason Orendorff [:jorendorff] 2013-12-05 10:24:33 PST
The exact thing that changed here is: We added support for a new function, Math.hypot(). This function will be specified in the ES6 standard.

Some Web developers may care about this, particularly game devs. But not many.
Comment 42 bhavana bajaj [:bajaj] 2013-12-05 10:58:06 PST
(In reply to Jason Orendorff [:jorendorff] from comment #41)
> The exact thing that changed here is: We added support for a new function,
> Math.hypot(). This function will be specified in the ES6 standard.
> 
> Some Web developers may care about this, particularly game devs. But not
> many.

Given this, I'll add this in the developer section on the upcoming beta notes for Fx27.

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