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: named and unnamed type assignment 3 of 3 #2367

Merged
merged 19 commits into from
Jul 19, 2024

Conversation

piux2
Copy link
Contributor

@piux2 piux2 commented Jun 17, 2024

This is the last part of the solution for issue #1141.
The 1 of 3 of the solution can be found in PR #1143.
The 2 of 3 of the solution can be found in PR #1246

It decomposes function calls that return multiple values in the preprocess.

Here is the problem to solve:

u1, n2 = x()

How do we ensure that the returned multiple values from a function call adhere to named and unnamed type assignment specifications? Additionally, we want to solve this problem during preprocessing instead of at runtime to minimize the impact on runtime performance.

The main ideas:

u1, n2 = x()  << decompose the statement to the following two lines 
// .tmp_1, .tmp_2 := x() 
// u1, n2 = .tmp_1, .tmp_2

then we can apply name and unname type conversion specs to the second line. 
u1, n2 = _tmp_1, _tmp_2

Here are the example code and the explanation

// decompose_filetest.gno
package main

  type nat []int

  func x() (nat, []int) {
    a := nat{1}
    b := []int{2}
    return a, b
  }

  func main() {
    var u1 []int
    var n2 nat

    u1, n2 = x() 
    // .tmp_1, .tmp_2 := x() 
    // u1, n2 = .tmp_1, .tmp_2

    println(u1)
    println(n2)

  }

  // Output:
  // slice[(1 int)]
  // (slice[(2 int)] main.nat)

Here is the simplified recursive tree of the transformation in the preprocess

image

Here are the major steps involved in this decomposition during preprocessing:

  • Create hidden temporary name expressions .tmp1, .tmp2. In Go, a leading dot is not valid in variable names, ensuring that users cannot create names that clash with these hidden variables.

  • Create two statements in the block: one for defining and one for assigning.

 .tmp1, .tmp2 := x() 
 u1, n2 = .tmp_1, .tmp_2
  • Preprocess each newly created statements

  • Replace the original statement with the two newly created statements.

Here are two additional changes to facilitate above.

  • Update the FuncValue's body in updates := pn.PrepareNewValues(pv) since its source Body has been changed during preprocessing.

  • Replace all for index := range Body with for i:=0; i < len(Body); i++ in transcribe.go since the body length might change due to the decomposition.

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
  • 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 the 📦 🤖 gnovm Issues or PRs gnovm related label Jun 17, 2024
Copy link

codecov bot commented Jun 17, 2024

Codecov Report

Attention: Patch coverage is 33.80282% with 47 lines in your changes missing coverage. Please review.

Project coverage is 54.99%. Comparing base (6432573) to head (e214285).
Report is 15 commits behind head on master.

Files Patch % Lines
gnovm/pkg/gnolang/preprocess.go 25.58% 29 Missing and 3 partials ⚠️
gnovm/pkg/gnolang/nodes.go 23.07% 10 Missing ⚠️
gnovm/pkg/gnolang/values.go 33.33% 3 Missing and 1 partial ⚠️
gnovm/pkg/gnolang/transcribe.go 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2367      +/-   ##
==========================================
- Coverage   55.02%   54.99%   -0.03%     
==========================================
  Files         595      595              
  Lines       79645    79960     +315     
==========================================
+ Hits        43824    43974     +150     
- Misses      32503    32670     +167     
+ Partials     3318     3316       -2     
Flag Coverage Δ
contribs/gnodev 26.00% <ø> (+0.34%) ⬆️
contribs/gnofaucet 15.31% <ø> (+0.85%) ⬆️
contribs/gnokeykc 0.00% <ø> (ø)
contribs/gnomd 0.00% <ø> (ø)
gno.land 64.24% <ø> (-0.16%) ⬇️
gnovm 60.25% <33.80%> (-0.06%) ⬇️

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.

@Kouteki Kouteki added this to the 🏗4️⃣ test4.gno.land milestone Jun 20, 2024
Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

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

This seems to work well. I made some suggestion to fix typos. Can you add some tests for a couple more cases?

  • Multiple of these types assignments within the same block
  • This type of assignment inside a global var block

gnovm/pkg/gnolang/nodes.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/nodes.go Show resolved Hide resolved
@piux2
Copy link
Contributor Author

piux2 commented Jul 7, 2024

There is conflict need to be resolved with #1426 first.
a,b = x(...) assignment check is not in the preprocess anymore.

