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.

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

feat(gnovm): align Gno constant handling with Go specifications #2828

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
3d5030a
feat(gnovm): align Gno constant handling with Go specifications
omarsy Sep 21, 2024
2543294
Merge branch 'master' into fix/2628
omarsy Oct 18, 2024
bb2d9fd
feat: correct test
omarsy Oct 18, 2024
c7643eb
Merge branch 'master' into fix/2628
omarsy Oct 22, 2024
b80a890
feat: add a todo for native type conversion
omarsy Oct 23, 2024
ff74523
feat: uncomment package branch
omarsy Oct 23, 2024
761ef1a
feat: use println
omarsy Oct 23, 2024
5fa41e9
feat: add test
omarsy Oct 23, 2024
90c8c5b
fix: use evalStaticType only for type
omarsy Oct 23, 2024
e74e6e9
feat: add more test
omarsy Oct 23, 2024
3a52e14
feat: add missing init expr
omarsy Oct 24, 2024
f53a19f
feat: add real and imag on todo
omarsy Nov 7, 2024
194883a
Merge remote-tracking branch 'upstream/master' into fix/2628
omarsy Nov 26, 2024
13136c5
fix: test
omarsy Nov 26, 2024
27e69b4
feat: correct merge
omarsy Nov 27, 2024
5dc615a
feat: add test
omarsy Nov 27, 2024
6fb3cf7
Merge branch 'master' into fix/2628
omarsy Nov 27, 2024
743962c
fix: test
omarsy Nov 27, 2024
35d6ae1
Merge branch 'master' into fix/2628
omarsy Dec 3, 2024
459f0da
Merge branch 'master' into fix/2628
omarsy Dec 7, 2024
1ba4872
refactor: replace checkConstantExpr with assertValidConstExpr for imp…
omarsy Dec 7, 2024
54878f0
refactor: rename assertValidConstantExprRecursively to assertValidCon…
omarsy Dec 9, 2024
72b8910
Merge branch 'master' into fix/2628
omarsy Dec 9, 2024
88dae32
Merge branch 'master' into fix/2628
omarsy Dec 10, 2024
59a7d85
Merge branch 'master' into fix/2628
omarsy Dec 11, 2024
9001059
fix(gnovm): check type
omarsy Dec 26, 2024
c45f63e
fix(gnovm): enhance constant evaluation for array and map types
omarsy Dec 26, 2024
7083830
refactore(gnovm): some refactor
omarsy Dec 26, 2024
4da7934
fix(gnovm): remove unnecessary panic for non-constant array type expr…
omarsy Dec 26, 2024
6ef45d4
Merge branch 'master' into fix/2628
omarsy Dec 27, 2024
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
2 changes: 1 addition & 1 deletion examples/gno.land/r/demo/keystore/keystore_test.gno
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
)

