review fixes & go-critic tests improving

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/preferWriteString.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/rangeExprCopy.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/strings_simplify.go → test/ruleguard/stringsSimplify.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/testdata/configs/gocritic-fix.yml

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

test/testdata/configs/gocritic.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/gocritic.go

@@ -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.*"
 }