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(cmd/gno): perform type checking when calling linter #1730

Merged
merged 81 commits into from
Dec 9, 2024

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Mar 4, 2024

Depends on (in order):

  1. feat(stdlibs): remove support for linkedType in native bindings #1700
  2. feat(gno.land): add go type checking to keeper + tx simulation in gnokey #1702

This PR uses the type checker added in #1702 to perform Gno type checking when calling gno lint. Additionally, it adds validation of gno.mod indirectly (the parsed gno mod is used to determine if a package is a draft, and if so skip type checking).

Because gno lint uses the TestStore, the resulting MemPackages may contain redefinitions, for overwriting standard libraries like AssertOriginCall. I changed the type checker to filter out the redefinitions before they reach the Go type checker.

Further improvements, which can be done after this:

@thehowl thehowl self-assigned this Mar 4, 2024
@thehowl thehowl requested a review from jaekwon as a code owner March 4, 2024 18:40
@thehowl
Copy link
Member Author

thehowl commented May 30, 2024

From review meeting, we decided to add this functionality on gno test, following comments on #2050 (comment) .

gnovm/cmd/gno/lint.go Show resolved Hide resolved
gnovm/pkg/gnolang/go2gno.go Show resolved Hide resolved
@zivkovicmilos
Copy link
Member

@thehowl

All good on my side 🫡

Please check the leftover comms and conflicts, and we're good to merge 🚀

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 <[email protected]>
Co-authored-by: Miloš Živković <[email protected]>
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 <[email protected]>
Co-authored-by: Miloš Živković <[email protected]>
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Dec 8, 2024

I'm a bot that assists the Gno Core team in maintaining this repository. My role is to ensure that contributors understand and follow our guidelines, helping to streamline the development process.

The following requirements must be fulfilled before a pull request can be merged.
Some requirement checks are automated and can be verified by the CI, while others need manual verification by a staff member.

These requirements are defined in this configuration file.

Automated Checks

🔴 Maintainers must be able to edit this pull request (more info)
🟢 The pull request head branch must be up-to-date with its base (more info)

Manual Checks

No manual checks match this pull request.

Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 On every pull request

Then

🔴 Requirement not satisfied
└── 🔴 Maintainer can modify this pull request

The pull request head branch must be up-to-date with its base (more info)

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Head branch (dev/morgan/typecheck-gno-lint) is up to date with base (master): behind by 0 / ahead by 81

@thehowl thehowl requested a review from mvertes December 8, 2024 17:19
Copy link
Member

@gfanton gfanton left a comment

Choose a reason for hiding this comment

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

LGTM, you can mostly ignore my comment as it is not directly linked to your PR

gnovm/cmd/gno/lint.go Outdated Show resolved Hide resolved
gnovm/cmd/gno/lint.go Outdated Show resolved Hide resolved
@thehowl thehowl merged commit 1fba5cf into master Dec 9, 2024
104 of 105 checks passed
@thehowl thehowl deleted the dev/morgan/typecheck-gno-lint branch December 9, 2024 10:13
Villaquiranm pushed a commit to Villaquiranm/gno that referenced this pull request Dec 9, 2024
Depends on (in order):

1. gnolang#1700 
2. gnolang#1702 

This PR uses the type checker added in gnolang#1702 to perform Gno type
checking when calling `gno lint`. Additionally, it adds validation of
gno.mod indirectly (the parsed gno mod is used to determine if a package
is a draft, and if so skip type checking).

Because `gno lint` uses the TestStore, the resulting `MemPackage`s may
contain redefinitions, for overwriting standard libraries like
`AssertOriginCall`. I changed the type checker to filter out the
redefinitions before they reach the Go type checker.

Further improvements, which can be done after this:

- Add shims for gonative special libraries (`fmt`, `os`...)
  - This will allow us to fully type check also tests and filetests
- Make the type checking on-chain (gnolang#1702) also typecheck tests
  - as a consequence of the above.
r3v4s pushed a commit to gnoswap-labs/gno that referenced this pull request Dec 10, 2024
Depends on (in order):

1. gnolang#1700 
2. gnolang#1702 

This PR uses the type checker added in gnolang#1702 to perform Gno type
checking when calling `gno lint`. Additionally, it adds validation of
gno.mod indirectly (the parsed gno mod is used to determine if a package
is a draft, and if so skip type checking).

Because `gno lint` uses the TestStore, the resulting `MemPackage`s may
contain redefinitions, for overwriting standard libraries like
`AssertOriginCall`. I changed the type checker to filter out the
redefinitions before they reach the Go type checker.

Further improvements, which can be done after this:

- Add shims for gonative special libraries (`fmt`, `os`...)
  - This will allow us to fully type check also tests and filetests
- Make the type checking on-chain (gnolang#1702) also typecheck tests
  - as a consequence of the above.
r3v4s pushed a commit to gnoswap-labs/gno that referenced this pull request Dec 10, 2024
Depends on (in order):

1. gnolang#1700 
2. gnolang#1702 

This PR uses the type checker added in gnolang#1702 to perform Gno type
checking when calling `gno lint`. Additionally, it adds validation of
gno.mod indirectly (the parsed gno mod is used to determine if a package
is a draft, and if so skip type checking).

Because `gno lint` uses the TestStore, the resulting `MemPackage`s may
contain redefinitions, for overwriting standard libraries like
`AssertOriginCall`. I changed the type checker to filter out the
redefinitions before they reach the Go type checker.

Further improvements, which can be done after this:

- Add shims for gonative special libraries (`fmt`, `os`...)
  - This will allow us to fully type check also tests and filetests
- Make the type checking on-chain (gnolang#1702) also typecheck tests
  - as a consequence of the above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants