feat(gnovm): add extensible linting framework with AVL001 and GLOBAL001 rules#5068
feat(gnovm): add extensible linting framework with AVL001 and GLOBAL001 rules#5068MikaelVallenet wants to merge 33 commits intognolang:masterfrom
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):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| @@ -0,0 +1,154 @@ | |||
| # Gno Lint | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| ## Command Line Options | ||
|
|
||
| | Flag | Default | Description | | ||
| |------|---------|-------------| |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| ## Available Rules | ||
|
|
||
| - **AVL001** (warning): Unbounded AVL tree iteration - detects `avl.Tree.Iterate()` or `ReverseIterate()` with empty bounds (`"", ""`) | ||
| - **GLOBAL001** (warning): Exported package-level variable - detects exported (uppercase) `var` declarations at package level |
There was a problem hiding this comment.
This rule is triggered on the variables of the next code snippets, which I don't think should.
If it's possible with the AST you are working with, we would have better to trigger this warning only on dangerous exported variable (e.g.: structure containing public method returning object, not readonly tainted value, whatever else).
package grc1155
import "errors"
var (
ErrInvalidAddress = errors.New("invalid address")
ErrMismatchLength = errors.New("accounts and ids length mismatch")
ErrCannotTransferToSelf = errors.New("cannot send transfer to self")
ErrTransferToRejectedOrNonGRC1155Receiver = errors.New("transfer to rejected or non GRC1155Receiver implementer")
ErrCallerIsNotOwnerOrApproved = errors.New("caller is not token owner or approved")
ErrInsufficientBalance = errors.New("insufficient balance for transfer")
ErrBurnAmountExceedsBalance = errors.New("burn amount exceeds balance")
ErrInvalidAmount = errors.New("invalid amount")
)davd@davd ~/P/g/e/g/p/d/t/grc1155 (mikael/experiment/linter-archi)> gno lint .
errors.gno:6:2: warning: exported package-level variable: ErrInvalidAddress (GLOBAL001)
errors.gno:7:2: warning: exported package-level variable: ErrMismatchLength (GLOBAL001)
errors.gno:8:2: warning: exported package-level variable: ErrCannotTransferToSelf (GLOBAL001)
errors.gno:9:2: warning: exported package-level variable: ErrTransferToRejectedOrNonGRC1155Receiver (GLOBAL001)
errors.gno:10:2: warning: exported package-level variable: ErrCallerIsNotOwnerOrApproved (GLOBAL001)
errors.gno:11:2: warning: exported package-level variable: ErrInsufficientBalance (GLOBAL001)
errors.gno:12:2: warning: exported package-level variable: ErrBurnAmountExceedsBalance (GLOBAL001)
errors.gno:13:2: warning: exported package-level variable: ErrInvalidAmount (GLOBAL001)
Found 8 issue(s): 0 error(s), 8 warning(s), 0 infoThere was a problem hiding this comment.
what about using the // nolint directive for these one
it's a way to acknowledge you know what you do IMO
will fix the others reviews asap
There was a problem hiding this comment.
I don't think it's a good idea, it's gonna pollute the code base especially if we have to refactor everything.
I feel we would have better to set it deactivated by default with a warning on that flag, fix the flag with only problematic global variable, or remove it entirely as the false positive will be too high.
| return false | ||
| }) | ||
|
|
||
| //nolint:GLOBAL001 |
There was a problem hiding this comment.
It would be great to be able to inline this comment (or add an example if it's already implemented)
var Config = DefaultConfig() //nolint:GLOBAL001There was a problem hiding this comment.
Out of scope IMO can be done in another PR
gnovm/pkg/lint/reporters/json.go
Outdated
|
|
||
| r.issues = append(r.issues, issue) | ||
|
|
||
| switch issue.Severity { |
There was a problem hiding this comment.
text and json implementations shared some common business logic, we could use a common base but i don't think it make sense here, just for 2 implem. can you developp your thinking
There was a problem hiding this comment.
This specific switch case is duplicated 4 times and is related to a constant behavior, I think this should be moved in a generic helper.
The sort function from the flush too, but I agree we don't have to use a common base in that case.
| // to allow reuse by other commands (run, test) without coupling them to the lint package. | ||
| type Reporter interface { | ||
| Report(issue Issue) | ||
| Flush() error |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
moonia
left a comment
There was a problem hiding this comment.
Overall, LGTM.
The implementation appears to be working as expected based on my testing. As Davphla mentioned, it might be worth adding this to docs/README.md
|
I'll review this when I can; can you make sure to include the additions you made in #4943 as well? :) |
davd-gzl
left a comment
There was a problem hiding this comment.
I tested every features, it all works great except the edge case of avl001
gnovm/pkg/lint/reporters/json.go
Outdated
|
|
||
| r.issues = append(r.issues, issue) | ||
|
|
||
| switch issue.Severity { |
There was a problem hiding this comment.
This specific switch case is duplicated 4 times and is related to a constant behavior, I think this should be moved in a generic helper.
The sort function from the flush too, but I agree we don't have to use a common base in that case.
| } | ||
| } | ||
|
|
||
| func (AVL001) Check(ctx *lint.RuleContext, node gnolang.Node) []lint.Issue { |
There was a problem hiding this comment.
For some reason this rule is not triggered on examples/gno.land/p/demo/microblog/microblog.gno:L39
fdbe30b to
b954995
Compare
b954995 to
484d008
Compare
|
@MikaelVallenet , the failed Lint CI check for "images/manifesto/boards2.jpeg" has been fixed in master. If you merge master again, then it should fix this CI check. |
fix #4421 & fix #1042
Summary (PR description is mostly AI generated and then adapted by me)
This PR adds an extensible linting framework to
gno lint. You can now write pluggable rules, choose differentoutput formats, and suppress warnings with
//nolintcomments.What's New
New Lint Package (
gnovm/pkg/lint/)Core stuff:
Issue- represents a single lint finding (severity, location, message)Severity- three levels: info, warning, errorRuleinterface - implement this to create your own lint rulesRuleContext- gives rules access to the file, source code, and parent nodesRegistry- handles rule registration (rules self-register viainit())Config- controls mode and which rules to disableNolintParser- reads//nolintcomments to suppress specific issuesReporters (how output gets formatted):
TextReporter- human-friendly output, sorted by file/line, with a summary at the endJSONReporter- machine-readable JSON array for tooling integrationDirectReporter- prints issues immediately as they're found (used bygno test)Built-in rules:
AVL001- warns when you iterate an entire AVL tree without bounds (tree.Iterate("", "", ...))GLOBAL001- warns about exported package-level variables (likevar Counter int)CLI Changes (
gnovm/cmd/gno/lint.go)New flags:
--mode- choose betweendefault,strict, orwarn-only--format- output astextorjson--list-rules- shows all available rules and exits--disable-rules- skip specific rules (e.g.,--disable-rules=AVL001,GLOBAL001)Output format changed:
Before
After
Docs
docs/resources/gno-lint.mdwith usage examples, rule descriptions, and a guide for writing new rulesTests
gnovm/pkg/lint/Breaking Changes
Output format - The lint output now includes severity and uses a different format (see above)
Exit codes - Now depend on the mode:
defaultstrictwarn-only