func TestRender(t *testing.T) {
const (
var (
author1 std.Address = testutils.TestAddress("author1")
author2 std.Address = testutils.TestAddress("author2")
)
Expand Down
2 changes: 1 addition & 1 deletion examples/gno.land/r/demo/microblog/microblog_test.gno
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
)

func TestMicroblog(t *testing.T) {
const (
var (
author1 std.Address = testutils.TestAddress("author1")
author2 std.Address = testutils.TestAddress("author2")
)
Expand Down
1 change: 1 addition & 0 deletions gnovm/pkg/gnolang/preprocess.go
Original file line number Diff line number Diff line change
Expand Up @@ -2292,6 +2292,7 @@ func preprocess1(store Store, ctx BlockNode, n Node) Node {
// NOTE: may or may not be a *ConstExpr,
// but if not, make one now.
for i, vx := range n.Values {
assertValidConstExpr(store, last, n, vx)
n.Values[i] = evalConst(store, last, vx)
}
} else {
Expand Down
135 changes: 135 additions & 0 deletions gnovm/pkg/gnolang/type_check.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,141 @@
}
}

func assertValidConstExpr(store Store, last BlockNode, n *ValueDecl, expr Expr) {
if n.Type != nil {
nt := evalStaticType(store, last, n.Type)
if xnt, ok := nt.(*NativeType); ok {
nt = go2GnoBaseType(xnt.Type)
}

Check warning on line 223 in gnovm/pkg/gnolang/type_check.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/type_check.go#L222-L223

Added lines #L222 - L223 were not covered by tests

if _, ok := baseOf(nt).(PrimitiveType); !ok {
panic(fmt.Sprintf("invalid constant type %s", nt.String()))
}
}

assertValidConstantExprRecursively(store, last, expr, nil)
}

func assertValidConstantExprRecursively(store Store, last BlockNode, currExpr, parentExpr Expr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func assertValidConstantExprRecursively(store Store, last BlockNode, currExpr, parentExpr Expr) {
func assertValidConstValue(store Store, last BlockNode, currExpr, parentExpr Expr) {

Copy link
Member Author

Choose a reason for hiding this comment

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

should be done here 54878f0

Main:
switch currExpr := currExpr.(type) {
case *NameExpr:
t := evalStaticTypeOf(store, last, currExpr)
if _, ok := t.(*ArrayType); ok {
Copy link
Contributor

@ltzmaxwell ltzmaxwell Nov 25, 2024

Choose a reason for hiding this comment

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

doubt about this, only with len(arr)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes array is only variable allowed in a constant.

this should work on go:

package main

func main() {
	var l = [2]string{}
	const i = len(l)
	println(i)
}

this should not work:

package main

func main() {
	var l = "test"
	const i = len(l)
	println(i)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

consider this:

package main

func main() {
	const a = [2]int{1, 2}
	println(a)
}

and

package main

func main() {
	a := [2]int{1, 2}

	const b = a
}

these should not work, but works on the current branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch I did a bad merge ^^

Copy link
Contributor

@ltzmaxwell ltzmaxwell Dec 9, 2024

Choose a reason for hiding this comment

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

hmmm, this logic seems incorrect:

if _, ok := t.(*ArrayType); ok {
	break Main
}

Due to this logic, the error in const43.gno was not caught.

I guess your intention is to check len(arr), right? I think that needs additional context to know if the outer expr is a built func call: len or max?

Not so sure, but we need to focus on ensuring that the logic of this function is correct(not limited to the above-mentioned).

break Main
ltzmaxwell marked this conversation as resolved.
Show resolved Hide resolved
}
panic(fmt.Sprintf("%s (variable of type %s) is not constant", currExpr.Name, t))
case *TypeAssertExpr:
panic(fmt.Sprintf("%s (comma, ok expression of type %s) is not constant", currExpr.String(), currExpr.Type))
case *IndexExpr:
panic(fmt.Sprintf("%s (variable of type %s) is not constant", currExpr.String(), currExpr.X))
case *CallExpr:
ift := evalStaticTypeOf(store, last, currExpr.Func)
switch baseOf(ift).(type) {
case *FuncType:
tup := evalStaticTypeOfRaw(store, last, currExpr).(*tupleType)

// check for built-in functions
petar-dambovaliev marked this conversation as resolved.
Show resolved Hide resolved
if cx, ok := currExpr.Func.(*ConstExpr); ok {
if fv, ok := cx.V.(*FuncValue); ok {
if fv.PkgPath == uversePkgPath {
// TODO: should support min, max, real, imag
switch {
case fv.Name == "len":
assertValidConstantExprRecursively(store, last, currExpr.Args[0], currExpr)
break Main
case fv.Name == "cap":
assertValidConstantExprRecursively(store, last, currExpr.Args[0], currExpr)
break Main
}
}
}
}

switch {
case len(tup.Elts) == 0:
panic(fmt.Sprintf("%s (no value) used as value", currExpr.String()))

Check warning on line 271 in gnovm/pkg/gnolang/type_check.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/type_check.go#L270-L271

Added lines #L270 - L271 were not covered by tests
case len(tup.Elts) == 1:
panic(fmt.Sprintf("%s (value of type %s) is not constant", currExpr.String(), tup.Elts[0]))
default:
panic(fmt.Sprintf("multiple-value %s (value of type %s) in single-value context", currExpr.String(), tup.Elts))
}
case *TypeType:
for _, arg := range currExpr.Args {
assertValidConstantExprRecursively(store, last, arg, currExpr)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please check this:

package main

const a = interface{}(nil)

func main() {
	println("ok")
}

seems also need to check the type, right?

Copy link
Member Author

@omarsy omarsy Dec 26, 2024

Choose a reason for hiding this comment

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

My bad break on my last refactoring when I move the check type

Nice catch ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

so the above clause: case *TypeType is not needed? didn't see any test case targeting this.

case *NativeType:
ltzmaxwell marked this conversation as resolved.
Show resolved Hide resolved
// Todo: should add a test after the fix of https://github.com/gnolang/gno/issues/3006
ty := evalStaticType(store, last, currExpr.Func)
panic(fmt.Sprintf("%s (variable of type %s) is not constant", currExpr.String(), ty))
default:
panic(fmt.Sprintf(
"unexpected func type %v (%v)",
ift, reflect.TypeOf(ift)))

Check warning on line 288 in gnovm/pkg/gnolang/type_check.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/type_check.go#L281-L288

Added lines #L281 - L288 were not covered by tests
}
case *BinaryExpr:
assertValidConstantExprRecursively(store, last, currExpr.Left, parentExpr)
assertValidConstantExprRecursively(store, last, currExpr.Right, parentExpr)
case *SelectorExpr:
xt := evalStaticTypeOf(store, last, currExpr.X)
switch xt := xt.(type) {
case *PackageType:
var pv *PackageValue
if cx, ok := currExpr.X.(*ConstExpr); ok {
// NOTE: *Machine.TestMemPackage() needs this
// to pass in an imported package as *ConstEzpr.
pv = cx.V.(*PackageValue)

Check warning on line 301 in gnovm/pkg/gnolang/type_check.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/type_check.go#L299-L301

Added lines #L299 - L301 were not covered by tests
} else {
// otherwise, packages can only be referred to by
// *NameExprs, and cannot be copied.
pvc := evalConst(store, last, currExpr.X)
pv_, ok := pvc.V.(*PackageValue)
if !ok {
panic(fmt.Sprintf(
"missing package in selector expr %s",
currExpr.String()))

Check warning on line 310 in gnovm/pkg/gnolang/type_check.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/type_check.go#L308-L310

Added lines #L308 - L310 were not covered by tests
}
pv = pv_
}
if pv.GetBlock(store).Source.GetIsConst(store, currExpr.Sel) {
break Main
}

tt := pv.GetBlock(store).Source.GetStaticTypeOf(store, currExpr.Sel)
panic(fmt.Sprintf("%s (variable of type %s) is not constant", currExpr.String(), tt))
case *PointerType, *DeclaredType, *StructType, *InterfaceType:
ltzmaxwell marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a counter case:

package main

type MyStruct struct {
	arr [2]int
}

const a = len(MyStruct{arr: [2]int{1, 2}}.arr)

func main() {
	println("ok")
}

Copy link
Member Author

Choose a reason for hiding this comment

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

:o I miss this case. Thank you ^^
Done here c45f63e

ty := evalStaticTypeOf(store, last, currExpr.X)
panic(fmt.Sprintf("%s (variable of type %s) is not constant", currExpr.String(), ty))
case *TypeType:
ty := evalStaticType(store, last, currExpr.X)
panic(fmt.Sprintf("%s (variable of type %s) is not constant", currExpr.String(), ty))
case *NativeType:
ty := evalStaticTypeOf(store, last, currExpr.X)
panic(fmt.Sprintf("%s (variable of type %s) is not constant", currExpr.String(), ty))
default:
panic(fmt.Sprintf(
"unexpected selector expression type %v",
reflect.TypeOf(xt)))

Check warning on line 332 in gnovm/pkg/gnolang/type_check.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/type_check.go#L326-L332

Added lines #L326 - L332 were not covered by tests
}

case *ArrayTypeExpr:
if parentExpr == nil {
ty := evalStaticTypeOf(store, last, currExpr)
panic(fmt.Sprintf("%s (variable of type %s) is not constant", currExpr.String(), ty))
}
case *ConstExpr:
case *BasicLitExpr:

Check warning on line 341 in gnovm/pkg/gnolang/type_check.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/type_check.go#L341

Added line #L341 was not covered by tests
case *CompositeLitExpr:
Copy link
Contributor

@ltzmaxwell ltzmaxwell Dec 26, 2024

Choose a reason for hiding this comment

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

can we just check parent and panic here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we verify the expression type before calling this function, this case should not occur. Removed for array: 4da7934

Copy link
Contributor

Choose a reason for hiding this comment

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

will it be more straightforward that just check if parentExpr exist, and the type of compositeLitExpr is array type, otherwise panic?

assertValidConstantExprRecursively(store, last, currExpr.Type, parentExpr)
default:
ift := evalStaticTypeOf(store, last, currExpr)
if _, ok := ift.(*TypeType); ok {
ift = evalStaticType(store, last, currExpr)
}
panic(fmt.Sprintf("%s (variable of type %s) is not constant", currExpr.String(), ift))
}
}

// checkValDefineMismatch checks for mismatch between the number of variables and values in a ValueDecl or AssignStmt.
func checkValDefineMismatch(n Node) {
var (
Expand Down
11 changes: 11 additions & 0 deletions gnovm/tests/files/const23.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package main

import "fmt"

func main() {
const t []string = []string{}
fmt.Println(t)
}

// Error:
// main/files/const23.gno:6:8: invalid constant type []string
79 changes: 79 additions & 0 deletions gnovm/tests/files/const24.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package main

import (
"fmt"
"time"
)

func main() {
const a int = 1_000_000
const b byte = byte(1)
const c float64 = 1_000_000.000
const d string = "Hello, World!"
const e rune = 'a'
const g bool = true
const h uint = 1_000
const i int8 = 1
const j int16 = 1
const k int32 = 1
const l int64 = 1
const m uint8 = 1
const n uint16 = 1
const o uint32 = 1
const p uint64 = 1
const r float32 = 1_000_000.000
const s = r
const t = len("s")
const u = 1 + len("s") + 3
ars := [10]string{}
const v = len(ars)
const w = cap(ars)
const x = time.Second

fmt.Println(a)
fmt.Println(b)
fmt.Println(c)
fmt.Println(d)
fmt.Println(e)
fmt.Println(g)
fmt.Println(h)
fmt.Println(i)
fmt.Println(j)
fmt.Println(k)
fmt.Println(l)
fmt.Println(m)
fmt.Println(n)
fmt.Println(o)
fmt.Println(p)
fmt.Println(r)
fmt.Println(s)
fmt.Println(t)
fmt.Println(u)
fmt.Println(v)
fmt.Println(w)
println(x)
}

// Output:
// 1000000
// 1
// 1e+06
// Hello, World!
// 97
// true
// 1000
// 1
// 1
// 1
// 1
// 1
// 1
// 1
// 1
// 1e+06
// 1e+06
// 1
// 5
// 10
// 10
// 1s
11 changes: 11 additions & 0 deletions gnovm/tests/files/const25.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package main

import "fmt"

func main() {
const t = []string{"1"}
fmt.Println(t)
}

// Error:
// main/files/const25.gno:6:8: [](const-type string) (variable of type []string) is not constant
15 changes: 15 additions & 0 deletions gnovm/tests/files/const26.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package main

import "fmt"

func v() string {
return ""
}

func main() {
const t = v()
fmt.Println(t)
}

// Error:
// main/files/const26.gno:10:8: v<VPBlock(3,0)>() (value of type string) is not constant
16 changes: 16 additions & 0 deletions gnovm/tests/files/const27.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package main

import "fmt"

func v() string {
return ""
}

func main() {
var i interface{} = 1
const t, ok = i.(int)
fmt.Println(t, ok)
}

// Error:
// main/files/const27.gno:11:8: i<VPBlock(1,0)>.((const-type int)) (comma, ok expression of type (const-type int)) is not constant
12 changes: 12 additions & 0 deletions gnovm/tests/files/const28.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package main

import "fmt"

func main() {
var s []string = []string{"1"}
const t, ok = s[0]
fmt.Println(t, ok)
}

// Error:
// main/files/const28.gno:7:8: s<VPBlock(1,0)>[(const (0 int))] (variable of type s<VPBlock(1,0)>) is not constant
12 changes: 12 additions & 0 deletions gnovm/tests/files/const29.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package main

import "fmt"

func main() {
s := "1"
const t = s
fmt.Println(t)
}

// Error:
// main/files/const29.gno:7:8: s (variable of type string) is not constant
15 changes: 15 additions & 0 deletions gnovm/tests/files/const30.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package main

import "fmt"

func v() {
return
}

func main() {
const t = v()
fmt.Println(t)
}

// Error:
// main/files/const30.gno:10:8: v<VPBlock(3,0)> (no value) used as value
15 changes: 15 additions & 0 deletions gnovm/tests/files/const31.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package main

import "fmt"

func v() (string, string) {
return "", ""
}

func main() {
const t, v = v()
fmt.Println(t)
}

// Error:
// main/files/const31.gno:10:8: multiple-value (const (v func()( string, string)))() (value of type [string string]) in single-value context
11 changes: 11 additions & 0 deletions gnovm/tests/files/const32.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package main

import "fmt"

func main() {
const t = 1 + 2 + len([]string{})
fmt.Println(t)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I test this one, seems not caught by your new added logic

package main

 import "fmt"

 func main() {
 	const t = 1 + 2 + len([2]string{})
 	fmt.Println(t)
 }

Copy link
Member Author

Choose a reason for hiding this comment

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

This will work because an array is permitted in a constant expression, whereas a slice is not.

// Error:
// main/files/const32.gno:6:8: [](const-type string) (variable of type []string) is not constant
12 changes: 12 additions & 0 deletions gnovm/tests/files/const33.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package main

var x = 1
var y = 1

const b = x == y

func main() {
println("ok")
}
// Error:
// main/files/const33.gno:6:7: x (variable of type int) is not constant
10 changes: 10 additions & 0 deletions gnovm/tests/files/const34.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package main

const a = func() { println("hey") }

func main() {
println("ok")
}

// Error:
// main/files/const34.gno:3:7: func func(){ (const (println func(xs ...interface{})()))((const ("hey" string))) } (variable of type func()()) is not constant
Loading
Loading