Skip to content

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

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 67 additions & 2 deletions gnovm/cmd/gno/tool_lint.go
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

@itsHabib itsHabib Apr 19, 2025

Choose a reason for hiding this comment

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

@thehowl I've simplified the code quite a bit by using the types.Package like you mentioned. One little catch is that we now lose position of the error/Render func. The pkg type has a Path method exposed which is what i use as the location of the error now e.g. gno.land/test. The function object expose a token.Pos but that is only useful if I also have a FileSet around. I decided not to do that since that would require forcing the parseCheckMemPackage to also return the token.FileSet along with a couple of its callers so that it can eventually get to the lintRenderSignature call. lmk what you think

Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const (
lintGnoError
lintParserError
lintTypeCheckError
lintRenderFuncSignature

// TODO: add new linter codes here.
)
Expand Down Expand Up @@ -181,9 +182,9 @@ func execLint(cfg *lintCfg, args []string, io commands.IO) error {
}

func lintTypeCheck(io commands.IO, memPkg *gnovm.MemPackage, testStore gno.Store) (errorsFound bool, err error) {
tcErr := gno.TypeCheckMemPackageTest(memPkg, testStore)
pkg, tcErr := gno.TypeCheckMemPackageTest(memPkg, testStore)
if tcErr == nil {
return false, nil
return lintRenderSignature(io, pkg), nil
}

errs := multierr.Errors(tcErr)
Expand Down Expand Up @@ -317,3 +318,67 @@ func issueFromError(pkgPath string, err error) lintIssue {
}
return issue
}

// lintRenderSignature checks if a Render function in the package has the
// expected signature: func Render(string) string
// Methods are ignored (e.g. func (t *Type) Render()).
// Returns true if the signature is incorrect.
func lintRenderSignature(io commands.IO, pkg *types.Package) bool {
if pkg == nil {
return false
}

o := pkg.Scope().Lookup("Render")
if o == nil {
// no Render
return false
}

var (
stringType = "string"
errPrintln = func() {
io.ErrPrintln(lintIssue{
Code: lintRenderFuncSignature,
Msg: fmt.Sprintf(
"The 'Render' function signature is incorrect for the '%s' package. The signature must be of the form: func Render(string) string",
pkg.Name(),
),
Confidence: 1,
Location: pkg.Path(),
})
}
isSingleString = func(t *types.Tuple) bool {
return t != nil &&
t.Len() == 1 &&
t.At(0) != nil &&
t.At(0).Type().String() == stringType
}
)

fn, ok := o.(*types.Func)
if !ok {
// not a function
return false
}

s, ok := fn.Type().(*types.Signature)
if !ok {
// not a valid function signature, would have been caught through
// another check so we return false here
return false
}

if s.Recv() != nil {
// ignore methods
return false
}

switch {
// ensure we have a single string parameter and return
case !isSingleString(s.Params()), !isSingleString(s.Results()):
errPrintln()
return true
default:
return false
}
}
148 changes: 148 additions & 0 deletions gnovm/cmd/gno/tool_lint_test.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
package main

import (
"bytes"
"go/types"
"strings"
"testing"

"github.com/stretchr/testify/require"

"github.com/gnolang/gno/gnovm"
gno "github.com/gnolang/gno/gnovm/pkg/gnolang"
"github.com/gnolang/gno/tm2/pkg/commands"
)

func TestLintApp(t *testing.T) {
Expand Down Expand Up @@ -75,3 +83,143 @@ func TestLintApp(t *testing.T) {
}
testMainCaseRun(t, tc)
}

func TestLintRenderSignature(t *testing.T) {
type test struct {
desc string
input string
err bool
}

const errMsg = "gno.land/test: The 'Render' function signature is incorrect for the 'test' package. The signature must be of the form: func Render(string) string (code=5)\n"

tests := []test{
{
desc: "no render function",
input: `
package test

func Random() {}
`,
},
{
desc: "ignore methods",
input: `
package test

type Test struct{}

func (t *Test) Render(input int) int {
return input
}
`,
},

{
desc: "wrong parameter type",
input: `
package test

func Render(input int) string {
return "hello"
}
`,
err: true,
},
{
desc: "wrong return type",
input: `
package test

func Render(input string) int {
return 9001
}
`,
err: true,
},
{
desc: "too many parameters",
input: `
package test

func Render(input string, extra int) string {
return input
}
`,
err: true,
},
{
desc: "correct signature",
input: `
package test

func Render(input string) string {
return input
}
`,
err: false,
},
}

for _, tc := range tests {
t.Run(tc.desc, func(t *testing.T) {
buf := new(bytes.Buffer)
io := commands.NewTestIO()
io.SetErr(commands.WriteNopCloser(buf))

pkg := typesPkg(t, tc.input)
hasErr := lintRenderSignature(io, pkg)

switch {
case tc.err:
require.True(t, hasErr)
require.Equal(t, errMsg, buf.String())
default:
require.False(t, hasErr)
require.Empty(t, buf.String())
}
})
}
}

// helper to take in a file body string and return a types.Package.
// makes plenty of assumptions on the path, pkg name, and files
func typesPkg(t *testing.T, input string) *types.Package {
t.Helper()

memPkg := &gnovm.MemPackage{
Name: "test",
Path: "gno.land/test",
Files: []*gnovm.MemFile{
{
Name: "main.gno",
Body: input,
},
},
}

pkg, err := gno.TypeCheckMemPackageTest(memPkg, &mockMemPkgGetter{name: "test", body: input})
require.NoError(t, err)

return pkg
}

// provides a simple impl of MemPackageGetter that only assumes a single file
// of 'main.go' using the underlying name and body of the struct
type mockMemPkgGetter struct {
name string
body string
}

func (m *mockMemPkgGetter) GetMemPackage(path string) *gnovm.MemPackage {
return &gnovm.MemPackage{
Name: m.name,
Path: path,
Files: []*gnovm.MemFile{
{
Name: "main.go",
Body: m.body,
},
},
}
}
14 changes: 8 additions & 6 deletions gnovm/pkg/gnolang/gotypecheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,19 @@ type MemPackageGetter interface {
// If format is true, the code will be automatically updated with the
// formatted source code.
func TypeCheckMemPackage(mempkg *gnovm.MemPackage, getter MemPackageGetter, format bool) error {
return typeCheckMemPackage(mempkg, getter, false, format)
_, err := typeCheckMemPackage(mempkg, getter, false, format)
return err
}

// TypeCheckMemPackageTest performs the same type checks as [TypeCheckMemPackage],
// but allows re-declarations.
//
// Note: like TypeCheckMemPackage, this function ignores tests and filetests.
func TypeCheckMemPackageTest(mempkg *gnovm.MemPackage, getter MemPackageGetter) error {
func TypeCheckMemPackageTest(mempkg *gnovm.MemPackage, getter MemPackageGetter) (*types.Package, error) {
return typeCheckMemPackage(mempkg, getter, true, false)
}

func typeCheckMemPackage(mempkg *gnovm.MemPackage, getter MemPackageGetter, testing, format bool) error {
func typeCheckMemPackage(mempkg *gnovm.MemPackage, getter MemPackageGetter, testing, format bool) (*types.Package, error) {
var errs error
imp := &gnoImporter{
getter: getter,
Expand All @@ -56,13 +57,14 @@ func typeCheckMemPackage(mempkg *gnovm.MemPackage, getter MemPackageGetter, test
}
imp.cfg.Importer = imp

_, err := imp.parseCheckMemPackage(mempkg, format)
pkg, err := imp.parseCheckMemPackage(mempkg, format)
// prefer to return errs instead of err:
// err will generally contain only the first error encountered.
if errs != nil {
return errs
return nil, errs
}
return err

return pkg, err
}

type gnoImporterResult struct {
Expand Down