fix(gnovm): len/array of array with function calls should not be constant #3714

omarsy · 2025-02-09T17:48:25Z

closes: #3701

…tant

Gno2D2 · 2025-02-09T17:50:14Z

🛠 PR Checks Summary

All Automated Checks passed. ✅

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)
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ 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
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: omarsy/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🔴 At least 1 user(s) of the organization reviewed the pull request (with state "APPROVED")
    │       ├── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)

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

…tion with seen tracking

codecov · 2025-02-11T18:39:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

thehowl · 2025-02-18T19:12:19Z

@ltzmaxwell can you take a look for a first review? I think there must be something better than implementing AsExpr, I think you might have a better idea

ltzmaxwell · 2025-02-20T09:38:53Z

gnovm/pkg/gnolang/type_check.go

+						switch fv.Name {
+						case "len", "cap":
 							at := evalStaticTypeOf(store, last, currExpr.Args[0])
-							if _, ok := unwrapPointerType(baseOf(at)).(*ArrayType); ok {
-								// ok
-								break Main
+							if AsExpr(currExpr.Args[0], &CallExpr{}, &seen[Expr]{}) {
+								panic(fmt.Sprintf("%s (value of type %s) is not constant", currExpr.String(), at))


can we just check into the compositLit's Elts, and do:
assertValidConstValue(store, last, elt.Value) ?

Your point is that the composite literal expression I think can't be used here due to the rule:

The expressions len(s) and cap(s) are constants if the type of s is an array or pointer to an array and the expression s does not contain channel receives or (non-constant) function calls; in this case s is not evaluated.

For instance, in the following code:

package main import "fmt" func S() [2]int { return [2]int{} } func main() { const s = len(S()) fmt.Println("Hello, 世界", s) }

the call to S() is a function call, which makes len(S()) non-constant.

ltzmaxwell · 2025-02-20T09:40:03Z

gnovm/tests/files/const53.gno

+package main
+
+import "fmt"
+
+func main() {
+	const a = len([1]int{
+		func() int { println("hey!!"); return 1 }(),
+	})
+	fmt.Println("Hello", a)
+}
+
+// Error:


we can also add another test to clarify, which is valid:

nestedArr := [1]int{func() int { println("hey!!"); return 1 }()} const a = len(nestedArr)

should be done here 8c71cc6 ^^

mvertes

This change introduces a regression at least for the following case:

package main

func main() {
    a := 1.0
    const c1 = len([1]int{int(a)})
    println(c1)
}

which now panics, but should not (and wasn't).

thehowl · 2025-03-06T15:14:23Z

@omarsy can you address the feedback on this PR?

…xpression retrieval to filter out constant functions

omarsy · 2025-03-06T21:32:57Z

This change introduces a regression at least for the following case:
package main

func main() {
    a := 1.0
    const c1 = len([1]int{int(a)})
    println(c1)
}
which now panics, but should not (and wasn't).

Good catch! I missed that in the spec. Constant function calls are indeed allowed. This should done here 8c71cc6.

ltzmaxwell · 2025-03-07T01:51:55Z

note this should be invalid:

	const a = len([1]int{len([]int{1})})

jaekwon · 2025-03-07T02:46:40Z

gnovm/pkg/gnolang/type_check.go

-								break Main
+							exps := getExpr(currExpr.Args[0], &CallExpr{}, &seen[Expr]{})
+							for _, exp := range exps {
+								cx := exp.(*CallExpr)


I don't understand how they are all CallExprs.

They are all CallExprs because the getExprs function is explicitly filtering for that type. When we call getExpr, we pass a *CallExpr as a template so that only nodes matching a call expression are collected.

jaekwon · 2025-03-07T02:48:38Z

gnovm/pkg/gnolang/type_check.go

@@ -1276,3 +1286,192 @@ func isBlankIdentifier(x Expr) bool {
 	}
 	return false
 }
+
+func getExpr(base Expr, expr Expr, seen *seen[Expr]) (res []Expr) {


not sure why Gno needs to do this, and why Go's typechecker can't just pick up on it, but it seems this name should be changed, as it does not return a single expression, and getExprs() still isn't descriptive.

I'm also wondering if this could be implemented more succinctly using Transcribe().
See findGotoLoopDefines or findLoopUses1 for example usage, see usage of TRANS_SKIP in findGotoLoopDefines for example, to skip over function bodies.

Nice suggestion ^^! I was looking into this as well and couldn’t find anything existing that did it the way I needed. Indeed, using Transcribe() is a better fit for this task, it simplifies the implementation. I've updated the code accordingly. Check out the commit here: [875c04f5].
Thanks again for the feedback ^^!

thehowl · 2025-03-07T18:26:05Z

Deferring to post-mainnet launch, as this code is rejected by the type checker, and thus not accepted on-chain. See comment on #3701.

fix(gnovm): len/array of array with function calls should not be cons…

04ec952

…tant

github-actions bot assigned omarsy Feb 9, 2025

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Feb 9, 2025

Gno2D2 requested a review from a team February 9, 2025 17:50

refactor(gnolang): enhance ExprAsExpr function to support cycle detec…

bae8615

…tion with seen tracking

omarsy added 3 commits February 11, 2025 21:22

test(gnolang): add tests for ExprAsExpr function

1d5c57d

refactor(gnolang): rename ExprAsExpr function to AsExpr for clarity

77f9979

test(gnolang): add tests for FuncLitExpr handling in TestAsExpr

720412a

omarsy marked this pull request as ready for review February 11, 2025 23:12

Gno2D2 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Feb 11, 2025

Kouteki mentioned this pull request Feb 17, 2025

Minutes: Core Staff Weekly Syncs [every Monday] gnolang/meetings#36

Open

zxxma mentioned this pull request Feb 17, 2025

Teritori's Builder Journey gnolang/hackerspace#7

Open

Merge branch 'master' into fix/3701

33726c5

thehowl added this to the 🚀 Mainnet beta launch milestone Feb 18, 2025

zivkovicmilos requested review from mvertes and ltzmaxwell February 20, 2025 07:53

ltzmaxwell requested changes Feb 20, 2025

View reviewed changes

Gno2D2 removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Feb 20, 2025

mvertes requested changes Feb 20, 2025

View reviewed changes

feat(gnovm): add new test cases for constant evaluation and improve e…

8c71cc6

…xpression retrieval to filter out constant functions

omarsy force-pushed the fix/3701 branch from 3457c13 to 8c71cc6 Compare March 6, 2025 21:25

Merge remote-tracking branch 'upstream/master' into fix/3701

2efcb40

omarsy added 2 commits March 6, 2025 22:42

fix(gnovm): remove unnecessary blank line in type_check.go

9afbf34

fix(gnovm): update error message in const62.gno for clarity

4796d30

jaekwon self-requested a review March 7, 2025 02:37

jaekwon reviewed Mar 7, 2025

View reviewed changes

thehowl modified the milestones: 🚀 Mainnet beta launch, ⏭️Next after mainnet beta Mar 7, 2025

omarsy and others added 3 commits March 9, 2025 16:46

feat(gnovm): use transcribe instead of getExprs

875c04f

Merge branch 'master' into fix/3701

bf60f26

fix: correct formatting in error message for const62.gno

1e88b9c

omarsy requested review from mvertes, ltzmaxwell and jaekwon March 9, 2025 16:03

omarsy added 2 commits March 9, 2025 17:04

edit comment

80bed71

bad delete

06b1926

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gnovm): len/array of array with function calls should not be constant #3714

fix(gnovm): len/array of array with function calls should not be constant #3714

omarsy commented Feb 9, 2025

Gno2D2 commented Feb 9, 2025 •

edited

Loading

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Then

If

Can be checked by

codecov bot commented Feb 11, 2025 •

edited

Loading

thehowl commented Feb 18, 2025

ltzmaxwell Feb 20, 2025

omarsy Mar 6, 2025

ltzmaxwell Feb 20, 2025

omarsy Mar 6, 2025

mvertes left a comment

thehowl commented Mar 6, 2025

omarsy commented Mar 6, 2025 •

edited

Loading

ltzmaxwell commented Mar 7, 2025

jaekwon Mar 7, 2025

omarsy Mar 9, 2025

jaekwon Mar 7, 2025

omarsy Mar 9, 2025

thehowl commented Mar 7, 2025

fix(gnovm): len/array of array with function calls should not be constant #3714

Are you sure you want to change the base?

fix(gnovm): len/array of array with function calls should not be constant #3714

Conversation

omarsy commented Feb 9, 2025

Gno2D2 commented Feb 9, 2025 • edited Loading

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Then

If

Can be checked by

codecov bot commented Feb 11, 2025 • edited Loading

Codecov Report

thehowl commented Feb 18, 2025

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

mvertes left a comment

Choose a reason for hiding this comment

thehowl commented Mar 6, 2025

omarsy commented Mar 6, 2025 • edited Loading

ltzmaxwell commented Mar 7, 2025

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

thehowl commented Mar 7, 2025

Gno2D2 commented Feb 9, 2025 •

edited

Loading

codecov bot commented Feb 11, 2025 •

edited

Loading

omarsy commented Mar 6, 2025 •

edited

Loading