New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reference type and implementation reality #467

Open
littledan opened this Issue Mar 9, 2016 · 16 comments

Comments

Projects
None yet
9 participants
@littledan
Member

littledan commented Mar 9, 2016

Does anyone have an idea of a resolution on this bug in the old repository?

https://bugs.ecmascript.org/show_bug.cgi?id=4379

Quoting @anba

Tests added to test262 [1] have revealed long-standing deviations [2] between the specification for the Reference type and actual implementation reality. Whether the specification should be changed to reflect implementation reality [3] or alternatively implementers should try to comply with specified behaviour should be discussed.

[1] Added/Proposed PRs for test262 coverage:
tc39/test262#91
tc39/test262#273
tc39/test262#275

[2] Mozilla source code from 1998 already shows the spec violation which is still present in modern engines. Details can be found in tc39/test262#273.

[3] The exact "implementation reality" still needs to be determined.

@ajklein

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Mar 18, 2016

Member

There is no resolution, and further the deviation continues to exist. I think if we want to change this it should be a full proposal because it is a core part of the semantics and changing this has been very controversial in the past. The difficult part in my mind is that the current spec seems correct and the biggest argment in favor of fixing this problem is that it's hard for implementations to do so. Maybe that's enough?

Member

bterlson commented Mar 18, 2016

There is no resolution, and further the deviation continues to exist. I think if we want to change this it should be a full proposal because it is a core part of the semantics and changing this has been very controversial in the past. The difficult part in my mind is that the current spec seems correct and the biggest argment in favor of fixing this problem is that it's hard for implementations to do so. Maybe that's enough?

@bterlson bterlson added the discussion label Mar 18, 2016

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Mar 22, 2016

Contributor

For reference, here's a couple of simple tests, with results on current (public) builds of various engines (ETA: and some older browsers). The current spec dictates that both should have hits = 1 immediately after execution; I report the value it actually attains.

var hits = 0, obj = {p: 0}, prop = {toString: function(){++hits; return 'p';}}; obj[prop]++;

  • SpiderMonkey: 1
  • v8: 2
  • WebkitCore: 2
  • ChakraCore: 2
  • Nashorn: 2
  • IE6: 1
  • IE8: 1
  • IE10: 2
  • Chrome 15: 2
  • Firefox 0.8: 1
  • Firefox 1.5: 1

var hits = 0, obj = {p: 0}, prop = {toString: function(){++hits; return 'p';}}; obj[prop]+=1;

  • SpiderMonkey: 2
  • v8: 2
  • WebkitCore: 2
  • ChakraCore: 2
  • Nashorn: 2
  • IE6: 1
  • IE8: 1
  • IE10: 2
  • Chrome 15: 2
  • Firefox 0.8: 2
  • Firefox 1.5: 2

Prefix and postfix increment and decrement all behave identically on a given platform, as do all of the compound assignment operators. Per the above, the only variation is that SpiderMonkey only performs ToPropertyKey once for increment/decrement. In all other cases, across this set of engines, ToPropertyKey is performed exactly twice, in defiance of spec.

ETA: I've added older versions of Chrome, Firefox, and Internet Explorer, in the interests of knowing how long-standing this is. IE6 and IE8, uniquely among the browsers I've tested, conform to the spec as it then stood and still stands.

Contributor

bakkot commented Mar 22, 2016

For reference, here's a couple of simple tests, with results on current (public) builds of various engines (ETA: and some older browsers). The current spec dictates that both should have hits = 1 immediately after execution; I report the value it actually attains.

var hits = 0, obj = {p: 0}, prop = {toString: function(){++hits; return 'p';}}; obj[prop]++;

  • SpiderMonkey: 1
  • v8: 2
  • WebkitCore: 2
  • ChakraCore: 2
  • Nashorn: 2
  • IE6: 1
  • IE8: 1
  • IE10: 2
  • Chrome 15: 2
  • Firefox 0.8: 1
  • Firefox 1.5: 1

var hits = 0, obj = {p: 0}, prop = {toString: function(){++hits; return 'p';}}; obj[prop]+=1;

  • SpiderMonkey: 2
  • v8: 2
  • WebkitCore: 2
  • ChakraCore: 2
  • Nashorn: 2
  • IE6: 1
  • IE8: 1
  • IE10: 2
  • Chrome 15: 2
  • Firefox 0.8: 2
  • Firefox 1.5: 2

Prefix and postfix increment and decrement all behave identically on a given platform, as do all of the compound assignment operators. Per the above, the only variation is that SpiderMonkey only performs ToPropertyKey once for increment/decrement. In all other cases, across this set of engines, ToPropertyKey is performed exactly twice, in defiance of spec.

