From 2c01ea7ff2bb8aefd99c428d21f66c9376b9f0d2 Mon Sep 17 00:00:00 2001
From: Sebastien Rosset <serosset@cisco.com>
Date: Tue, 2 Nov 2021 11:34:19 -0700
Subject: [PATCH] gocritic: add support for variable substitution in ruleguard
path settings (#2308)
* Add variable for ruleguard config directory
* Add variable for ruleguard config directory
* Add variable for ruleguard config directory
* Add variable for ruleguard config directory
* Add unit tests
* Add unit tests for ruleguard
* Add unit tests for ruleguard
* Add unit tests for ruleguard
* Add unit tests for ruleguard, fix package name
---
.golangci.example.yml | 5 ++++-
go.mod | 1 +
go.sum | 1 +
pkg/commands/executor.go | 1 +
pkg/commands/linters.go | 1 +
pkg/commands/run.go | 2 ++
pkg/config/config.go | 8 +++++++-
pkg/config/reader.go | 5 +++++
pkg/config/run.go | 2 +-
pkg/golinters/gocritic.go | 14 +++++++++-----
test/linters_test.go | 2 +-
test/ruleguard/README.md | 1 +
test/ruleguard/dup.go | 22 ++++++++++++++++++++++
test/ruleguard/strings_simplify.go | 18 ++++++++++++++++++
test/testdata/configs/gocritic.yml | 11 +++++++++++
test/testdata/gocritic.go | 13 +++++++++++++
16 files changed, 98 insertions(+), 9 deletions(-)
create mode 100644 test/ruleguard/README.md
create mode 100644 test/ruleguard/dup.go
create mode 100644 test/ruleguard/strings_simplify.go
diff --git a/.golangci.example.yml b/.golangci.example.yml
index af2ccefc..78ef68ae 100644
--- a/.golangci.example.yml
+++ b/.golangci.example.yml
@@ -196,7 +196,10 @@ linters-settings:
# To check which checks are enabled run `GL_DEBUG=gocritic golangci-lint run`
# By default list of stable checks is used.
enabled-checks:
- - rangeValCopy
+ - nestingReduce
+ - unnamedresult
+ - ruleguard
+ - truncateCmp
# Which checks should be disabled; can't be combined with 'enabled-checks'; default is empty
disabled-checks:
diff --git a/go.mod b/go.mod
index 57318d59..8d4c3241 100644
--- a/go.mod
+++ b/go.mod
@@ -63,6 +63,7 @@ require (
github.com/pkg/errors v0.9.1
github.com/polyfloyd/go-errorlint v0.0.0-20210722154253-910bb7978349
github.com/prometheus/procfs v0.6.0 // indirect
+ github.com/quasilyte/go-ruleguard/dsl v0.3.10
github.com/ryancurrah/gomodguard v1.2.3
github.com/ryanrolds/sqlclosecheck v0.3.0
github.com/sanposhiho/wastedassign/v2 v2.0.6
diff --git a/go.sum b/go.sum
index 38d35d7e..6a0338af 100644
--- a/go.sum
+++ b/go.sum
@@ -622,6 +622,7 @@ github.com/quasilyte/go-ruleguard v0.3.1-0.20210203134552-1b5a410e1cc8/go.mod h1
github.com/quasilyte/go-ruleguard v0.3.13 h1:O1G41cq1jUr3cJmqp7vOUT0SokqjzmS9aESWJuIDRaY=
github.com/quasilyte/go-ruleguard v0.3.13/go.mod h1:Ul8wwdqR6kBVOCt2dipDBkE+T6vAV/iixkrKuRTN1oQ=
github.com/quasilyte/go-ruleguard/dsl v0.3.0/go.mod h1:KeCP03KrjuSO0H1kTuZQCWlQPulDV6YMIXmpQss17rU=
+github.com/quasilyte/go-ruleguard/dsl v0.3.10 h1:4tVlVVcBT+nNWoF+t/zrAMO13sHAqYotX1K12Gc8f8A=
github.com/quasilyte/go-ruleguard/dsl v0.3.10/go.mod h1:KeCP03KrjuSO0H1kTuZQCWlQPulDV6YMIXmpQss17rU=
github.com/quasilyte/go-ruleguard/rules v0.0.0-20201231183845-9e62ed36efe1/go.mod h1:7JTjp89EGyU1d6XfBiXihJNG37wB2VRkd125Q1u7Plc=
github.com/quasilyte/go-ruleguard/rules v0.0.0-20210428214800-545e0d2e0bf7/go.mod h1:4cgAphtvu7Ftv7vOT2ZOYhC6CvBxZixcasr8qIOTA50=
diff --git a/pkg/commands/executor.go b/pkg/commands/executor.go
index 60c44d82..0ad37dde 100644
--- a/pkg/commands/executor.go
+++ b/pkg/commands/executor.go
@@ -55,6 +55,7 @@ type Executor struct {
flock *flock.Flock
}
+// NewExecutor creates and initializes a new command executor.
func NewExecutor(version, commit, date string) *Executor {
startedAt := time.Now()
e := &Executor{
diff --git a/pkg/commands/linters.go b/pkg/commands/linters.go
index 873dab81..bb096942 100644
--- a/pkg/commands/linters.go
+++ b/pkg/commands/linters.go
@@ -20,6 +20,7 @@ func (e *Executor) initLinters() {
e.initRunConfiguration(e.lintersCmd)
}
+// executeLinters runs the 'linters' CLI command, which displays the supported linters.
func (e *Executor) executeLinters(_ *cobra.Command, args []string) {
if len(args) != 0 {
e.log.Fatalf("Usage: golangci-lint linters")
diff --git a/pkg/commands/run.go b/pkg/commands/run.go
index 1fb8c7b5..23a9b064 100644
--- a/pkg/commands/run.go
+++ b/pkg/commands/run.go
@@ -323,6 +323,7 @@ func fixSlicesFlags(fs *pflag.FlagSet) {
})
}
+// runAnalysis executes the linters that have been enabled in the configuration.
func (e *Executor) runAnalysis(ctx context.Context, args []string) ([]result.Issue, error) {
e.cfg.Run.Args = args
@@ -444,6 +445,7 @@ func (e *Executor) createPrinter() (printers.Printer, error) {
return p, nil
}
+// executeRun executes the 'run' CLI command, which runs the linters.
func (e *Executor) executeRun(_ *cobra.Command, args []string) {
needTrackResources := e.cfg.Run.IsVerbose || e.cfg.Run.PrintResourcesUsage
trackResourcesEndCh := make(chan struct{})
diff --git a/pkg/config/config.go b/pkg/config/config.go
index 06fca64b..f41705c8 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -2,7 +2,8 @@ package config
// Config encapsulates the config data specified in the golangci yaml config file.
type Config struct {
- Run Run
+ cfgDir string // The directory containing the golangci config file.
+ Run Run
Output Output
@@ -16,6 +17,11 @@ type Config struct {
InternalTest bool // Option is used only for testing golangci-lint code, don't use it
}
+// getConfigDir returns the directory that contains golangci config file.
+func (c *Config) GetConfigDir() string {
+ return c.cfgDir
+}
+
func NewDefault() *Config {
return &Config{
LintersSettings: defaultLintersSettings,
diff --git a/pkg/config/reader.go b/pkg/config/reader.go
index 6e97277d..9f368341 100644
--- a/pkg/config/reader.go
+++ b/pkg/config/reader.go
@@ -71,6 +71,11 @@ func (r *FileReader) parseConfig() error {
r.log.Warnf("Can't pretty print config file path: %s", err)
}
r.log.Infof("Used config file %s", usedConfigFile)
+ usedConfigDir := filepath.Dir(usedConfigFile)
+ if usedConfigDir, err = filepath.Abs(usedConfigDir); err != nil {
+ return fmt.Errorf("can't get config directory")
+ }
+ r.cfg.cfgDir = usedConfigDir
if err := viper.Unmarshal(r.cfg); err != nil {
return fmt.Errorf("can't unmarshal config by viper: %s", err)
diff --git a/pkg/config/run.go b/pkg/config/run.go
index 43a5ff2e..c091ee84 100644
--- a/pkg/config/run.go
+++ b/pkg/config/run.go
@@ -12,7 +12,7 @@ type Run struct {
Concurrency int
PrintResourcesUsage bool `mapstructure:"print-resources-usage"`
- Config string
+ Config string // The path to the golangci config file, as specified with the --config argument.
NoConfig bool
Args []string
diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go
index ffb44058..0c32a856 100644
--- a/pkg/golinters/gocritic.go
+++ b/pkg/golinters/gocritic.go
@@ -78,7 +78,10 @@ func normalizeCheckerInfoParams(info *gocriticlinter.CheckerInfo) gocriticlinter
return ret
}
-func configureCheckerInfo(info *gocriticlinter.CheckerInfo, allParams map[string]config.GocriticCheckSettings) error {
+func configureCheckerInfo(
+ lintCtx *linter.Context,
+ info *gocriticlinter.CheckerInfo,
+ allParams map[string]config.GocriticCheckSettings) error {
params := allParams[strings.ToLower(info.Name)]
if params == nil { // no config for this checker
return nil
@@ -88,7 +91,7 @@ func configureCheckerInfo(info *gocriticlinter.CheckerInfo, allParams map[string
for k, p := range params {
v, ok := infoParams[k]
if ok {
- v.Value = normalizeCheckerParamsValue(p)
+ v.Value = normalizeCheckerParamsValue(lintCtx, p)
continue
}
@@ -117,7 +120,7 @@ func configureCheckerInfo(info *gocriticlinter.CheckerInfo, allParams map[string
// then we have to convert value types into the expected value types.
// Maybe in the future, this kind of conversion will be done in go-critic itself.
//nolint:exhaustive // only 3 types (int, bool, and string) are supported by CheckerParam.Value
-func normalizeCheckerParamsValue(p interface{}) interface{} {
+func normalizeCheckerParamsValue(lintCtx *linter.Context, p interface{}) interface{} {
rv := reflect.ValueOf(p)
switch rv.Type().Kind() {
case reflect.Int64, reflect.Int32, reflect.Int16, reflect.Int8, reflect.Int:
@@ -125,7 +128,8 @@ func normalizeCheckerParamsValue(p interface{}) interface{} {
case reflect.Bool:
return rv.Bool()
case reflect.String:
- return rv.String()
+ // Perform variable substitution.
+ return strings.ReplaceAll(rv.String(), "${configDir}", lintCtx.Cfg.GetConfigDir())
default:
return p
}
@@ -141,7 +145,7 @@ func buildEnabledCheckers(lintCtx *linter.Context, linterCtx *gocriticlinter.Con
continue
}
- if err := configureCheckerInfo(info, allParams); err != nil {
+ if err := configureCheckerInfo(lintCtx, info, allParams); err != nil {
return nil, err
}
diff --git a/test/linters_test.go b/test/linters_test.go
index 35537048..4431a288 100644
--- a/test/linters_test.go
+++ b/test/linters_test.go
@@ -23,7 +23,7 @@ func runGoErrchk(c *exec.Cmd, defaultExpectedLinter string, files []string, t *t
if err != nil {
var exitErr *exec.ExitError
require.ErrorAs(t, err, &exitErr)
- require.Equal(t, exitcodes.IssuesFound, exitErr.ExitCode())
+ require.Equal(t, exitcodes.IssuesFound, exitErr.ExitCode(), "Unexpected exit code: %s", string(output))
}
fullshort := make([]string, 0, len(files)*2)
diff --git a/test/ruleguard/README.md b/test/ruleguard/README.md
new file mode 100644
index 00000000..2e244169
--- /dev/null
+++ b/test/ruleguard/README.md
@@ -0,0 +1 @@
+This directory contains ruleguard files that are used in functional tests.
diff --git a/test/ruleguard/dup.go b/test/ruleguard/dup.go
new file mode 100644
index 00000000..944651b6
--- /dev/null
+++ b/test/ruleguard/dup.go
@@ -0,0 +1,22 @@
+// go:build ruleguard
+package ruleguard
+
+import "github.com/quasilyte/go-ruleguard/dsl"
+
+// Suppose that we want to report the duplicated left and right operands of binary operations.
+//
+// But if the operand has some side effects, this rule can cause false positives:
+// `f() && f()` can make sense (although it's not the best piece of code).
+//
+// This is where *filters* come to the rescue.
+func DupSubExpr(m dsl.Matcher) {
+ // All filters are written as a Where() argument.
+ // In our case, we need to assert that $x is "pure".
+ // It can be achieved by checking the m["x"] member Pure field.
+ m.Match(`$x || $x`,
+ `$x && $x`,
+ `$x | $x`,
+ `$x & $x`).
+ Where(m["x"].Pure).
+ Report(`suspicious identical LHS and RHS`)
+}
diff --git a/test/ruleguard/strings_simplify.go b/test/ruleguard/strings_simplify.go
new file mode 100644
index 00000000..2e73a50b
--- /dev/null
+++ b/test/ruleguard/strings_simplify.go
@@ -0,0 +1,18 @@
+// go:build ruleguard
+package ruleguard
+
+import "github.com/quasilyte/go-ruleguard/dsl"
+
+func StringsSimplify(m dsl.Matcher) {
+ // Some issues have simple fixes that can be expressed as
+ // a replacement pattern. Rules can use Suggest() function
+ // to add a quickfix action for such issues.
+ m.Match(`strings.Replace($s, $old, $new, -1)`).
+ Report(`this Replace call can be simplified`).
+ Suggest(`strings.ReplaceAll($s, $old, $new)`)
+
+ // Suggest() can be used without Report().
+ // It'll print the suggested template to the user.
+ m.Match(`strings.Count($s1, $s2) == 0`).
+ Suggest(`!strings.Contains($s1, $s2)`)
+}
diff --git a/test/testdata/configs/gocritic.yml b/test/testdata/configs/gocritic.yml
index 268f2fb9..5cdda373 100644
--- a/test/testdata/configs/gocritic.yml
+++ b/test/testdata/configs/gocritic.yml
@@ -3,6 +3,17 @@ linters-settings:
enabled-checks:
- rangeValCopy
- flagDeref
+ - ruleguard
settings:
rangevalcopy:
sizethreshold: 2
+ ruleguard:
+ debug: dupSubExpr
+ failOn: dsl,import
+ # comma-separated paths to ruleguard files.
+ # The ${configDir} is substituted by the directory containing the golangci-lint config file.
+ # Note about the directory structure for functional tests:
+ # The ruleguard files used in functional tests cannot be under the 'testdata' directory.
+ # This is because they import the 'github.com/quasilyte/go-ruleguard/dsl' package,
+ # which needs to be added to go.mod. The testdata directory is ignored by go mod.
+ rules: '${configDir}/../../ruleguard/strings_simplify.go,${configDir}/../../ruleguard/dup.go'
diff --git a/test/testdata/gocritic.go b/test/testdata/gocritic.go
index 5e821a4e..1768304d 100644
--- a/test/testdata/gocritic.go
+++ b/test/testdata/gocritic.go
@@ -5,6 +5,7 @@ package testdata
import (
"flag"
"log"
+ "strings"
)
var _ = *flag.Bool("global1", false, "") // ERROR `flagDeref: immediate deref in \*flag.Bool\(.global1., false, ..\) is most likely an error; consider using flag\.BoolVar`
@@ -29,3 +30,15 @@ func gocriticRangeValCopySize2(ss []size2) {
log.Print(s)
}
}
+
+func gocriticStringSimplify() {
+ s := "Most of the time, travellers worry about their luggage."
+ s = strings.Replace(s, ",", "", -1) // ERROR "ruleguard: this Replace call can be simplified.*"
+ log.Print(s)
+}
+
+func gocriticDup(x bool) {
+ if x && x { // ERROR "ruleguard: suspicious identical LHS and RHS.*"
+ log.Print("x is true")
+ }
+}