-
Notifications
You must be signed in to change notification settings - Fork 405
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
951a228
137a78b
bc002c8
19833c1
90f8621
e8f00f5
82b10c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# testing gno tool lint command: Render function with multiple cardinality errors (too many parameters and returns) | ||
|
||
! gno tool lint ./multiple_cardinality_errors.gno | ||
|
||
cmp stdout stdout.golden | ||
cmp stderr stderr.golden | ||
|
||
-- multiple_cardinality_errors.gno -- | ||
package main | ||
|
||
// multiple cardinality errors: too many parameters and return values | ||
func Render(path string, extra string) (string, error) { | ||
return "Hello World " + path, nil | ||
} | ||
|
||
-- gno.mod -- | ||
module gno.land/p/test | ||
|
||
-- stdout.golden -- | ||
-- stderr.golden -- | ||
multiple_cardinality_errors.gno:4: Render function should have exactly one parameter, found: 2 parameters, expected: func Render(string) string (code=5) | ||
multiple_cardinality_errors.gno:4: Render function should return exactly one string value, found: 2 returns, expected: func Render(string) string (code=5) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# testing gno tool lint command: Render function with multiple type errors (wrong parameter and return types) | ||
|
||
! gno tool lint ./multiple_type_errors.gno | ||
|
||
cmp stdout stdout.golden | ||
cmp stderr stderr.golden | ||
|
||
-- multiple_type_errors.gno -- | ||
package main | ||
|
||
// multiple type errors: wrong parameter type and wrong return type | ||
func Render(path int) int { | ||
return path | ||
} | ||
|
||
-- gno.mod -- | ||
module gno.land/p/test | ||
|
||
-- stdout.golden -- | ||
-- stderr.golden -- | ||
multiple_type_errors.gno:4: Render function parameter should be of type string, found: int type, expected: func Render(string) string (code=5) | ||
multiple_type_errors.gno:4: Render function should return a single string, found: int type: expected func Render(string) string (code=5) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# testing gno tool lint command: Render function with no parameter | ||
|
||
! gno tool lint ./no_param.gno | ||
|
||
cmp stdout stdout.golden | ||
cmp stderr stderr.golden | ||
|
||
-- no_param.gno -- | ||
package test | ||
|
||
// no parameter | ||
func Render() string { | ||
return "Hello World" | ||
} | ||
|
||
-- gno.mod -- | ||
module gno.land/p/test | ||
|
||
-- stdout.golden -- | ||
-- stderr.golden -- | ||
no_param.gno:4: Render function should have exactly one parameter, found: 0 parameters, expected: func Render(string) string (code=5) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# testing gno tool lint command: Render function with too many parameters | ||
|
||
! gno tool lint ./too_many_params.gno | ||
|
||
cmp stdout stdout.golden | ||
cmp stderr stderr.golden | ||
|
||
-- too_many_params.gno -- | ||
package main | ||
|
||
// too many parameters | ||
func Render(path string, extra string) string { | ||
return "Hello World " + path + extra | ||
} | ||
|
||
-- gno.mod -- | ||
module gno.land/p/test | ||
|
||
-- stdout.golden -- | ||
-- stderr.golden -- | ||
too_many_params.gno:4: Render function should have exactly one parameter, found: 2 parameters, expected: func Render(string) string (code=5) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# testing gno tool lint command: Render function with too many return values | ||
|
||
! gno tool lint ./too_many_returns.gno | ||
|
||
cmp stdout stdout.golden | ||
cmp stderr stderr.golden | ||
|
||
-- too_many_returns.gno -- | ||
package main | ||
|
||
// too many return values | ||
func Render(path string) (string, error) { | ||
return "Hello World " + path, nil | ||
} | ||
|
||
-- gno.mod -- | ||
module gno.land/p/test | ||
|
||
-- stdout.golden -- | ||
-- stderr.golden -- | ||
too_many_returns.gno:4: Render function should return exactly one string value, found: 2 returns, expected: func Render(string) string (code=5) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# testing gno tool lint command: valid Render function signature | ||
|
||
gno tool lint ./valid_render.gno | ||
|
||
cmp stdout stdout.golden | ||
cmp stdout stderr.golden | ||
|
||
-- valid_render.gno -- | ||
package test | ||
|
||
type Test struct{} | ||
// valid as well since it contains a receiver | ||
func (t *Test) Render(n int) int { | ||
return n | ||
} | ||
|
||
// valid Render | ||
func Render(path string) string { | ||
return "Hello World" | ||
} | ||
|
||
-- gno.mod -- | ||
module gno.land/p/test | ||
|
||
-- stdout.golden -- | ||
-- stderr.golden -- |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# testing gno tool lint command: all invalid Render function signature cases | ||
|
||
! gno tool lint ./wrong_param_type.gno | ||
|
||
cmp stdout stdout.golden | ||
cmp stderr stderr.golden | ||
|
||
-- wrong_param_type.gno -- | ||
package test | ||
|
||
import "fmt" | ||
|
||
// wrong parameter type (int vs string) | ||
func Render(path int) string { | ||
return "Hello World" + fmt.Sprint(path) | ||
} | ||
|
||
-- gno.mod -- | ||
module gno.land/p/test | ||
|
||
-- stdout.golden -- | ||
-- stderr.golden -- | ||
wrong_param_type.gno:6: Render function parameter should be of type string, found: int type, expected: func Render(string) string (code=5) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# testing gno tool lint command: Render function with wrong return type | ||
|
||
! gno tool lint ./wrong_return_type.gno | ||
|
||
cmp stdout stdout.golden | ||
cmp stderr stderr.golden | ||
|
||
-- wrong_return_type.gno -- | ||
package main | ||
|
||
// wrong return type (int vs string) | ||
func Render(path string) int { | ||
return 42 | ||
} | ||
|
||
-- gno.mod -- | ||
module gno.land/p/test | ||
|
||
-- stdout.golden -- | ||
-- stderr.golden -- | ||
wrong_return_type.gno:4: Render function should return a single string, found: int type: expected func Render(string) string (code=5) |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you @thehowl! i will get to it when i can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thehowl I've simplified the code quite a bit by using the |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,7 @@ | |
lintGnoError | ||
lintParserError | ||
lintTypeCheckError | ||
lintRenderFuncSignature | ||
|
||
// TODO: add new linter codes here. | ||
) | ||
|
@@ -171,6 +172,11 @@ | |
if hasRuntimeErr { | ||
hasError = true | ||
} | ||
|
||
// ensure Render function signature matches what's expected | ||
if renderErr := lintRenderSignature(io, memPkg); renderErr { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be better to continue the lint check regardless of whether a runtime error occurs. |
||
hasError = true | ||
} | ||
} | ||
|
||
if hasError { | ||
|
@@ -317,3 +323,99 @@ | |
} | ||
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()). | ||
func lintRenderSignature(io commands.IO, memPkg *gnovm.MemPackage) bool { | ||
var ( | ||
hasError bool | ||
exp = "func Render(string) string" | ||
stringType = "string" | ||
|
||
errParameterCount = func(count int) string { | ||
return fmt.Sprintf("Render function should have exactly one parameter, found: %d parameters, expected: %s", count, exp) | ||
} | ||
errParameterType = func(t string) string { | ||
return fmt.Sprintf("Render function parameter should be of type string, found: %s type, expected: %s", t, exp) | ||
} | ||
errReturnCount = func(count int) string { | ||
return fmt.Sprintf("Render function should return exactly one string value, found: %d returns, expected: %s", count, exp) | ||
} | ||
errReturnType = func(t string) string { | ||
return fmt.Sprintf("Render function should return a single string, found: %s type: expected %s", t, exp) | ||
} | ||
errPrintln = func(errMsg, file string, line int) { | ||
hasError = true | ||
io.ErrPrintln(lintIssue{ | ||
Code: lintRenderFuncSignature, | ||
Msg: errMsg, | ||
Confidence: 1, | ||
Location: fmt.Sprintf("%s:%d", file, line), | ||
}) | ||
} | ||
) | ||
|
||
for _, file := range memPkg.Files { | ||
if !strings.HasSuffix(file.Name, ".gno") { | ||
continue | ||
} | ||
|
||
fn, err := gno.ParseFile(file.Name, file.Body) | ||
// if we can't parse the file for whatever reason, there is no point in | ||
// attempting a lint check on the Render func | ||
if err != nil { | ||
continue | ||
} | ||
|
||
for _, decl := range fn.Decls { | ||
// ignore non-function declarations | ||
funcDecl, ok := decl.(*gno.FuncDecl) | ||
if !ok { | ||
continue | ||
} | ||
|
||
// ignore non-Render functions | ||
if funcDecl.NameExpr.Name.String() != "Render" { | ||
continue | ||
} | ||
|
||
// ignore methods | ||
if funcDecl.IsMethod { | ||
continue | ||
} | ||
|
||
// ensure we have a single string parameter | ||
n := len(funcDecl.Type.Params) | ||
switch n { | ||
case 1: | ||
param := funcDecl.Type.Params[0] | ||
paramType, ok := param.Type.(*gno.NameExpr) | ||
if !ok || paramType.Name.String() != stringType { | ||
msg := errParameterType(paramType.Name.String()) | ||
errPrintln(msg, file.Name, funcDecl.GetLine()) | ||
} | ||
default: | ||
msg := errParameterCount(n) | ||
errPrintln(msg, file.Name, funcDecl.GetLine()) | ||
} | ||
|
||
// ensure return type is a single string | ||
n = len(funcDecl.Type.Results) | ||
switch n { | ||
case 1: | ||
ret := funcDecl.Type.Results[0] | ||
retType, ok := ret.Type.(*gno.NameExpr) | ||
if !ok || retType.Name.String() != stringType { | ||
msg := errReturnType(retType.Name.String()) | ||
errPrintln(msg, file.Name, funcDecl.GetLine()) | ||
} | ||
default: | ||
msg := errReturnCount(n) | ||
errPrintln(msg, file.Name, funcDecl.GetLine()) | ||
} | ||
} | ||
} | ||
|
||
return hasError | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,6 +112,8 @@ const ( | |
|
||
type Name string | ||
|
||
func (n Name) String() string { return string(n) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. happy to remove this change if its adding unnecessary changes to the PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it's not really necessary anyway |
||
|
||
// ---------------------------------------- | ||
// Location | ||
// Acts as an identifier for nodes. | ||
|
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 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 currentlyThere 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'd say to have these as unit tests in
tool_lint_test.go
; they're definitely too "table-like" to have as integration tests.