const-check #4

Merged
dstepanov-yadro merged 3 commits from achuprov/linters:const-check into master 2024-09-04 19:51:22 +00:00
8 changed files with 275 additions and 13 deletions
Showing only changes of commit 1646e26225 - Show all commits

View file

@ -12,6 +12,7 @@
## Linters Configuration ## Linters Configuration
The settings for linters are available if golangci-lint >= 1.5.4 is used.
### noliteral ### noliteral
```yml ```yml
@ -22,6 +23,8 @@ linters-settings:
original-url: git.frostfs.info/TrueCloudLab/linters.git original-url: git.frostfs.info/TrueCloudLab/linters.git
settings: settings:
target-methods : ["reportFlushError", "reportError"] #optional. Enabled by default "Debug", "Info", "Warn", "Error" 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
fyrchik marked this conversation as resolved Outdated

Isn't it a package, not repository?

Isn't it a `package`, not `repository`?
``` ```
## Installation ## Installation
@ -44,6 +47,7 @@ Add to .golangci.yml
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
fyrchik marked this conversation as resolved Outdated

?

?
linters: linters:
enable: enable:
custom-linters custom-linters

View file

@ -3,6 +3,8 @@ package noliteral
import ( import (
"go/ast" "go/ast"
"go/token" "go/token"
"strings"
"sync"
"golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis"
) )
@ -15,8 +17,13 @@ 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"`
ConstantsPackage string `mapstructure:"constants-package"`
} }
var Config = Configuration{ var Config = Configuration{
@ -32,11 +39,23 @@ func run(pass *analysis.Pass) (interface{}, error) {
} }
isLog, _ := isLogDot(expr.Fun) isLog, _ := isLogDot(expr.Fun)
if !isLog { if !isLog || len(expr.Args) == 0 {
return true 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]) == "" {
fyrchik marked this conversation as resolved Outdated

It seems we use isConstLogsFIeld only here, and we still do getPackageName inside and on this line. What about removing isConstLogsField and using real == alias || real == ""?

It seems we use `isConstLogsFIeld` only here, and we still do `getPackageName` inside and on this line. What about removing `isConstLogsField` and using `real == alias || real == ""`?
return true
fyrchik marked this conversation as resolved Outdated

It could be better to cache it, but ok.

It could be better to cache it, but ok.
}
pass.Report(analysis.Diagnostic{
Pos: expr.Pos(),
End: expr.End(),
Category: LinterName,
Message: "Wrong package for constants",
fyrchik marked this conversation as resolved Outdated

What kind of resolving problems can we have here? Isn't it just wrong package for constants?

What kind of resolving problems can we have here? Isn't it just `wrong package for constants`?
SuggestedFixes: nil,
})
return true return true
} }
@ -82,3 +101,53 @@ func isStringValue(expr ast.Expr) bool {
basicLit, ok := expr.(*ast.BasicLit) basicLit, ok := expr.(*ast.BasicLit)
return ok && basicLit.Kind == token.STRING 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 {
fyrchik marked this conversation as resolved Outdated

What about file.Imports?

What about `file.Imports`?

*ast.File.Imports is a slice, it can't be used as a key. Thus, we would have to add a second argument solely to use it as a key. Should I do it this way?

*ast.File.Imports is a slice, it can't be used as a key. Thus, we would have to add a second argument solely to use it as a key. Should I do it this way?

No, I mean isn't is the case that file.Imports == getImportSpecs(file)?

No, I mean isn't is the case that `file.Imports == getImportSpecs(file)`?

fixed

fixed
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

@ -11,14 +11,17 @@ import (
"golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis"
) )
func init() {
Config.ConstantsPackage = "git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs"
}
func TestAnalyzer_negative(t *testing.T) { func TestAnalyzer_negative(t *testing.T) {
const countNegativeCases = 4 const countNegativeCases = 4
_, filename, _, _ := runtime.Caller(0) _, filename, _, _ := runtime.Caller(0)
dir := filepath.Dir(filename) dir := filepath.Dir(filename)
fset := token.NewFileSet() 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 { if err != nil {
t.Fatal(err) 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) _, filename, _, _ := runtime.Caller(0)
dir := filepath.Dir(filename) dir := filepath.Dir(filename)
fset := token.NewFileSet() 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 { if err != nil {
t.Fatal(err) t.Fatal(err)
} }

View file

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

View file

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

View file

@ -2,8 +2,11 @@ package src_test
import ( import (
"fmt" "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() { func (c *cfg) info_ok() {
c.log.Info(logs.MSG) //acceptable c.log.Info(logs.MSG) //acceptable
} }

View file

@ -26,6 +26,7 @@ func New(conf any) ([]*analysis.Analyzer, error) {
} }
noliteral.Config.TargetMethods = append(noliteral.Config.TargetMethods, config.TargetMethods...) noliteral.Config.TargetMethods = append(noliteral.Config.TargetMethods, config.TargetMethods...)
noliteral.Config.ConstantsPackage = config.ConstantsPackage
} }
return []*analysis.Analyzer{noliteral.LogsAnalyzer}, nil return []*analysis.Analyzer{noliteral.LogsAnalyzer}, nil