toLeopard: List splice problems ("delete item of list")

[towerofnix]

The following three blocks...

1. delete 0 / 0 of list
2. delete 0 of list
3. delete 4 of list

...are converted into the following three(-ish) scripts:

1. Depends:
   • <list>.splice(this.toNumber(0 / 0) - 1, 1) w/ traits or strict NaN; this is equivalent to <list>.splice(-1, 1)
   • <list>.splice(0 / 0 - 1, 1) in curr. Leopard; this is equivalent to <list>.splice(NaN, 1), which JavaScript treats the same as <list>.splice(0, 1)
2. <list>.splice(-1, 1) (0 - 1 = -1)
3. <list>.splice(3, 1) (4 - 1 = 3)

This is trouble, because splice wraps behind the start. To summarize the real effects:

1. Depends:
   • w/ traits or strict NaN: .splice(-1, 1) is the same as .pop()
   • w/ curr. Leopard: .splice(NaN, 1) is the same as .shift()
2. .splice(-1, 1) is the same as .pop()
3. .splice(3, 1) works correctly - splice only wraps negative numbers, it does nothing if the positive number is beyond the list's length. Or it deletes the fourth item, of course!

In Scratch, specifying zero or a negative number for the index to delete means no item will be deleted at all - the list will be left as it is. Of course, we're not reflecting that here, and this makes for project incompatibility.

---

The question is, what approach do we want to take to fix this?

The easiest is to just add a new function in Leopard specifically for "delete item of". For example, this.deleteItem(this.stage.vars.list, -1) would correctly do nothing. We already have functions that help us deal with lists, indexInArray and itemOf. So maybe adding a new function isn't so bad? We're already missing out on indexOf, and even plain old array[index] item accessing.

Of course, it'd be nice to just get to use splice and indexOf and normal item accessing (and even shift and pop). I feel like some of it must be possible - at least making certain combinations of blocks convert more nicely, even if we can't arbitrarily convert all scripts into the nicest form. Some of this might depend on inspecting the shape/traits/even opcode of a contained block (#98), some might be possible just with smarter use of desired traits (#150). We'd like to work it out but right now it feels, uh, to put it simply, too intimidating to treat as a high-pressure project — better to implement a simple if clumsy, universally applicable fix (this.deleteItem) and work out nicer, more particular solutions after.

@PullJosh Mainly we've assigned you asking for a blessing on adding deleteItem to Leopard, and checking if you think a different name would be better.

Does it make sense for the args to be (1. array, 2. index), sort of like array.splice(index), or should they be flipped to line up closer with Scratch? I think of it as "delete an item of (array) at (index)"... of course, I'm used to that because I use splice and I usually specify child things after parent things... but, I figure that if we can't use splice, we may as well get closer to it in our custom syntax, right? But there's a case to be made that dot syntax (list.splice(index)) is fundamentally different from "act on the thing I've passed" syntax (splice(list, index)), so if you feel a different name or argument order is better, we probably wouldn't argue with it much.

Thanks for your time taking a close look at this fairly awkward corner in the guts of Leopard project conversion!

[PullJosh]

This is a good catch. I don't see a better solution than adding a this.deleteItem(arr, index) helper method.

I still think it would be nice to prefer the native JS .splice() function whenever we can determine that it will definitely work correctly. (I might even prefer .pop() and .shift() where appropriate. Not sure how challenging the implementation for pop would be.)

[towerofnix]

Thanks, sounds good. Totally agreed that .splice() where possible is preferred. The difficulty ATM is that it's a more complex communication between the input (e.g. a reporter) and the containing block ("delete item of list")... at the moment (once merged) we use desired traits only to declare what kind of result we need, generally as a result of casting. But casting is only sufficient here if we cast to Infinity, and that includes casting negative numbers to Infinity, which could get ugly. I'm not sure it's worth the additional syntax around this.toNumber().

However, that doesn't mean we're totally out of luck for opportunistic splice(). It just means casting won't get us there (nicely).

What we can do instead is have desired input traits NonNegative and BlankIfWouldCast. These have to be used together (and alongside IsNumber of course), if we don't implement a cast for non-negatives (would be CastToInfinity). But that's OK! The point is that blockToJS (thus inputToJS) would return "" if it can't guarantee the value is a non-negative number without casting. Then the consuming block, "delete item of list", detects the input is "", and uses a completely alternate form based around this.deleteItem() instead.

(I'm suggesting a blank string rather than null because, uh, TypeScript. It would be a pain in the rear if the return type were suddenly string | null.)

---

I think pop() and shift() are both easy to implement, because they always correspond to specific block forms that we should be able to detect pretty easily:

• delete (length of (list)) of (list) where both blocks have the same list input
  • or delete (last) of (list), which technically exists, sort of
• delete (1) of (list)

Ideally, we can do .splice(0, 1) if you prefer that to shift(), though.

This would all be in a follow-up PR after the basic, "always use this.deleteItem()" implementation, so that the more complex code for niceness is supplemental to the simple compatibility fix.

[PullJosh]

I just realized that we have a this.letterOf(str, index) method in Leopard, which I think supports adding a consistently-designed this.deleteItem(arr, index) method.