chore(gnovm): optimize err msg #4936

ltzmaxwell · 2025-12-03T07:39:10Z

this PR improves the error message for comparison mismatch that mentioned in #4911.

Gno2D2 · 2025-12-03T07:39:37Z

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

☑️ 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: ltzmaxwell/gno)
Then
🟢 Requirement 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

codecov · 2025-12-03T07:52:32Z

Codecov Report

❌ Patch coverage is 70.58824% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gnovm/pkg/gnolang/type_check.go	61.11%	4 Missing and 3 partials ⚠️
gnovm/pkg/gnolang/preprocess.go	78.57%	2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

MikaelVallenet · 2025-12-04T19:40:22Z

gnovm/pkg/gnolang/type_check.go

+				err := checkAssignableTo(x, xt, dt)
+				if err != nil {
+					panic(fmt.Sprintf("invalid operation: %v (mismatched types %v and %v)", x, xt, dt))
+				}


Should we wrap the error in the panic message ?
We may loose some information isn't ? Looks fine to me since the message indicate the position and show both types so it's enough to understand where & what is the problem.

Also in preprocess.go we show this message, should we have make the panic messages similar for consistence ?

if checkAssignableTo(n, tt, st) != nil { panic( fmt.Sprintf( "cannot use %v (value of type %s) as %s value in assignment", valueExpr.String(), tt.String(), st.String(), ), ) }

Also what about editing directly assertAssignableTo to print your improved message instead doing it specifically at one place.

Should we wrap the error in the panic message ?

I think the low-level message is useful for debugging, but not for end users. We should log it instead of returning. :51625b8

Also in preprocess.go we show this message, should we have make the panic messages similar for consistence ?

it looks the correct, see gnovm/tests/files/assign33.gno.

Also what about editing directly assertAssignableTo to print your improved message instead doing it specifically at one place.

for me it still make sense that it provides lower-level infos.

MikaelVallenet · 2025-12-05T08:08:57Z

gnovm/pkg/gnolang/type_check.go

 }

-func assertAssignableTo(n Node, xt, dt Type) {
+func mustAssignableTo(n Node, xt, dt Type) {


nit: but why must overt assert
IMO, i use must when i expect something in return like: "mustGetEntity" and assert when i don't expect a return but it's how i see it, just is there a reason behind this change ?

for me, must is more of a convention from Go stdlibs, e.g.

regexp.MustCompile(...) (Panics if regex is invalid) template.Must(...) (Panics if template parsing fails)

while assert is always used in unit test, e.g. testify/assert.

MikaelVallenet

LGTM

ltzmaxwell · 2025-12-05T09:28:38Z

gnovm/pkg/gnolang/type_check.go

+				err := checkAssignableTo(x, xt, dt)
+				if err != nil {
+					if debug {
+						debug.Printf("checkAssignableTo fail: %v\n", err)
+					}
+					panic(fmt.Sprintf("invalid operation: %v (mismatched types %v and %v)", x, xt, dt))
+				}


log low-level error and return user friendly info, which is more in line with Go.

thehowl

looks like an improvement, but i would avoid re-writing the full expression in the error

thehowl · 2025-12-09T17:55:48Z

gnovm/pkg/gnolang/type_check.go

+					if debug {
+						debug.Printf("checkAssignableTo fail: %v\n", err)
+					}
+					panic(fmt.Sprintf("invalid operation: %v (mismatched types %v and %v)", x, xt, dt))


security-wise, we should maybe avoid panicking with the whole expression, as it might be very deep. what about making the error "types %v and %v are not comparable"?

optimize err msg

440dbaf

github-project-automation bot added this to 💪 Bounties & Worx and 🧙‍♂️Gno.land development Dec 3, 2025

github-project-automation bot moved this to Triage in 🧙‍♂️Gno.land development Dec 3, 2025

github-actions bot assigned ltzmaxwell Dec 3, 2025

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

Kouteki requested review from MikaelVallenet and jaekwon December 4, 2025 18:00

Kouteki moved this from Triage to In Review in 🧙‍♂️Gno.land development Dec 4, 2025

Kouteki requested a review from thehowl December 4, 2025 18:00

MikaelVallenet reviewed Dec 4, 2025

View reviewed changes

log low-leval error

51625b8

MikaelVallenet reviewed Dec 5, 2025

View reviewed changes

MikaelVallenet approved these changes Dec 5, 2025

View reviewed changes

add test

7663fdc

ltzmaxwell mentioned this pull request Dec 5, 2025

Incorrect relations between interfaces can cause realm panics (cont.) #4911

Open

ltzmaxwell changed the title ~~WIP chore(gnovm): optimize err msg~~ chore(gnovm): optimize err msg Dec 5, 2025

ltzmaxwell commented Dec 5, 2025

View reviewed changes

thehowl reviewed Dec 9, 2025

View reviewed changes

chore(gnovm): optimize err msg #4936

Are you sure you want to change the base?

chore(gnovm): optimize err msg #4936

Uh oh!

Conversation

ltzmaxwell commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gno2D2 commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

Uh oh!

codecov bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MikaelVallenet Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltzmaxwell Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikaelVallenet Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

ltzmaxwell Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

MikaelVallenet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltzmaxwell Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

thehowl left a comment

Choose a reason for hiding this comment

Uh oh!

thehowl Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ltzmaxwell commented Dec 3, 2025 •

edited

Loading

Gno2D2 commented Dec 3, 2025 •

edited

Loading

codecov bot commented Dec 3, 2025 •

edited

Loading

MikaelVallenet Dec 4, 2025 •

edited

Loading

ltzmaxwell Dec 5, 2025 •

edited

Loading

MikaelVallenet left a comment •

edited

Loading