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

omarsy · 2024-09-21T14:11:15Z

Related Issues:
Closes #2628

This PR aims to align Gno's constant handling with the Go specification regarding constant expressions (see Go Specification on Constants).

Primitive Type Requirement

Should Work:
```
const t = 1
```

Should Return an Error:

const t = []string{"1"}

Error:

(const (slice[("1" string)] []string)) (value of type []string) is not constant

Function Calls Disallowed
Only built-in functions should be allowed.

Should Work:
```
const t = len("s")
```

Should Return an Error:

func v() string {
    return ""
}
const t = v()

Error:

v<VPBlock(3,0)>() (value of type string) is not constant

Constant Operands Requirement
Constant expressions may contain only constant operands and are evaluated at compile time.
- Should Work:
```
const t = 1
const v = t
```
- Should Raise an Error:
```
t := 1
const v = t
```
  Error:
```
t (variable of type int) is not constant
```

Type Assertion Forbidden

This code:

var i interface{} = 1
const t, ok = i.(int)

Should Raise This Error:

i.(int) (comma, ok expression of type int) is not constant

Index Expression Forbidden

This code:
```
var i = []string{}
const t, ok = i[0]
```

Should Return This Error:

i[0] (variable of type string) is not constant

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 · 2024-09-21T14:12:44Z

Codecov Report

Attention: Patch coverage is 65.18519% with 47 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gnovm/pkg/gnolang/type_check.go	64.92%	37 Missing and 10 partials ⚠️

📢 Thoughts on this report? Let us know!

jefft0 · 2024-10-19T06:45:29Z

@omarsy , can you fix the lint-pr-title CI check?

gnovm/pkg/gnolang/type_check.go

petar-dambovaliev · 2024-11-30T08:38:53Z

Part of your PR related to this but from what i see doesn't address the whole problem. I've fixed it here.

Gno2D2 · 2024-11-30T08:39:38Z

🛠 PR Checks Summary

🔴 Maintainers must be able to edit this pull request (more info)

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)

✅ Automated Checks (for Contributors):

🔴 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include BREAKING CHANGE notes.
- Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

Debug

Automated Checks
Maintainers must be able to edit this pull request (more info)
If
🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: TERITORI/gno)
Then
🔴 Requirement not satisfied
└── 🔴 Maintainer can modify this pull request
Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)
If
🟢 Condition met
└── 🟢 On every pull request
Can be checked by

Any user with comment edit permission

petar-dambovaliev · 2024-12-02T17:51:55Z

Part of your PR related to this but from what i see doesn't address the whole problem. I've fixed it here.

@omarsy i am going to close my PR. You can integrate that weird Go feature for arrays in your PR. Better in the preprocessor so no functions are called during runtime.

omarsy · 2024-12-03T22:37:32Z

Part of your PR related to this but from what i see doesn't address the whole problem. I've fixed it here.

@omarsy i am going to close my PR. You can integrate that weird Go feature for arrays in your PR. Better in the preprocessor so no functions are called during runtime.

I think it is better to handle it in another PR. WDYT ?

gnovm/pkg/gnolang/type_check.go

…roved validation

ltzmaxwell · 2024-12-09T13:29:46Z

gnovm/pkg/gnolang/type_check.go

+	switch vx := vx.(type) {
+	case *NameExpr:
+		t := evalStaticTypeOf(store, last, vx)
+		if _, ok := t.(*ArrayType); ok {


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).

ltzmaxwell · 2024-12-09T13:32:30Z

gnovm/pkg/gnolang/type_check.go

+	assertValidConstantExprRecursively(store, last, expr, nil)
+}
+
+func assertValidConstantExprRecursively(store Store, last BlockNode, currExpr, parentExpr Expr) {


Suggested change

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

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

should be done here 54878f0

…stValue for clarity

thehowl · 2024-12-21T00:07:12Z

@ltzmaxwell ping me if you approve and would like me to take a look as well, otherwise go ahead and merge after you've approved

ltzmaxwell

trying to find more counter cases, please check.

ltzmaxwell · 2024-12-26T03:06:28Z

gnovm/pkg/gnolang/type_check.go

+		case *TypeType:
+			for _, arg := range currExpr.Args {
+				assertValidConstValue(store, last, arg, currExpr)
+			}


please check this:

package main const a = interface{}(nil) func main() { println("ok") }

seems also need to check the type, right?

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

Nice catch ^^

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

ltzmaxwell · 2024-12-26T04:01:32Z

gnovm/pkg/gnolang/type_check.go

+
+			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:


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") }

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

ltzmaxwell · 2024-12-26T04:01:54Z

gnovm/pkg/gnolang/type_check.go

+		}
+	case *ConstExpr:
+	case *BasicLitExpr:
+	case *CompositeLitExpr:


