const-check #4
|
@ -13,7 +13,7 @@ jobs:
|
|||
with:
|
||||
go-version: '1.20'
|
||||
cache: true
|
||||
- name: Run staticcheck
|
||||
- name: Build lib
|
||||
run: make lib
|
||||
|
||||
lint:
|
||||
|
|
79
README.md
|
@ -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
|
||||
|
||||
```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
fyrchik
commented
? ?
|
||||
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).
|
||||
|
||||
|
|
|
@ -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
fyrchik
commented
It seems we use 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
fyrchik
commented
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
fyrchik
commented
What kind of resolving problems can we have here? Isn't it just 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
fyrchik
commented
What about What about `file.Imports`?
achuprov
commented
*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?
fyrchik
commented
No, I mean isn't is the case that No, I mean isn't is the case that `file.Imports == getImportSpecs(file)`?
achuprov
commented
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 {
|
||||
fyrchik
commented
Have you checked what happens when the directory is Have you checked what happens when the directory is `pkg/xxx` and there is `package yyy` file inside?
achuprov
commented
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
fyrchik
commented
I mean https://go.dev/ref/spec#Import_declarations
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
dstepanov-yadro
commented
Looks like this condition can be checked before replace and split. Looks like this condition can be checked before replace and split.
achuprov
commented
fixed fixed
|
||||
}
|
||||
return ""
|
||||
}
|
||||
|
|
|
@ -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)
|
||||
}
|
||||
|
|
|
@ -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
|
||||
}
|
|
@ -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
|
||||
}
|
|
@ -47,4 +47,4 @@ var logs = struct {
|
|||
|
||||
type cfg struct {
|
||||
log Logger
|
||||
}
|
||||
}
|
|
@ -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
|
@ -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
|
||||
}
|
||||
|
|
Isn't it a
package
, notrepository
?