[#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). |

nofmt is too generic (we have go fmt), maybe strconv?

`nofmt` is too generic (we have `go fmt`), maybe `strconv`?
## 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:
dstepanov-yadro marked this conversation as resolved Outdated

Is such name mandatory? Maybe truecloudlab or frostfs is better?

Is such name mandatory? Maybe `truecloudlab` or `frostfs` is better?
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
dstepanov-yadro marked this conversation as resolved Outdated

Is it really required? As is see, if you do not pass this value, the linter just will be turned off.

I suggest to correct the comment, for example if not defined, then noliteral check will be disabled

Is it really required? As is see, if you do not pass this value, the linter just will be turned off. I suggest to correct the comment, for example `if not defined, then noliteral check will be disabled`
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

I think noliteral may also have a separate section.

I think noliteral may also have a separate section.

noliteral already has its own section in the README.md d0450b6301/README.md (noliteral)

`noliteral` already has its own section in the README.md https://git.frostfs.info/TrueCloudLab/linters/src/commit/d0450b63015359a704073c6d85559082e6f54d92/README.md#noliteral
| 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"
) )
dstepanov-yadro marked this conversation as resolved Outdated

Our projects usually use two import groups.

Our projects usually use two import groups.
@ -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
}