TrueCloudLab/linters
22
0
Fork 7
generated from TrueCloudLab/basic
Code Issues 5 Pull requests 1 Projects Releases 1 Packages Wiki Activity Actions

noliteral: Add ignoring of nested functions #17

Merged
fyrchik merged 2 commits from achuprov/linters:bugfix/noliteral_ignoring_nested_functions into master 2024-09-04 19:51:22 +00:00
Conversation 0 Commits 2 Files changed 3 +92 -43
3 changed files with 92 additions and 43 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

94
internal/analyzers/noliteral/linter.go
View file

@ -2,6 +2,7 @@ package noliteral
import (
"go/ast"
"go/token"
astutils "git.frostfs.info/TrueCloudLab/linters/pkg/ast-utils"
"github.com/mitchellh/mapstructure"
@ -51,49 +52,66 @@ func run(pass *analysis.Pass) (interface{}, error) {
}
for _, file := range pass.Files {
ast.Inspect(file, func(n ast.Node) bool {
expr, ok := n.(*ast.CallExpr)
if !ok {
return true
}
isLog, _ := astutils.IsTargetMethod(expr.Fun, Config.TargetMethods)
if !isLog || len(expr.Args) == 0 || astutils.HasNoLintComment(pass, expr.Pos()) {
return true
}
if !astutils.IsStringValue(expr.Args[0]) {
alias, _ := astutils.GetAliasByPkgName(file, Config.ConstantsPackage)
if Config.ConstantsPackage == "" || astutils.GetPackageName(expr.Args[0]) == alias || astutils.GetPackageName(expr.Args[0]) == "" {
return true
switch v := n.(type) {
// a := zap.Error()
case *ast.AssignStmt:
if _, ok := v.Rhs[0].(*ast.CallExpr); ok {
return false
}
fyrchik commented 2024-02-06 08:29:59 +00:00
Outdated
Review
Copy link

Using Pos() checks feels wrong: we are traversing ast, why do we need to compare positions of bytes in text?

I looks the only thing that needed to be changed is IsTargetMethod: sel.X is the part before the dot and in our case it should be some object, not the package. Or is it not enough?

Using `Pos()` checks feels wrong: we are traversing ast, why do we need to compare positions of bytes in text? I looks the only thing that needed to be changed is `IsTargetMethod`: `sel.X` is the part before the dot and in our case it should be some object, not the package. Or is it not enough?
achuprov commented 2024-02-12 14:22:19 +00:00
Outdated
Review
Copy link

You are right. We can choose not to use Pos().

We can't exclude all functions that are not part of an object. This is because there are packages with package-level logging, like https://github.com/sirupsen/logrus. My solution has a drawback: we can't distinguish between &zap.Error(...) and &log.Error(...), but this does not seem to be a common issue.

You are right. We can choose not to use `Pos()`. We can't exclude all functions that are not part of an object. This is because there are packages with package-level logging, like https://github.com/sirupsen/logrus. My solution has a drawback: we can't distinguish between `&zap.Error(...)` and `&log.Error(...)`, but this does not seem to be a common issue.
for _, pkgName := range Config.DisablePackages {
if pkgName == astutils.GetPackageName(expr.Args[0]) {
return true
}
// a := &log.Error()
case *ast.UnaryExpr:
if _, ok := v.X.(*ast.CallExpr); ok && v.Op == token.AND {
return false
}
// log.Error()
case *ast.CallExpr:
if expr, ok := n.(*ast.CallExpr); ok {
return analyzeCallExpr(pass, expr, file)
}
pass.Report(analysis.Diagnostic{
Pos: expr.Pos(),
End: expr.End(),
Category: LinterName,
Message: "Wrong package for constants",
SuggestedFixes: nil,
})
return true
}
pass.Report(analysis.Diagnostic{
Pos: expr.Pos(),
End: expr.End(),
Category: LinterName,
Message: "Literals are not allowed in the body of the logger",
SuggestedFixes: nil,
})
return false
return true
})
}
return nil, nil
}
func analyzeCallExpr(pass *analysis.Pass, expr *ast.CallExpr, file *ast.File) bool {
isLog, _ := astutils.IsTargetMethod(expr.Fun, Config.TargetMethods)
if !isLog || len(expr.Args) == 0 || astutils.HasNoLintComment(pass, expr.Pos()) {
return false
}
if !astutils.IsStringValue(expr.Args[0]) {
alias, _ := astutils.GetAliasByPkgName(file, Config.ConstantsPackage)
if Config.ConstantsPackage == "" || astutils.GetPackageName(expr.Args[0]) == alias || astutils.GetPackageName(expr.Args[0]) == "" {
return false
}
for _, pkgName := range Config.DisablePackages {
if pkgName == astutils.GetPackageName(expr.Args[0]) {
return false
aarifullin commented 2024-02-06 08:06:59 +00:00
Outdated
Review
Copy link

I still do not see any check for non-package usage like zap.Error(obj.Error)

Could this solve the problem? :)

func IsTypedValueSpec(expr ast.Expr) bool {
	selExp, ok := expr.(*ast.SelectorExpr)
	if !ok {
		return false
	}
	ident, ok := selExp.X.(*ast.Ident)
	if ok {
		if valExp, ok := ident.Obj.Decl.(*ast.ValueSpec); ok {
			if valExp.Type != nil {
				return true
			}
		}
	}
	return false
}
I still do not see any check for non-package usage like `zap.Error(obj.Error)` Could this solve the problem? :) ```go func IsTypedValueSpec(expr ast.Expr) bool { selExp, ok := expr.(*ast.SelectorExpr) if !ok { return false } ident, ok := selExp.X.(*ast.Ident) if ok { if valExp, ok := ident.Obj.Decl.(*ast.ValueSpec); ok { if valExp.Type != nil { return true } } } return false } ```
achuprov commented 2024-02-12 14:28:46 +00:00
Outdated
Review
Copy link

We cannot rely on valExp.Type because it is only available when explicitly declared as var name Type. Example: dba7fe3257

We cannot rely on `valExp.Type` because it is only available when explicitly declared as `var name Type`. Example: https://astexplorer.net/#/gist/e53ef7889f8764c62cd8719f823b20c0/dba7fe32576e8272abdd466f6d7e00264266e87b
}
}
pass.Report(analysis.Diagnostic{
Pos: expr.Pos(),
End: expr.End(),
Category: LinterName,
Message: "Wrong package for constants",
SuggestedFixes: nil,
})
return false
}
pass.Report(analysis.Diagnostic{
Pos: expr.Pos(),
End: expr.End(),
Category: LinterName,
Message: "Literals are not allowed in the body of the logger",
SuggestedFixes: nil,
})
return false
}

31
internal/analyzers/noliteral/test-case/const-location/positive._go
View file

@ -4,25 +4,46 @@ package logs //declaration package
import (
"fmt"
tochno_ne_const_dly_log "git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs"// The alias of the package with constants differs from the one used
tochno_ne_dly_const_log "git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs"// The alias of the package with constants differs from the one used
)
func (c *cfg) info_ok() {
c.log.Info(tochno_ne_const_dly_log.MSG)
c.log.Info(tochno_ne_dly_const_log.MSG)
}
func (c *cfg) debug_ok() {
c.log.Debug(tochno_ne_const_dly_log.MSG)
c.log.Debug(tochno_ne_dly_const_log.MSG)
}
func (c *cfg) error_ok() {
c.log.Error(tochno_ne_const_dly_log.MSG)
c.log.Error(tochno_ne_dly_const_log.MSG)
}
func (c *cfg) custom_ok_const() {
c.log.Abyr(tochno_ne_const_dly_log.MSG)
c.log.Abyr(tochno_ne_dly_const_log.MSG)
}
// issue: https://git.frostfs.info/TrueCloudLab/linters/issues/16.
// Check that the check does not trigger a false positive for "zap.Error(invalid_pkg.Error)" passed as an argument.
func (c *cfg) debug1_ok() {
c.log.Debug(tochno_ne_dly_const_log.MSG, zap.Stringer("cid", bkt.CID), zap.String("oid", obj.VersionID), zap.Error(invalid_pkg.Error))
}
func (c *cfg) error_ok() {
field:=zap.Error(invalid_pkg.Error)
}
func (c *cfg) error_ok() {
callBack:=&zap.Error(invalid_pkg.Error)
}
func (c *cfg) error_ok() {
field=zap.Error(invalid_pkg.Error)
}
func (c *cfg) error_ok() {
callBack=&zap.Error(invalid_pkg.Error)
}
type Logger interface {
Debug(msg string)

10
internal/analyzers/noliteral/test-case/literals/positive._go
View file

@ -15,6 +15,16 @@ func (c *cfg) debug_ok() {
c.log.Debug(logs.MSG) //acceptable
}
// issue: https://git.frostfs.info/TrueCloudLab/linters/issues/16.
// Check that the check does not trigger a false positive for "zap.Error("literal")" passed as an argument.
func (c *cfg) debug1_ok() {
c.log.Debug(logs.MSG, zap.Stringer("cid", bkt.CID), zap.String("oid", obj.VersionID), zap.Error("literal"))
}
func (c *cfg) debug1_ok() {
f(zap.Error("logs.MSG"))
}
func (c *cfg) error_ok() {
c.log.Error(logs.MSG) //acceptable
}