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

docs: gno does not support shadowing native types #1711

Merged
merged 6 commits into from
Mar 20, 2024

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Mar 1, 2024

closes #1091

This is a simple documentation update to clarify that gno does not support shadowing native types. I've discussed with @jaekwon and this is not something we want to support.

@deelawn deelawn requested a review from a team as a code owner March 1, 2024 00:04
@deelawn deelawn changed the title doc: gno does not support shadowing native types docs: gno does not support shadowing native types Mar 1, 2024
Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

Makes sense 👍

@thehowl
Copy link
Member

thehowl commented Mar 1, 2024

BTW to fully close #1091 I suggest we catch the error explicitly and return a useful error message (currently, the fact that we don't allow shadowing native types seems accidental).

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

deelawn commented Mar 1, 2024

BTW to fully close #1091 I suggest we catch the error explicitly and return a useful error message (currently, the fact that we don't allow shadowing native types seems accidental).

Sounds good. I modified the error message that gets returned when this happens.

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.47%. Comparing base (d32b583) to head (38a5b53).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1711   +/-   ##
=======================================
  Coverage   47.47%   47.47%           
=======================================
  Files         385      385           
  Lines       61376    61375    -1     
=======================================
  Hits        29139    29139           
+ Misses      29802    29801    -1     
  Partials     2435     2435           

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

@deelawn
Copy link
Contributor Author

deelawn commented Mar 2, 2024

Still good with this @leohhhn?

Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

@deelawn should it be "built-in"? Otherwise just the capitalization on language names.

docs/reference/go-gno-compatibility.md Outdated Show resolved Hide resolved
@thehowl
Copy link
Member

thehowl commented Mar 3, 2024

Sorry for the back and forth, should have checked earlier, but I guess the closest term to what is on the Go spec is predeclared. The spec still uses built-in, however in relation to functions.

I guess that also raises the question of whether built-in functions are also considered predeclared, and hence not shadowable, or not.

deelawn and others added 2 commits March 4, 2024 01:56
Co-authored-by: Leon Hudak <33522493+leohhhn@users.noreply.github.com>
Co-authored-by: Leon Hudak <33522493+leohhhn@users.noreply.github.com>
@deelawn
Copy link
Contributor Author

deelawn commented Mar 6, 2024

Sorry for the back and forth, should have checked earlier, but I guess the closest term to what is on the Go spec is predeclared. The spec still uses built-in, however in relation to functions.

I guess that also raises the question of whether built-in functions are also considered predeclared, and hence not shadowable, or not.

Should we bring this up during the PR discussion meeting or just change the name to predeclared? I don't have a strong opinion on this.

@leohhhn leohhhn merged commit 7596f42 into gnolang:master Mar 20, 2024
72 of 74 checks passed
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
Status: No status
Archived in project
Development

Successfully merging this pull request may close these issues.

builtin identifiers cannot be shadowed
4 participants