feat(gnolint): add lint checks for `Render` #4083

itsHabib · 2025-04-09T05:32:23Z

Issue

Closes gno lint: check Render func signature. #4026

Description

Adds a func lintRenderSignature that asserts a public Render func follows the required signature func Render(string) string)
Adds test files in testdata/lint to assert the above

Testing Validation

go test ./gnovm/cmd/gno/ -run Test_Scripts/lint

itsHabib · 2025-04-09T05:33:17Z

gnovm/pkg/gnolang/nodes.go

@@ -112,6 +112,8 @@ const (

 type Name string

+func (n Name) String() string { return string(n) }


happy to remove this change if its adding unnecessary changes to the PR

yes, it's not really necessary anyway

Gno2D2 · 2025-04-09T05:33:30Z

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)
The pull request description provides enough details

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include BREAKING CHANGE notes.
- Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

Debug

Automated Checks

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

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: itsHabib/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🟢 At least 1 user(s) of the organization reviewed the pull request (with state "APPROVED")
    │       ├── 🔴 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)

Manual Checks

**IGNORE** the bot requirements for this PR (force green CI check)
If
🟢 Condition met
└── 🟢 On every pull request
Can be checked by

Any user with comment edit permission
The pull request description provides enough details
If
🟢 Condition met
└── 🟢 And
    ├── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
    └── 🟢 Not (🔴 Pull request author is user: dependabot[bot])
Can be checked by

team core-contributors

itsHabib · 2025-04-09T05:36:15Z

gnovm/cmd/gno/testdata/lint/render_multiple_cardinality_errors.txtar

@@ -0,0 +1,22 @@
+# testing gno tool lint command: Render function with multiple cardinality errors (too many parameters and returns)


I wanted to either have all these tests in a single file or have a testdata/lint/render but figured this is fine for now and can be done in a separate PR if thats needed to keep this one focused. I wasn't sure how to do the former(if its supported) and the latter isn't supported currently

I'd say to have these as unit tests in tool_lint_test.go; they're definitely too "table-like" to have as integration tests.

notJoon

LGTM 👍

remove: review/triage-pending flag

itsHabib · 2025-04-09T13:46:14Z

appreciate the review @notJoon, should i be expected to merge now? seems like its still pending a review from tech-staff but i noticed the label is now removed, is that expected?

itsHabib · 2025-04-09T15:50:15Z

I see i broke some test. I will see what i can do to fix

Update: found the problem, the panic occurs due to me using MustParseFile. might just be better to return early/continue since cant lint a render if the file itself cant be parsed

itsHabib · 2025-04-09T16:16:52Z

gnovm/cmd/gno/tool_lint.go

@@ -171,6 +172,11 @@ func execLint(cfg *lintCfg, args []string, io commands.IO) error {
 		if hasRuntimeErr {
 			hasError = true
 		}
+
+		// ensure Render function signature matches what's expected
+		if renderErr := lintRenderSignature(io, memPkg); renderErr {


question for reviewers: should the lint render be called only if the above runtime err is false? or could there still be value in it as maybe some file could still have a Render that can be checked?

I think it would be better to continue the lint check regardless of whether a runtime error occurs.

codecov · 2025-04-09T17:01:42Z

Codecov Report

Attention: Patch coverage is 97.26027% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gnovm/cmd/gno/tool_lint.go	97.22%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

notJoon · 2025-04-10T08:32:17Z

should i be expected to merge now? seems like its still pending a review from tech-staff but i noticed the label is now removed, is that expected?

@itsHabib

actually not right now. the first round of reviews is over, so I think it will be take another time to being merge this PR.

thehowl

Thanks for your contribution! I think we can do better, as to avoid an additional parsing round in the VM, by re-using the information we already get from the Go type checker :)

thehowl · 2025-04-14T14:48:49Z

gnovm/cmd/gno/tool_lint.go

We can do better; the code is not really adapted to easily allow for more code-level lints, but I think we can adapt it easily:

TypeCheckMemPackage should be able to return the *types.Package returned by parseCheckMemPackage

This way we can use pkg.Scope() to find a Render() function, if any, and then verify its signature.

The code in this file should be significantly shorter.

Thank you @thehowl! i will get to it when i can

thehowl · 2025-04-14T14:49:40Z

gnovm/cmd/gno/testdata/lint/render_multiple_cardinality_errors.txtar

@@ -0,0 +1,22 @@
+# testing gno tool lint command: Render function with multiple cardinality errors (too many parameters and returns)


I'd say to have these as unit tests in tool_lint_test.go; they're definitely too "table-like" to have as integration tests.

itsHabib added 2 commits April 9, 2025 00:12

lint render + tests

951a228

ignore methods, add test

137a78b

github-project-automation bot moved this to Triage in 🧙‍♂️gno.land core team Apr 9, 2025

github-project-automation bot added this to 🧙‍♂️gno.land core team and 💪 Bounties & Worx Apr 9, 2025

github-actions bot assigned itsHabib Apr 9, 2025

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Apr 9, 2025

itsHabib commented Apr 9, 2025

View reviewed changes

Gno2D2 requested a review from a team April 9, 2025 05:33

itsHabib marked this pull request as ready for review April 9, 2025 05:35

Gno2D2 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Apr 9, 2025

itsHabib commented Apr 9, 2025

View reviewed changes

notJoon approved these changes Apr 9, 2025

View reviewed changes

notJoon removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Apr 9, 2025

leohhhn requested review from thehowl and mvertes and removed request for a team April 9, 2025 15:28

fix panic in app lint tests

bc002c8

itsHabib commented Apr 9, 2025

View reviewed changes

thehowl requested changes Apr 14, 2025

View reviewed changes

github-project-automation bot moved this to In Progress in 💪 Bounties & Worx Apr 14, 2025

Kouteki moved this from Triage to In Review in 🧙‍♂️gno.land core team Apr 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gnolint): add lint checks for `Render` #4083

feat(gnolint): add lint checks for `Render` #4083

itsHabib commented Apr 9, 2025

itsHabib Apr 9, 2025

thehowl Apr 14, 2025

Gno2D2 commented Apr 9, 2025 •

edited

Loading

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Then

If

Can be checked by

If

Can be checked by

itsHabib Apr 9, 2025 •

edited

Loading

thehowl Apr 14, 2025

notJoon left a comment

itsHabib commented Apr 9, 2025

itsHabib commented Apr 9, 2025 •

edited

Loading

itsHabib Apr 9, 2025

notJoon Apr 10, 2025

codecov bot commented Apr 9, 2025

notJoon commented Apr 10, 2025

thehowl left a comment

thehowl Apr 14, 2025

itsHabib Apr 14, 2025

thehowl Apr 14, 2025

		@@ -112,6 +112,8 @@ const (

		type Name string

		func (n Name) String() string { return string(n) }

		@@ -0,0 +1,22 @@
		# testing gno tool lint command: Render function with multiple cardinality errors (too many parameters and returns)

feat(gnolint): add lint checks for Render #4083

Are you sure you want to change the base?

feat(gnolint): add lint checks for Render #4083

Conversation

itsHabib commented Apr 9, 2025

Issue

Description

Testing Validation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gno2D2 commented Apr 9, 2025 • edited Loading

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Then

If

Can be checked by

If

Can be checked by

itsHabib Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notJoon left a comment

Choose a reason for hiding this comment

itsHabib commented Apr 9, 2025

itsHabib commented Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 9, 2025

Codecov Report

notJoon commented Apr 10, 2025

thehowl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(gnolint): add lint checks for `Render` #4083

feat(gnolint): add lint checks for `Render` #4083

Gno2D2 commented Apr 9, 2025 •

edited

Loading

itsHabib Apr 9, 2025 •

edited

Loading

itsHabib commented Apr 9, 2025 •

edited

Loading