-
Notifications
You must be signed in to change notification settings - Fork 404
Rewrite AST prior go type checking to avoid inter-realm type check errors. #4156
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
base: heapitems_for_3693
Are you sure you want to change the base?
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
|
gnovm/pkg/gnolang/gotypecheck.go
Outdated
if len(n.Args) > 0 { | ||
a = n.Args[0] | ||
} |
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.
nitpick: len of n.Args can only be 1, right?
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.
Yes. I should probably validate the syntax of cross()
and crossing()
while they are still available for check!
|
||
// Validate Gno syntax and type check. | ||
if err := gno.TypeCheckMemPackageTest(memPkg, m.Store); err != nil { | ||
return runResult{Error: err.Error()} | ||
} | ||
|
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.
For my understanding, this is a signal that we’re no longer adding new features to the gno type checker, right?
Do we already have a clear clarification on this plan?
I think what we need to do is review the existing issues related to the gno type checker—either mark them appropriately or close them.
There are also some related PRs, such as #4033. Should we go ahead and merge them? @thehowl @mvertes
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.
From what jae said, we should go through the PRs and issues relating to the type-checker and label them. I think we can close them for now, then recover them if we want to go ahead and improve our own preprocessor / type checker.
Longer term, I hope that we can migrate away from the GnoVM sooner than we consider removing our reliance on the go type checlker.
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 good to me.
No description provided.