-
Notifications
You must be signed in to change notification settings - Fork 413
feat: transpile to Gno 0.9 (latest interrealm spec) with @cross #4264
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
Conversation
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):No automated checks match this pull request. ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
@@ -115,7 +114,7 @@ func TestCreateMembers(t *testing.T) { | |||
} | |||
|
|||
func addMembers(ms MembersByTier, c int, tier string) { | |||
mt := avl.NewTree() | |||
// mt := avl.NewTree() XXX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
examples *_test.gno files were never type-checked, and mt isn't used.
// File Name must contain a single dot and extension. | ||
// File Name may be ALLCAPS.xxx or lowercase.xxx; extensions lowercase. | ||
// e.g. OK: README.md, readme.md, readme.txt, READ.me | ||
// e.g. NOT OK: Readme.md, readme.MD, README, .readme |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// e.g. NOT OK: Readme.md, readme.MD, README, .readme | |
// e.g. NOT OK: Readme.md, readme.MD, README.md, .readme |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
README.md is OK. README is not.
tm2/pkg/std/memfile.go
Outdated
"sort" | ||
"strings" | ||
) | ||
|
||
// XXX rename to mempackage.go | ||
|
||
const fileNameLimit = 256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we consider a lower limit, especially since it's only the file part that can be added to an already long path or URL?
Maybe in gnovm's additional validation logic.
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still reading through, some initial comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have to be in the same package as gnolang
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be close to filenode, and gnovm specific mempackage logic; and gnovm/pkg/gnomod should be its dependency. It should really be part of the PackageNode, so yes.
subTestName := path | ||
isHidden := strings.HasPrefix(path, ".") | ||
if isHidden { | ||
t.Run(subTestName, func(t *testing.T) { | ||
t.Skip("skipping hidden") | ||
}) | ||
return nil | ||
} | ||
isLong := strings.HasSuffix(path, "_long.gno") | ||
if isLong && testing.Short() { | ||
t.Run(subTestName, func(t *testing.T) { | ||
t.Skip("skipping in -short") | ||
t.Skip("skipping long (-short)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could determine a skipReason
first and then do t.Run with skip if given maybe. would avoid. also we don't need a subTestName anymore, we can just use path
gnovm/pkg/gnolang/go2gno.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What justifies changing these into spans?
I think it makes the error formats different and harder to integrate into ie. text editors (a lot of editors now understand file:line:col:
and make them clickable) and unintuitive to use compared to what folks are used to, while adding little information.
I think what we really need is to add more information to the errors that require it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nodes can have the same starting position. C(X)(Y(Z())) <-- C, C(X), C(X)(...) are all in the same location.
If you want to do transforms on them, or do any sort of lookup, you can't do so with just the file:line:col. This was needed in gno fix because I want to use the gno type information from the preprocessor, but the gno AST doesn't print/fmt well, so instead the desired transforms are written by file:span, and then translated to actual Go AST node references, and then finally astutil.Apply does Go AST mutations. Along the way I found the need for spans for debugging due to the potential redundancy.
It's easy enough to filter these and strip the unwanted portion of a span. It adds little but necessary information; without it, it cannot serve as an identifier for the node.
If an IDE wants to highlight a selection of text and show the error next to it, it needs the span, again because of the redundancy issue. Existing text editors are just less informative than they should be, and the gnovm doesn't make compromises for the past 30 years of tooling, because it's meant for the next 1000.
gnovm/stdlibs/testing/base/README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that this is necessary indicates to me there's likely a bug in the typechecker, no?
We should be able to break these cycles with test if we have xxx_test files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you're referring to, but the typechecker was buggy with respect to _tests and _filetests, and it wasn't showing because type-checking wasn't being performed on testfiles and filetests.
for _, file := range mempkg.Files { | ||
if isGnoFile(file.Name) || file.Name == "gno.mod" { | ||
return false | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like an helper we also want in gno, maybe IsGnoMempackageEmpty().
Co-authored-by: Manfred Touron <[email protected]>
8467591
to
2cc60ed
Compare
if err != nil { | ||
return false, fmt.Errorf("unable to parse `gno.mod`: %w", err) | ||
for _, file := range mempkg.Files { | ||
if isGnoFile(file.Name) || file.Name == "gno.mod" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is gnodev
, not gnovm
or gno.land
. In my opinion, we should perform minimal checks here unless we want to log something for the user, as this could lead to duplicate logic. Robust checks should happen behind when triggering AddPackage
.
WIP