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: implement shadowing rules #1793

Merged
merged 12 commits into from
Mar 23, 2024
Merged

feat: implement shadowing rules #1793

merged 12 commits into from
Mar 23, 2024

Conversation

petar-dambovaliev
Copy link
Contributor

@petar-dambovaliev petar-dambovaliev commented Mar 19, 2024

implements this

Most of the changes are fixing stdlib code where we get errors because of the new shadowing rules.

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Mar 19, 2024
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.49%. Comparing base (41ff374) to head (395bfd0).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1793      +/-   ##
==========================================
+ Coverage   44.80%   47.49%   +2.69%     
==========================================
  Files         459      388      -71     
  Lines       67651    61305    -6346     
==========================================
- Hits        30310    29117    -1193     
+ Misses      34801    29750    -5051     
+ Partials     2540     2438     -102     

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

@petar-dambovaliev petar-dambovaliev added the breaking change Functionality that contains breaking changes label Mar 19, 2024
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Mar 19, 2024
@petar-dambovaliev petar-dambovaliev marked this pull request as ready for review March 19, 2024 12:31
@petar-dambovaliev petar-dambovaliev removed the request for review from thehowl March 19, 2024 19:04
@deelawn
Copy link
Contributor

deelawn commented Mar 19, 2024

Apart from my comment, I think it all looks good and makes sense so long as we're in agreement that this is want we want to do.

@kristovatlas kristovatlas requested a review from a team March 20, 2024 18:40
@kristovatlas
Copy link
Contributor

Would it make sense to include tests for all Go built-in functions? I count about 19 of these currently listed here: https://go.dev/ref/spec#Built-in_functions

@kristovatlas
Copy link
Contributor

I'm supportive of the concept for improved code security.

@deelawn
Copy link
Contributor

deelawn commented Mar 20, 2024

Would it make sense to include tests for all Go built-in functions? I count about 19 of these currently listed here: https://go.dev/ref/spec#Built-in_functions

I think it's more than that -- probably everything grouped as a Predeclared Identifier on that page.

@petar-dambovaliev
Copy link
Contributor Author

Would it make sense to include tests for all Go built-in functions? I count about 19 of these currently listed here: https://go.dev/ref/spec#Built-in_functions

done

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks good 💯

I'm not a fan of introducing even more panics into the codebase, but I realize there is no other option to stop execution currently (errors would require a much larger refactor)

@petar-dambovaliev petar-dambovaliev merged commit 52485ce into master Mar 23, 2024
190 of 191 checks passed
@petar-dambovaliev petar-dambovaliev deleted the shadow branch March 23, 2024 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Functionality that contains breaking changes 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants