handle no `deleteCount` to `splice()` in Opera #465

tjenkinson · 2019-07-30T13:10:12Z

In Opera 10.6 and 9.8, when the fix is applied and deleteCount is missing, no elements are deleted.
This is incorrect - when deleteCount is missing the behavior should be to delete all elements from start onwards

In Opera 10.6 and 9.8, when the fix is applied and `deleteCount` is missing, no elements are deleted. This is incorrect - when `deleteCount` is missing the behavior should be to delete all elements from `start` onwards

ljharb

Which version(s) of Opera? If there's not already failing tests, can we add some?

tjenkinson · 2019-08-04T09:24:33Z

I can find out the version on Monday when I have access to the device again.

How should I test this? We need to force adding the implementation, which I didn't see done anywhere else

FYI I opened a related PR on typescript here: microsoft/TypeScript#32643

ljharb · 2019-08-05T04:02:33Z

I'm not sure what you mean by "force adding the implementation" - splice is force-replaced here, here, and here.

tjenkinson · 2019-08-05T07:47:19Z

I mean we want a test that tests

es5-shim/es5-shim.js

Lines 819 to 880 in e11b3f7

    
           splice: function splice(start, deleteCount) { 
        
               var O = ES.ToObject(this); 
        
               var A = []; 
        
               var len = ES.ToUint32(O.length); 
        
               var relativeStart = ES.ToInteger(start); 
        
               var actualStart = relativeStart < 0 ? max((len + relativeStart), 0) : min(relativeStart, len); 
        
               var actualDeleteCount = min(max(ES.ToInteger(deleteCount), 0), len - actualStart); 
        
               var k = 0; 
        
               var from; 
        
               while (k < actualDeleteCount) { 
        
                   from = $String(actualStart + k); 
        
                   if (owns(O, from)) { 
        
                       A[k] = O[from]; 
        
                   } 
        
                   k += 1; 
        
               } 
        
               var items = arraySlice(arguments, 2); 
        
               var itemCount = items.length; 
        
               var to; 
        
               if (itemCount < actualDeleteCount) { 
        
                   k = actualStart; 
        
                   var maxK = len - actualDeleteCount; 
        
                   while (k < maxK) { 
        
                       from = $String(k + actualDeleteCount); 
        
                       to = $String(k + itemCount); 
        
                       if (owns(O, from)) { 
        
                           O[to] = O[from]; 
        
                       } else { 
        
                           delete O[to]; 
        
                       } 
        
                       k += 1; 
        
                   } 
        
                   k = len; 
        
                   var minK = len - actualDeleteCount + itemCount; 
        
                   while (k > minK) { 
        
                       delete O[k - 1]; 
        
                       k -= 1; 
        
                   } 
        
               } else if (itemCount > actualDeleteCount) { 
        
                   k = len - actualDeleteCount; 
        
                   while (k > actualStart) { 
        
                       from = $String(k + actualDeleteCount - 1); 
        
                       to = $String(k + itemCount - 1); 
        
                       if (owns(O, from)) { 
        
                           O[to] = O[from]; 
        
                       } else { 
        
                           delete O[to]; 
        
                       } 
        
                       k -= 1; 
        
                   } 
        
               } 
        
               k = actualStart; 
        
               for (var i = 0; i < items.length; ++i) { 
        
                   O[k] = items[i]; 
        
                   k += 1; 
        
               } 
        
               O.length = len - actualDeleteCount + itemCount; 
        
               return A; 
        
           }

But that code only executes based on some of the other conditions

tjenkinson · 2019-08-05T08:40:51Z

It's Opera 9.8

ljharb · 2019-08-09T06:48:38Z

@tjenkinson thanks!

As for a test, anything that invokes .splice and omits deleteCount seems like it would test this change?

tjenkinson · 2019-08-09T06:57:14Z

But it would only test the change on something that has one of these issues right?

es5-shim/es5-shim.js

Line 881 in e11b3f7

}, !spliceWorksWithLargeSparseArrays || !spliceWorksWithSmallSparseArrays);

Would the tests run on a browser old enough to have these issues?

ljharb · 2019-08-09T06:58:13Z

CI won't, but I can manually run them against that version of Opera :-)

tjenkinson · 2019-08-09T07:07:57Z

es5-shim/tests/spec/s-array.js

Lines 1364 to 1370 in e11b3f7

    
                   /* This test is disabled, because ES6 normalizes actual 
        
                    * browser behavior, contradicting ES5. 
        
                    */ 
        
                   xit('treats undefined deleteCount as 0', function () { 
        
                       expect(test.splice(0).length).toBe(0); 
        
                       expect(test.splice(0)).toEqual(test.splice(0, 0)); 
        
                   });

:D

tjenkinson · 2019-11-29T15:28:19Z

hi @ljharb any chance this could be merged soon?

ljharb · 2019-11-30T07:23:52Z

Hmm, looks like the oldest Opera I can test in is 10.6, but the test seems to fail there.

ljharb · 2019-11-30T07:25:11Z

It seems tho, like your fix ends up causing 2 additional failures that weren't there before.

tjenkinson · 2019-12-01T19:54:56Z

:/ Which tests failed for you?

ljharb · 2019-12-02T07:36:31Z

should return right result 3
Expected [ '~0', '~ 1', '~ 2', 'F1', 'P', 'LLL', 'CCC', 'YYY', 'XXX', 'F7', 'F8', 'F9', 'F10', 'F11', 'F12', 'F13', 'F14', 'F15', 'F16', 'F17', 'F18', 'F19', 'F20', 'F21', 'F22', 'F23', 'F24', 'F25', 'F26', 1, 23, 4, 5, 6, 7, 8, '~ 4', '~ 5', '~ 6', '~ 7', 7, 8, 9, 10, 11, 2, 4, 5, 6, 7, 8, 9, 'CCC', 'YYY', 'XXX', 'F7', 'F8', 'F9', 'F10', 'F11', 'F12', 'F13', 'F14', 'F15', 'F16', 'F17', 'F18', 'F19', 'F20', 'F21', 'F22', 'F23', 'F24', 'F25', 'F26', 1, 23, 4, 10, 1, 2, 3, 4, 5, 6, 7, 8, 9, 1, 2, 3 ] to equal [ '~0', '~ 1', '~ 2', 'F1', 'P', 'LLL', 'CCC', 'YYY', 'XXX', 'F7', 'F8', 'F9', 'F10', 'F11', 'F12', 'F13', 'F14', 'F15', 'F16', 'F17', 'F18', 'F19', 'F20', 'F21', 'F22', 'F23', 'F24', 'F25', 'F26', 1, 23, 4, 5, 6, 7, 8, '~ 4', '~ 5', '~ 6', '~ 7', 7, 8, 9, 10, 11, 2, 4, 5, 6, 7, 8, 9, 'CCC', 'YYY', 'XXX', 'F7', 'F8', 'F9', 'F10', 'F11', 'F12', 'F13', 'F14', 'F15', 'F16', 'F17', 'F18', 'F19', 'F20', 'F21', 'F22', 'F23', 'F24', 'F25', 'F26', 1, 23, 4, 9, 10, 1, 2, 3, 4, 5, 6, 7, 8, 9, 'YYY', 'XXX', 'F6', 'F7', 'F8', 'F9', 'F10', 'F11', 'F12', 'F13', 'F14', 'F15', 'F16', 'F17', 'F18', 'F19', 'F20', 'F21', 'F22', 'F23', 'F24', 'F25', 'F26', 3, 4, 5, 6, 7, 8, 9, '-0', '- 1', '- 2', '- 3', '- 4', '- 5', '- 6', '- 7', 1, 2, 3 ].
run
should do nothing if method called with no arguments
Expected [ 1, 'a', [ 'b' ] ] to equal [ ].
Expected [ ] to equal [ 1, 'a', [ 'b' ] ].

ljharb · 2020-01-07T07:29:58Z

@tjenkinson i know it's been awhile, but is there any chance you're interested in completing this PR?

tjenkinson · 2020-01-07T07:44:12Z

@ljharb I am, but I'm not sure when I'm going to get time at the moment. Will close for now and maybe reopen later

ljharb · 2020-01-07T07:46:55Z

I'd rather keep it open in the meantime so I can track it :-) please return to it when you get time.

tjenkinson · 2020-01-07T19:46:05Z

@ljharb I am seeing a lot of failures like this, even on master

  32) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  33) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  34) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  35) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  36) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  37) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  38) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

any idea why?

ljharb · 2020-01-07T19:48:40Z

In Opera specifically? Which version, I can probably fix those.

tjenkinson · 2020-01-07T19:49:36Z

This is just from running npm test on e938fda

ljharb · 2020-01-07T19:50:43Z

On which node version? travis-ci passes on a great many node versions: https://travis-ci.com/es-shims/es5-shim/builds/143354113

tjenkinson · 2020-01-07T19:51:16Z

v11.13.0

ljharb · 2020-01-07T19:51:36Z

