486340 - Vector.splice deleteCount (2nd) parameter does not follow documentation

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Chris Peyer]

The vector documentation for the 2nd splice parameter states:
deleteCount:uint — An integer that specifies the number of elements to be deleted. ... If you do not specify a value for the deleteCount parameter, the method deletes all of the values from the startIndex element to the last element in the Vector. ...

However not specifying the deleteCount parameter results in the vector not being modified:
var vSplice3:Vector.<int> = Vector.<int>([0,1,2,3,4,5,6,7,8,9]);
vSplice3.splice(3);
trace(vSplice3);

Actual Result:
Vector is not modified: 0,1,2,3,4,5,6,7,8,9

Expected Result:
Vector is modified: 0,1,2,3

Comment 1

[Tom Harwood]

Attachment: Handles omitted, non-numeric, and negative deleteCount values

Passed the deleteCount parameter as * so it can be compared to undefined -- passing as Number and looking for NaN causes splice(x,"zort",...) to delete to end-of-Vector, which doesn't seem right since converting "zort" to uint yields 0.  Negative deleteCount numbers are slightly less clear-cut; in some ways, throwing a RangeError is more pedantically correct, but deleting -2 elements can't add 2 elements out of the bit bucket, so coercing the value to zero seems like the least surprising strategy.

Comment 2

[Erik Tierney]

Comment on attachment 403567 [details] [diff] [review]
Handles omitted, non-numeric, and negative deleteCount values

Looks good.

Comment 3

[Lars T Hansen]

