diff --git a/README.md b/README.md index ed1c8e3..d19166d 100644 --- a/README.md +++ b/README.md @@ -1,25 +1,25 @@ -# linters +# linters ## Overview -`linters` is a project that enables the integration of custom linting rules into the [golangci-lint](https://github.com/golangci/golangci-lint) framework. +`linters` is a project that enables the integration of custom linting rules into the [golangci-lint](https://github.com/golangci/golangci-lint) framework. ## Usage -Add to .golangci.yml +Add to .golangci.yml ```yml linters-settings: custom: - custom-linters: + truecloudlab-linters: path: <Path to the directory with libraries> original-url: git.frostfs.info/TrueCloudLab/linters linters: enable: - custom-linters + truecloudlab-linters ``` @@ -33,39 +33,44 @@ Add to .golangci.yml ## Available linters -| Name | Description | -| ----------------------- | --------------------------------------------------------------------------- | -| [noliteral](#noliteral) | The tool prohibits the use of literal string arguments in logging functions | +| Name | Description | +|-------------------------|-------------------------------------------------------------------------------------------------------------------------------| +| [noliteral](#noliteral) | The tool prohibits the use of literal string arguments in logging functions. | +| [useStrconv](#useStrconv) | The `useStrconv` linter recommends the utilization of `strconv` over `fmt` when performing string conversions of primitive data types. Detailed guidelines can be found in the [official Uber Go Style Guide](https://github.com/uber-go/guide/blob/master/style.md#prefer-strconv-over-fmt). | + ## Linters Configuration Settings via a configuration file are available if golangci-lint >= 1.5.4 is used -### noliteral +### noliteral ##### File Configuration ```yml linters-settings: custom: - noliteral: + truecloudlab-linters: path: .bin/external_linters.so - original-url: git.frostfs.info/TrueCloudLab/linters.git + 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 - + noliteral: + enable: true # optional + 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 +### useStrconv -| 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). - +##### File Configuration +```yml +linters-settings: + custom: + truecloudlab-linters: + path: .bin/external_linters.so + original-url: git.frostfs.info/TrueCloudLab/linters.git + settings: + useStrconv: # optional + enable: true +``` diff --git a/internal/analyzers/noliteral/linter.go b/internal/analyzers/noliteral/linter.go index 1e88b89..107613d 100644 --- a/internal/analyzers/noliteral/linter.go +++ b/internal/analyzers/noliteral/linter.go @@ -2,10 +2,9 @@ package noliteral import ( "go/ast" - "go/token" - "strings" - "sync" + astutils "git.frostfs.info/TrueCloudLab/linters/pkg/ast-utils" + "github.com/mitchellh/mapstructure" "golang.org/x/tools/go/analysis" ) @@ -17,21 +16,39 @@ var LogsAnalyzer = &analysis.Analyzer{ Run: run, } -var ( - aliasCache = sync.Map{} -) - type Configuration struct { TargetMethods []string `mapstructure:"target-methods"` DisablePackages []string `mapstructure:"disable-packages"` ConstantsPackage string `mapstructure:"constants-package"` + Enable bool `mapstructure:"enable"` } var Config = Configuration{ TargetMethods: []string{"Debug", "Info", "Warn", "Error"}, } +func New(conf any) (*analysis.Analyzer, error) { + var config Configuration + config.Enable = true + + err := mapstructure.Decode(conf, &config) + if err != nil { + Config.Enable = true + return LogsAnalyzer, nil + } + + Config.TargetMethods = append(Config.TargetMethods, config.TargetMethods...) + Config.ConstantsPackage = config.ConstantsPackage + Config.DisablePackages = config.DisablePackages + Config.Enable = config.Enable + + return LogsAnalyzer, nil +} + func run(pass *analysis.Pass) (interface{}, error) { + if !Config.Enable { + return nil, nil + } for _, file := range pass.Files { ast.Inspect(file, func(n ast.Node) bool { expr, ok := n.(*ast.CallExpr) @@ -39,19 +56,19 @@ func run(pass *analysis.Pass) (interface{}, error) { return true } - isLog, _ := isLogDot(expr.Fun) - if !isLog || len(expr.Args) == 0 { + isLog, _ := astutils.IsTargetMethod(expr.Fun, Config.TargetMethods) + if !isLog || len(expr.Args) == 0 || astutils.HasNoLintComment(pass, expr.Pos()) { return true } - if !isStringValue(expr.Args[0]) { - alias, _ := getAliasByPkgName(file, Config.ConstantsPackage) - if Config.ConstantsPackage == "" || getPackageName(expr.Args[0]) == alias || getPackageName(expr.Args[0]) == "" { + 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 == getPackageName(expr.Args[0]) { + if pkgName == astutils.GetPackageName(expr.Args[0]) { return true } } @@ -80,81 +97,3 @@ func run(pass *analysis.Pass) (interface{}, error) { return nil, nil } - -func isLogDot(expr ast.Expr) (bool, string) { - sel, ok := expr.(*ast.SelectorExpr) - if !ok { - return false, "" - } - - for _, method := range Config.TargetMethods { - if isIdent(sel.Sel, method) { - return true, method - } - } - return false, "" -} - -func isIdent(expr ast.Expr, ident string) bool { - id, ok := expr.(*ast.Ident) - if ok && id.Name == ident { - return true - } - - return false -} - -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 c582b59..b968853 100644 --- a/internal/analyzers/noliteral/linter_test.go +++ b/internal/analyzers/noliteral/linter_test.go @@ -13,6 +13,7 @@ import ( func init() { Config.ConstantsPackage = "git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs" + Config.Enable = true } func TestAnalyzer_negative(t *testing.T) { diff --git a/internal/analyzers/use-strconv/linter.go b/internal/analyzers/use-strconv/linter.go new file mode 100644 index 0000000..304d822 --- /dev/null +++ b/internal/analyzers/use-strconv/linter.go @@ -0,0 +1,104 @@ +package usestrconv + +import ( + "go/ast" + + astutils "git.frostfs.info/TrueCloudLab/linters/pkg/ast-utils" + "github.com/mitchellh/mapstructure" + "golang.org/x/tools/go/analysis" +) + +const LinterName = "useStrconv" + +var LogsAnalyzer = &analysis.Analyzer{ + Name: LinterName, + Doc: LinterName + " linter recommends the utilization of `strconv` over `fmt` when performing string conversions of primitive data types.", + Run: run, +} + +type Configuration struct { + Enable bool `mapstructure:"enable"` +} + +var Config = Configuration{ + Enable: true, +} +var modyficators = []string{"%d", "%f", "%t", "%x"} + +func New(conf any) (*analysis.Analyzer, error) { + configMap, ok := conf.(map[string]any) + if !ok { + Config.Enable = true + return LogsAnalyzer, nil + } + + var config Configuration + config.Enable = true + err := mapstructure.Decode(configMap, &config) + if err != nil { + return nil, err + } + + Config.Enable = config.Enable + return LogsAnalyzer, nil +} + +func run(pass *analysis.Pass) (interface{}, error) { + if !Config.Enable { + return nil, nil + } + + for _, file := range pass.Files { + ast.Inspect(file, func(n ast.Node) bool { + expr, ok := n.(*ast.CallExpr) + if !ok { + return true + } + + expectedPackageAlias, err := astutils.GetAliasByPkgName(file, "fmt") + if err != nil { + return false + } + if expectedPackageAlias != astutils.GetPackageName(expr.Fun) { + return true + } + + isTarget, _ := astutils.IsTargetMethod(expr.Fun, []string{"Sprintf"}) + + if !isTarget || !astutils.IsStringValue(expr.Args[0]) { + return true + } + + stringLiteral, ok := expr.Args[0].(*ast.BasicLit) + + if !ok || astutils.HasNoLintComment(pass, expr.Pos()) { + return true + } + + modValue := unquote(stringLiteral.Value) + for _, modificator := range modyficators { + if modValue == modificator { + pass.Report(analysis.Diagnostic{ + Pos: expr.Pos(), + End: expr.End(), + Category: LinterName, + Message: `Usage of fmt.Sprintf(` + modificator + `) is not allowed`, + SuggestedFixes: nil, + }) + return false + } + } + + return true + }) + } + + return nil, nil +} + +func unquote(s string) string { + if len(s) < 2 { + return s + } + return s[1 : len(s)-1] +} diff --git a/internal/analyzers/use-strconv/linter_test.go b/internal/analyzers/use-strconv/linter_test.go new file mode 100644 index 0000000..e7b51de --- /dev/null +++ b/internal/analyzers/use-strconv/linter_test.go @@ -0,0 +1,74 @@ +package usestrconv + +import ( + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "runtime" + "testing" + + "golang.org/x/tools/go/analysis" +) + +func init() { + Config.Enable = true +} +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/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_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/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) + } +} diff --git a/internal/analyzers/use-strconv/test-case/negative._go b/internal/analyzers/use-strconv/test-case/negative._go new file mode 100644 index 0000000..cfe69e0 --- /dev/null +++ b/internal/analyzers/use-strconv/test-case/negative._go @@ -0,0 +1,15 @@ +package src_test + +import ( + "fmt" +) + +func main(){ + for i := 0; i < b.N; i++ { + s := fmt.Sprintf("%d", height) + s := fmt.Sprintf("%x", height) + s := fmt.Sprintf("%f", height) + s := fmt.Sprintf("%t", height) + + } +} diff --git a/internal/analyzers/use-strconv/test-case/positive._go b/internal/analyzers/use-strconv/test-case/positive._go new file mode 100644 index 0000000..17fdca2 --- /dev/null +++ b/internal/analyzers/use-strconv/test-case/positive._go @@ -0,0 +1,11 @@ +package src_test + +import ( + "fmt" +) + +func main(){ + for i := 0; i < b.N; i++ { + s := fmt.Sprintf("%d ", height) + } +} diff --git a/main.go b/main.go index 633c6c3..e7d3b03 100644 --- a/main.go +++ b/main.go @@ -1,11 +1,8 @@ package main import ( - "os" - "strings" - - noliteral "git.frostfs.info/TrueCloudLab/linters/internal/analyzers/noliteral" - "github.com/mitchellh/mapstructure" + "git.frostfs.info/TrueCloudLab/linters/internal/analyzers/noliteral" + useStrconv "git.frostfs.info/TrueCloudLab/linters/internal/analyzers/use-strconv" "golang.org/x/tools/go/analysis" ) @@ -21,31 +18,22 @@ func (*analyzerPlugin) GetAnalyzers() []*analysis.Analyzer { // for version ci-lint >= '1.5.4'. func New(conf any) ([]*analysis.Analyzer, error) { - targetMethods := strings.Split(os.Getenv("NOLITERAL_TARGET_METHODS"), ",") - disablePackages := strings.Split(os.Getenv("NOLITERAL_DISABLE_PACKAGES"), ",") - constantsPackage := os.Getenv("NOLITERAL_CONSTANTS_PACKAGE") + confMap, ok := conf.(map[string]any) - configMap := map[string]any{ - "target-methods": targetMethods, - "constants-package": constantsPackage, - "disable-packages": disablePackages, + var noliteralConfig, useStrconvConfig any + if ok { + noliteralConfig = confMap["noliteral"] + useStrconvConfig = confMap["useStrconv"] } - - if confMap, ok := conf.(map[string]any); ok { - for k, v := range confMap { - configMap[k] = v - } - } - - var config noliteral.Configuration - err := mapstructure.Decode(configMap, &config) + noliteral, err := noliteral.New(noliteralConfig) 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 + useStrconv, err := useStrconv.New(useStrconvConfig) + if err != nil { + return nil, err + } - return []*analysis.Analyzer{noliteral.LogsAnalyzer}, nil + return []*analysis.Analyzer{noliteral, useStrconv}, nil } diff --git a/pkg/ast-utils/ast.go b/pkg/ast-utils/ast.go new file mode 100644 index 0000000..df22a59 --- /dev/null +++ b/pkg/ast-utils/ast.go @@ -0,0 +1,111 @@ +package astutils + +import ( + "go/ast" + "go/token" + "strings" + "sync" + + "golang.org/x/tools/go/analysis" +) + +type aliasCacheKey struct { + File *ast.File + PkgName string +} + +var ( + aliasCache = sync.Map{} +) + +func IsTargetMethod(expr ast.Expr, targetMethods []string) (bool, string) { + sel, ok := expr.(*ast.SelectorExpr) + if !ok { + return false, "" + } + + for _, method := range targetMethods { + if IsIdent(sel.Sel, method) { + return true, method + } + } + return false, "" +} + +func IsIdent(expr ast.Expr, ident string) bool { + id, ok := expr.(*ast.Ident) + if ok && id.Name == ident { + return true + } + + return false +} + +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) { + key := aliasCacheKey{File: file, PkgName: pkgName} + if alias, ok := aliasCache.Load(key); ok { + return alias.(string), nil + } + + var alias string + specs := file.Imports + + for _, spec := range specs { + alias = GetAliasFromImportSpec(spec, pkgName) + if alias != "" { + break + } + } + + aliasCache.Store(key, 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 "" +} + +func HasNoLintComment(pass *analysis.Pass, pos token.Pos) bool { + for _, commentGroup := range pass.Files[0].Comments { + if commentGroup.End() < pos { + for _, comment := range commentGroup.List { + if strings.Contains(comment.Text, "nolint") { + return true + } + } + } + } + return false +}