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

fix(gnolang): reject invalid undefined/nil type conversions #1347

Closed
wants to merge 2 commits into from

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Nov 9, 2023

Fixed to throw an error when trying to convert the type of nil, such as int(nil) or bool(nil) and so on.

related with #1341

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.

@notJoon notJoon requested review from jaekwon and moul as code owners November 9, 2023 12:33
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Nov 9, 2023
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (789f4de) 47.91% compared to head (73b28ae) 47.90%.
Report is 19 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1347      +/-   ##
==========================================
- Coverage   47.91%   47.90%   -0.01%     
==========================================
  Files         372      368       -4     
  Lines       63019    62130     -889     
==========================================
- Hits        30194    29762     -432     
+ Misses      30363    29951     -412     
+ Partials     2462     2417      -45     
Files Coverage Δ
gnovm/pkg/gnolang/values_conversions.go 3.83% <ø> (-0.83%) ⬇️

... and 38 files with indirect coverage changes

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

@thehowl thehowl changed the title feat: Handle undefined/nil type conversion fix(gnolang): reject invalid undefined/nil type conversions Nov 9, 2023
@thehowl
Copy link
Member

thehowl commented Nov 9, 2023

mostly LGTM, I would just change the place for the tests :)

@mvertes
Copy link
Contributor

mvertes commented Dec 6, 2023

I think that this PR is made un-necessary if we apply first #1177 which fixes the root cause of attempt to convert untyped values, specially the block here

Note that converting undefined values is still legitimate in Go if you convert nil to a pointer, a slice or array type, or a map for example. Those cases should not be excluded as you did.

@thehowl
Copy link
Member

thehowl commented Jan 18, 2024

Closing this PR.

With Marc's work on #1177 and Maxwell's work on #1426, we have already fixed the issues tackled by this PR.

@notJoon, you are still welcome to open another PR which only adds the test that you added for this one :)

@thehowl thehowl closed this Jan 18, 2024
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
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants