Introduce reverseInPlace in Variable/Fixed size Array

[darkdrag00nv2]

Work towards #2605

Description

Introduce reverseInplace function for in-place reversal of a Variable/Fixed size Array value.

---

• Targeted PR against master branch
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work
• Code follows the standards mentioned here
• Updated relevant documentation
• Re-reviewed Files changed in the Github PR explorer
• Added appropriate labels

[codecov]

Codecov Report

Merging #2626 (5056603) into master (5f84e78) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2626   +/-   ##
=======================================
  Coverage   78.60%   78.61%           
=======================================
  Files         336      337    +1     
  Lines       77869    77901   +32     
=======================================
+ Hits        61207    61239   +32     
  Misses      14379    14379           
  Partials     2283     2283           

Flag	Coverage Δ
unittests	78.61% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
runtime/interpreter/value.go	68.85% <100.00%> (+0.01%)	⬆️
runtime/sema/type.go	89.57% <100.00%> (+0.03%)	⬆️

... and 2 files with indirect coverage changes

[turbolent]

Nice!

[turbolent]

Could you please also add test cases where the elements are structs and resources?

[darkdrag00nv2]

Hmm, thanks for pointing it out.

When I added the test case for struct, it doesn't work.

pub struct TestStruct {
  pub var test: Int

  init(_ t: Int) {
	self.test = t
  }
}

fun reverseStructArray(): [Int] {
  let sa = [TestStruct(1), TestStruct(2), TestStruct(3)]
  sa.reverse()
  
  let res: [Int] = [];
  for s in sa {
    res.append(s.test)
  }

  return res
}

I get error: member test is used before it has been initialized. So it seems like using Get and Set messes up the struct members.

[darkdrag00nv2]

By adding transfer call, I was able to get it to work for struct.

leftValue := v.Get(interpreter, locationRange, leftIndex)
rightValue := v.Get(interpreter, locationRange, rightIndex)

leftValue = leftValue.Transfer(
	interpreter,
	locationRange,
	v.array.Address(),
	true,
	nil,
)
rightValue = rightValue.Transfer(
	interpreter,
	locationRange,
	v.array.Address(),
	true,
	nil,
)

v.Set(interpreter, locationRange, leftIndex, rightValue)
v.Set(interpreter, locationRange, rightIndex, leftValue)

It doesn't work for resource because after the first Set call, we end up with two resources at the leftIndex which returns the error // error: two parents are captured for the slab.

I'll look further into how we swap resources atomically. Another option might be to just not support reverse for resource typed arrays. Most of the array functions do that already.

[SupunS]

So the problem is, structs and resources behave differently when "Transferred". Structs are copied, whereas references are just 'moved'. Here what you would want to do is, instead of v.Get, do a v.Remove, which will "remove" the value from the array. Then instead of setting (i.e: v.Set), do a v.Insert, because by removing earlier, we literally remove the value from the array, hence the values are shifted by one index.

Now one thing to note is that, because the remaining values are shifted by one index by removing, if you try to remove the first element (leftValue) first, and then try to remove the last element (rightElement), it will mess-up the indexing for the second/right value removal. (could get an array-out-of-bound error / or could end up removing the wrong value) because after the first removal, the size of the array is (n-1).
So, you'll have to swap the removals, to first remove from the rear-end (right index) first, and then remove from the front (the left index).
i.e:

// Remove the right index (from the rear end) first, because removing the left index (from the front of the array) first could mess up the indexes of the rest of the elements.
rightValue := v.Remove(interpreter, locationRange, rightIndex)
leftValue := v.Remove(interpreter, locationRange, leftIndex)

// Similarly, insert the left index first, before the right index.
v.Insert(interpreter, locationRange, leftIndex, rightValue)
v.Insert(interpreter, locationRange, rightIndex, leftValue)

[SupunS]

Also no need for an additional "Transfer" because Remove and Insert already do a transfer underneath.

[bluesign]

This feels too expensive to me; transfers here are not necessary, Why move stuff to stack and then move again to strange with totally new storageID and slab tree?

We are 100% sure that storage address will not change in this operation. Why not add swap to atree.Array ? @fxamacker can confirm, but I think this way, all array is read and written again.

[darkdrag00nv2]

I thought so as well and asked Bastian about adding support for reverse in atree on #2605 (comment). Didn't ask about swap but that will work nicely as well.

We can also think about adding it using Remove and Insert and later optimizing it using support in atree.

[SupunS]

Alternatively, as it has been briefly discussed in issue #2605, maybe we could start with the function that returns a new array with entries reversed, which should be easy to implement. And then later add this as the "optimized" version which does the same in-place. So the functionality is there, if someone needs it.

[darkdrag00nv2]

@SupunS Makes sense, opened up #2654 for copy-based reversal.

[fxamacker]

We are 100% sure that storage address will not change in this operation. Why not add swap to atree.Array ?

Good point 👍 . I opened onflow/atree#326 to add Array.Swap.

[darkdrag00nv2]

Have decided to update the function name to reverseInPlace based on the suggestion on #2654.