From 420dd98c24db97f1d78ed7b64239f1b4f367e1b4 Mon Sep 17 00:00:00 2001 From: Alexander Chuprov Date: Fri, 4 Aug 2023 14:26:40 +0300 Subject: [PATCH] [#4] linters: Add check for source of constants Signed-off-by: Alexander Chuprov --- .golangci.yml | 2 +- README.md | 14 ++-- internal/analyzers/noliteral/linter.go | 75 ++++++++++++++++++- internal/analyzers/noliteral/linter_test.go | 70 ++++++++++++++++- .../test-case/const-location/negative._go | 61 +++++++++++++++ .../test-case/const-location/positive._go | 62 +++++++++++++++ .../test-case/{src => literals}/negative._go | 2 +- .../test-case/{src => literals}/positive._go | 3 + main.go | 1 + 9 files changed, 276 insertions(+), 14 deletions(-) create mode 100644 internal/analyzers/noliteral/test-case/const-location/negative._go create mode 100644 internal/analyzers/noliteral/test-case/const-location/positive._go rename internal/analyzers/noliteral/test-case/{src => literals}/negative._go (99%) rename internal/analyzers/noliteral/test-case/{src => literals}/positive._go (81%) diff --git a/.golangci.yml b/.golangci.yml index dba7f0c..de20d1f 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -61,4 +61,4 @@ linters: - gocognit - contextcheck disable-all: true - fast: false + fast: false \ No newline at end of file diff --git a/README.md b/README.md index bf11cbb..e75a2c9 100644 --- a/README.md +++ b/README.md @@ -12,16 +12,19 @@ ## Linters Configuration +The settings for linters are available if golangci-lint >= 1.5.4 is used. ### noliteral ```yml linters-settings: custom: - noliteral: - path: .bin/external_linters.so - original-url: git.frostfs.info/TrueCloudLab/linters.git - settings: - target-methods : ["reportFlushError", "reportError"] #optional. Enabled by default "Debug", "Info", "Warn", "Error" + noliteral: + path: .bin/external_linters.so + original-url: git.frostfs.info/TrueCloudLab/linters.git + settings: + target-methods : ["reportFlushError", "reportError"] #optional. Enabled by default "Debug", "Info", "Warn", "Error" + constants-package: "git.frostfs.info/rep/logs" #if not set, then the check is disabled + ``` ## Installation @@ -44,6 +47,7 @@ Add to .golangci.yml path: original-url: git.frostfs.info/TrueCloudLab/linters + linters: enable: custom-linters diff --git a/internal/analyzers/noliteral/linter.go b/internal/analyzers/noliteral/linter.go index 828959a..ab2012c 100644 --- a/internal/analyzers/noliteral/linter.go +++ b/internal/analyzers/noliteral/linter.go @@ -3,6 +3,8 @@ package noliteral import ( "go/ast" "go/token" + "strings" + "sync" "golang.org/x/tools/go/analysis" ) @@ -15,8 +17,13 @@ var LogsAnalyzer = &analysis.Analyzer{ Run: run, } +var ( + aliasCache = sync.Map{} +) + type Configuration struct { - TargetMethods []string `mapstructure:"target-methods"` + TargetMethods []string `mapstructure:"target-methods"` + ConstantsPackage string `mapstructure:"constants-package"` } var Config = Configuration{ @@ -32,11 +39,23 @@ func run(pass *analysis.Pass) (interface{}, error) { } isLog, _ := isLogDot(expr.Fun) - if !isLog { + if !isLog || len(expr.Args) == 0 { return true } - if len(expr.Args) == 0 || !isStringValue(expr.Args[0]) { + if !isStringValue(expr.Args[0]) { + alias, _ := getAliasByPkgName(file, Config.ConstantsPackage) + if Config.ConstantsPackage == "" || getPackageName(expr.Args[0]) == alias || 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 } @@ -82,3 +101,53 @@ func isStringValue(expr ast.Expr) bool { basicLit, ok := expr.(*ast.BasicLit) return ok && basicLit.Kind == token.STRING } + +func getAliasByPkgName(file *ast.File, pkgName string) (string, error) { + if alias, ok := aliasCache.Load(file); ok { + return alias.(string), nil + } + + var alias string + specs := file.Imports + + for _, spec := range specs { + alias = getAliasFromImportSpec(spec, pkgName) + if alias != "" { + break + } + } + + aliasCache.Store(file, alias) + return alias, nil +} + +func getAliasFromImportSpec(spec *ast.ImportSpec, pkgName string) string { + if spec == nil { + return "" + } + importName := strings.Replace(spec.Path.Value, "\"", "", -1) + if importName != pkgName { + return "" + } + + split := strings.Split(importName, "/") + + if len(split) == 0 { + return "" + } + alias := split[len(split)-1] + if spec.Name != nil { + alias = spec.Name.Name + } + + return alias +} + +func getPackageName(expr ast.Expr) string { + if selectorExpr, ok := expr.(*ast.SelectorExpr); ok { + if ident, ok := selectorExpr.X.(*ast.Ident); ok { + return ident.Name + } + } + return "" +} diff --git a/internal/analyzers/noliteral/linter_test.go b/internal/analyzers/noliteral/linter_test.go index e528e08..c582b59 100644 --- a/internal/analyzers/noliteral/linter_test.go +++ b/internal/analyzers/noliteral/linter_test.go @@ -11,14 +11,17 @@ import ( "golang.org/x/tools/go/analysis" ) +func init() { + Config.ConstantsPackage = "git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs" +} + func TestAnalyzer_negative(t *testing.T) { const countNegativeCases = 4 _, filename, _, _ := runtime.Caller(0) dir := filepath.Dir(filename) - fset := token.NewFileSet() - f, err := parser.ParseFile(fset, filepath.Join(dir, "test-case/src/negative._go"), nil, parser.AllErrors) + f, err := parser.ParseFile(fset, filepath.Join(dir, "test-case/literals/negative._go"), nil, parser.AllErrors) if err != nil { t.Fatal(err) } @@ -42,12 +45,71 @@ func TestAnalyzer_negative(t *testing.T) { } } -func TestAnalyzer_positive(t *testing.T) { +func TestAnalyzer_literals_positive(t *testing.T) { _, filename, _, _ := runtime.Caller(0) dir := filepath.Dir(filename) fset := token.NewFileSet() - f, err := parser.ParseFile(fset, filepath.Join(dir, "test-case/src/positive._go"), nil, parser.AllErrors) + f, err := parser.ParseFile(fset, filepath.Join(dir, "test-case/literals/positive._go"), nil, parser.AllErrors) + if err != nil { + t.Fatal(err) + } + + flag := false + pass := &analysis.Pass{ + Fset: fset, + Files: []*ast.File{f}, + Report: func(diag analysis.Diagnostic) { + flag = true + }, + } + _, err = run(pass) + + if flag { + t.Fail() + } + + if err != nil { + t.Fatal(err) + } +} + +func TestAnalyzer_const_location_negative(t *testing.T) { + const countNegativeCases = 3 + _, filename, _, _ := runtime.Caller(0) + dir := filepath.Dir(filename) + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, filepath.Join(dir, "test-case/const-location/negative._go"), nil, parser.AllErrors) + if err != nil { + t.Fatal(err) + } + + Count := 0 + pass := &analysis.Pass{ + Fset: fset, + Files: []*ast.File{f}, + Report: func(diag analysis.Diagnostic) { + Count++ + }, + } + + _, err = run(pass) + + if Count != countNegativeCases { + t.Fail() + } + if err != nil { + t.Fatal(err) + } +} + +func TestAnalyzer_const_location_positive(t *testing.T) { + _, filename, _, _ := runtime.Caller(0) + dir := filepath.Dir(filename) + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, filepath.Join(dir, "test-case/const-location/positive._go"), nil, parser.AllErrors) if err != nil { t.Fatal(err) } diff --git a/internal/analyzers/noliteral/test-case/const-location/negative._go b/internal/analyzers/noliteral/test-case/const-location/negative._go new file mode 100644 index 0000000..62b2181 --- /dev/null +++ b/internal/analyzers/noliteral/test-case/const-location/negative._go @@ -0,0 +1,61 @@ +package src_test + +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 +) + +/*After upgrading to golangci-lint version 1.5.4 and adding linter configuration, it will be possible to get rid of 'magic repositories'*/ + +func (c *cfg) info_n() { + c.log.Info(logs.MSG) +} + +func (c *cfg) debug_n() { + c.log.Debug(logs.MSG) +} + +func (c *cfg) error_n() { + c.log.Error(logs.MSG) +} + +func (c *cfg) reportFlushError_n() { + c.log.reportFlushError(logs.MSG) +} + +func (c *cfg) reportError_n() { + c.log.reportError(logs.MSG) +} + + + +type Logger interface { + Debug(msg string) + Info(msg string) + Warn(msg string) + Error(msg string) + Abyr(msg string) +} + +type RealLogger struct{} + +func (l RealLogger) Debug(msg string) { +} +func (l RealLogger) Info(msg string) { +} +func (l RealLogger) Warn(msg string) { +} +func (l RealLogger) Error(msg string) { +} +func (l RealLogger) Abyr(msg string) { +} + +var logs = struct { + MSG string +}{ + MSG: "some message", +} + +type cfg struct { + log Logger +} diff --git a/internal/analyzers/noliteral/test-case/const-location/positive._go b/internal/analyzers/noliteral/test-case/const-location/positive._go new file mode 100644 index 0000000..013132e --- /dev/null +++ b/internal/analyzers/noliteral/test-case/const-location/positive._go @@ -0,0 +1,62 @@ +package logs //declaration package + +/*After upgrading to golangci-lint version 1.5.4 and adding linter configuration, it will be possible to get rid of 'magic repositories'*/ + +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 +) + +func (c *cfg) info_ok() { + c.log.Info(tochno_ne_const_dly_log.MSG) +} + +func (c *cfg) debug_ok() { + c.log.Debug(tochno_ne_const_dly_log.MSG) +} + +func (c *cfg) error_ok() { + c.log.Error(tochno_ne_const_dly_log.MSG) +} + +func (c *cfg) custom_ok_const() { + c.log.Abyr(tochno_ne_const_dly_log.MSG) +} + + +type Logger interface { + Debug(msg string) + Info(msg string) + Warn(msg string) + Error(msg string) + Abyr(msg string) +} + +type RealLogger struct{} + +func (l RealLogger) Debug(msg string) { +} +func (l RealLogger) Info(msg string) { +} +func (l RealLogger) Warn(msg string) { +} +func (l RealLogger) Error(msg string) { +} +func (l RealLogger) Abyr(msg string) { +} + +type RealLogger struct{} + +func (l RealLogger) Info(msg string) { + fmt.Println(msg) +} + +var logs = struct { + MSG string +}{ + MSG: "some message", +} + +type cfg struct { + log Logger +} diff --git a/internal/analyzers/noliteral/test-case/src/negative._go b/internal/analyzers/noliteral/test-case/literals/negative._go similarity index 99% rename from internal/analyzers/noliteral/test-case/src/negative._go rename to internal/analyzers/noliteral/test-case/literals/negative._go index 5f06b70..acfd3f4 100644 --- a/internal/analyzers/noliteral/test-case/src/negative._go +++ b/internal/analyzers/noliteral/test-case/literals/negative._go @@ -47,4 +47,4 @@ var logs = struct { type cfg struct { log Logger -} +} \ No newline at end of file diff --git a/internal/analyzers/noliteral/test-case/src/positive._go b/internal/analyzers/noliteral/test-case/literals/positive._go similarity index 81% rename from internal/analyzers/noliteral/test-case/src/positive._go rename to internal/analyzers/noliteral/test-case/literals/positive._go index 79a895a..5aebeb9 100644 --- a/internal/analyzers/noliteral/test-case/src/positive._go +++ b/internal/analyzers/noliteral/test-case/literals/positive._go @@ -2,8 +2,11 @@ package src_test import ( "fmt" + "git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs" ) +/*After upgrading to golangci-lint version 1.5.4 and adding linter configuration, it will be possible to get rid of 'magic repositories'*/ + func (c *cfg) info_ok() { c.log.Info(logs.MSG) //acceptable } diff --git a/main.go b/main.go index 0840323..a34fc49 100644 --- a/main.go +++ b/main.go @@ -26,6 +26,7 @@ func New(conf any) ([]*analysis.Analyzer, error) { } noliteral.Config.TargetMethods = append(noliteral.Config.TargetMethods, config.TargetMethods...) + noliteral.Config.ConstantsPackage = config.ConstantsPackage } return []*analysis.Analyzer{noliteral.LogsAnalyzer}, nil