ETA: I've added older versions of Chrome, Firefox, and Internet Explorer, in the interests of knowing how long-standing this is. IE6 and IE8, uniquely among the browsers I've tested, conform to the spec as it then stood and still stands.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 25, 2016

Member

@bakkot Would you be interested in writing up a proposal for a spec change here? Or do you prefer the semantics of the spec?

Member

littledan commented Mar 25, 2016

@bakkot Would you be interested in writing up a proposal for a spec change here? Or do you prefer the semantics of the spec?

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Mar 25, 2016

Contributor

@littledan - I much prefer the semantics in the spec. The situation in question seems similar to obj[f()]++, which surely should not call f twice.

Contributor

bakkot commented Mar 25, 2016

@littledan - I much prefer the semantics in the spec. The situation in question seems similar to obj[f()]++, which surely should not call f twice.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 29, 2016

Member

@bakkot No, f() would only be called once; it's ToPropertyKey on the result which is called multiple times.

Member

littledan commented Mar 29, 2016

@bakkot No, f() would only be called once; it's ToPropertyKey on the result which is called multiple times.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Mar 29, 2016

Contributor

@littledan, right, that's my objection to changing the spec. To the user it appears that there is a single operation being performed, which is getting the name of the property. For half of that step to be repeated seems overcomplicated and confusing. Since f obviously should not be (and is not) called more than once, nor should ToPropertyKey.

Contributor

bakkot commented Mar 29, 2016

@littledan, right, that's my objection to changing the spec. To the user it appears that there is a single operation being performed, which is getting the name of the property. For half of that step to be repeated seems overcomplicated and confusing. Since f obviously should not be (and is not) called more than once, nor should ToPropertyKey.

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Mar 29, 2016

Member

@bakkot Curious, considering that implementations have had this bug for two decades now, do you want the spec to continue to say something implementers aren't motivated to fix and might not be web-compatible, or would you prefer the spec to reflect reality of what implementers do and the web (and other code) may depend on?

Member

bterlson commented Mar 29, 2016

@bakkot Curious, considering that implementations have had this bug for two decades now, do you want the spec to continue to say something implementers aren't motivated to fix and might not be web-compatible, or would you prefer the spec to reflect reality of what implementers do and the web (and other code) may depend on?

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Mar 29, 2016

Contributor

@bterlson, I have no strong feelings either way.

I'd be surprised to learn anyone was particularly depending on the current behavior, though, both because of that one SpiderMonkey difference above and because I believe it's rare to have side-effecting toStrings. If that's the case, the best thing in my opinion would be for implementations to change to reflect the current spec; given, as you say, implementers maybe aren't going to bother, I'm largely ambivalent as to what the spec should say.

Contributor

bakkot commented Mar 29, 2016

@bterlson, I have no strong feelings either way.

I'd be surprised to learn anyone was particularly depending on the current behavior, though, both because of that one SpiderMonkey difference above and because I believe it's rare to have side-effecting toStrings. If that's the case, the best thing in my opinion would be for implementations to change to reflect the current spec; given, as you say, implementers maybe aren't going to bother, I'm largely ambivalent as to what the spec should say.

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Mar 29, 2016

Member

@bakkot, thanks this is helpful. For what it's worth, I don't think there is any (or much) disagreement that the spec text is currently better than how implementations behave. The problem is that this is a long-standing deviation implementations have and even for an edge case like this the likelihood of the web depending on this behavior seems high. The problem is that an implementation will have to proceed carefully under the assumption that this pattern exists on the web, and that's the hard issue to work around. I'm not sure any of us are willing to go through the motions for this.

Member

bterlson commented Mar 29, 2016

@bakkot, thanks this is helpful. For what it's worth, I don't think there is any (or much) disagreement that the spec text is currently better than how implementations behave. The problem is that this is a long-standing deviation implementations have and even for an edge case like this the likelihood of the web depending on this behavior seems high. The problem is that an implementation will have to proceed carefully under the assumption that this pattern exists on the web, and that's the hard issue to work around. I'm not sure any of us are willing to go through the motions for this.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Mar 29, 2016

Contributor

@bterlson: In the interests of knowing how likely it is that anyone is depending on this... it turns out that IE6 and IE8 got this right, that is, they conform to spec. I'll try to test some other browsers when I get a chance. ETA: Done; see updated table. IE8 conforms to spec, IE10 does not. The oldest versions of Chrome and Firefox I have easy access to do not conform.

Contributor

bakkot commented Mar 29, 2016

