From 2c4e1d8b534a7e38a3a43988e4888e3799cb8faa Mon Sep 17 00:00:00 2001 From: Alexander Chuprov Date: Mon, 12 Feb 2024 16:36:46 +0300 Subject: [PATCH 1/2] [#16] noliteral: Refactor Signed-off-by: Alexander Chuprov --- internal/analyzers/noliteral/linter.go | 85 ++++++++++--------- .../test-case/const-location/positive._go | 10 +-- 2 files changed, 51 insertions(+), 44 deletions(-) diff --git a/internal/analyzers/noliteral/linter.go b/internal/analyzers/noliteral/linter.go index 107613d..ffb1635 100644 --- a/internal/analyzers/noliteral/linter.go +++ b/internal/analyzers/noliteral/linter.go @@ -51,49 +51,56 @@ 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 n.(type) { + // log.Error() + case *ast.CallExpr: + if expr, ok := n.(*ast.CallExpr); ok { + return analyzeCallExpr(pass, expr, file) } - - for _, pkgName := range Config.DisablePackages { - if pkgName == astutils.GetPackageName(expr.Args[0]) { - return true - } - } - - 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 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 + } + + for _, pkgName := range Config.DisablePackages { + if pkgName == astutils.GetPackageName(expr.Args[0]) { + return true + } + } + + 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 +} diff --git a/internal/analyzers/noliteral/test-case/const-location/positive._go b/internal/analyzers/noliteral/test-case/const-location/positive._go index 013132e..96af649 100644 --- a/internal/analyzers/noliteral/test-case/const-location/positive._go +++ b/internal/analyzers/noliteral/test-case/const-location/positive._go @@ -4,23 +4,23 @@ 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) } -- 2.45.2 From 7b767571bc462ff9eaefccf0c8362928cab8dd4d Mon Sep 17 00:00:00 2001 From: Alexander Chuprov Date: Mon, 12 Feb 2024 17:13:12 +0300 Subject: [PATCH 2/2] [#16] noliteral: Fix nested functions Signed-off-by: Alexander Chuprov --- internal/analyzers/noliteral/linter.go | 21 ++++++++++++++----- .../test-case/const-location/positive._go | 21 +++++++++++++++++++ .../noliteral/test-case/literals/positive._go | 10 +++++++++ 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/internal/analyzers/noliteral/linter.go b/internal/analyzers/noliteral/linter.go index ffb1635..acd7173 100644 --- a/internal/analyzers/noliteral/linter.go +++ b/internal/analyzers/noliteral/linter.go @@ -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,7 +52,17 @@ func run(pass *analysis.Pass) (interface{}, error) { } for _, file := range pass.Files { ast.Inspect(file, func(n ast.Node) bool { - switch n.(type) { + switch v := n.(type) { + // a := zap.Error() + case *ast.AssignStmt: + if _, ok := v.Rhs[0].(*ast.CallExpr); ok { + return false + } + // 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 { @@ -69,18 +80,18 @@ func analyzeCallExpr(pass *analysis.Pass, expr *ast.CallExpr, file *ast.File) bo isLog, _ := astutils.IsTargetMethod(expr.Fun, Config.TargetMethods) if !isLog || len(expr.Args) == 0 || astutils.HasNoLintComment(pass, expr.Pos()) { - return true + 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 true + return false } for _, pkgName := range Config.DisablePackages { if pkgName == astutils.GetPackageName(expr.Args[0]) { - return true + return false } } @@ -91,7 +102,7 @@ func analyzeCallExpr(pass *analysis.Pass, expr *ast.CallExpr, file *ast.File) bo Message: "Wrong package for constants", SuggestedFixes: nil, }) - return true + return false } pass.Report(analysis.Diagnostic{ diff --git a/internal/analyzers/noliteral/test-case/const-location/positive._go b/internal/analyzers/noliteral/test-case/const-location/positive._go index 96af649..a6b4582 100644 --- a/internal/analyzers/noliteral/test-case/const-location/positive._go +++ b/internal/analyzers/noliteral/test-case/const-location/positive._go @@ -23,6 +23,27 @@ func (c *cfg) custom_ok_const() { 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) diff --git a/internal/analyzers/noliteral/test-case/literals/positive._go b/internal/analyzers/noliteral/test-case/literals/positive._go index 5aebeb9..ca45759 100644 --- a/internal/analyzers/noliteral/test-case/literals/positive._go +++ b/internal/analyzers/noliteral/test-case/literals/positive._go @@ -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 } -- 2.45.2