diff --git a/.forgejo/workflows/tests.yml b/.forgejo/workflows/tests.yml index 107d960..7883a2f 100644 --- a/.forgejo/workflows/tests.yml +++ b/.forgejo/workflows/tests.yml @@ -13,7 +13,7 @@ jobs: with: go-version: '1.20' cache: true - - name: Run staticcheck + - name: Build lib run: make lib lint: diff --git a/README.md b/README.md index bf11cbb..ed1c8e3 100644 --- a/README.md +++ b/README.md @@ -4,34 +4,6 @@ `linters` is a project that enables the integration of custom linting rules into the [golangci-lint](https://github.com/golangci/golangci-lint) framework. -## Available linters - -| Name | Description | -| ----------------------- | --------------------------------------------------------------------------- | -| [noliteral](#noliteral) | The tool prohibits the use of literal string arguments in logging functions | - -## Linters Configuration - -### 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" -``` - -## Installation - -```bash - git clone git.frostfs.info/TrueCloudLab/linters - cd linters - make lib OUT_DIR= -``` - ## Usage Add to .golangci.yml @@ -44,7 +16,56 @@ Add to .golangci.yml path: original-url: git.frostfs.info/TrueCloudLab/linters + linters: enable: custom-linters -``` \ No newline at end of file +``` + + +## Installation + +```bash + git clone git.frostfs.info/TrueCloudLab/linters + cd linters + make lib OUT_DIR= +``` + +## Available linters + +| Name | Description | +| ----------------------- | --------------------------------------------------------------------------- | +| [noliteral](#noliteral) | The tool prohibits the use of literal string arguments in logging functions | + +## Linters Configuration + +Settings via a configuration file are available if golangci-lint >= 1.5.4 is used + +### noliteral + +##### File Configuration + +```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" + disable-packages: ["pkg1", "pkg2"] # List of packages for which the check should be disabled. + constants-package: "git.frostfs.info/rep/logs" # if not set, then the check is disabled + +``` +##### ENV Configuration + + +| Variable | Description | +| ----------------------------- | ------------------------------------------------------- | +| `NOLITERAL_TARGET_METHODS` | List of methods to analyze | +| `NOLITERAL_DISABLE_PACKAGES` | List of packages for which the check should be disabled | +| `NOLITERAL_CONSTANTS_PACKAGE` | Path to the package with constants | + + +**Note:** You may need to clear the golangci-lint cache when configuring through ENV. More details can be found [here](https://golangci-lint.run/usage/configuration/#cache). + diff --git a/internal/analyzers/noliteral/linter.go b/internal/analyzers/noliteral/linter.go index 828959a..1e88b89 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,14 @@ var LogsAnalyzer = &analysis.Analyzer{ Run: run, } +var ( + aliasCache = sync.Map{} +) + type Configuration struct { - TargetMethods []string `mapstructure:"target-methods"` + TargetMethods []string `mapstructure:"target-methods"` + DisablePackages []string `mapstructure:"disable-packages"` + ConstantsPackage string `mapstructure:"constants-package"` } var Config = Configuration{ @@ -32,11 +40,29 @@ 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 + } + + for _, pkgName := range Config.DisablePackages { + if pkgName == 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 +108,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..633c6c3 100644 --- a/main.go +++ b/main.go @@ -1,6 +1,9 @@ package main import ( + "os" + "strings" + noliteral "git.frostfs.info/TrueCloudLab/linters/internal/analyzers/noliteral" "github.com/mitchellh/mapstructure" "golang.org/x/tools/go/analysis" @@ -12,21 +15,37 @@ type analyzerPlugin struct{} // for version ci-lint < '1.5.4'. func (*analyzerPlugin) GetAnalyzers() []*analysis.Analyzer { - return []*analysis.Analyzer{noliteral.LogsAnalyzer} + analyzer, _ := New(nil) + return analyzer } // for version ci-lint >= '1.5.4'. func New(conf any) ([]*analysis.Analyzer, error) { - var config noliteral.Configuration + targetMethods := strings.Split(os.Getenv("NOLITERAL_TARGET_METHODS"), ",") + disablePackages := strings.Split(os.Getenv("NOLITERAL_DISABLE_PACKAGES"), ",") + constantsPackage := os.Getenv("NOLITERAL_CONSTANTS_PACKAGE") + + configMap := map[string]any{ + "target-methods": targetMethods, + "constants-package": constantsPackage, + "disable-packages": disablePackages, + } if confMap, ok := conf.(map[string]any); ok { - err := mapstructure.Decode(confMap, &config) - if err != nil { - return nil, err + for k, v := range confMap { + configMap[k] = v } - - noliteral.Config.TargetMethods = append(noliteral.Config.TargetMethods, config.TargetMethods...) } + var config noliteral.Configuration + err := mapstructure.Decode(configMap, &config) + if err != nil { + return nil, err + } + + noliteral.Config.TargetMethods = append(noliteral.Config.TargetMethods, config.TargetMethods...) + noliteral.Config.ConstantsPackage = config.ConstantsPackage + noliteral.Config.DisablePackages = config.DisablePackages + return []*analysis.Analyzer{noliteral.LogsAnalyzer}, nil }