[#10] linters: add useStrconv linter #10

Merged
dstepanov-yadro merged 3 commits from achuprov/linters:nofmt into master 2024-09-04 19:51:22 +00:00
9 changed files with 389 additions and 141 deletions

View file

@ -12,14 +12,14 @@ Add to .golangci.yml
linters-settings: linters-settings:
custom: custom:
custom-linters: truecloudlab-linters:
path: <Path to the directory with libraries> path: <Path to the directory with libraries>
original-url: git.frostfs.info/TrueCloudLab/linters original-url: git.frostfs.info/TrueCloudLab/linters
linters: linters:
enable: enable:
custom-linters truecloudlab-linters
``` ```
@ -34,8 +34,10 @@ Add to .golangci.yml
## Available linters ## Available linters
| Name | Description | | Name | Description |
| ----------------------- | --------------------------------------------------------------------------- | |-------------------------|-------------------------------------------------------------------------------------------------------------------------------|
| [noliteral](#noliteral) | The tool prohibits the use of literal string arguments in logging functions | | [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 ## Linters Configuration
@ -48,24 +50,27 @@ Settings via a configuration file are available if golangci-lint >= 1.5.4 is use
```yml ```yml
linters-settings: linters-settings:
custom: custom:
noliteral: truecloudlab-linters:
path: .bin/external_linters.so path: .bin/external_linters.so
original-url: git.frostfs.info/TrueCloudLab/linters.git original-url: git.frostfs.info/TrueCloudLab/linters.git
settings: settings:
noliteral:
enable: true # optional
target-methods: ["reportFlushError", "reportError"] # optional. Enabled by default "Debug", "Info", "Warn", "Error" 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. 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 constants-package: "git.frostfs.info/rep/logs" # if not set, then the check is disabled
``` ```
##### ENV Configuration
### useStrconv
| Variable | Description | ##### File Configuration
| ----------------------------- | ------------------------------------------------------- | ```yml
| `NOLITERAL_TARGET_METHODS` | List of methods to analyze | linters-settings:
| `NOLITERAL_DISABLE_PACKAGES` | List of packages for which the check should be disabled | custom:
| `NOLITERAL_CONSTANTS_PACKAGE` | Path to the package with constants | truecloudlab-linters:
path: .bin/external_linters.so
original-url: git.frostfs.info/TrueCloudLab/linters.git
**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). settings:
useStrconv: # optional
enable: true
```

View file

@ -2,10 +2,9 @@ package noliteral
import ( import (
"go/ast" "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" "golang.org/x/tools/go/analysis"
) )
@ -17,21 +16,39 @@ var LogsAnalyzer = &analysis.Analyzer{
Run: run, Run: run,
} }
var (
aliasCache = sync.Map{}
)
type Configuration struct { type Configuration struct {
TargetMethods []string `mapstructure:"target-methods"` TargetMethods []string `mapstructure:"target-methods"`
DisablePackages []string `mapstructure:"disable-packages"` DisablePackages []string `mapstructure:"disable-packages"`
ConstantsPackage string `mapstructure:"constants-package"` ConstantsPackage string `mapstructure:"constants-package"`
Enable bool `mapstructure:"enable"`
} }
var Config = Configuration{ var Config = Configuration{
TargetMethods: []string{"Debug", "Info", "Warn", "Error"}, 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) { func run(pass *analysis.Pass) (interface{}, error) {
if !Config.Enable {
return nil, nil
}
for _, file := range pass.Files { for _, file := range pass.Files {
ast.Inspect(file, func(n ast.Node) bool { ast.Inspect(file, func(n ast.Node) bool {
expr, ok := n.(*ast.CallExpr) expr, ok := n.(*ast.CallExpr)
@ -39,19 +56,19 @@ func run(pass *analysis.Pass) (interface{}, error) {
return true return true
} }
isLog, _ := isLogDot(expr.Fun) isLog, _ := astutils.IsTargetMethod(expr.Fun, Config.TargetMethods)
if !isLog || len(expr.Args) == 0 { if !isLog || len(expr.Args) == 0 || astutils.HasNoLintComment(pass, expr.Pos()) {
return true return true
} }
if !isStringValue(expr.Args[0]) { if !astutils.IsStringValue(expr.Args[0]) {
alias, _ := getAliasByPkgName(file, Config.ConstantsPackage) alias, _ := astutils.GetAliasByPkgName(file, Config.ConstantsPackage)
if Config.ConstantsPackage == "" || getPackageName(expr.Args[0]) == alias || getPackageName(expr.Args[0]) == "" { if Config.ConstantsPackage == "" || astutils.GetPackageName(expr.Args[0]) == alias || astutils.GetPackageName(expr.Args[0]) == "" {
return true return true
} }
for _, pkgName := range Config.DisablePackages { for _, pkgName := range Config.DisablePackages {
if pkgName == getPackageName(expr.Args[0]) { if pkgName == astutils.GetPackageName(expr.Args[0]) {
return true return true
} }
} }
@ -80,81 +97,3 @@ func run(pass *analysis.Pass) (interface{}, error) {
return nil, nil 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 ""
}

View file

@ -13,6 +13,7 @@ import (
func init() { func init() {
Config.ConstantsPackage = "git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs" Config.ConstantsPackage = "git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs"
Config.Enable = true
} }
func TestAnalyzer_negative(t *testing.T) { func TestAnalyzer_negative(t *testing.T) {

View file

@ -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]
}

View file

@ -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)
}
}

View file

@ -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)
}
}

View file

@ -0,0 +1,11 @@
package src_test
import (
"fmt"
)
func main(){
for i := 0; i < b.N; i++ {
s := fmt.Sprintf("%d ", height)
}
}

38
main.go
View file

@ -1,11 +1,8 @@
package main package main
import ( import (
"os" "git.frostfs.info/TrueCloudLab/linters/internal/analyzers/noliteral"
"strings" useStrconv "git.frostfs.info/TrueCloudLab/linters/internal/analyzers/use-strconv"
noliteral "git.frostfs.info/TrueCloudLab/linters/internal/analyzers/noliteral"
"github.com/mitchellh/mapstructure"
"golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis"
) )
@ -21,31 +18,22 @@ func (*analyzerPlugin) GetAnalyzers() []*analysis.Analyzer {
// for version ci-lint >= '1.5.4'. // for version ci-lint >= '1.5.4'.
func New(conf any) ([]*analysis.Analyzer, error) { func New(conf any) ([]*analysis.Analyzer, error) {
targetMethods := strings.Split(os.Getenv("NOLITERAL_TARGET_METHODS"), ",") confMap, ok := conf.(map[string]any)
disablePackages := strings.Split(os.Getenv("NOLITERAL_DISABLE_PACKAGES"), ",")
constantsPackage := os.Getenv("NOLITERAL_CONSTANTS_PACKAGE")
configMap := map[string]any{ var noliteralConfig, useStrconvConfig any
"target-methods": targetMethods, if ok {
"constants-package": constantsPackage, noliteralConfig = confMap["noliteral"]
"disable-packages": disablePackages, useStrconvConfig = confMap["useStrconv"]
} }
noliteral, err := noliteral.New(noliteralConfig)
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)
if err != nil { if err != nil {
return nil, err return nil, err
} }
noliteral.Config.TargetMethods = append(noliteral.Config.TargetMethods, config.TargetMethods...) useStrconv, err := useStrconv.New(useStrconvConfig)
noliteral.Config.ConstantsPackage = config.ConstantsPackage if err != nil {
noliteral.Config.DisablePackages = config.DisablePackages return nil, err
}
return []*analysis.Analyzer{noliteral.LogsAnalyzer}, nil return []*analysis.Analyzer{noliteral, useStrconv}, nil
} }

111
pkg/ast-utils/ast.go Normal file
View file

@ -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
}