https://travis-ci.com/es-shims/es5-shim/jobs/272983149 passes, i'll try locally.

tjenkinson · 2020-01-07T19:56:31Z

Weird. I wonder if it's related to my timezone somehow? I'm GMT+0100 (Central European Standard Time) at the moment

ljharb · 2020-01-07T20:00:49Z

interesting, i see those failures locally too, and i'm in UTC-7

tjenkinson · 2020-01-07T20:01:43Z

Ah when I change my timezone to London I get Error: Expected 44 to be 59., so it looks like it's having an effect

tjenkinson · 2020-01-07T20:43:00Z

hopefully with the last few commits the tests will be fixed now 🤞

ljharb · 2020-01-07T21:33:19Z

It appears that old v8, old spidermonkey, jsc, and xs all print out 59 for this test; new v8, new spidermonkey, and chakra all print out 1, so i'm looking into whether it's a spec change, or a browser bug.

tjenkinson · 2020-01-08T07:28:20Z

Are you referring to the date test or splice?

ljharb · 2020-01-08T07:34:59Z

The date test :-) i'll take a look at your splice changes soon.

tjenkinson · 2020-03-22T11:45:57Z

bump!

ljharb · 2020-03-22T17:18:47Z

OK, with your test but without your fix, Opera 10.6 has 10 failures; with your fix, it has 8, none of which are related to splice. Let's get this in :-)

tjenkinson · 2020-03-22T20:13:44Z

🎉 thanks

[Fix] handle no deleteCount to splice() in Opera

f0dd919

In Opera 10.6 and 9.8, when the fix is applied and `deleteCount` is missing, no elements are deleted. This is incorrect - when `deleteCount` is missing the behavior should be to delete all elements from `start` onwards

ljharb added ES3 browsers noncompliant ES5 labels Aug 3, 2019

ljharb requested changes Aug 3, 2019

View reviewed changes

tjenkinson closed this Jan 7, 2020

ljharb reopened this Jan 7, 2020

ljharb force-pushed the fix-splice-opera branch from 21d6d82 to f0dd919 Compare March 22, 2020 17:19

ljharb approved these changes Mar 22, 2020

View reviewed changes

ljharb merged commit f0dd919 into es-shims:master Mar 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle no `deleteCount` to `splice()` in Opera #465

handle no `deleteCount` to `splice()` in Opera #465

tjenkinson commented Jul 30, 2019 •

edited by ljharb

Loading

ljharb left a comment

tjenkinson commented Aug 4, 2019

ljharb commented Aug 5, 2019

tjenkinson commented Aug 5, 2019

tjenkinson commented Aug 5, 2019

ljharb commented Aug 9, 2019

tjenkinson commented Aug 9, 2019

ljharb commented Aug 9, 2019

tjenkinson commented Aug 9, 2019

tjenkinson commented Nov 29, 2019

ljharb commented Nov 30, 2019

ljharb commented Nov 30, 2019

tjenkinson commented Dec 1, 2019

ljharb commented Dec 2, 2019

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 8, 2020

ljharb commented Jan 8, 2020

tjenkinson commented Mar 22, 2020

ljharb commented Mar 22, 2020

tjenkinson commented Mar 22, 2020

handle no deleteCount to splice() in Opera #465

handle no deleteCount to splice() in Opera #465

Conversation

tjenkinson commented Jul 30, 2019 • edited by ljharb Loading

ljharb left a comment

Choose a reason for hiding this comment

tjenkinson commented Aug 4, 2019

ljharb commented Aug 5, 2019

tjenkinson commented Aug 5, 2019

tjenkinson commented Aug 5, 2019

ljharb commented Aug 9, 2019

tjenkinson commented Aug 9, 2019

ljharb commented Aug 9, 2019

tjenkinson commented Aug 9, 2019

tjenkinson commented Nov 29, 2019

ljharb commented Nov 30, 2019

ljharb commented Nov 30, 2019

tjenkinson commented Dec 1, 2019

ljharb commented Dec 2, 2019

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

tjenkinson commented Jan 7, 2020

ljharb commented Jan 7, 2020

tjenkinson commented Jan 8, 2020

ljharb commented Jan 8, 2020

tjenkinson commented Mar 22, 2020

ljharb commented Mar 22, 2020

tjenkinson commented Mar 22, 2020

handle no `deleteCount` to `splice()` in Opera #465

handle no `deleteCount` to `splice()` in Opera #465

tjenkinson commented Jul 30, 2019 •

edited by ljharb

Loading