@ltzmaxwell
Copy link
Contributor

ltzmaxwell commented Jul 7, 2024

There is conflict need to be resolved with #1426 first. a,b = x(...) assignment check is not in the preprocess anymore.

Hi @piux2 , part of the logic for *AssignStmt has been moved to type_check.go:

func (x *AssignStmt) AssertCompatible(store Store, last BlockNode) {

LMK if there'a any trouble about the conflicts.

@piux2
Copy link
Contributor Author

piux2 commented Jul 7, 2024

There is conflict need to be resolved with #1426 first. a,b = x(...) assignment check is not in the preprocess anymore.

Hi @piux2 , part of the logic for *AssignStmt has been moved to type_check.go:

func (x *AssignStmt) AssertCompatible(store Store, last BlockNode) {

LMK if there'a any trouble about the conflicts.

Thank you @ltzmaxwell the conflict is resulted, it is ready to merge

gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
@zivkovicmilos zivkovicmilos removed this from the 🏗4️⃣ test4.gno.land milestone Jul 9, 2024
@piux2 piux2 requested a review from ltzmaxwell July 9, 2024 21:15
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

overall lgtm, a few nits.

gnovm/pkg/gnolang/values.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
@jaekwon
Copy link
Contributor

jaekwon commented Jul 15, 2024

i'll take a look this week

@petar-dambovaliev petar-dambovaliev self-requested a review July 16, 2024 05:57
Copy link
Contributor

@jaekwon jaekwon left a comment

Choose a reason for hiding this comment

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

just a few asks but otherwise good to go.

@piux2 piux2 merged commit 1180def into gnolang:master Jul 19, 2024
88 of 89 checks passed
gfanton pushed a commit to gfanton/gno that referenced this pull request Jul 23, 2024
This is the last part of the solution for issue gnolang#1141. 
The  1 of 3 of the solution can be found in PR  gnolang#1143.
The  2 of 3 of the solution can be found in PR gnolang#1246 

It decomposes function calls that return multiple values in the
preprocess.

### Here is the problem to solve: 

`  u1, n2 = x() `

How do we ensure that the returned multiple values from a function call
adhere to named and unnamed type assignment specifications?
Additionally, we want to solve this problem during preprocessing instead
of at runtime to minimize the impact on runtime performance.

### The main ideas:

    u1, n2 = x()  << decompose the statement to the following two lines 
    // .tmp_1, .tmp_2 := x() 
    // u1, n2 = .tmp_1, .tmp_2

then we can apply name and unname type conversion specs to the second
line.
    u1, n2 = _tmp_1, _tmp_2


### Here are the example code and the explanation

```
// decompose_filetest.gno
package main

  type nat []int

  func x() (nat, []int) {
    a := nat{1}
    b := []int{2}
    return a, b
  }

  func main() {
    var u1 []int
    var n2 nat

    u1, n2 = x() 
    // .tmp_1, .tmp_2 := x() 
    // u1, n2 = .tmp_1, .tmp_2

    println(u1)
    println(n2)

  }

  // Output:
  // slice[(1 int)]
  // (slice[(2 int)] main.nat)

```

### Here is the simplified recursive tree of the transformation in the
preprocess

<img width="1336" alt="image"
src="https://github.com/gnolang/gno/assets/90544084/306a4770-457d-4131-a82a-2de5c6b0dadf">

### Here are the major steps involved in this decomposition during
preprocessing:

- Create hidden temporary name expressions .tmp1, .tmp2. In Go, a
leading dot is not valid in variable names, ensuring that users cannot
create names that clash with these hidden variables.


- Create two statements in the block: one for defining and one for
assigning.

```
 .tmp1, .tmp2 := x() 
 u1, n2 = .tmp_1, .tmp_2
```

- Preprocess each newly created statements

- Replace the original statement with the two newly created statements. 


### Here are two additional changes to facilitate above.

- Update the FuncValue's body in `updates := pn.PrepareNewValues(pv)
`since its source Body has been changed during preprocessing.

- Replace all ` for index := range Body` with `for i:=0; i < len(Body);
i++` in transcribe.go since the body length might change due to the
decomposition.


<!-- please provide a detailed description of the changes made in this
pull request. -->

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

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [x] No breaking changes were made
- [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: Miloš Živković <[email protected]>
Co-authored-by: Morgan <[email protected]>
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: Done
Development

Successfully merging this pull request may close these issues.

7 participants