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(gnovm): prevent cyclic references in struct declarations #2081

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

ltzmaxwell
Copy link
Contributor

@ltzmaxwell ltzmaxwell commented May 13, 2024

Address #2008.

In this pull request, we're implementing a special handling for struct declarations. Due to their unique nature, we must meticulously search through their fields to identify potential cyclic reference.

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.

@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels May 13, 2024
Copy link

codecov bot commented May 13, 2024

Codecov Report

Attention: Patch coverage is 76.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 52.00%. Comparing base (8cc5636) to head (0442b2a).
Report is 49 commits behind head on master.

Files Patch % Lines
gnovm/pkg/gnolang/preprocess.go 62.50% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2081      +/-   ##
==========================================
+ Coverage   46.27%   52.00%   +5.73%     
==========================================
  Files         483      388      -95     
  Lines       68851    63348    -5503     
==========================================
+ Hits        31862    32946    +1084     
+ Misses      34356    27729    -6627     
- Partials     2633     2673      +40     
Flag Coverage Δ
gnovm 42.07% <76.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@ltzmaxwell ltzmaxwell marked this pull request as draft May 13, 2024 11:36
@zivkovicmilos zivkovicmilos linked an issue May 15, 2024 that may be closed by this pull request
@Kouteki Kouteki added this to the 🏗4️⃣ test4.gno.land milestone May 15, 2024
@petar-dambovaliev
Copy link
Contributor

petar-dambovaliev commented May 15, 2024

This might be solvable by adding some logic in transcribe, without a dependency graph.

This might be a better place than transcribe.

var runDeclarationFor func(fn *FileNode, decl Decl)

I don't know. Try it out and see what is the most appropriate place for this.

@ltzmaxwell ltzmaxwell changed the title WIP: fix(gnovm): Prevent cyclic references in struct declarations fix(gnovm): Prevent cyclic references in struct declarations May 15, 2024
@ltzmaxwell ltzmaxwell marked this pull request as ready for review May 15, 2024 16:25
@ltzmaxwell
Copy link
Contributor Author

This might be solvable by adding some logic in transcribe, without a dependency graph.

This might be a better place than transcribe.

var runDeclarationFor func(fn *FileNode, decl Decl)

I don't know. Try it out and see what is the most appropriate place for this.

This is great and proves to be the right way to go. It's already updated, please take a look @petar-dambovaliev .

@ltzmaxwell ltzmaxwell changed the title fix(gnovm): Prevent cyclic references in struct declarations fix(gnovm): prevent cyclic references in struct declarations May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: No status
Status: In Review
Development

Successfully merging this pull request may close these issues.

bug: self-referential types crash the VM
3 participants