forbidigo: better support for configuring complex rules (#3612)

Author: pohly

.golangci.reference.yml

@@ -351,14 +351,31 @@ linters-settings:
     # Forbid the following identifiers (list of regexp).
     # Default: ["^(fmt\\.Print(|f|ln)|print|println)$"]
     forbid:
+      # Builtin function:
       - ^print.*$
-      - 'fmt\.Print.*'
-      # Optionally put comments at the end of the regex, surrounded by `(# )?`
-      # Escape any special characters.
+      # Optional message that gets included in error reports.
+      - p: ^fmt\.Print.*$
+        msg: Do not commit print statements.
+      # Alternatively, put messages at the end of the regex, surrounded by `(# )?`
+      # Escape any special characters. Those messages get included in error reports.
       - 'fmt\.Print.*(# Do not commit print statements\.)?'
+      # Forbid spew Dump, whether it is called as function or method.
+      # Depends on analyze-types below.
+      - ^spew\.(ConfigState\.)?Dump$
+      # The package name might be ambiguous.
+      # The full import path can be used as additional criteria.
+      # Depends on analyze-types below.
+      - p: ^v1.Dump$
+        pkg: ^example.com/pkg/api/v1$
     # Exclude godoc examples from forbidigo checks.
     # Default: true
-    exclude_godoc_examples: false
+    exclude-godoc-examples: false
+    # Instead of matching the literal source code,
+    # use type information to replace expressions with strings that contain the package name
+    # and (for methods and fields) the type name.
+    # This makes it possible to handle import renaming and forbid struct fields and methods.
+    # Default: false
+    analyze-types: true
 
   funlen:
     # Checks the number of lines in a function.

go.mod

@@ -71,6 +71,7 @@ require (
 	github.com/mgechev/revive v1.3.2
 	github.com/mitchellh/go-homedir v1.1.0
 	github.com/mitchellh/go-ps v1.0.0
+	github.com/mitchellh/mapstructure v1.5.0
 	github.com/moricho/tparallel v0.3.1
 	github.com/nakabonne/nestif v0.3.1
 	github.com/nishanths/exhaustive v0.10.0
@@ -153,7 +154,6 @@ require (
 	github.com/mattn/go-isatty v0.0.17 // indirect
 	github.com/mattn/go-runewidth v0.0.9 // indirect
 	github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect
-	github.com/mitchellh/mapstructure v1.5.0 // indirect
 	github.com/nbutton23/zxcvbn-go v0.0.0-20210217022336-fa2cb2858354 // indirect
 	github.com/olekukonko/tablewriter v0.0.5 // indirect
 	github.com/pelletier/go-toml v1.9.5 // indirect

pkg/config/linters_settings.go

@@ -1,8 +1,11 @@
 package config
 
 import (
+	"encoding"
 	"errors"
 	"runtime"
+
+	"gopkg.in/yaml.v3"
 )
 
 var defaultLintersSettings = LintersSettings{
@@ -330,8 +333,45 @@ type ExhaustructSettings struct {
 }
 
 type ForbidigoSettings struct {
-	Forbid               []string `mapstructure:"forbid"`
-	ExcludeGodocExamples bool     `mapstructure:"exclude-godoc-examples"`
+	Forbid               []ForbidigoPattern `mapstructure:"forbid"`
+	ExcludeGodocExamples bool               `mapstructure:"exclude-godoc-examples"`
+	AnalyzeTypes         bool               `mapstructure:"analyze-types"`
+}
+
+var _ encoding.TextUnmarshaler = &ForbidigoPattern{}
+
+// ForbidigoPattern corresponds to forbidigo.pattern and adds mapstructure support.
+// The YAML field names must match what forbidigo expects.
+type ForbidigoPattern struct {
+	// patternString gets populated when the config contains a string as entry in ForbidigoSettings.Forbid[]
+	// because ForbidigoPattern implements encoding.TextUnmarshaler
+	// and the reader uses the mapstructure.TextUnmarshallerHookFunc as decoder hook.
+	//
+	// If the entry is a map, then the other fields are set as usual by mapstructure.
+	patternString string
+
+	Pattern string `yaml:"p" mapstructure:"p"`
+	Package string `yaml:"pkg,omitempty" mapstructure:"pkg,omitempty"`
+	Msg     string `yaml:"msg,omitempty" mapstructure:"msg,omitempty"`
+}
+
+func (p *ForbidigoPattern) UnmarshalText(text []byte) error {
+	// Validation happens when instantiating forbidigo.
+	p.patternString = string(text)
+	return nil
+}
+
+// MarshalString converts the pattern into a string as needed by forbidigo.NewLinter.
+//
+// MarshalString is intentionally not called MarshalText,
+// although it has the same signature
+// because implementing encoding.TextMarshaler led to infinite recursion when yaml.Marshal called MarshalText.
+func (p *ForbidigoPattern) MarshalString() ([]byte, error) {
+	if p.patternString != "" {
+		return []byte(p.patternString), nil
+	}
+
+	return yaml.Marshal(p)
 }
 
 type FunlenSettings struct {

pkg/config/reader.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 
 	"github.com/mitchellh/go-homedir"
+	"github.com/mitchellh/mapstructure"
 	"github.com/spf13/viper"
 	"golang.org/x/exp/slices"
 
@@ -91,7 +92,14 @@ func (r *FileReader) parseConfig() error {
 	}
 	r.cfg.cfgDir = usedConfigDir
 
-	if err := viper.Unmarshal(r.cfg); err != nil {
+	if err := viper.Unmarshal(r.cfg, viper.DecodeHook(mapstructure.ComposeDecodeHookFunc(
+		// Default hooks (https://github.com/spf13/viper/blob/518241257478c557633ab36e474dfcaeb9a3c623/viper.go#L135-L138).
+		mapstructure.StringToTimeDurationHookFunc(),
+		mapstructure.StringToSliceHookFunc(","),
+
+		// Needed for forbidigo.
+		mapstructure.TextUnmarshallerHookFunc(),
+	))); err != nil {
 		return fmt.Errorf("can't unmarshal config by viper: %s", err)
 	}
 

pkg/golinters/forbidigo.go

@@ -10,6 +10,7 @@ import (
 	"github.com/golangci/golangci-lint/pkg/config"
 	"github.com/golangci/golangci-lint/pkg/golinters/goanalysis"
 	"github.com/golangci/golangci-lint/pkg/lint/linter"
+	"github.com/golangci/golangci-lint/pkg/logutils"
 	"github.com/golangci/golangci-lint/pkg/result"
 )
 
@@ -40,6 +41,9 @@ func NewForbidigo(settings *config.ForbidigoSettings) *goanalysis.Linter {
 		},
 	}
 
+	// Without AnalyzeTypes, LoadModeSyntax is enough.
+	// But we cannot make this depend on the settings and have to mirror the mode chosen in GetAllSupportedLinterConfigs,
+	// therefore we have to use LoadModeTypesInfo in all cases.
 	return goanalysis.NewLinter(
 		forbidigoName,
 		"Forbids identifiers",
@@ -55,16 +59,34 @@ func runForbidigo(pass *analysis.Pass, settings *config.ForbidigoSettings) ([]go
 		forbidigo.OptionExcludeGodocExamples(settings.ExcludeGodocExamples),
 		// disable "//permit" directives so only "//nolint" directives matters within golangci-lint
 		forbidigo.OptionIgnorePermitDirectives(true),
+		forbidigo.OptionAnalyzeTypes(settings.AnalyzeTypes),
 	}
 
-	forbid, err := forbidigo.NewLinter(settings.Forbid, options...)
+	// Convert patterns back to strings because that is what NewLinter accepts.
+	var patterns []string
+	for _, pattern := range settings.Forbid {
+		buffer, err := pattern.MarshalString()
+		if err != nil {
+			return nil, err
+		}
+		patterns = append(patterns, string(buffer))
+	}
+
+	forbid, err := forbidigo.NewLinter(patterns, options...)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create linter %q: %w", forbidigoName, err)
 	}
 
 	var issues []goanalysis.Issue
 	for _, file := range pass.Files {
-		hints, err := forbid.RunWithConfig(forbidigo.RunConfig{Fset: pass.Fset}, file)
+		runConfig := forbidigo.RunConfig{
+			Fset:     pass.Fset,
+			DebugLog: logutils.Debug(logutils.DebugKeyForbidigo),
+		}
+		if settings != nil && settings.AnalyzeTypes {
+			runConfig.TypesInfo = pass.TypesInfo
+		}
+		hints, err := forbid.RunWithConfig(runConfig, file)
 		if err != nil {
 			return nil, fmt.Errorf("forbidigo linter failed on file %q: %w", file.Name.String(), err)
 		}

pkg/lint/lintersdb/manager.go

@@ -427,6 +427,10 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
 		linter.NewConfig(golinters.NewForbidigo(forbidigoCfg)).
 			WithSince("v1.34.0").
 			WithPresets(linter.PresetStyle).
+			// Strictly speaking,
+			// the additional information is only needed when forbidigoCfg.AnalyzeTypes is chosen by the user.
+			// But we don't know that here in all cases (sometimes config is not loaded),
+			// so we have to assume that it is needed to be on the safe side.
 			WithLoadForGoAnalysis().
 			WithURL("https://github.com/ashanbrown/forbidigo"),
 

pkg/logutils/logutils.go

@@ -22,6 +22,7 @@ const (
 	DebugKeyExcludeRules       = "exclude_rules"
 	DebugKeyExec               = "exec"
 	DebugKeyFilenameUnadjuster = "filename_unadjuster"
+	DebugKeyForbidigo          = "forbidigo"
 	DebugKeyGoEnv              = "goenv"
 	DebugKeyLinter             = "linter"
 	DebugKeyLintersContext     = "linters_context"

test/testdata/configs/forbidigo_struct.yml

@@ -0,0 +1,8 @@
+linters-settings:
+  forbidigo:
+    analyze-types: true
+    forbid:
+      - p: fmt\.Print.*
+        pkg: ^fmt$
+      - p: time.Sleep
+        msg: no sleeping!

test/testdata/forbidigo_example.go

@@ -4,10 +4,12 @@ package testdata
 
 import (
 	"fmt"
+	fmt2 "fmt"
 	"time"
 )
 
 func Forbidigo() {
 	fmt.Printf("too noisy!!!")  // want "use of `fmt\\.Printf` forbidden by pattern `fmt\\\\.Print\\.\\*`"
+	fmt2.Printf("too noisy!!!") // Not detected because analyze-types is false by default for backward compatbility.
 	time.Sleep(time.Nanosecond) // want "no sleeping!"
 }

test/testdata/forbidigo_struct_config.go

@@ -0,0 +1,13 @@
+//golangcitest:args -Eforbidigo
+//golangcitest:config_path testdata/configs/forbidigo_struct.yml
+package testdata
+
+import (
+	fmt2 "fmt"
+	"time"
+)
+
+func Forbidigo() {
+	fmt2.Printf("too noisy!!!") // want "use of `fmt2\\.Printf` forbidden by pattern `fmt\\\\.Print\\.\\*`"
+	time.Sleep(time.Nanosecond) // want "no sleeping!"
+}