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.

Thanks for reporting this! Could you provide a minimal testcase to better reproduce this?

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.

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. :(

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.

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.

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.

> 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.

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.

It's a "regression" to the extent that it's a bug in newly in newly introduced functionality.

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.

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...

remove unrelated change

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.

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 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 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.

Setting needinfo?dherman with the understanding that an answer may not be immediately forthcoming.

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.

Do we have any news on this?

Not yet. I asked Dave to ask at the TC39 meeting, but I haven't heard from him.

TC39 says Math.hypot is n-ary. Not sure about Math.hypot.length yet. I'll wait for rwaldron to post the meeting notes.

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?

> 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.

David, thanks for offering to take this on. I've assigned the bug to you.

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.

Small update over last patch: added check of return value in ToNumber() and also removed old (#ifdef'ed) hypot() implementation code.

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

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.

(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.

Added to: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/27#JavaScript Updated: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/hypot

Fwiw, I think an explanation about *how* this computes the sum of squares would have been helpful as a comment.

Then file a followup bug with the patch!

I don't understand it myself, so I can't write the comment :)

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.

Nom'ing for Fx 27 relnote.

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.

(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.