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 2 of 3 #1246

Merged
merged 12 commits into from
May 31, 2024

Conversation

piux2
Copy link
Contributor

@piux2 piux2 commented Oct 15, 2023

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

In this part of the solution, we have made several improvements:

  • Support both named and unnamed type assignments in assignment statements and function return values.
  • Resolved the issue related to incorrect method selectors that is caused by mixing named and unnamed assignments.
  • Added 62 file tests to ensure the correctness of the code.
  • Included 2 realm tests to further validate the cross realm assignment and method selector.
  • Enhanced the support for GNO file tests in nested directories. This allows us to organize tests in intuitively named folders.

To achieve the above improvements in the preprocessing phase, we made the following changes:

  • Introduced an isNamed() function on the Type Interface and marked named types with isNamed() returning true. This helps distinguish between named and unnamed types.
  • Followed the specifications to convert the right-hand side type into a constant function type.
  • As for determining the package associated with a test file, we've maintained the original convention. We keeps relying on the comment directive "//PKGPATH: gno.land/r/xyz" in the test file itself to identify the package it belongs to. We do not using the local folder structure to derive the package for file tests. Therefore the tests in tests/files folder can be stored in any intuitively named sub directories.

NOTE: The named and unnamed type conversions that involve the decomposition of function calls returning multiple values in the preprocess have not yet been included in this pull request. This functionality will be addressed in part 3 of 3 of the entire solution.

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, or a BREAKING CHANGE: xxx message was included in the description
  • 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.

@piux2 piux2 requested review from jaekwon, moul and a team as code owners October 15, 2023 23:33
@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related labels Oct 15, 2023
@piux2 piux2 changed the title feat:named and unnamed type assignment 2 of 3 feat: named and unnamed type assignment 2 of 3 Oct 15, 2023
@codecov
Copy link

codecov bot commented Oct 15, 2023

Codecov Report

Attention: Patch coverage is 74.64789% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 54.63%. Comparing base (ad71860) to head (b2180d5).
Report is 1 commits behind head on master.

Files Patch % Lines
gnovm/pkg/gnolang/types.go 60.97% 16 Missing ⚠️
gnovm/pkg/gnolang/preprocess.go 93.10% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1246   +/-   ##
=======================================
  Coverage   54.62%   54.63%           
=======================================
  Files         578      578           
  Lines       77809    77868   +59     
=======================================
+ Hits        42503    42541   +38     
- Misses      32132    32154   +22     
+ Partials     3174     3173    -1     
Flag Coverage Δ
gnovm 59.99% <74.64%> (+0.01%) ⬆️

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.

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.

Most comments pertain to clarity, ease of future debugging, and refactoring to make the code more readable. Overall good work! 🎉

gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go 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/types.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/types.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/types.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/values_string.go Outdated Show resolved Hide resolved
gnovm/tests/file_test.go Outdated Show resolved Hide resolved
@thehowl
Copy link
Member

thehowl commented Nov 9, 2023

I've also pushed a commit to gofumpt all of the files you added. Many were misaligned and were bothering me in review -- the only additional changes I've done were to fix up the line numbers that as a consequence did not match.

@zivkovicmilos
Copy link
Member

@piux2 @thehowl
What is the status of this PR?

@piux2 piux2 requested a review from a team as a code owner April 4, 2024 05:16
@piux2
Copy link
Contributor Author

piux2 commented Apr 9, 2024

It's ready to review again. @thehowl @zivkovicmilos @deelawn

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.

🎉

gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
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.

Pre-approving. Let's merge it this week.

@thehowl thehowl merged commit b4a07ed into gnolang:master May 31, 2024
46 checks passed
DIGIX666 pushed a commit to DIGIX666/gno that referenced this pull request Jun 2, 2024
This is part 2 of 3 of the solution for issue gnolang#1141. The part 1 of 3 of
the solution can be found in issue gnolang#1143.

In this part of the solution, we have made several improvements:

- Support both named and unnamed type assignments in assignment
statements and function return values.
- Resolved the issue related to incorrect method selectors that is
caused by mixing named and unnamed assignments.
- Added 62 file tests to ensure the correctness of the code.
- Included 2 realm tests to further validate the cross realm assignment
and method selector.
- Enhanced the support for GNO file tests in nested directories. This
allows us to organize tests in intuitively named folders.

To achieve the above improvements in the preprocessing phase, we made
the following changes:

- Introduced an isNamed() function on the Type Interface and marked
named types with isNamed() returning true. This helps distinguish
between named and unnamed types.
- Followed the specifications to convert the right-hand side type into a
constant function type.
- As for determining the package associated with a test file, we've
maintained the original convention. We keeps relying on the comment
directive "//PKGPATH: gno.land/r/xyz" in the test file itself to
identify the package it belongs to. We do not using the local folder
structure to derive the package for file tests. Therefore the tests in
tests/files folder can be stored in any intuitively named sub
directories.

**NOTE:** The named and unnamed type conversions that involve the
decomposition of function calls returning multiple values in the
preprocess have not yet been included in this pull request. This
functionality will be addressed in part 3 of 3 of the entire solution.

<!-- 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
- [ ] 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: deelawn <dboltz03@gmail.com>
Co-authored-by: Morgan Bazalgette <morgan@morganbaz.com>
omarsy pushed a commit to TERITORI/gno that referenced this pull request Jun 3, 2024
This is part 2 of 3 of the solution for issue gnolang#1141. The part 1 of 3 of
the solution can be found in issue gnolang#1143.

In this part of the solution, we have made several improvements:

- Support both named and unnamed type assignments in assignment
statements and function return values.
- Resolved the issue related to incorrect method selectors that is
caused by mixing named and unnamed assignments.
- Added 62 file tests to ensure the correctness of the code.
- Included 2 realm tests to further validate the cross realm assignment
and method selector.
- Enhanced the support for GNO file tests in nested directories. This
allows us to organize tests in intuitively named folders.

To achieve the above improvements in the preprocessing phase, we made
the following changes:

- Introduced an isNamed() function on the Type Interface and marked
named types with isNamed() returning true. This helps distinguish
between named and unnamed types.
- Followed the specifications to convert the right-hand side type into a
constant function type.
- As for determining the package associated with a test file, we've
maintained the original convention. We keeps relying on the comment
directive "//PKGPATH: gno.land/r/xyz" in the test file itself to
identify the package it belongs to. We do not using the local folder
structure to derive the package for file tests. Therefore the tests in
tests/files folder can be stored in any intuitively named sub
directories.

**NOTE:** The named and unnamed type conversions that involve the
decomposition of function calls returning multiple values in the
preprocess have not yet been included in this pull request. This
functionality will be addressed in part 3 of 3 of the entire solution.

<!-- 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
- [ ] 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: deelawn <dboltz03@gmail.com>
Co-authored-by: Morgan Bazalgette <morgan@morganbaz.com>
@jaekwon
Copy link
Contributor

jaekwon commented Jun 5, 2024

Good stuff!

piux2 added a commit that referenced this pull request Jul 19, 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

<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ć <milos.zivkovic@tendermint.com>
Co-authored-by: Morgan <git@howl.moe>
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ć <milos.zivkovic@tendermint.com>
Co-authored-by: Morgan <git@howl.moe>
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 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Status: 🚀 Needed for Launch
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants