const-check #4

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

View file

@ -13,7 +13,7 @@ jobs:
with:
go-version: '1.20'
cache: true
- name: Run staticcheck
- name: Build lib
run: make lib
lint:

View file

@ -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=<Path to the directory with libraries>
```
## Usage
Add to .golangci.yml
@ -44,7 +16,56 @@ Add to .golangci.yml
path: <Path to the directory with libraries>
original-url: git.frostfs.info/TrueCloudLab/linters
linters:
enable:
custom-linters
```
```
## Installation
fyrchik marked this conversation as resolved Outdated

Isn't it a package, not repository?

Isn't it a `package`, not `repository`?
```bash
git clone git.frostfs.info/TrueCloudLab/linters
cd linters
make lib OUT_DIR=<Path to the directory with libraries>
```
## 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:
fyrchik marked this conversation as resolved Outdated

?

?
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).

View file

@ -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)
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 == ""`?
if Config.ConstantsPackage == "" || getPackageName(expr.Args[0]) == alias || getPackageName(expr.Args[0]) == "" {
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.
return true
}
for _, pkgName := range Config.DisablePackages {
if pkgName == getPackageName(expr.Args[0]) {
return true
}
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`?
}
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
}
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
}
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 {

Have you checked what happens when the directory is pkg/xxx and there is package yyy file inside?

Have you checked what happens when the directory is `pkg/xxx` and there is `package yyy` file inside?

I tested it in the case with aliases. If I understood correctly, this is the same situation

I tested it in the case with aliases. If I understood correctly, this is the same situation

I mean https://go.dev/ref/spec#Import_declarations

If the PackageName is omitted, it defaults to the identifier specified in the package clause of the imported package.

It seem you use directory name instead here.

I mean https://go.dev/ref/spec#Import_declarations >If the PackageName is omitted, it defaults to the identifier specified in the package clause of the imported package. It seem you use _directory_ name instead here.
if ident, ok := selectorExpr.X.(*ast.Ident); ok {
return ident.Name
}
dstepanov-yadro marked this conversation as resolved Outdated

Looks like this condition can be checked before replace and split.

Looks like this condition can be checked before replace and split.

fixed

fixed
}
return ""
}

View file

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

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

@ -47,4 +47,4 @@ var logs = struct {
type cfg struct {
log Logger
}
}

View file

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

33
main.go
View file

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