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

Add recommendation to Effective Gno: limit init to only being declarable once #1482

Open
1 task
waymobetta opened this issue Dec 21, 2023 · 5 comments
Open
1 task
Labels
📖 documentation Improvements or additions to documentation

Comments

@waymobetta
Copy link
Contributor

waymobetta commented Dec 21, 2023

Description

Similarly to Go, a .gno file can support multiple init functions within the same file. This is odd to see though, apparently, is completely valid; deploys, runs and is able to be queried.

Playground example

Result: data: ("bar" std.Address), meaning it operates like the LIFO (last-in; first-out) principle.

Perhaps, to avoid confusion, and to further differentiate it from Go's init, we make the Gno init behave more like the main function in that it can only be declared once.

TODO:

  • add to Effective Gno once PR is merged
@waymobetta
Copy link
Contributor Author

Furthering the conversation on this, since we are not using this in the same way as Go- rather we are using it as a constructor function to execute only a single time during its lifecycle (at deployment)- having multiple constructors could actually be problematic since those coming from other smart contract languages (eg, Solidity) are accustomed to only declaring a single constructor function within their application, despite having multiple files; in other words, there is no constructor "overloading" allowed.

If we choose to continue allowing constructor "overloading," at the very least we should make it apparent in the documentation that whatever data is set within these constructors may get overwritten (especially if the same variable is mentioned) which could lead to unintended consequences or, at least, create a confusing order of operations when determining which init executes before the other if there are multiple found present in an app.

@thehowl
Copy link
Member

thehowl commented Jan 10, 2024

Just a quick note: there will almost certainly always be ways to create "multiple constructor functions", even if we restrict init to being declarable only once.

The reason is that there is a ""secret"" alternative way to express the init function:

package hello

func main() {
  println(x)
}

var x int

var _ = func() int {
  x = 11
  return 0
}()

playground

This will output 11 -- and is intended behaviour.

Variables can be initialised to any expression without restriction per the Go spec (excluding cyclical references, whereby then initialization order could not be determined).

So, while I get your point in theory, unless we also change variable initialization radically for Gno (such as, disabling it entirely, which I don't think is a good idea) there will always be ways for people to have "hidden initialisation side-effects" in other files.

@waymobetta
Copy link
Contributor Author

waymobetta commented Jan 10, 2024

That's true, but your example isn't a widely used concept and one that is expressed within Go standard documentation (+ Effective Go); in other words, though it works, it feels more like a hack.

What I am suggesting is taking what Go programmers ordinarily understand to be normal behavior (multiple init scattered throughout their app for db initializations, etc.) and restricting it to a single declaration so they don't do something which could create unintended consequences because the behavior of our init is different than that of Go's.

@thehowl
Copy link
Member

thehowl commented Jan 18, 2024

I'm not completely opposed. It does simplify some things in many areas, and regardless of what road we undertake we should encourage having at most one init function, be that through the linter and/or Effective Gno.

That being said, we still need to consider that:

  • Restricting init to only being declarable once would entail a deviation from the Go spec, marking the first "true" incompatibility from the Go specification (the ones we have, like the lack of complex numbers, goroutines and channels, are all features that we'll probably consider adding in some capacity)
  • Restricting init to being declarable only once does not improve "security" when auditing a contract, but the existance of the feature to a biased Go programmer may invoke a false sense of security (due to being unaware of variable initialisation through self-executing closures).

For this reason, I think it makes more sense to have init declarable once as a recommendation rather than a language change.

@waymobetta waymobetta changed the title Limit init to only being declarable once Add recommendation to Effective Gno: limit init to only being declarable once Jan 18, 2024
@waymobetta
Copy link
Contributor Author

Yep, I hear what you are saying in terms of deviating from Go spec and I like that recommendation as a good middleground without introducing further complications. It's good we've had a discussion on this to refer back to and support what is found in Effective Gno/best practices/recommendations.

@waymobetta waymobetta added the 📖 documentation Improvements or additions to documentation label Jan 18, 2024
@waymobetta waymobetta moved this to Awaiting/Blocked in 👥 Team: Devrels Jan 18, 2024
thehowl added a commit that referenced this issue Mar 21, 2024
<!-- please provide a detailed description of the changes made in this
pull request. -->
This PR ensures in `MemPackage.Validate` that the files that it is being
passed are correctly sorted. This is called in many places, among which
the VM keeper.

**Context:**
It was discovered that during the add package process, gno files get
processed in an unpredictable order.

This relates to the below related issue (#1482) regarding multiple
`init` statements being allowed since these `init` statements would
potentially be processed in unknown order.

Related: #1482

<details><summary>Contributors' checklist...</summary>

- [ ] 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
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Co-authored-by: Morgan Bazalgette <morgan@morganbaz.com>
@leohhhn leohhhn moved this from Awaiting/Blocked to Backlog in 👥 Team: Devrels Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📖 documentation Improvements or additions to documentation
Projects
Status: Backlog
Development

No branches or pull requests

2 participants