IndexGet() of Array/ImmutableArray/Undefined types about tengo HOT 7 CLOSED

I'll experiment with array-returning-undefined scenario, just to make sure it doesn't have any other side effects, and, it works well with other language constructs. A couple other things I want to test with:

• String.IndexGet and Bytes.IndexGet
• Array.IndexSet, String.IndexSet, and, Bytes.IndexSet
• Slicing on Array, String, and Bytes

from tengo.

I think IndexSet would have to retain the current behavior of runtime error as it makes no sense to set an item at an invalid index. Perhaps we can allow empty index to append to the array, but that is a different conversation altogether.

I can think of three ways of handling the Slicing operator:

• out-of-bounds error
• Return only values within the valid range. For instance if low > max returns empty slice; low < max and high > max returns [low:max]
• Return Undefined for invalid range. I'm not sure it makes much sense to do this though.

The upside of leaving the runtime error for IndexGet makes it consistent with IndexSet behavior. Given that index is actually out of range, and arrays can't contain objects at any random index, this makes sense. That said, I think Undefined actually takes care of the problem quite well as the programmer can decide whether to crash out or not.

The other alternative is to create some kind of an operator to deal with this and leave the behavior as is now.

One possibility is the ?? operator that I was considering. It would require some way to catch runtime exceptions in the VM and deal with them. On the plus side, I suppose an exception handling mechanism would have to be built alongside it.

The other is something similar to optional unwrapping in Swift. Something like:

a := b?[1]?.c  // a == undefined

The presence of ? would have to be passed to the IndexGet and IndexSet methods so they could handle them as preferable. The syntax looks a little clunky but is readable and is probably simpler to deal with while keeping the current behavior.

from tengo.

I've been testing this myself and returning undefined on slicing on array/string requires concatenation for strings and bytes and the builtin functions len() and append() are changed to work with undefined.

It's not very intuitive to check whether the resulting slice is undefined before performing these actions.

Also:

a := [1, 2, 3]
b := a[1:5] // undefined here is nonintuitive

If the goal is to avoid runtime errors on slicing, then the more intuitive behavior would be to return the values within the range or when there are no values within the range, an empty slice.

from tengo.

Ok. I think we can try this:

• Sequence types: Array, ImmutableArray, String, Bytes
• IndexGet on sequence types: returns undefined if index is out of bounds
• IndexSet on Array type (other sequence types are immutable): raises a run-time error if index is out of bounds
• Slicing on sequence types: returns a new sequence of same type (except for ImmutableArray which returns Array) with the elements within the range (or empty sequence)
• IndexGet on Undefined type: always returns undefined

Let me know what you think @earncef.

from tengo.

Looks good but I'll run some tests and get back to you.

from tengo.

Ok, I found this while I was testing. In vm.go:650:

				if highIdx < 0 {
					highIdx = numElements
				} else if highIdx < 0 || highIdx > numElements {
					return fmt.Errorf("index out of bounds: %d", highIdx)
				}

Either the else if makes no sense or the if doesn't. Not sure what the expected behavior is. Should the highIdx be 0 or numElements in the changes we discussed? I'd assume it should be 0, but it's being set to numElements right now so I'm a bit confused. There are no tests checking this that I could find.

So:

a := [1, 2, 3][0:-1] //a = [1, 2, 3]

This also seems inconsistent given that the following results in an error:

a := [1, 2, 3][2:1] //a = error

I think the intuitive behavior would be that if highIdx < lowIdx then irrespective of anything else it would either:

• error out
• return empty slice

Setting highIdx to numElements is confusing.

from tengo.

Ah, I see what the code is doing. It's designed to handle the likes of

a := [1, 2, 3][:]

I think it might be better to rewrite that part without using highIdx as -1

from tengo.