Pop/push slice returns old element

[n0izn0iz]

Pop/push slice returns old element

Description

If you pop an element from a slice (via slice = slice[:len(slice)-1]) and append a new element afterwards, the last element of the slice will be the popped element and not the new element

(Sorry for the confusing issue name I can't find a better short name)

Your environment

• macOS 13.3.1
• go version go1.20.4 darwin/arm64
• gno: master
• commit: d684985

Steps to reproduce

• Deploy this realm

package bugmin

import (
	"strconv"
	"strings"
)

var slice = []string{"undead-element"}

func Pop() {
	slice = slice[:len(slice)-1]
}

func Push() {
	slice = append(slice, "new-element")
}

func Render(path string) string {
	str := strings.Join(slice, ", ") + "\n\n"
	str += "len: " + strconv.Itoa(len(slice)) + "\n\n"
	return str
}

• Call Pop then Push

Expected behaviour

The slice is []string{"new-element"} after Pop and Push

Actual behaviour

The slice is []string{"undead-element"} after Pop and Push

Additional info

If you give me some pointer (namely where the slice access operator [:] and append are handled), I could try debug this

You can work around this by creating a new slice:

func Pop() {
	// slice = slice[:len(slice)-1] // this is bugged

	newSlice := []string{}
	for i := 0; i < len(slice)-1; i++ {
		newSlice = append(newSlice, slice[i])
	}
	slice = newSlice // this works
}

The bug seem to be at storage layer level since this works as expected:

package bugmin

import (
	"strconv"
	"strings"
)

func Render(path string) string {
	slice := []string{"undead-element"}
	slice = slice[:len(slice)-1]
	slice = append(slice, "new-element")

	str := "slice: " + strings.Join(slice, ", ") + "\n\n"
	str += "len(slice): " + strconv.Itoa(len(slice)) + "\n\n"
	return str
}

[moul]

Hey, could you try opening this bug as a new challenge in this folder, please? https://github.com/gnolang/gno/tree/master/gnovm/tests/challenges

[n0izn0iz]

I tried to add it in challenges like so:

package main

var slice = []string{"undead-element"}

func main() {
	slice = slice[:len(slice)-1]
	slice = append(slice, "new-element")

	println(slice)
}

// Output:
// slice[("new-element" string)]

but this does not fail, I think it needs to happen in two separate transactions, I could add a realm in examples/gno.land if you want

[peter7891]

@n0izn0iz

Creating a slice
The slice elements are pushed for evaluation here
func (m *Machine) doOpCompositeLit()

Then, each of the elements will pass through
func (m *Machine) doOpEval()

Then, everything gets assembled here
func (m *Machine) doOpArrayLit()

Reading from a slice
For the accessing of elements it first goes through
func (m *Machine) doOpEval()
in the case *SliceExpr:

and gets the value here
func (m *Machine) doOpSlice()

[n0izn0iz]

I found something interesting

At this line:

gno/gnovm/pkg/gnolang/realm.go

Line 1111 in 6d137df

Maxcap: cv.Maxcap,

If I set Maxcap: cv.Length instead of Maxcap: cv.Maxcap, it solves my minimal test, this probably breaks other stuff though

Maxcap seems to not be properly handled somewhere, my guess is serialization/de-serialization since the pop/push works when done in a single TX

I'll keep digging ⛏️

[n0izn0iz]

After digging more, I feel that assigning to a slice does not properly "Marks dirty" the slice or it's base array

copyValueWithRefs is never called for the new-element string (it is called for the undead-element string)
SetObject is never called with array[("new-element" string)] (it is called for array[("undead-element" string)])
realm.updated does not contains the new slice at the end of the Push tx

[n0izn0iz]

[EDIT]: ignore this post, since the underlying array is the same, this seems normal in the end

Something here does not seem right

gno/gnovm/pkg/gnolang/values.go

Lines 279 to 282 in f50f33d

	oo1 := pv.TV.GetFirstObject(store)
	pv.TV.Assign(alloc, tv2, cu)
	oo2 := pv.TV.GetFirstObject(store)
	rlm.DidUpdate(pv.Base.(Object), oo1, oo2)

oo1 address is the same as oo2 when assigning the slice during Push
but pv.TV is (slice[] []string) and tv2 is (slice[("new-element" string)] []string)
I'm guessing oo1 and oo2 should be two different objects

seems like it's assigning the new slice to the new slice

not sure what the intended behavior is

(sorry for the raw information and thoughts, I barely understand the code for now)

[n0izn0iz]

Could someone explain

• what is an escaped object?
• what is a real object?
• what is a dirty object? (I think I understand this one but asking just to be sure)

[peter7891]

Could someone explain

• what is an escaped object?
• what is a real object?
• what is a dirty object? (I think I understand this one but asking just to be sure)

These are part of the ownership system.
You can see where its invoked here.

[peter7891]

@n0izn0iz i haven't tested it but my hunch is that there is no problem with the Maxcap.
From what i can tell by reading the code, it looks like SliceValue does not implement Object.
So here the slice is saved as an ArrayValue and the information about the slice view (like which part of the array its pointing to) is then lost.
The fact that it works, when you make a new slice can be explained that a new backing array is created for the new slice.

Can you test out this theory? For example, make a 10 element slice and slice = slice[4:5].
If the theory is correct, in the next transaction it should have the slice with all 10 elements.

[n0izn0iz]

@peter7891 this makes a lot of sense

Your test does not work like you expect though

For this realm:

package slice_splice

import (
	"strings"
)

var slice = []string{"0", "1", "2", "3", "4", "5", "6", "7", "8", "9"}

func Splice() []string {
	slice = slice[4:5]
	return slice
}

func Render(path string) string {
	return strings.Join(slice, ",")
}

Render returns 4 after calling Splice

[peter7891]

@peter7891 this makes a lot of sense

Your test does not work like you expect though

For this realm:

package slice_splice

import (
	"strings"
)

var slice = []string{"0", "1", "2", "3", "4", "5", "6", "7", "8", "9"}

func Splice() []string {
	slice = slice[4:5]
	return slice
}

func Render(path string) string {
	return strings.Join(slice, ",")
}

Render returns 4 after calling Splice

Right. My theory was wrong but it eliminated the possibility of there being a bug with the slicing.
So, the problem should be with the append step, which was in your initial example.
I'll dig more.

[jaekwon]

Maybe it thinks the new slice’s capacity is already hit, then it will first copy to new larger array, as per spec.

[deelawn]

This should be fixed by #1305

[deelawn]

Fixed by #1305