@bterlson: In the interests of knowing how likely it is that anyone is depending on this... it turns out that IE6 and IE8 got this right, that is, they conform to spec. I'll try to test some other browsers when I get a chance. ETA: Done; see updated table. IE8 conforms to spec, IE10 does not. The oldest versions of Chrome and Firefox I have easy access to do not conform.

@evilpie

This comment has been minimized.

Show comment
Hide comment
@evilpie

evilpie Mar 30, 2016

Contributor

Eric just fixed the += case in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1260577
I don't think it's likely that sites depend on some specific toString behavior here, at least I have never seen it. Especially considering IE had the correct behavior here.

Contributor

evilpie commented Mar 30, 2016

Eric just fixed the += case in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1260577
I don't think it's likely that sites depend on some specific toString behavior here, at least I have never seen it. Especially considering IE had the correct behavior here.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Mar 30, 2016

Member

Does anyone have an idea of a resolution on this bug in the old repository?

My recollection (I could be wrong) the last time TC39 discussed this issue at a meeting, we decided to retain the current (and legacy) spec. language. I meeting notes search (it may predate github notes) would be required to verify that.

Member

allenwb commented Mar 30, 2016

Does anyone have an idea of a resolution on this bug in the old repository?

My recollection (I could be wrong) the last time TC39 discussed this issue at a meeting, we decided to retain the current (and legacy) spec. language. I meeting notes search (it may predate github notes) would be required to verify that.

@lars-t-hansen

This comment has been minimized.

Show comment
Hide comment
@lars-t-hansen

lars-t-hansen Mar 30, 2016

Contributor

@bterlson, of historical interest: Opera 9 (March 2009) got this right (conforms to the spec), as did likely Opera versions going back to Opera 7 at least (ca 2004). Opera 12 gets it wrong. (I only tested the ++ case but IIRC this was all handled through some ToPropertyKey type operation in the bytecode for RMW ops in the older engine.)

Contributor

lars-t-hansen commented Mar 30, 2016

@bterlson, of historical interest: Opera 9 (March 2009) got this right (conforms to the spec), as did likely Opera versions going back to Opera 7 at least (ca 2004). Opera 12 gets it wrong. (I only tested the ++ case but IIRC this was all handled through some ToPropertyKey type operation in the bytecode for RMW ops in the older engine.)

@claudepache

This comment has been minimized.

Show comment
Hide comment
@claudepache

claudepache Apr 26, 2016

Contributor

Another interesting testcase (related to tc39/test262#273):

var hits = '';
var base = {};
var prop = function () { 
    hits += 'A';
    return { toString: function () { hits += 'B'; } };
}
var expr = function () { 
    hits += 'C';
}
base[prop()] = expr();
hits;

Expected: "ABC"
Actual: "ACB" (tested on current Chrome, Edge, Firefox, Opera, Safari)

Old Opera 9 had it right ("ABC"). Unable to test old IEs.

Changing the spec to match current implementations might be tricky, as it implies to split the algorithm in 12.3.2.1 in two parts: steps 1-4 produces "A" in my testcase, steps 5-8 produces "B".

Contributor

claudepache commented Apr 26, 2016

Another interesting testcase (related to tc39/test262#273):

var hits = '';
var base = {};
var prop = function () { 
    hits += 'A';
    return { toString: function () { hits += 'B'; } };
}
var expr = function () { 
    hits += 'C';
}
base[prop()] = expr();
hits;

Expected: "ABC"
Actual: "ACB" (tested on current Chrome, Edge, Firefox, Opera, Safari)

Old Opera 9 had it right ("ABC"). Unable to test old IEs.

Changing the spec to match current implementations might be tricky, as it implies to split the algorithm in 12.3.2.1 in two parts: steps 1-4 produces "A" in my testcase, steps 5-8 produces "B".

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Apr 27, 2016

Member

When this was discussed at TC39, it seemed like the resolution was to stick with the current spec, pending Mozilla shipping its implementation and demonstrating that it was web-compatible. Microsoft emphasized that fixing the bug on their side was low priority. @efaust How has shipping that patch gone? Do you pass the test that @claudepache mentioned?

Member

littledan commented Apr 27, 2016

When this was discussed at TC39, it seemed like the resolution was to stick with the current spec, pending Mozilla shipping its implementation and demonstrating that it was web-compatible. Microsoft emphasized that fixing the bug on their side was low priority. @efaust How has shipping that patch gone? Do you pass the test that @claudepache mentioned?

@domenic domenic added the web reality label Jul 28, 2016

@hax

This comment has been minimized.

Show comment
Hide comment
@hax

hax Feb 13, 2017

@claudepache IE6/IE8 return "ABC".

hax commented Feb 13, 2017

@claudepache IE6/IE8 return "ABC".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment