fix(gnovm): Improve Error Message in evalStaticTypeOf function
#2695
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2695 +/- ##
==========================================
+ Coverage 54.99% 62.82% +7.82%
==========================================
Files 595 550 -45
Lines 79775 82366 +2591
==========================================
+ Hits 43872 51744 +7872
+ Misses 32581 27183 -5398
- Partials 3322 3439 +117
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
notJoon
changed the title
evalStaticTypeOf functionevalStaticTypeOf function
| } | ||
| return "<unknown function>" |
There was a problem hiding this comment.
This shouldn't happend. I added it just in case.
gnovm/pkg/gnolang/preprocess.go
Outdated
| func getFunctionName(x Expr) string { | ||
| switch expr := x.(type) { | ||
| case *CallExpr: | ||
| if id, ok := expr.Func.(*NameExpr); ok { |
There was a problem hiding this comment.
another name other than id?
| msg = fmt.Sprintf("%s: %s() (no value) used as value", loc, funcName) | ||
| } else { | ||
| msg = fmt.Sprintf("%s: %s() (%d values) used as single value", loc, funcName, valueCount) | ||
| } | ||
|
|
||
| panic(fmt.Errorf("%s\nHint: Ensure the function returns a single value, or use multiple assignment", msg)) |
There was a problem hiding this comment.
This hint may not be very effective if it corresponds to one of the messages above, such as:
panic: main/func.gno:3:1: f() (no value) used as value
Hint: Ensure the function returns a single value, or use multiple assignment.
I mean in this case, "or use multiple assignment" is superfluous。
There was a problem hiding this comment.
Or not, this check should be checked somewhere other than evalStaticTypeOf?
There was a problem hiding this comment.
well I think this part feels odd to me(the original logic in evalStaticTypeOf:
if len(tt.Elts) != 1 {
panic(fmt.Sprintf(
"evalStaticTypeOf() only supports *CallExpr with 1 result, got %s",
tt.String(),
))at the very least for case like:
n := f2()
func f2() (string, int) {
return "hey", 0
}the check can happened in *AssignStmt, to identify a mismatch on LHS and RHS.
There was a problem hiding this comment.
Writing so much just for the sake of discussion.
There was a problem hiding this comment.
Yes @ltzmaxwell I think this check should be done in AssignStmt and ValueDecl in trans_enter
|
|
||
| // getFunctionName attempts to extract the function name from the expression | ||
| func getFunctionName(x Expr) string { | ||
| switch expr := x.(type) { |
There was a problem hiding this comment.
This is a nitpick, but it is a bit strange to have a switch with only one case
|
|
||
| var msg string | ||
| if valueCount == 0 { | ||
| msg = fmt.Sprintf("%s: %s() (no value) used as value", loc, funcName) |
There was a problem hiding this comment.
Maybe the message could be improved here. "no value" doesn't seem as clear as saying something like "function does not return a value".
|
Did you get a chance to see @ltzmaxwell's comments? 🙏 |
Kouteki
added
review/triage-pending
Description
Closes #1082
Enhanced the error message in
evalStaticTypeOffunction to match with go's error message and provide more informative feeback.Example of Errors
Sample code (taken from linked issue)
Before
After
Go version (go playground)
Further Works
I think it would be good to fix
getTypeOffunction as well, but I can't think of a case where it would cause panic.Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description