Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce reverseInPlace in Variable/Fixed size Array #2626

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Introduce reverseInPlace in Variable/Fixed size Array #2626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
74ca799
Introduce reverse in Variable/Fixed size Array
darkdrag00nv2 Jul 1, 2023
db38baf
tab to whitespace in interpreter test case
darkdrag00nv2 Jul 1, 2023
78bab43
Improve naming by introducing local variables.
darkdrag00nv2 Jul 5, 2023
e68bcf4
Remove unnecessary slice allocation
darkdrag00nv2 Jul 5, 2023
64bf9c1
Return Void instead of VoidValue{}
darkdrag00nv2 Jul 5, 2023
6cff2d2
Make return type of Reverse a Value
darkdrag00nv2 Jul 5, 2023
5056603
declare a constant for 'reverse'
darkdrag00nv2 Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions runtime/interpreter/value.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2192,6 +2192,22 @@ func (v *ArrayValue) FirstIndex(interpreter *Interpreter, locationRange Location
return NilOptionalValue
}

func (v *ArrayValue) Reverse(interpreter *Interpreter, locationRange LocationRange) Value {
count := v.Count()

for leftIndex := 0; leftIndex < count/2; leftIndex++ {
rightIndex := count - leftIndex - 1

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

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

return Void
}

func (v *ArrayValue) Contains(
interpreter *Interpreter,
locationRange LocationRange,
Expand Down Expand Up @@ -2371,6 +2387,18 @@ func (v *ArrayValue) GetMember(interpreter *Interpreter, locationRange LocationR
},
)

case sema.ArrayTypeReverseFunctionName:
return NewHostFunctionValue(
interpreter,
sema.ArrayReverseFunctionType,
func(invocation Invocation) Value {
return v.Reverse(
invocation.Interpreter,
invocation.LocationRange,
)
},
)

case "contains":
return NewHostFunctionValue(
interpreter,
Expand Down
23 changes: 23 additions & 0 deletions runtime/sema/type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1714,6 +1714,12 @@ Returns the index of the first element matching the given object in the array, n
Available if the array element type is not resource-kinded and equatable.
`

const ArrayTypeReverseFunctionName = "reverse"

const arrayTypeReverseFunctionDocString = `
Reverses the elements of the array.
`

const arrayTypeContainsFunctionDocString = `
Returns true if the given object is in the array
`
Expand Down Expand Up @@ -1865,6 +1871,18 @@ func getArrayMembers(arrayType ArrayType) map[string]MemberResolver {
)
},
},
ArrayTypeReverseFunctionName: {
Kind: common.DeclarationKindFunction,
Resolve: func(memoryGauge common.MemoryGauge, identifier string, targetRange ast.Range, report func(error)) *Member {
return NewPublicFunctionMember(
memoryGauge,
arrayType,
identifier,
ArrayReverseFunctionType,
arrayTypeReverseFunctionDocString,
)
},
},
}

// TODO: maybe still return members but report a helpful error?
Expand Down Expand Up @@ -2116,6 +2134,11 @@ func ArrayFirstIndexFunctionType(elementType Type) *FunctionType {
),
}
}

var ArrayReverseFunctionType *FunctionType = &FunctionType{
ReturnTypeAnnotation: NewTypeAnnotation(VoidType),
}

func ArrayContainsFunctionType(elementType Type) *FunctionType {
return &FunctionType{
Parameters: []Parameter{
Expand Down
30 changes: 30 additions & 0 deletions runtime/tests/checker/arrays_dictionaries_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1078,6 +1078,36 @@ func TestCheckInvalidResourceFirstIndex(t *testing.T) {
assert.IsType(t, &sema.ResourceLossError{}, errs[2])
}

func TestCheckArrayReverse(t *testing.T) {

t.Parallel()

_, err := ParseAndCheck(t, `
fun test() {
let x = [1, 2, 3]
x.reverse()
}
`)

require.NoError(t, err)
}

func TestCheckInvalidArrayReverse(t *testing.T) {

t.Parallel()

_, err := ParseAndCheck(t, `
fun test() {
let x = [1, 2, 3]
x.reverse(100)
}
`)

errs := RequireCheckerErrors(t, err, 1)

assert.IsType(t, &sema.ArgumentCountError{}, errs[0])
}

func TestCheckArrayContains(t *testing.T) {

t.Parallel()
Expand Down
61 changes: 61 additions & 0 deletions runtime/tests/interpreter/interpreter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10395,6 +10395,67 @@ func TestInterpretArrayFirstIndexDoesNotExist(t *testing.T) {
)
}

func TestInterpretArrayReverse(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@darkdrag00nv2 darkdrag00nv2 Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@SupunS SupunS Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@SupunS SupunS Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


t.Parallel()

inter := parseCheckAndInterpret(t, `
let xs = [1, 2, 3, 100, 200]
let ys = [100, 467, 297, 23]

fun reversexs() {
return xs.reverse()
}

fun reverseys() {
return ys.reverse()
}
`)

_, err := inter.Invoke("reversexs")
require.NoError(t, err)

AssertValuesEqual(
t,
inter,
interpreter.NewArrayValue(
inter,
interpreter.EmptyLocationRange,
interpreter.VariableSizedStaticType{
Type: interpreter.PrimitiveStaticTypeInt,
},
common.ZeroAddress,
interpreter.NewUnmeteredIntValueFromInt64(200),
interpreter.NewUnmeteredIntValueFromInt64(100),
interpreter.NewUnmeteredIntValueFromInt64(3),
interpreter.NewUnmeteredIntValueFromInt64(2),
interpreter.NewUnmeteredIntValueFromInt64(1),
),
inter.Globals.Get("xs").GetValue(),
)

_, err = inter.Invoke("reverseys")
require.NoError(t, err)

AssertValuesEqual(
t,
inter,
interpreter.NewArrayValue(
inter,
interpreter.EmptyLocationRange,
interpreter.VariableSizedStaticType{
Type: interpreter.PrimitiveStaticTypeInt,
},
common.ZeroAddress,
interpreter.NewUnmeteredIntValueFromInt64(23),
interpreter.NewUnmeteredIntValueFromInt64(297),
interpreter.NewUnmeteredIntValueFromInt64(467),
interpreter.NewUnmeteredIntValueFromInt64(100),
),
inter.Globals.Get("ys").GetValue(),
)
}

func TestInterpretOptionalReference(t *testing.T) {

t.Parallel()
Expand Down