fix: handle untyped constant expressions in append()

[mvertes]

This is a particular case of Go append builtin where the array type is inferred from the first argument. We detect an untyped const passed as argument, then we convert it to the array type prior to proceed further. It fixes further consistency checks and value generation.

Fixes #1136, fixes #1149, fixes #1341.

Contributors' checklist...

• Added new tests, or not needed, or not feasible
• Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
• Updated the official documentation or not needed
• No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
• Added references to related issues and PRs
• Provided any useful hints for running manual tests
• Added new benchmarks to generated graphs, if any. More info here.

[codecov]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (568a087) 56.08% compared to head (04614ec) 55.84%.

Files	Patch %	Lines
gnovm/pkg/gnolang/preprocess.go	83.33%	2 Missing and 1 partial ⚠️
gnovm/pkg/gnolang/values_conversions.go	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1177      +/-   ##
==========================================
- Coverage   56.08%   55.84%   -0.24%     
==========================================
  Files         432      431       -1     
  Lines       65989    65768     -221     
==========================================
- Hits        37007    36728     -279     
- Misses      26094    26166      +72     
+ Partials     2888     2874      -14     

Flag	Coverage Δ
go-1.21.x	∅ <ø> (∅)
misc	∅ <ø> (∅)
misc-_test.genstd	∅ <ø> (∅)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mvertes]

Seeing if I can make the fix even more generic and also handle any untyped constant value, as to fix also #1149.

[moul]

LGTM, @piux2 can you make a complementary review, please?

[tbruyelle]

Does that also fix the bug we had on gnochess #1170 ?

Note that to reproduce it, you need to broadcast multiple tx, so the current test system in tests/files cannot be used.

[thehowl]

LGTM 👏

[thehowl]

Does that also fix the bug we had on gnochess #1170 ?

Not as far as I can see, alas. This is specifically tackling constant expressions in append; #1170 is about the internal pointers not being fixed with appends.

[piux2]

@thehowl, @moul,

Issue #1136 and issue #1149 are rooted in different issues.

Regarding #1136:
In Go, it's not permissible to append a nil to concrete types like []int. In the test case discussed, error is a built-in interface type. In Go, the default zero value for an interface type is nil, so it's allowable to append a nil to a slice of type []error. However, the behavior described in issue #1149 is different. If one attempts to append a nil to []int, it should, and typically does, cause a panic. Notably, after this fix, this panic no longer occurs when appending a nil to []int.

For #1149:
As the error message hinted. The core of the problem lies in converting variadic generic type arguments into a slice before they are passed into the append function. It's important to note that this issue isn't exclusive to append but also impacts any function that accepts variadic generic type arguments.

We will need to dig more and fix it at the root.

[thehowl]

In Go, it's not permissible to append a nil to concrete types like []int. In the test case discussed, error is a built-in interface type. In Go, the default zero value for an interface type is nil, so it's allowable to append a nil to a slice of type []error. However, the behavior described in issue #1149 is different. If one attempts to append a nil to []int, it should, and typically does, cause a panic. Notably, after this fix, this panic no longer occurs when appending a nil to []int.

I was surprised to find out you're right (because it seemed to me that this PR didn't special-case nil):

package main

func main() {
        var ints []int
        ints = append(ints, nil, nil)
        println(ints)
}

// Output: slice[(0 int),(0 int)]

This executes, albeit incorrectly. However, this is actually a phenomenon caused by a root issue: we allow converting nil to int (and presumably other types):

package main

func main() {
        println(int(nil))
}

// Output: 0

I think the approach @mvertes has used is correct (given append([]T, T...), attempt to convert the variadic args into T before appending). However, we suffer a different problem about nil, thus allowing to do what you mentioned (appending nil to a slice of ints).

As the error message hinted. The core of the problem lies in converting variadic generic type arguments into a slice before they are passed into the append function. It's important to note that this issue isn't exclusive to append but also impacts any function that accepts variadic generic type arguments.

This one, empirically, does not check out (for the last statement). Try this one, and playing out with changing it (from master):

func main() {
	var n []int
	x := 2
	vari(n, 1, x)
}

func vari(n []int, params ...interface{}) {}

It's append which is special in this case.

[piux2]

Does that also fix the bug we had on gnochess #1170 ?

Note that to reproduce it, you need to broadcast multiple tx, so the current test system in tests/files cannot be used.

For issue #1170, the root cause is located in realm.go, the underlying problem arises from the underlying array of the slice not being marked as "dirty" during the persistence of a realm object when that object is assigned to itself. I'm still evaluating the issue to identify an optimal solution.

[piux2]

@moul

LGTM, @piux2 can you make a complementary review, please?

Yes, done

[piux2]

I think here is the right place to fix #1149

gno/gnovm/pkg/gnolang/types.go

Line 1163 in ce258b1

vargt = varg.T

and add following condition:

	+ if isUntyped(varg.T) {
        +   vargt =	defaultTypeOf(varg.T)
	+ }else{
			vargt = varg.T
	 + }

[piux2]

We will have further discussion and update the changes

[piux2]

we will dig further

[mvertes]

@piux2 I examined and tried your suggestion to apply vargt = defaultTypeOf(varg.T), and I finally decided to change ConvertTo instead, which is necessary anyway and allows to catch invalid conversions not only at preprocessing, but also during execution (if nil is passed as value to convert).

[thehowl]

@mvertes, since you commented on #1347, can you add a test like the one in #1341 to verify that nil conversions now fail correctly when they should?

[zivkovicmilos]

@piux2
Ping for unblocking this PR

[jaekwon]

LGTM

[thehowl]

@mvertes I've made a small commit to address the comment by Jae. Then I noticed a small chance for optimisation (re-using the result of Call), then I also fixed some tests that was failing on the CI :)

If the changes look good to you, please merge. The code's been reviewed by Jae, if there are further changes requested they can be made in a separate PR :)

[mvertes]

@piux2 ping

Jae reviewed and approved this. Agreed with Milos to merge it as it fixes a number of Gno issues. If you have further comments, feel free to open a new PR or issue

[piux2]

@piux2 ping

@mvertes sorry, I thought it was already approved

@thehowl thank you.