can we just check parent and panic here?

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

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

…essions

ltzmaxwell · 2024-12-31T08:01:13Z

gnovm/pkg/gnolang/type_check.go

+
+			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, *TypeType, *NativeType:


seems *TypeType does not apply. please double check.

ltzmaxwell · 2024-12-31T08:06:59Z

gnovm/pkg/gnolang/type_check.go

+
+func isParentCallExprWithArrayArg(currType Type, parentExpr Expr) bool {
+	_, okArray := baseOf(currType).(*ArrayType)
+	_, okCallExpr := parentExpr.(*CallExpr)


please add a comment.

done here 29b6ab0

ltzmaxwell · 2024-12-31T09:01:36Z

gnovm/pkg/gnolang/type_check.go

+	assertValidConstValue(store, last, expr, nil)
+}
+
+func assertValidConstValue(store Store, last BlockNode, currExpr, parentExpr Expr) {


At the same time, I’m wondering—many of the checks here are for functions like len and cap. So, is this recursive call really necessary? Could most of the cases be handled under callExpr instead? Please consider this.

Apologies for the continuous iterations in reviewing this PR. Thank you for understanding.

Thank you for pointing this out! To clarify, we don't check only if the call expression has len or cap. Our implementation ensures that all expressions are validated for their correctness as constant expressions. For instance, a binary expression with a valid type we will check if it is a valid constant expression.

Here's an example to illustrate this:

package main func main() { v := 5 const a = 1 + 2 + len([2]int{1, 2}) + v println("ok") }

In this example, the constant expression 1 + 2 + len([2]int{1, 2}) is valid because all sub-expressions are constant and have valid types. However, the addition of v, which is a variable, makes the entire expression invalid as a constant expression, and this is correctly flagged by our checks.

github-actions bot assigned omarsy Sep 21, 2024

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Sep 21, 2024

omarsy force-pushed the fix/2628 branch 4 times, most recently from c9b02d2 to f49c088 Compare September 25, 2024 11:54

github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Sep 25, 2024

omarsy force-pushed the fix/2628 branch from f49c088 to b6c5edc Compare September 25, 2024 15:55

zxxma mentioned this pull request Sep 25, 2024

Teritori's Builder Journey gnolang/hackerspace#7

Open

omarsy force-pushed the fix/2628 branch 2 times, most recently from 355d799 to e9a9be7 Compare September 26, 2024 10:31

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

3d5030a

omarsy force-pushed the fix/2628 branch from e9a9be7 to 3d5030a Compare October 18, 2024 20:51

omarsy marked this pull request as ready for review October 18, 2024 20:52

omarsy requested review from jaekwon, moul, piux2, thehowl, mvertes and a team as code owners October 18, 2024 20:52

omarsy changed the title ~~feat(gnovm): align Gno constant handling with Go specifications~~ WIP feat(gnovm): align Gno constant handling with Go specifications Oct 18, 2024

omarsy and others added 2 commits October 19, 2024 00:01

Merge branch 'master' into fix/2628

2543294

feat: correct test

bb2d9fd

jefft0 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 19, 2024

omarsy commented Oct 19, 2024

View reviewed changes

gnovm/pkg/gnolang/type_check.go Outdated Show resolved Hide resolved

omarsy changed the title ~~WIP feat(gnovm): align Gno constant handling with Go specifications~~ feat(gnovm): align Gno constant handling with Go specifications Oct 19, 2024

Kouteki added this to the 🚀 Mainnet launch milestone Oct 22, 2024

omarsy requested a review from ltzmaxwell November 28, 2024 20:55

petar-dambovaliev mentioned this pull request Dec 2, 2024

fix: builtin const func arg eval #3246

Closed

Merge branch 'master' into fix/2628

35d6ae1

ltzmaxwell requested changes Dec 6, 2024

View reviewed changes

gnovm/pkg/gnolang/type_check.go Show resolved Hide resolved

omarsy and others added 2 commits December 7, 2024 14:06

Merge branch 'master' into fix/2628

459f0da

refactor: replace checkConstantExpr with assertValidConstExpr for imp…

1ba4872

…roved validation

ltzmaxwell requested changes Dec 9, 2024

View reviewed changes

omarsy and others added 4 commits December 9, 2024 20:40

refactor: rename assertValidConstantExprRecursively to assertValidCon…

54878f0

…stValue for clarity

Merge branch 'master' into fix/2628

72b8910

Merge branch 'master' into fix/2628

88dae32

Merge branch 'master' into fix/2628

59a7d85

omarsy requested a review from ltzmaxwell December 11, 2024 20:36

ltzmaxwell reviewed Dec 26, 2024

View reviewed changes

fix(gnovm): check type

9001059

omarsy force-pushed the fix/2628 branch from 9dab0d3 to 9001059 Compare December 26, 2024 14:04

fix(gnovm): enhance constant evaluation for array and map types

c45f63e

omarsy force-pushed the fix/2628 branch from 532a25d to c45f63e Compare December 26, 2024 15:00

omarsy and others added 3 commits December 26, 2024 17:23

refactore(gnovm): some refactor

7083830

fix(gnovm): remove unnecessary panic for non-constant array type expr…

4da7934

…essions

Merge branch 'master' into fix/2628

6ef45d4

ltzmaxwell reviewed Dec 31, 2024

View reviewed changes

omarsy and others added 2 commits January 1, 2025 17:00

feat(gnolang): add comment

29b6ab0

Merge branch 'master' into fix/2628

524b995

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

omarsy commented Sep 21, 2024 •

edited by zivkovicmilos

Loading

codecov bot commented Sep 21, 2024 •

edited

Loading

jefft0 commented Oct 19, 2024

petar-dambovaliev commented Nov 30, 2024

Gno2D2 commented Nov 30, 2024 •

edited

Loading

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

petar-dambovaliev commented Dec 2, 2024

omarsy commented Dec 3, 2024

ltzmaxwell Dec 9, 2024 •

edited

Loading

ltzmaxwell Dec 9, 2024

omarsy Dec 10, 2024

thehowl commented Dec 21, 2024

ltzmaxwell left a comment

ltzmaxwell Dec 26, 2024

omarsy Dec 26, 2024 •

edited

Loading

ltzmaxwell Dec 31, 2024

ltzmaxwell Dec 26, 2024

omarsy Dec 26, 2024

ltzmaxwell Dec 26, 2024 •

edited

Loading

omarsy Dec 26, 2024

ltzmaxwell Dec 31, 2024

ltzmaxwell Dec 31, 2024 •

edited

Loading

ltzmaxwell Dec 31, 2024

omarsy Jan 1, 2025

ltzmaxwell Dec 31, 2024

omarsy Jan 1, 2025

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

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

Are you sure you want to change the base?

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

Conversation

omarsy commented Sep 21, 2024 • edited by zivkovicmilos Loading

codecov bot commented Sep 21, 2024 • edited Loading

Codecov Report

jefft0 commented Oct 19, 2024

petar-dambovaliev commented Nov 30, 2024

Gno2D2 commented Nov 30, 2024 • edited Loading

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

petar-dambovaliev commented Dec 2, 2024

omarsy commented Dec 3, 2024

ltzmaxwell Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehowl commented Dec 21, 2024

ltzmaxwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarsy Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltzmaxwell Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltzmaxwell Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarsy commented Sep 21, 2024 •

edited by zivkovicmilos

Loading

codecov bot commented Sep 21, 2024 •

edited

Loading

Gno2D2 commented Nov 30, 2024 •

edited

Loading

ltzmaxwell Dec 9, 2024 •

edited

Loading

omarsy Dec 26, 2024 •

edited

Loading

ltzmaxwell Dec 26, 2024 •

edited

Loading

ltzmaxwell Dec 31, 2024 •

edited

Loading