gnovm doesn't perform terminating statement analysis

[thehowl]

Originally discovered by @petar-dambovaliev , re-discovered by me in development of gnochess.

package main

func main() {
	z := y()
	println(z)
}

func y() int {
	x := 9
}

This code is invalid Go code, but it is valid Gno at the time being. This results in a cryptic panic:

panic running expression main(): interface conversion: gnolang.Stmt is nil, not *gnolang.AssignStmt

At the VM level, this is the difference between having a return x at the end of y() (red) and not having it (green):

Key takeaways:

• As a consequence of not having a return statement, the Frame is not popped
• +v (9 int) is not done, and as a consequence when defining z in main(), the value popped is instead (y func()( int)).

This leads to the stacks not being effectively synchronised in popping/pushing, and to general corruption of the gnovm state, often leading to panics of various nature (the above is an example of around 3/4 I've seen).

The solution is properly implementing the Terminating Statement part of the spec, rejecting at pre-processing any function body which does not end in a terminating statement.

The official implementation for terminating statements seems to be the following: https://cs.opensource.google/go/go/+/refs/tags/go1.21.0:src/go/types/return.go;l=17

[ajnavarro]

@thehowl @moul What do you propose to fix this problem? Shall we add a static analysis phase when interpreting the code? Thanks.

[thehowl]

What do you propose to fix this problem? Shall we add a static analysis phase when interpreting the code? Thanks.

@ajnavarro My proposal is to add static analysis as a step done in the preprocessor when finishing parsing a function literal or declaration. This is how we can catch the error at the preprocessing stage, before execution.

[KemalBekir]

What do you think about looping through all function statements, if the function is declared to return something and if there is non terminating statement, assert the function is ending with a return statement?
Looks like this code can be added somewhere here

gno/gnovm/pkg/gnolang/preprocess.go

Line 462 in 1c6d184

case *FuncDecl:

[petar-dambovaliev]

What do you think about looping through all function statements, if the function is declared to return something and if there is non terminating statement, assert the function is ending with a return statement? Looks like this code can be added somewhere here

gno/gnovm/pkg/gnolang/preprocess.go

Line 462 in 1c6d184

case *FuncDecl:

That sounds reasonable.
Keep in mind that this needs to work with function literals as well.
Your algorithm would need to maintain a stack of contexts for nested functions, so
it can differentiate which return statement refers to which function.

func Foo() {
     a := func() int {
         return 5
     }
}

And this PR needs to come with a lot of tests.

[thehowl]

@KemalBekir:

What do you think about looping through all function statements, if the function is declared to return something and if there is non terminating statement, assert the function is ending with a return statement?
Looks like this code can be added somewhere here

approach/location looks good to me. Do note that whatever happens in FuncDecl should be matched with what happens with FuncLitExpr. Aside from that, looks good. Probably a good idea to iterate from the bottom and check using the "algorithm" here. Sorry for the delay getting back at you.

[petar-dambovaliev]

@KemalBekir:

What do you think about looping through all function statements, if the function is declared to return something and if there is non terminating statement, assert the function is ending with a return statement?
Looks like this code can be added somewhere here

approach/location looks good to me. Do note that whatever happens in FuncDecl should be matched with what happens with FuncLitExpr. Aside from that, looks good. Probably a good idea to iterate from the bottom and check using the "algorithm" here. Sorry for the delay getting back at you.

Yea. I think, @KemalBekir should paste these rules from the Go spec as a comment next to the code that does the analysis.

[moul]

SGTM, you should proceed. Thank you.