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(stdlibs): remove support for linkedType in native bindings #1700

Merged
merged 21 commits into from
Mar 18, 2024

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Feb 28, 2024

Split from #1695 for ease of reviewing. Related to #814. Merge order:

  1. feat(stdlibs): remove support for linkedType in native bindings #1700 (this one!)
  2. feat(gno.land): add go type checking to keeper + tx simulation in gnokey #1702
  3. feat(transpiler): transpile gno standard libraries #1695

I scrapped the "linked identifier" feature of native bindings. The reasoning mostly stems from the changes subsequently implemented in #1695, as having "linked identifiers" means that:

  • the Gno linked types must have a different name from Go's if they are within the same package, so their names don't conflict after transpilation
  • in order for the "linked types" to match in the generated code, the AST has to be rewritten to make a type alias (ie. type Address = crypto.Bech32Address)
  • while still allowing the type to be "modifiable" by the std package, because we want to add our own methods -- this is not possible for imported types, obviously
  • and if we try removing methods, this creates errors because the methods on the original types don't match 1-1 those implemented in AST

Although, I think a decent case can be made besides easing our (my) life in transpiling:

  • Under the hood, the linked type mechanism works with Store.AddGno2GoMapping. This uses gonative (read: the bane of my existence -- [chore] Remove NativeValue and NativeType from the VM entirely #1361). If we remove all linked types, we can still pass data between Go and Gno using type literals, which don't incur in naming conflicts.
  • This also makes the workings of "native bindings" clearer in general, as they don't have any special linked types (which were currently hardcoded in the misc/genstd source)

Reviewing notes

  • Banker got severely refactored as it was mapping entire go interfaces into Go; it now uses simple elementary functions, with its old behaviour split between Go and Gno.
  • many other functions (std/native.gno) have also been changed so that their native function only uses primitive types (so everything that used an Address, now uses a string).
  • Due to the naming conflicts already mentioned, Go's Banker has been changed to BankerInterface so that it doesn't conflict.
  • AddGo2GnoMapping is unused in the codebase. This has been removed from Store to disencourage any further usage; removal of Go2Gno code is out of scope for this PR (see [chore] Remove NativeValue and NativeType from the VM entirely #1361)

@thehowl thehowl self-assigned this Feb 28, 2024
@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Feb 28, 2024
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.49%. Comparing base (01e91be) to head (3674d26).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1700      +/-   ##
==========================================
- Coverage   47.51%   47.49%   -0.03%     
==========================================
  Files         388      388              
  Lines       61373    61311      -62     
==========================================
- Hits        29159    29117      -42     
+ Misses      29772    29756      -16     
+ Partials     2442     2438       -4     

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

@thehowl thehowl requested a review from harry-hov March 8, 2024 12:11
@thehowl thehowl merged commit 0f2e755 into master Mar 18, 2024
190 of 191 checks passed
@thehowl thehowl deleted the dev/morgan/natbind-no-linkedtype branch March 18, 2024 16:43
thehowl added a commit that referenced this pull request May 13, 2024
…key (#1702)

Split from #1695 for ease of reviewing. Merge order:

1. #1700 
2. #1702 (this one!)
3. #1695 \
   #1730

This PR removes `TranspileAndCheckMempkg` in favour of performing the
type checking it was supposed to do using `go/types` with a custom
importer. This importer works together with Gno's `Store`, and can as
such be used to type check Gno packages without ever writing a single
file to disk. It is important to note that by "Go type check" I mean a
variety of compile-time checks the Go compiler performs; in fact, this
is much more powerful than running "gofmt" as we are currently doing.

Additionally, it adds a new flag to gnokey, `-simulate`, to control
transaction simulation before committing a transaction. See [this issue
comment](#1702 (comment))

Resolves #1661.

## Reviewing notes

- transpiler.TranspileAndCheckMempkg has been removed from the gnokey
client and gnoclient, in favour of having this step be performed on the
vm keeper. This paves the way for clients to not have to include the
entire GnoVM, which I call a win.
- Stdlib io had a precompiling error due to an unused variable
(`remaining`); I updated it to the latest code on Go's standard
libraries.
- `Store` changes
- `Store` has been changed to have its `getPackage` method work by
detecting import cycles, without risking race conditions (the current
implementation is not thread-safe). This is done by creating a new
store, `importerStore`, which contains the previously imported paths in
the current chain. Cyclic imports are still (correctly) detected in the
tests.
- `GetMemPackage` has been changed to return nil when a package cannot
be found. This matches its behaviour with `GetMemFile`, which already
did this when the file does not exist.
- `GetMemPackage`, if a package is not found in the store, now attempts
retrieving it using Store.GetPackage first. The underlying reason is
that the Gno importer for the type checker needs to access the source of
the standard libraries; however, these are never in any transaction and
are not executed "per se" when the blockchain start. As a consequence,
they may not exist within the Store; as a solution, when using
GetMemPackage, we ensure that a package does not exist by checking if
GetPackage does not retrieve it through getMemPackage and save it.
DIGIX666 pushed a commit to kazai777/gno that referenced this pull request May 15, 2024
…key (gnolang#1702)

Split from gnolang#1695 for ease of reviewing. Merge order:

1. gnolang#1700 
2. gnolang#1702 (this one!)
3. gnolang#1695 \
   gnolang#1730

This PR removes `TranspileAndCheckMempkg` in favour of performing the
type checking it was supposed to do using `go/types` with a custom
importer. This importer works together with Gno's `Store`, and can as
such be used to type check Gno packages without ever writing a single
file to disk. It is important to note that by "Go type check" I mean a
variety of compile-time checks the Go compiler performs; in fact, this
is much more powerful than running "gofmt" as we are currently doing.

Additionally, it adds a new flag to gnokey, `-simulate`, to control
transaction simulation before committing a transaction. See [this issue
comment](gnolang#1702 (comment))

Resolves gnolang#1661.

## Reviewing notes

- transpiler.TranspileAndCheckMempkg has been removed from the gnokey
client and gnoclient, in favour of having this step be performed on the
vm keeper. This paves the way for clients to not have to include the
entire GnoVM, which I call a win.
- Stdlib io had a precompiling error due to an unused variable
(`remaining`); I updated it to the latest code on Go's standard
libraries.
- `Store` changes
- `Store` has been changed to have its `getPackage` method work by
detecting import cycles, without risking race conditions (the current
implementation is not thread-safe). This is done by creating a new
store, `importerStore`, which contains the previously imported paths in
the current chain. Cyclic imports are still (correctly) detected in the
tests.
- `GetMemPackage` has been changed to return nil when a package cannot
be found. This matches its behaviour with `GetMemFile`, which already
did this when the file does not exist.
- `GetMemPackage`, if a package is not found in the store, now attempts
retrieving it using Store.GetPackage first. The underlying reason is
that the Gno importer for the type checker needs to access the source of
the standard libraries; however, these are never in any transaction and
are not executed "per se" when the blockchain start. As a consequence,
they may not exist within the Store; as a solution, when using
GetMemPackage, we ensure that a package does not exist by checking if
GetPackage does not retrieve it through getMemPackage and save it.
thehowl added a commit that referenced this pull request Jun 19, 2024
Merge order:

1. #1700 
2. #1702
3. #1695 (this one!) -- review earlier ones first, if they're still
open!

This PR modifies the Gno transpiler (fka precompiler) to use Gno's
standard libraries rather than Go's when performing transpilation. This
creates the necessity to transpile Gno standard libraries, and as such
support their native bindings. And it removes the necessity for a
package like `stdshim`, and a mechanism like `stdlibWhitelist`.

- Fixes #668. Fixes #1865.
- Resolves #892.
- Part of #814. 
- Makes #1475 / #1576 possible without using hacks like `stdshim`.

cc/ @leohhhn @tbruyelle, as this relates to your work

## Why?

- This PR enables us to perform Go type-checking across the board, and
not use Go's standard libraries in transpiled code. This enables us to
_properly support our own standard libraries_, such as `std` but any
others we might want or need.
- It also paves the way further to go full circle, and have Gno code be
transpiled to Go, and then have "compilable" gno code

## Summary of changes

- The transpiler has been thoroughly refactored.
- The biggest change is described above: instead of maintaing the import
paths like `"strconv"` and `"math"` the same (so using Gno's stdlibs in
Gno, and Go's in Go), the import paths for standard libraries is now
also updated to point to the Gno standard libraries.
- Native functions are handled by removing their definitions when
transpiling, and changing their call expressions where appropriate. This
links the transpiled code directly to their native counterparts.
  - This removes the necessity for `stdlibWhitelist`. 
- As a consequence, `stdshim` is no longer needed and has been removed.
- Test files are still not "strictly checked": they may reference
stdlibs with no matching source, and will not be tested when running
with `--gobuild`. This is because packages like `fmt` have no
representation in Gno code; they only exist as injections in
`tests/imports.go`. I'll fix this eventually :)
- The CLI (`gno transpile`) has been changed to reflect the above
changes.
- Flag `--skip-fmt` has been removed (the result of transpile is always
formatted, anyway), and `--gofmt-binary` too, obviously. `gno transpile`
does not perform validation, but will gladly provide helpful validation
with the `--gobuild` flag.
- There is another PR that adds type checking in `gno lint`, without
needing to run through the transpilation step first:
#1730
- It now works by default by looking at "packages" rather than
individual files. This is necessary so that when performing `transpile`
on the `examples` directory, we can skip those where the gno.mod marks
the module as draft. These modules make use of packages like "fmt",
which because they don't have an underlying gno/go source, cannot be
transpiled.
- Running with `-gobuild` now handles more errors correctly; ie., all
errors not previously captured by the `errorRe` which only matches those
pertaining to a specific file/line.
  - `gnoFilesFromArgs` was unused and as such deleted
- `gnomod`'s behaviour was slightly changed.
- I am of the opinion that `gno mod download` should not precompile what
it downloads; _especially_ to gather the dependencies it has. I've
changed it so that it does a `OnlyImports` parse of the file it
downloads to fetch additional dependencies

Misc:

- `Makefile` now contains a recipe to calculate the coverage for
`gnovm/cmd/gno`, and also view it via the HTML interface. This is needed
as it has a few extra steps (which @gfanton already previously added in
the CI).
- Realms r/demo/art/gnoface and r/x/manfred_outfmt have been marked as
draft, as they depend on packages which are not actually present in the
Gno standard libraries.
  - The transpiler now ignores draft packages by default.
- `ReadMemPackage` now also considers Go files. This is meant to have
on-chain the code for standard libraries like `std` which have native
bindings. We still exclude Go code if it's not in a standard library.
- `//go:build` constraints have been removed from standard libraries, as
go files can only have one and we already add our own when transpiling

## Further improvements

after this PR

- Scope understanding in `transpiler` (so call expressions are not
incorrectly rewritten)
- Correctly transpile gno.mod

---------

Co-authored-by: Antonio Navarro Perez <antnavper@gmail.com>
Co-authored-by: Miloš Živković <milos.zivkovic@tendermint.com>
gfanton pushed a commit to gfanton/gno that referenced this pull request Jul 23, 2024
Merge order:

1. gnolang#1700 
2. gnolang#1702
3. gnolang#1695 (this one!) -- review earlier ones first, if they're still
open!

This PR modifies the Gno transpiler (fka precompiler) to use Gno's
standard libraries rather than Go's when performing transpilation. This
creates the necessity to transpile Gno standard libraries, and as such
support their native bindings. And it removes the necessity for a
package like `stdshim`, and a mechanism like `stdlibWhitelist`.

- Fixes gnolang#668. Fixes gnolang#1865.
- Resolves gnolang#892.
- Part of gnolang#814. 
- Makes gnolang#1475 / gnolang#1576 possible without using hacks like `stdshim`.

cc/ @leohhhn @tbruyelle, as this relates to your work

## Why?

- This PR enables us to perform Go type-checking across the board, and
not use Go's standard libraries in transpiled code. This enables us to
_properly support our own standard libraries_, such as `std` but any
others we might want or need.
- It also paves the way further to go full circle, and have Gno code be
transpiled to Go, and then have "compilable" gno code

## Summary of changes

- The transpiler has been thoroughly refactored.
- The biggest change is described above: instead of maintaing the import
paths like `"strconv"` and `"math"` the same (so using Gno's stdlibs in
Gno, and Go's in Go), the import paths for standard libraries is now
also updated to point to the Gno standard libraries.
- Native functions are handled by removing their definitions when
transpiling, and changing their call expressions where appropriate. This
links the transpiled code directly to their native counterparts.
  - This removes the necessity for `stdlibWhitelist`. 
- As a consequence, `stdshim` is no longer needed and has been removed.
- Test files are still not "strictly checked": they may reference
stdlibs with no matching source, and will not be tested when running
with `--gobuild`. This is because packages like `fmt` have no
representation in Gno code; they only exist as injections in
`tests/imports.go`. I'll fix this eventually :)
- The CLI (`gno transpile`) has been changed to reflect the above
changes.
- Flag `--skip-fmt` has been removed (the result of transpile is always
formatted, anyway), and `--gofmt-binary` too, obviously. `gno transpile`
does not perform validation, but will gladly provide helpful validation
with the `--gobuild` flag.
- There is another PR that adds type checking in `gno lint`, without
needing to run through the transpilation step first:
gnolang#1730
- It now works by default by looking at "packages" rather than
individual files. This is necessary so that when performing `transpile`
on the `examples` directory, we can skip those where the gno.mod marks
the module as draft. These modules make use of packages like "fmt",
which because they don't have an underlying gno/go source, cannot be
transpiled.
- Running with `-gobuild` now handles more errors correctly; ie., all
errors not previously captured by the `errorRe` which only matches those
pertaining to a specific file/line.
  - `gnoFilesFromArgs` was unused and as such deleted
- `gnomod`'s behaviour was slightly changed.
- I am of the opinion that `gno mod download` should not precompile what
it downloads; _especially_ to gather the dependencies it has. I've
changed it so that it does a `OnlyImports` parse of the file it
downloads to fetch additional dependencies

Misc:

- `Makefile` now contains a recipe to calculate the coverage for
`gnovm/cmd/gno`, and also view it via the HTML interface. This is needed
as it has a few extra steps (which @gfanton already previously added in
the CI).
- Realms r/demo/art/gnoface and r/x/manfred_outfmt have been marked as
draft, as they depend on packages which are not actually present in the
Gno standard libraries.
  - The transpiler now ignores draft packages by default.
- `ReadMemPackage` now also considers Go files. This is meant to have
on-chain the code for standard libraries like `std` which have native
bindings. We still exclude Go code if it's not in a standard library.
- `//go:build` constraints have been removed from standard libraries, as
go files can only have one and we already add our own when transpiling

## Further improvements

after this PR

- Scope understanding in `transpiler` (so call expressions are not
incorrectly rewritten)
- Correctly transpile gno.mod

---------

Co-authored-by: Antonio Navarro Perez <antnavper@gmail.com>
Co-authored-by: Miloš Živković <milos.zivkovic@tendermint.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants