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

Author: sebastien-rosset

.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:

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

go.sum

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

pkg/commands/executor.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")

pkg/commands/linters.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{})

pkg/commands/run.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,

pkg/config/config.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)

pkg/config/reader.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

pkg/config/run.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,15 +120,16 @@ 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:
 		return int(rv.Int())
 	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
 		}
 

pkg/golinters/gocritic.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)

test/linters_test.go

@@ -0,0 +1 @@
+This directory contains ruleguard files that are used in functional tests.

test/ruleguard/README.md

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

test/ruleguard/dup.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)`)
+}

test/ruleguard/strings_simplify.go

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

test/testdata/configs/gocritic.yml

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