TrueCloudLab/linters
22
0
Fork 7
generated from TrueCloudLab/basic
Code Issues 5 Pull requests 1 Projects Releases 1 Packages Wiki Activity Actions

[#1] linters: add logs linter #2

Merged
dstepanov-yadro merged 1 commit from achuprov/linters:master into master 2023-07-26 21:08:06 +00:00
Conversation 4 Commits 1 Files changed 14 +146 -556
14 changed files with 146 additions and 556 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
Makefile
View file

@ -1,8 +1,8 @@
OUT_DIR?=./.bin
PLUGIN_SOURCE?=main.go
OUT_DIR ?= ./.bin
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2023-07-21 08:57:55 +00:00
Outdated
Review
Copy link

Nitpick: whitespace aroung ?= is insignificant https://www.gnu.org/software/make/manual/html_node/Setting.html
This is the style we use in our code https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/Makefile#L4 , you may find it more readable.

Nitpick: whitespace aroung `?=` is insignificant https://www.gnu.org/software/make/manual/html_node/Setting.html This is the style we use in our code https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/Makefile#L4 , you may find it more readable.
achuprov commented 2023-07-24 13:45:02 +00:00
Outdated
Review
Copy link

fixed

fixed
PLUGIN_SOURCE ?= main.go
test:
@go test -v ./...
@go test -v ./... -count=1
dstepanov-yadro marked this conversation as resolved Outdated
dstepanov-yadro commented 2023-07-18 10:29:57 +00:00
Outdated
Review
Copy link

Be default go test caches test result. This may cause flaky tests to be cached. @go test ./... -count=1 will disable caching.

Be default `go test` caches test result. This may cause flaky tests to be cached. `@go test ./... -count=1` will disable caching.
lib:
@mkdir -pv $(OUT_DIR)
@ -11,3 +11,8 @@ lib:
lint:
@golangci-lint run
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro commented 2023-07-18 10:42:07 +00:00
Review
Copy link

Please add .golangci.yml file with linter config like here: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/.golangci.yml

Please add `.golangci.yml` file with linter config like here: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/.golangci.yml
achuprov commented 2023-07-18 16:23:47 +00:00
Review
Copy link

The empty repository template already has a set of minimally necessary linters, therefore .golangci.yml is not present in the PR

The empty repository template already has a set of minimally necessary linters, therefore .golangci.yml is not present in the PR
staticcheck-install:
@go install honnef.co/go/tools/cmd/staticcheck@latest
staticcheck-run:
@staticcheck ./...

6
README.md
View file

@ -8,8 +8,8 @@
## Available linters
| Name | Description |
| ----------- | ----------- |
| Name | Description |
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2023-07-18 08:29:43 +00:00
Outdated
Review
Copy link

What editor do you use? I believe VSCode can align tables if you press Tab inside the field.

What editor do you use? I believe VSCode can align tables if you press `Tab` inside the field.
achuprov commented 2023-07-24 13:45:19 +00:00
Outdated
Review
Copy link

fixed

fixed
| --------- | --------------------------------------------------------------------------- |
| noliteral | The tool prohibits the use of literal string arguments in logging functions |
@ -25,7 +25,7 @@
## Usage
Add to .golagci.yml
Add to .golangci.yml
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2023-07-21 09:03:08 +00:00
Outdated
Review
Copy link

typo .golaNgci.yml

typo `.golaNgci.yml`
achuprov commented 2023-07-24 13:45:30 +00:00
Outdated
Review
Copy link

fixed

fixed
```yml

264
internal/analyzers/no-literal/linter_test.go
View file

@ -1,264 +0,0 @@
package noliteral
import (
"go/ast"
"go/parser"
"go/token"
"path/filepath"
"runtime"
"testing"
"golang.org/x/tools/go/analysis"
)
func TestAnalyzerA_n(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/a_negative._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) {
t.Log(diag.Message)
Flag = true
},
}
_, err = run(pass)
if !Flag {
t.Fail()
}
if err != nil {
t.Fatal(err)
}
}
func TestAnalyzerA_p(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/a_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) {
t.Log(diag.Message)
flag = true
},
}
_, err = run(pass)
if flag {
t.Fail()
}
if err != nil {
t.Fatal(err)
}
}
func TestAnalyzerB_n(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/b_negative._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) {
t.Log(diag.Message)
Flag = true
},
}
_, err = run(pass)
if !Flag {
t.Fail()
}
if err != nil {
t.Fatal(err)
}
}
func TestAnalyzerB_p(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/b_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) {
t.Log(diag.Message)
flag = true
},
}
_, err = run(pass)
if flag {
t.Fail()
}
if err != nil {
t.Fatal(err)
}
}
func TestAnalyzerC_n(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/c_negative._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) {
t.Log(diag.Message)
Flag = true
},
}
_, err = run(pass)
if !Flag {
t.Fail()
}
if err != nil {
t.Fatal(err)
}
}
func TestAnalyzerC_p(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/c_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) {
t.Log(diag.Message)
flag = true
},
}
_, err = run(pass)
if flag {
t.Fail()
}
if err != nil {
t.Fatal(err)
}
}
func TestAnalyzerD_n(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/d_negative._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) {
t.Log(diag.Message)
Flag = true
},
}
_, err = run(pass)
if Flag {
t.Fail()
}
if err != nil {
t.Fatal(err)
}
}
func TestAnalyzerD_p(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/d_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) {
t.Log(diag.Message)
flag = true
},
}
_, err = run(pass)
if flag {
t.Fail()
}
if err != nil {
t.Fatal(err)
}
}

36
internal/analyzers/no-literal/test-case/src/a_negative._go
View file

@ -1,36 +0,0 @@
package src_test
func (c *cfg) f1_n() {
c.log.Info("logs.MSG") //unacceptable
}
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
}

46
internal/analyzers/no-literal/test-case/src/a_positive._go
View file

@ -1,46 +0,0 @@
package src_test
import (
"fmt"
)
func (c *cfg) f1_ok() {
c.log.Info(logs.MSG) //acceptable
}
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
}

36
internal/analyzers/no-literal/test-case/src/c_negative._go
View file

@ -1,36 +0,0 @@
package src_test
func (c *cfg) f1_n() {
c.log.Error("logs.MSG") //unacceptable
}
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
}

46
internal/analyzers/no-literal/test-case/src/c_positive._go
View file

@ -1,46 +0,0 @@
package src_test
import (
"fmt"
)
func (c *cfg) f1_ok() {
c.log.Error(logs.MSG) //acceptable
}
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
}

36
internal/analyzers/no-literal/test-case/src/d_negative._go
View file

@ -1,36 +0,0 @@
package src_test
func (c *cfg) f1_n() {
c.log.Abyr("logs.MSG") //unacceptable
}
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
}

46
internal/analyzers/no-literal/test-case/src/d_positive._go
View file

@ -1,46 +0,0 @@
package src_test
import (
"fmt"
)
func (c *cfg) f1_ok() {
c.log.Abyr(logs.MSG) //acceptable
}
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
}

48
internal/analyzers/no-literal/linter.go → internal/analyzers/noliteral/linter.go
View file

@ -3,8 +3,6 @@ package noliteral
import (
"go/ast"
"go/token"
"strings"
"sync"
"golang.org/x/tools/go/analysis"
)
@ -17,11 +15,7 @@ var LogsAnalyzer = &analysis.Analyzer{
Run: run,
}
var (
methodsToSearchOnce = &sync.Once{}
methodsToSearch = []string{"Debug", "Info", "Warn", "Error"}
customLogs = "reportFlushError, reportError"
)
var methodsToSearch = []string{"Debug", "Info", "Warn", "Error", "reportFlushError", "reportError"}
fyrchik commented 2023-07-21 09:05:32 +00:00
Outdated
Review
Copy link

reportFlushError is not some common name. Can we move it to the configuration (maybe later)?

`reportFlushError` is not some common name. Can we move it to the configuration (maybe later)?
func run(pass *analysis.Pass) (interface{}, error) {
for _, file := range pass.Files {
@ -32,17 +26,23 @@ func run(pass *analysis.Pass) (interface{}, error) {
}
isLog, _ := isLogDot(expr.Fun)
if isLog && len(expr.Args) > 0 && isStringValue(expr.Args[0]) {
pass.Report(analysis.Diagnostic{
Pos: expr.Pos(),
End: 0,
Category: LinterName,
Message: "Literals are not allowed in the body of the logger",
SuggestedFixes: nil,
})
return false
if !isLog {
return true
}
return true
if len(expr.Args) == 0 || !isStringValue(expr.Args[0]) {
return true
}
pass.Report(analysis.Diagnostic{
Pos: expr.Pos(),
End: expr.End(),
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2023-07-19 08:29:30 +00:00
Outdated
Review
Copy link

Is 0 here intended?

Is `0` here intended?
👍 1
achuprov commented 2023-07-24 13:45:46 +00:00
Outdated
Review
Copy link

fixed

fixed
Category: LinterName,
Message: "Literals are not allowed in the body of the logger",
SuggestedFixes: nil,
})
return false
})
}
@ -54,14 +54,6 @@ func isLogDot(expr ast.Expr) (bool, string) {
if !ok {
return false, ""
}
methodsToSearchOnce.Do(func() {
for _, cl := range strings.Split(customLogs, ",") {
cl = strings.Trim(cl, " ")
if len(cl) > 0 {
methodsToSearch = append(methodsToSearch, cl)
}
}
})
for _, method := range methodsToSearch {
if isIdent(sel.Sel, method) {
@ -73,12 +65,10 @@ func isLogDot(expr ast.Expr) (bool, string) {
func isIdent(expr ast.Expr, ident string) bool {
id, ok := expr.(*ast.Ident)
if !ok {
return false
}
if id.Name == ident {
if ok && id.Name == ident {
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2023-07-21 09:00:59 +00:00
Outdated
Review
Copy link

So 2 ifs can be replaced with return ok && id.Name == ident?
You actually have already done this in isStringValue function.

So 2 ifs can be replaced with `return ok && id.Name == ident`? You actually have already done this in `isStringValue` function.
achuprov commented 2023-07-24 13:46:20 +00:00
Outdated
Review
Copy link

fixed

fixed
return true
}
return false
}

70
internal/analyzers/noliteral/linter_test.go Normal file
View file

@ -0,0 +1,70 @@
package noliteral
import (
"go/ast"
"go/parser"
"go/token"
"path/filepath"
"runtime"
"testing"
"golang.org/x/tools/go/analysis"
)
func TestAnalyzer_negative(t *testing.T) {
dstepanov-yadro marked this conversation as resolved Outdated
dstepanov-yadro commented 2023-07-19 06:51:31 +00:00
Outdated
Review
Copy link

typo: negative

typo: nega**t**ive
_, 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)
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 != 6 {
t.Fail()
}
if err != nil {
t.Fatal(err)
}
}
func TestAnalyzer_positive(t *testing.T) {
_, filename, _, _ := runtime.Caller(0)
dir := filepath.Dir(filename)
dstepanov-yadro marked this conversation as resolved Outdated
dstepanov-yadro commented 2023-07-19 06:52:17 +00:00
Outdated
Review
Copy link

Please remove empty line after method definition

Please remove empty line after method definition
fset := token.NewFileSet()
f, err := parser.ParseFile(fset, filepath.Join(dir, "test-case/src/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)
}
}

25
internal/analyzers/no-literal/test-case/src/b_negative._go → internal/analyzers/noliteral/test-case/src/negative._go
View file

@ -1,9 +1,32 @@
package src_test
func (c *cfg) f1_n() {
func (c *cfg) info_n() {
c.log.Info("logs.MSG") //unacceptable
}
func (c *cfg) info_n() {
log.Info("logs.MSG") //unacceptable
}
func (c *cfg) debug_n() {
c.log.Debug("logs.MSG") //unacceptable
}
func (c *cfg) error_n() {
c.log.Error("logs.MSG") //unacceptable
}
func (c *cfg) reportFlushError_n() {
c.log.reportFlushError("logs.MSG") //unacceptable
}
func (c *cfg) reportError_n() {
c.log.reportError("logs.MSG") //unacceptable
}
type Logger interface {
Debug(msg string)
Info(msg string)

22
internal/analyzers/no-literal/test-case/src/b_positive._go → internal/analyzers/noliteral/test-case/src/positive._go
View file

@ -4,10 +4,30 @@ import (
"fmt"
)
func (c *cfg) f1_ok() {
func (c *cfg) info_ok() {
c.log.Info(logs.MSG) //acceptable
}
func (c *cfg) debug_ok() {
c.log.Debug(logs.MSG) //acceptable
}
func (c *cfg) error_ok() {
c.log.Error(logs.MSG) //acceptable
}
func (c *cfg) custom_ok_const() {
c.log.Abyr(logs.MSG) //acceptable
}
func (c *cfg) custom_ok_lit() {
c.log.Abyr("logs.MSG") //acceptable
}
func (c *cfg) custom_ok_lit() {
log.Abyr("logs.MSG") //acceptable
}
type Logger interface {
Debug(msg string)
Info(msg string)

10
main.go
View file

@ -1,7 +1,7 @@
package main
import (
noliteral "git.frostfs.info/TrueCloudLab/linters/internal/analyzers/no-literal"
noliteral "git.frostfs.info/TrueCloudLab/linters/internal/analyzers/noliteral"
"golang.org/x/tools/go/analysis"
)
@ -9,14 +9,6 @@ var AnalyzerPlugin analyzerPlugin
type analyzerPlugin struct{}
// for version ci-lint < '1.5.4'.
func (*analyzerPlugin) GetAnalyzers() []*analysis.Analyzer {
return []*analysis.Analyzer{noliteral.LogsAnalyzer}
}
/*
// for version ci-lint >= '1.5.4'.
func New(conf any) ([]*analysis.Analyzer, error) {
return []*analysis.Analyzer{noliteral.LogsAnalyzer}, nil
}
*/