Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(gnolang): print nil slices as undefined #1380

Merged
merged 15 commits into from
Dec 5, 2023
Merged

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Nov 17, 2023

The println function was encountering issues when trying to process nil values(e.g. nil slice). This was causing memory errors. The issue has been addressed by adding checks for nil values and handling them appropriately to prevent memory errors.

related with: #1377

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Nov 17, 2023
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (1126d9f) 55.98% compared to head (5ee22c9) 56.54%.
Report is 14 commits behind head on master.

Files Patch % Lines
gnovm/pkg/gnolang/values_string.go 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1380      +/-   ##
==========================================
+ Coverage   55.98%   56.54%   +0.55%     
==========================================
  Files         420      423       +3     
  Lines       65443    66448    +1005     
==========================================
+ Hits        36640    37570     +930     
- Misses      25940    25986      +46     
- Partials     2863     2892      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Nov 20, 2023
@notJoon notJoon marked this pull request as ready for review November 20, 2023 02:49
@notJoon notJoon requested review from jaekwon, moul and a team as code owners November 20, 2023 02:49
@github-actions github-actions bot removed the 🧾 package/realm Tag used for new Realms or Packages. label Nov 30, 2023
@deelawn
Copy link
Contributor

deelawn commented Nov 30, 2023

Awesome. Thanks for addressing my comments. I'll take another look shortly.

@deelawn
Copy link
Contributor

deelawn commented Nov 30, 2023

I've been looking at this closely and I think the most correct way to fix this issue may be by following example of how nil function variables are printed -- nil <function type>. So rather than do any kind of check before the type assertion switch case block, the changes could be limited to the complex types here:

case *ArrayType:
return tv.V.(*ArrayValue).String()
case *SliceType:
return tv.V.(*SliceValue).String()
case *StructType:
return tv.V.(*StructValue).String()
case *MapType:
return tv.V.(*MapValue).String()
case *FuncType:
switch fv := tv.V.(type) {
case nil:
ft := tv.T.String()
return "nil " + ft
case *FuncValue:
return fv.String()
case *BoundMethodValue:
return fv.String()
default:
panic(fmt.Sprintf(
"unexpected func type %v",
reflect.TypeOf(tv.V)))
}
case *InterfaceType:
if debug {
if tv.DebugHasValue() {
panic("should not happen")
}
}
return nilStr
case *TypeType:
return tv.V.(TypeValue).String()
case *DeclaredType:
panic("should not happen")
case *PackageType:
return tv.V.(*PackageValue).String()
case *ChanType:
panic("not yet implemented")
// return tv.V.(*ChanValue).String()
case *NativeType:
return fmt.Sprintf("%v",
tv.V.(*NativeValue).Value.Interface())

I think PointerType and InterfaceType can be left as is, along with any other types that just panic. But the other cases that do type assertions on tv.V should add a check to see if it is nil. If it is, then it should result in a string with the value nil <tv.T.String()>.

What do you think about that approach?

@notJoon
Copy link
Member Author

notJoon commented Dec 1, 2023

should result in a string with the value nil <tv.T.String()>.

Yes, that seems reasonable. This approach will ensure nil values are treated consistently across all types.

Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job; looks good to me 👍

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you 👍

@moul moul merged commit 71ba96d into gnolang:master Dec 5, 2023
184 of 186 checks passed
@notJoon notJoon deleted the issue-1377 branch December 6, 2023 00:42
gfanton pushed a commit to moul/gno that referenced this pull request Jan 18, 2024
The `println` function was encountering issues when trying to process
nil values(e.g. nil slice). This was causing memory errors. The issue
has been addressed by adding checks for nil values and handling them
appropriately to prevent memory errors.

related with: gnolang#1377
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants