-
Notifications
You must be signed in to change notification settings - Fork 375
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(gonative): add constant information when we define a go native value #2848
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2848 +/- ##
==========================================
+ Coverage 60.90% 60.97% +0.07%
==========================================
Files 564 564
Lines 75273 75279 +6
==========================================
+ Hits 45844 45902 +58
+ Misses 26057 26008 -49
+ Partials 3372 3369 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all values declared using gonative should be not modifiable.
Can we just have that if you have an assignment on a value which is statically typed as a native value, you cannot change it?
We have an example of editing a native value here: https://github.com/gnolang/gno/blob/master/gnovm/tests/files/assign0b_native.gno. I'm unsure if this is something we want to prohibit. |
The fact that values are modifiable makes functions modifiable, too: package main
import (
"fmt"
)
func main() {
fmt.Sprintf = func(v string, p ...interface{}) string { return "123" }
println(fmt.Sprintf("a", "b"))
}
// Output:
// 123 Seeing as we eventually want to remove gonative (#1361), I think it's sensible to remove the ability to modify these entirely. |
Yes maybe we should forbid the edit only for functions and constants. WDYT ? |
Whatever is simplest. |
Added here: 0671c88 Nice catch for the issue ^^ |
First part of the fix: #2836
The second part is addressed here: #2731