extensibleweb / webidl.js

An implementation of WebIDL in ECMAScript
55 stars 14 forks source link

Implement octet type #5

Closed marcoscaceres closed 11 years ago

marcoscaceres commented 11 years ago

http://www.w3.org/TR/WebIDL/#idl-octet

marcoscaceres commented 11 years ago

There appears to be a bug in WebIDL's JS to Octet type converter that doesn't handle negative numbers. For instance, given the current conversion algorithm in WebIDL, -50 will result in -50 (when 50 is expected).

I've emailed the spec's editor about this to confirm if it's a bug or not.

FremyCompany commented 11 years ago

I agree that the algorithm seems a bit strange, especially this line:

Set x to sign(x) * floor(abs(x)).

Maybe the author expect the modulo to work even for negative numbers. That would mean that -50 becomes 256-50. Do we know any API on which we can test? Usually, API use either Integer or Double, which is why I'm not sure about the outcome.

marcoscaceres commented 11 years ago

Yeah, looks like that was the assumption... But it's strange because he calls abs on x but then just reapplies the sign. Cameron usually responds fairly quickly, so let's see what he says.

FremyCompany commented 11 years ago

Hi guys, I found yet another possible issue with the WebIDL spec.

In the "any" conversion algorithm, it's specified that if the ECMAScript value is a Number, it should be converted using the 'double' conversion at section 4.2.14.

[http://dev.w3.org/2006/webapi/WebIDL/#es-any]

In this section, it's said to throw a type error if the number has the value "NaN" or is infinite.

[http://dev.w3.org/2006/webapi/WebIDL/#es-double]

I can prove this is not what browsers do by looking at the setTimeout function, which takes 'any' arguments.

[http://www.w3.org/TR/2011/WD-html5-20110525/timers.html#timers]

In the 'get timeout' algorithm that converts the received value into a number, we can read the following:

"If timeout is an Infinity value, a Not-a-Number (NaN) value, or negative, let timeout be zero." [http://www.w3.org/TR/2011/WD-html5-20110525/timers.html#get-the-timeout]

I tried calling setTimeout(alert, Number.NaN) and that works perfectly fine in all browsers (alert is executed straightaway), and that doesn't throw a conversion type error because of the 4.2.14 algorithm.

My belief is that the specification should have referenced the "4.2.15 unrestricted double" algorithm instead.

I sent this mail in CC to Cameron & the public-script-coord mailing list, let's see what their response will be.

wibblymat commented 11 years ago

"What the spec says" and "what the browsers do" is not always the same. I think it is likely that the browser behaviour is non-conforming for historical/practical reasons than that the spec is wrong somehow.

On 10 January 2013 11:01, FremyCompany notifications@github.com wrote:

Hi guys, I found yet another possible issue with the WebIDL spec.

In the "any" conversion algorithm, it's specified that if the ECMAScript value is a Number, it should be converted using the 'double' conversion at section 4.2.14.

[http://dev.w3.org/2006/webapi/WebIDL/#es-any]

In this section, it's said to throw a type error if the number has the value "NaN" or is infinite.

[http://dev.w3.org/2006/webapi/WebIDL/#es-double]

I can prove this is not what browsers do by looking at the setTimeout function, which takes 'any' arguments.

[http://www.w3.org/TR/2011/WD-html5-20110525/timers.html#timers]

In the 'get timeout' algorithm that converts the received value into a number, we can read the following:

"If timeout is an Infinity value, a Not-a-Number (NaN) value, or negative, let timeout be zero." [http://www.w3.org/TR/2011/WD-html5-20110525/timers.html#get-the-timeout]

I tried calling setTimeout(alert, Number.NaN) and that works perfectly fine in all browsers (alert is executed straightaway), and that doesn't throw a conversion type error because of the 4.2.14 algorithm.

My belief is that the specification should have referenced the "4.2.15 unrestricted double" algorithm instead.

I sent this mail in CC to Cameron & the public-script-coord mailing list, let's see what their response will be.

— Reply to this email directly or view it on GitHubhttps://github.com/extensibleweb/webidl.js/issues/5#issuecomment-12089939.

FremyCompany commented 11 years ago

On Thu, Jan 10, 2013 at 12:01 PM, François REMY fremycompany_pub@yahoo.fr wrote:

My belief is that the specification should have referenced the "4.2.15 unrestricted double" algorithm instead.

Yeah it's a bug. unrestricted double is new and originally double had the semantics of unrestricted double.

http://annevankesteren.nl/

FremyCompany commented 11 years ago

Yeah it's a bug. unrestricted double is new and originally double had the semantics of unrestricted double.

Thanks Anne for the confirmation and quick response.

FremyCompany commented 11 years ago

François REMY:

My belief is that the specification should have referenced the "4.2.15 unrestricted double" algorithm instead.

Anne van Kesteren:

Yeah it's a bug. unrestricted double is new and originally double had the semantics of unrestricted double.

Yep, fixed now, thanks.

marcoscaceres commented 11 years ago

@FremyCompany turns out "%" is not actually "modulus" (it's remainder), so I need to update the code.

ES6 says:

The notation “x modulo y” (y must be finite and nonzero) computes a value k of the same sign as y (or zero) such that abs(k) < abs(y) and x−k = q × y for some integer q.

I'm going to use:

function modulo (num, mod) {
            var remain = num % mod;
            return Math.floor(remain >= 0 ? remain : remain + mod);
}

From: http://www.sitecrafting.com/blog/modulus-remainder/

Can you please confirm that that is correct.

bkardell commented 11 years ago

On Mon, Jan 14, 2013 at 11:27 AM, Marcos Caceres notifications@github.comwrote:

@FremyCompany https://github.com/FremyCompany turns out ["%" is not actually "modulus" (it's remainder)] http://wiki.ecmascript.org/doku.php?id=strawman:modulo_operator), so I need to update the code.

ES6 says:

The notation "x modulo y" (y must be finite and nonzero) computes a value k of the same sign as y (or zero) such that abs(k) < abs(y) and x-k = q × y for some integer q.

I'm going to use:

function modulo (num, mod) { var remain = num % mod; return Math.floor(remain >= 0 ? remain : remain + mod);}

From: http://www.sitecrafting.com/blog/modulus-remainder/

Can you please confirm that that is correct.

Reply to this email directly or view it on GitHubhttps://github.com/extensibleweb/webidl.js/issues/5#issuecomment-12226197.

Confirm.

Brian Kardell :: @briankardell :: hitchjs.com

FremyCompany commented 11 years ago

To me it seems that we want abs(x%y) because -4%3==-1 not -2.

marcoscaceres commented 11 years ago

So, I found the algorithm in ES spec. For example:

n = Math.pow(2, 16);
int16bit = ((posInt % n) + n) % n;

So I'm switching to that.

marcoscaceres commented 11 years ago

See also: http://stackoverflow.com/questions/4467539/javascript-modulo-not-behaving

FremyCompany commented 11 years ago

No, sorry, I read too fast. If you want the true mathematical modulo you want the formula you wrote indeed. However if you click on modulo in the web idl spec you get redirected to section 5.2 of ES6 which defines modulo as

The notation “x modulo y” (y must be finite and nonzero) computes a value k of the same sign as y (or zero) such that abs(k) < abs(y) and xk = q  y for some integer q.

So when they use modulo in webidl they mean remainder it seems. Am I reading right?

bkardell commented 11 years ago

On Mon, Jan 14, 2013 at 12:30 PM, FremyCompany notifications@github.comwrote:

No, sorry, I read too fast. If you want the true mathematical modulo you want the formula you wrote indeed. However if you click on modulo in the web idl spec you get redirected to section 5.2 of ES6 which defines modulo as

The notation “x modulo y” (y must be finite and nonzero) computes a value k of the same sign as y (or zero) such that abs(k) < abs(y) and xk = q  y for some integer q.

So when they use modulo in webidl they mean remainder it seems. Am I reading right?

— Reply to this email directly or view it on GitHubhttps://github.com/extensibleweb/webidl.js/issues/5#issuecomment-12229395.

I think perhaps we should confirm with the relevant groups...?

Brian Kardell :: @briankardell :: hitchjs.com

FremyCompany commented 11 years ago

We should maybe report to cameron but it seems to me using % is what the spec intended.

marcoscaceres commented 11 years ago

I asked Cameron, and he said:

The algorithms from which #es-octet and others are copied are http://people.mozilla.org/~jorendorff/es6-draft.html#sec-9.1.7. There, the "modulo" operation takes on the sign of the rhs, not the lhs, which is what the JS % operator does. I will link "modulo" to the spec.

But we can email es-discuss to get confirmation.

bkardell commented 11 years ago

Francois, can you follow up with Cameron just to be sure. Either way like I think you are implying, good to do so and we can confirm

On Mon, Jan 14, 2013 at 12:31 PM, FremyCompany notifications@github.comwrote:

We should maybe report to cameron but it seems to me using % is what the spec intended.

— Reply to this email directly or view it on GitHubhttps://github.com/extensibleweb/webidl.js/issues/5#issuecomment-12229503.

Brian Kardell :: @briankardell :: hitchjs.com

marcoscaceres commented 11 years ago

agreed.

FremyCompany commented 11 years ago

The guys at ES discuss don't really care about web idl. If Cameron confirms, I see no reason not to trust him. Do you?

marcoscaceres commented 11 years ago

Ok, but make sure you phrase the question as yes/no (or of these two, which is right?). Cam tends to be vague in his responses (not on purpose, he just assumes a high degree of understanding - for me, this has been a problem in the past because I only pretend to know what I'm talking about :smile: ).

marcoscaceres commented 11 years ago

Blocked by: https://github.com/extensibleweb/webidl.js/issues/39

marcoscaceres commented 11 years ago

Submitted tests as part of: https://github.com/extensibleweb/webidl.js/pull/42

marcoscaceres commented 11 years ago

Needs code review: https://github.com/extensibleweb/webidl.js/blob/master/lib/types/Octet.js