review fixes & go-critic tests improving

Author: Antonboom

pkg/golinters/gocritic.go

@@ -100,7 +100,7 @@ func (w *goCriticWrapper) init(settings *config.GoCriticSettings, logger logutil
 
 	settingsWrapper := newGoCriticSettingsWrapper(settings, logger)
 	settingsWrapper.InferEnabledChecks()
-	// NOTE(Antonboom): Validate must be after InferEnabledChecks, not before.
+	// Validate must be after InferEnabledChecks, not before.
 	// Because it uses gathered information about tags set and finally enabled checks.
 	if err := settingsWrapper.Validate(); err != nil {
 		logger.Fatalf("%s: invalid settings: %s", goCriticName, err)
@@ -158,11 +158,7 @@ func (w *goCriticWrapper) buildEnabledCheckers(linterCtx *gocriticlinter.Context
 	return enabledCheckers, nil
 }
 
-func runGocriticOnPackage(
-	linterCtx *gocriticlinter.Context,
-	checks []*gocriticlinter.Checker,
-	files []*ast.File,
-) []result.Issue {
+func runGocriticOnPackage(linterCtx *gocriticlinter.Context, checks []*gocriticlinter.Checker, files []*ast.File) []result.Issue {
 	var res []result.Issue
 	for _, f := range files {
 		filename := filepath.Base(linterCtx.FileSet.Position(f.Pos()).Filename)
@@ -214,7 +210,7 @@ func (w *goCriticWrapper) configureCheckerInfo(
 		return nil
 	}
 
-	// NOTE(ldez): lowercase info param keys here because golangci-lint's config parser lowercases all strings.
+	// To lowercase info param keys here because golangci-lint's config parser lowercases all strings.
 	infoParams := normalizeMap(info.Params)
 	for k, p := range params {
 		v, ok := infoParams[k]
@@ -274,12 +270,14 @@ type goCriticSettingsWrapper struct {
 
 	allCheckers []*gocriticlinter.CheckerInfo
 
-	allChecks           map[string]struct{}
-	allChecksLowerCased map[string]struct{}
-	allChecksByTag      map[string][]string
-	allTagsSorted       []string
+	allChecks             map[string]struct{}
+	allChecksByTag        map[string][]string
+	allTagsSorted         []string
+	inferredEnabledChecks map[string]struct{}
 
-	inferredEnabledChecks           map[string]struct{}
+	// *LowerCased* fields are used for GoCriticSettings.SettingsPerCheck validation only.
+
+	allChecksLowerCased             map[string]struct{}
 	inferredEnabledLowerCasedChecks map[string]struct{}
 }
 
@@ -334,7 +332,7 @@ func (s *goCriticSettingsWrapper) InferEnabledChecks() {
 			enabledChecks[info.Name] = struct{}{}
 		}
 	} else if !s.DisableAll {
-		// NOTE(Antonboom): enable-all/disable-all revokes the default settings.
+		// enable-all/disable-all revokes the default settings.
 		enabledChecks = make(map[string]struct{}, len(enabledByDefaultChecks))
 		for _, check := range enabledByDefaultChecks {
 			enabledChecks[check] = struct{}{}

test/ruleguard/README.md

@@ -1 +1,14 @@
-This directory contains ruleguard files that are used in functional tests.
+This directory contains ruleguard files that are used in functional tests:
+
+```bash
+T=gocritic.go make test_linters
+```
+
+```bash
+T=gocritic.go make test_fix
+```
+
+Helpful:
+
+- https://go-critic.com/overview.html
+- https://github.com/go-critic/go-critic/blob/master/checkers/rules/rules.go

test/ruleguard/dup.go

@@ -0,0 +1,12 @@
+//go:build ruleguard
+
+package ruleguard
+
+import "github.com/quasilyte/go-ruleguard/dsl"
+
+func preferWriteString(m dsl.Matcher) {
+	m.Match(`$w.Write([]byte($s))`).
+		Where(m["w"].Type.Implements("io.StringWriter")).
+		Suggest("$w.WriteString($s)").
+		Report(`$w.WriteString($s) should be preferred to the $$`)
+}

test/ruleguard/preferWriteString.go

@@ -6,7 +6,7 @@ import (
 	"github.com/quasilyte/go-ruleguard/dsl"
 )
 
-func RangeExprVal(m dsl.Matcher) {
+func rangeExprCopy(m dsl.Matcher) {
 	m.Match(`for _, $_ := range $x { $*_ }`, `for _, $_ = range $x { $*_ }`).
 		Where(m["x"].Addressable && m["x"].Type.Size >= 512).
 		Report(`$x copy can be avoided with &$x`).

test/ruleguard/rangeExprCopy.go

@@ -4,7 +4,7 @@ package ruleguard
 
 import "github.com/quasilyte/go-ruleguard/dsl"
 
-func StringsSimplify(m dsl.Matcher) {
+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.

test/ruleguard/strings_simplify.go renamed to test/ruleguard/stringsSimplify.go

@@ -4,5 +4,4 @@ linters-settings:
       - ruleguard
     settings:
       ruleguard:
-        rules: 'ruleguard/rangeExprCopy.go,ruleguard/strings_simplify.go'
-
+        rules: 'ruleguard/rangeExprCopy.go,ruleguard/stringsSimplify.go'

test/testdata/configs/gocritic-fix.yml

@@ -1,20 +1,20 @@
 linters-settings:
   gocritic:
+    disabled-checks:
+      - appendAssign
+      - switchTrue
     enabled-checks:
-      - rangeValCopy
+      - hugeParam
       - ruleguard
-    disabled-checks:
-      - dupSubExpr # So as not to overlap `ruleguard`.
     settings:
-      rangeValCopy:
-        sizeThreshold: 2
+      hugeParam:
+        sizeThreshold: 24
       ruleguard:
-        debug: dupSubExpr
         failOn: dsl,import
-        # comma-separated paths to ruleguard files.
+        # 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'
+        rules: '${configDir}/../../ruleguard/preferWriteString.go,${configDir}/../../ruleguard/stringsSimplify.go'

test/testdata/configs/gocritic.yml

@@ -4,45 +4,69 @@ package testdata
 
 import (
 	"flag"
+	"io"
 	"log"
 	"strings"
 )
 
 var _ = *flag.Bool("global1", false, "") // want `flagDeref: immediate deref in \*flag.Bool\(.global1., false, ..\) is most likely an error; consider using flag\.BoolVar`
 
 type size1 struct {
-	a bool
+	a [12]bool
 }
 
 type size2 struct {
 	size1
-	b bool
+	b [12]bool
 }
 
-func gocriticRangeValCopySize1(ss []size1) {
-	for _, s := range ss {
-		log.Print(s)
+func gocriticAppendAssign() {
+	var positives, negatives []int
+	positives = append(negatives, 1)
+	negatives = append(negatives, -1)
+	log.Print(positives, negatives)
+}
+
+func gocriticDupSubExpr(x bool) {
+	if x && x { // want "dupSubExpr: suspicious identical LHS and RHS.*"
+		log.Print("x is true")
 	}
 }
 
-func gocriticRangeValCopySize2(ss []size2) {
-	for _, s := range ss { // want "rangeValCopy: each iteration copies 2 bytes.*"
-		log.Print(s)
+func gocriticHugeParamSize1(ss size1) {
+	log.Print(ss)
+}
+
+func gocriticHugeParamSize2(ss size2) { // want "hugeParam: ss is heavy \\(24 bytes\\); consider passing it by pointer"
+	log.Print(ss)
+}
+
+func gocriticHugeParamSize2Ptr(ss *size2) {
+	log.Print(*ss)
+}
+
+func gocriticSwitchTrue() {
+	switch true {
+	case false:
+		log.Print("false")
+	default:
+		log.Print("default")
 	}
 }
 
+func goCriticPreferStringWriter(w interface {
+	io.Writer
+	io.StringWriter
+}) {
+	w.Write([]byte("test")) // want "ruleguard: w\\.WriteString\\(\"test\"\\) should be preferred.*"
+}
+
 func gocriticStringSimplify() {
 	s := "Most of the time, travellers worry about their luggage."
 	s = strings.Replace(s, ",", "", -1) // want "ruleguard: this Replace call can be simplified.*"
 	log.Print(s)
 }
 
-func gocriticDup(x bool) {
-	if x && x { // want "ruleguard: suspicious identical LHS and RHS.*"
-		log.Print("x is true")
-	}
-}
-
 func gocriticRuleWrapperFunc() {
 	strings.Replace("abcabc", "a", "d", -1) // want "ruleguard: this Replace call can be simplified.*"
 }