(In reply to comment #1)
> Created an attachment (id=403567) [details]
> Handles omitted, non-numeric, and negative deleteCount values
> 
> Passed the deleteCount parameter as * so it can be compared to undefined --
> passing as Number and looking for NaN causes splice(x,"zort",...) to delete to
> end-of-Vector, which doesn't seem right since converting "zort" to uint yields
> 0.  Negative deleteCount numbers are slightly less clear-cut; in some ways,
> throwing a RangeError is more pedantically correct, but deleting -2 elements
> can't add 2 elements out of the bit bucket, so coercing the value to zero seems
> like the least surprising strategy.

If deleteCount is not uint - and I believe it /should/ be typed as uint, with a default value of 0, but that's just a possibly uninformed opinion, and I know AS3 conversion can cause trouble - but *, then we should follow ES3 / ES5 on the semantics, or, if our Array semantics are different, follow our own Array semantics.

BTW the behavior in ES is "Let actualDeleteCount be min(max(ToInteger(deleteCount),0), len – actualStart)", ie, it's clamped below to zero, as you've decided here.  I'm just pointing out that it's better to justify it as "because ES does it" than "it seems like the least surprising strategy", if given the choice.

Comment 4

[Edwin Smith]

Comment on attachment 403567 [details] [diff] [review]
Handles omitted, non-numeric, and negative deleteCount values

removing SR? until above comments are addressed.  should deleteCount be uint?

Comment 5

[Tom Harwood]

(In reply to comment #4)

> should deleteCount be uint?

Is there a way to detect a missing uint parameter?  All the values for uint are in the range of valid values.  I suppose we could have a uint parameter that defaults to uint.MAX_VALUE on the principle that this should take care of deleting to the end of the Vector.

Comment 6

[Edwin Smith]

if the behavior of missing-delete-count is the same as the behavior of delete-count = constant (0?) then the default can be that constant.

but if the behavior of a missing parameter can't be simulated by picking a particular constant then you have to do something else.

Comment 7

[Tom Harwood]

(In reply to comment #3)

> I believe [deleteCount] /should/ be typed as uint, with a
> default value of 0

This is the existing behavior, more or less (modulo conversion of a NaN to uint).  As this defect notes, this behavior is incorrect.

Defaulting deleteCount to uint.MAX_INT would be an acceptable dodge, except that the following parameter is an Array and it needs a compile-time constant initializer, which isn't an Array literal.

Submit that the original patch, although somewhat klunky, is the more robust solution.

Comment 8

[Tom Harwood]

Comment on attachment 403567 [details] [diff] [review]
Handles omitted, non-numeric, and negative deleteCount values

In the absence of other comments, I suggest we go with the fix as originally coded.

Comment 9

[Lars T Hansen]

Arguably the documentation is wrong.  In ES3 the deleteCount is not optional (but of course it will be provided as undefined if not provided by the program).  Undefined is then converted to integer yielding 0, the result will be to delete no elements.

Where is the documentation?

Comment 10

[Tom Harwood]

(In reply to comment #9)

> Where is the documentation?

http://help.adobe.com/en_US/AS3LCR/Flash_10.0/Vector.html#splice%28%29

Changing the doc does have the best backwards compatibility.

Comment 11

[Lars T Hansen]

I think I see what's going on.  Firefox distinguishes between the case where undefined is passed explicitly (in this case it ends up as deleteCount=0) and where no argument is passed, in which case it is interpreted as deleteCount=length-start, more or less.  I was misled by the ECMAScript spec, which does not allow deleteCount not to be passed.

Given that, our documentation is probably correct, and presumably your fix is correct as well.

Comment 12

[Tom Harwood]

(In reply to comment #11)
> [...] distinguishes between the case where undefined is passed explicitly 
> (in this case it ends up as deleteCount=0) and where no argument is passed,
> in which case it is interpreted as deleteCount=length-start, more or less. 

Ouch.  The candidate fix wouldn't handle explicitly passing undefined correctly.
What the language really needs is a metadata structure that has a definite answer to the "is this argument missing" question.

Cancelling SR again to see if the uint.MAX_VALUE solution can be made workable.

Comment 13

[Tom Harwood]

Comment on attachment 403567 [details] [diff] [review]
Handles omitted, non-numeric, and negative deleteCount values

Superseded by uint-typed patch.

Comment 14

[Tom Harwood]

Attachment: Type of deleteCount made explicitly uint, inappropriate use of clamp() replaced

One unpleasant language feature and one surprise in this patch.

- Unpleasant feature: uint(negativeNumber) conversion yields a large unsigned number, e.g., foo.splice(0,-1,"foo") leaves the Vector containing ["foo"].  But that is what the routine's types say it should do.

- Surprise: Using clamp() to compute the count doesn't work under any circumstances, because clamp() yields a value to be used as an index, not a count.

Comment 15

[Tom Harwood]

Comment on attachment 404074 [details] [diff] [review]
Type of deleteCount made explicitly uint, inappropriate use of clamp() replaced

Pushed 2675:1d44c74fcf85

Comment 16

[Chris Peyer]

Media added: http://hg.mozilla.org/tamarin-redux/rev/2705

Comment 18

[Trevor Baker]

This needs to be reverted since due to lack of swf versioning.

Comment 19

[Lars T Hansen]

Still can't land until versioning stuff (bug #535770) is in place.

Comment 20

[Dan Smith]

Assigning to STeven for landing.

Comment 21

[Dan Smith]

Transfering from Steven to Jason for landing in Brannan with appropriate versioning check.

Comment 22

[Jason Boyer]

Attachment: Modified patch with version checking and backward compatibility

There did not seem to be a way to determine that a uint arg was missing, though this could be due to my unfamiliarity with ActionScript.  I left the deleteCount args as type Number, and then checked in VectorImpl.as whether the arg is undefined.  Then, based on the bug compatibility flag, splice will chop off the elements from start (SWF 15+) or leave the vector alone (legacy behavior).

The test case for this bug uses the public::splice method when testing the missing arg, as the AS3::splice version throws an ArgumentError #1063. There's an additional test case for that behavior.

Comment 23

[Lars T Hansen]

I need to look at this some more - and re-read the bug at least once more, not to mention the user-facing documentation and the relevant specs - before deciding what to do with it.

As deleteCount is typed as Number it will never, ever hold the value "undefined", thus the comparison with undefined is wrong.  At a minimum you should be comparing with the number to which undefined converts, ie, NaN, which you're already doing.

It seems to me that the primary purpose of this bug needs to be not to munge the code, but to figure out what the proper API of Vector.splice is.  (Then the munging should not be so hard.)

Comment 24

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Lars T Hansen from comment #23)
> It seems to me that the primary purpose of this bug needs to be not to munge
> the code, but to figure out what the proper API of Vector.splice is.  (Then
> the munging should not be so hard.)

Interestingly, the current Adobe docs for Vector.AS3::splice claim that it takes a uint, which does not match the original Vector code nor the current state of that code.

  http://tinyurl.com/docs-vector-splice

(The linked content is presumably transient, since the docs are updated periodically.)

I'm guessing that the docs are regenerated at some middle-ground frequency where the frequency is high enough that it grabbed the change from rev 2675 that introduced deleteCount : uint, but the frequency is low enough that it has not grabbed the reversion from rev 3254.  (But is that plausible, or is something else going on with the documentation service?)

Comment 25

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #24)
> (In reply to Lars T Hansen from comment #23)
> > It seems to me that the primary purpose of this bug needs to be not to munge
> > the code, but to figure out what the proper API of Vector.splice is.  (Then
> > the munging should not be so hard.)
> 
> Interestingly, the current Adobe docs for Vector.AS3::splice claim that it
> takes a uint, which does not match the original Vector code nor the current
> state of that code.

Oh, this is not interesting after all; this is just a subset of what cpeyer said in the original description for this bug.  I have no idea how our docs for Vector are generated, but I guess they've always claimed that deleteCount is a uint, thus leading to e.g. comment 10.

Comment 26

[Lars T Hansen]

Note: Since Vector is final and cannot be subclassed (also a bug in the documentation -- it talks about what the signature must look like if overriding splice in a subclass), and as AS3 does not have function types, we can change the signature of the splice method if we want without versioning that change.

Comment 27

[Lars T Hansen]

It seems to me that the method adopted by Firefox for Array.prototype.splice, which we have also adopted for that method and more notably also for AS3::splice on Array, is the most reasonable:

- At least one argument must be supplied, it is coerced to Integer (ie represented
  as a double but without a fractional part, *not* as int) and clamped as an
  index to uint.  This all happens on the C++ side.

- If the second argument is present it is coerced to Integer and clamped: if
  less than zero it becomes zero; otherwise it is coerced to uint32_t in the
  standard manner.

- If the second argument is not present it is interpreted as length-start, ie,
  we delete to the end of the array

- Additional arguments beyond the second are inserted and can be any type.

The problem is that it is gnarly to express the "optional but has no known default value" idiom in AS3, especially in a way that's sensible to the common reader.  If we look at the code for Array, the signature is simply:

  AS3 function splice(...args)

and then the argument list is parsed from that.  However the Array documentation does not say this, instead it uses this signature:

  AS3 function splice(start:int, deleteCount:uint, ...values)

and then has some florid language about what happens if deleteCount is not present.  Very notably it does *not* provide a default value for deleteCount.

The use of rest arguments for all the parameters of Array.prototype.splice and Array.AS3::splice is unfortunate because it gives us worse error checking in strict mode than we could have had, but since Array is not final there's nothing to be done about it - changing signatures now would break working code.

Here is what I think should happen for Vector, then:

- We should spec Vector's splice like we've specified Array's splice: two typed
  arguments and a rest argument, but with *no* default value for deleteCount, and
  *with* the florid language about the absent parameter being interpreted a 
  certain way.

- The spec's language about possible Vector subclasses needs to be removed, it
  seems to have been copied from the Array spec.

- We should implement the method using the signature (start:int, ...rest) and
  then we should parse "rest" to get the rest of the parameters and to
  distinguish between an absent deleteCount and one that's present but of some
  run-time type.  This gives us better error checking than for Array, yet is
  easy to implement.

Comment 28

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 559287 [details] [diff] [review]
Modified patch with version checking and backward compatibility

(removing self from request queue; I'll wait for the next patch.)

Comment 29

[Jason Boyer]

(In reply to Lars T Hansen from comment #27)
> - We should implement the method using the signature (start:int, ...rest) and
>   then we should parse "rest" to get the rest of the parameters and to
>   distinguish between an absent deleteCount and one that's present but of
> some
>   run-time type.  This gives us better error checking than for Array, yet is
>   easy to implement.

This has the effect of changing the behavior in AS3 mode. Currently an ArgumentError: Error #1063 will be thrown when the 2nd argument is missing in AS3 mode. Should we maintain that behavior for legacy swfs, on the off chance that someone is relying on it?

Comment 30

[Lars T Hansen]

(In reply to Jason Boyer from comment #29)
> (In reply to Lars T Hansen from comment #27)
> > - We should implement the method using the signature (start:int, ...rest) and
> >   then we should parse "rest" to get the rest of the parameters and to
> >   distinguish between an absent deleteCount and one that's present but of
> > some
> >   run-time type.  This gives us better error checking than for Array, yet is
> >   easy to implement.
> 
> This has the effect of changing the behavior in AS3 mode. Currently an
> ArgumentError: Error #1063 will be thrown when the 2nd argument is missing
> in AS3 mode. Should we maintain that behavior for legacy swfs, on the off
> chance that someone is relying on it?

What you're saying is that an existing SWF that calls AS3::splice without the second argument will now experience the lack of an error.  (I'm only clarifying, as "AS3 mode" has nothing to do with it, strictly speaking: code compiled without -AS3 can also call the AS3 methods but has work harder to set those calls up.)

I guess that what you're suggesting is that when we get into AS3::splice, if ...rest is empty and bugzilla(whatever) is false, then we throw an error to simulate the old behavior where that argument was required.  Personally I almost think that's too much, but it is maximally backwards compatible, and it's probably best to do it.

Comment 31

[Lars T Hansen]

One more observation.  Earlier I wrote:

  class Vector ...
  {
    AS3 function splice(start:int, ...rest) ...

The reason I wrote that was to give strict mode something to work with.  However, this will mean that any Number argument given to splice will be coerced to int, and then clamped to an index; the Array code, with the signature splice(...rest), does it differently, it converts to an Integer and then clamps to uint.  That yields different and more predictable results.  Consider passing 2^32 for the "start" argument.  In the Array implementation this eventually ends up being clamped to the length of the Array.  In the Vector implementation it will be zero.

There's a tension here between strict-mode type checking on the one hand (more typed parameters == better) and desirable implementation (late coercion == better).  For that reason, I think the type of "start" needs to be Number, not int, so that we can correctly do the clamping inside the implementation.  This will yield equally good type checking _and_ the correct behavior.

Comment 32

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: Add doc comments for varied splice helper functions

Lars: I got annoyed by the lack of docs for _spliceHelper and friends.  So, please confirm that this doc comment (which is identical in the three spots apart from alpha-renaming) correctly describes the intended specification.

If you don't like having "redundant" copies of the doc comment, I can live with adding only one of these -- in that case, I'd vote to keep the VectorClass.h variant because it and the one in VectorImpl.as need it more than the one on avmplusList.h.

Comment 33

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch D v2: doc comments for varied splice helper functions

(fix poor word choice; "extracted from args" is more likely to be interpreted as "deleted from args" than as "loaded from args")

Comment 34

[Lars T Hansen]

Comment on attachment 565544 [details] [diff] [review]
patch D v2: doc comments for varied splice helper functions

I shudder at the all-over-the-map naming and capitalization but that is not your fault.

Comment 35

[Tamarin Bot]

changeset: 6630:5afed71b114c
user:      Felix S Klock II <[e-mail]>
summary:   Bug 486340: drive-by doc addition for varied splice helper functions (r=lhansen).

http://hg.mozilla.org/tamarin-redux/rev/5afed71b114c