From aa112f6e0813dd56c01f71c179f2b9f14c88b875 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Sun, 18 Dec 2022 15:06:54 +0100 Subject: [PATCH] forbidigo: better support for configuring complex rules forbidigo 1.4.0 introduced a new configuration mechanism with multiple fields per rule. It still supports a single string, but allowing structs as alternative to such strings makes the configuration easier to read and write. A new global flag is needed to enable the more advanced analysis. --- .golangci.reference.yml | 25 +++++++++++-- go.mod | 7 ++-- go.sum | 4 +- pkg/config/linters_settings.go | 43 +++++++++++++++++++++- pkg/config/reader.go | 10 ++++- pkg/golinters/forbidigo.go | 35 ++++++++++++++++-- pkg/lint/lintersdb/manager.go | 15 ++++++-- pkg/logutils/logutils.go | 1 + test/testdata/configs/forbidigo_struct.yml | 8 ++++ test/testdata/forbidigo_example.go | 2 + test/testdata/forbidigo_struct_config.go | 13 +++++++ 11 files changed, 142 insertions(+), 21 deletions(-) create mode 100644 test/testdata/configs/forbidigo_struct.yml create mode 100644 test/testdata/forbidigo_struct_config.go diff --git a/.golangci.reference.yml b/.golangci.reference.yml index 241fb7fe4df2..13ba449cfc10 100644 --- a/.golangci.reference.yml +++ b/.golangci.reference.yml @@ -361,14 +361,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. + - pattern: ^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. + - pattern: ^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. diff --git a/go.mod b/go.mod index 3bae316d165a..1e3e06a6c41b 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,7 @@ require ( github.com/OpenPeeDeeP/depguard v1.1.1 github.com/alexkohler/prealloc v1.0.0 github.com/alingse/asasalint v0.0.11 - github.com/ashanbrown/forbidigo v1.4.0 + github.com/ashanbrown/forbidigo v1.5.1 github.com/ashanbrown/makezero v1.1.1 github.com/bkielbasa/cyclop v1.2.0 github.com/blizzy78/varnamelen v0.8.0 @@ -148,12 +148,11 @@ 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/mitchellh/mapstructure v1.5.0 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 github.com/pelletier/go-toml/v2 v2.0.5 // indirect - github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c // indirect github.com/prometheus/client_golang v1.12.1 // indirect @@ -186,6 +185,6 @@ require ( golang.org/x/text v0.6.0 // indirect google.golang.org/protobuf v1.28.0 // indirect gopkg.in/ini.v1 v1.67.0 // indirect - gopkg.in/yaml.v2 v2.4.0 // indirect + gopkg.in/yaml.v2 v2.4.0 mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b // indirect ) diff --git a/go.sum b/go.sum index 9096ff5693d5..9617738b5b69 100644 --- a/go.sum +++ b/go.sum @@ -69,8 +69,8 @@ github.com/alingse/asasalint v0.0.11 h1:SFwnQXJ49Kx/1GghOFz1XGqHYKp21Kq1nHad/0WQ github.com/alingse/asasalint v0.0.11/go.mod h1:nCaoMhw7a9kSJObvQyVzNTPBDbNpdocqrSP7t/cW5+I= github.com/andybalholm/brotli v1.0.2/go.mod h1:loMXtMfwqflxFJPmdbJO0a3KNoPuLBgiu3qAvBg8x/Y= github.com/andybalholm/brotli v1.0.3/go.mod h1:fO7iG3H7G2nSZ7m0zPUDn85XEX2GTukHGRSepvi9Eig= -github.com/ashanbrown/forbidigo v1.4.0 h1:spdPbupaSqtWORq1Q4eHBoPBmHtwVyLKwaedbSLc5Sw= -github.com/ashanbrown/forbidigo v1.4.0/go.mod h1:IvgwB5Y4fzqSAj/WVXKWigoTkB0dzI2FBbpKWuh7ph8= +github.com/ashanbrown/forbidigo v1.5.1 h1:WXhzLjOlnuDYPYQo/eFlcFMi8X/kLfvWLYu6CSoebis= +github.com/ashanbrown/forbidigo v1.5.1/go.mod h1:Y8j9jy9ZYAEHXdu723cUlraTqbzjKF1MUyfOKL+AjcU= github.com/ashanbrown/makezero v1.1.1 h1:iCQ87C0V0vSyO+M9E/FZYbu65auqH0lnsOkf5FcB28s= github.com/ashanbrown/makezero v1.1.1/go.mod h1:i1bJLCRSCHOcOa9Y6MyF2FTfMZMFdHvxKHxgO5Z1axI= github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q= diff --git a/pkg/config/linters_settings.go b/pkg/config/linters_settings.go index fb805f020179..e762c149366b 100644 --- a/pkg/config/linters_settings.go +++ b/pkg/config/linters_settings.go @@ -1,8 +1,11 @@ package config import ( + "encoding" "errors" "runtime" + + "gopkg.in/yaml.v2" ) var defaultLintersSettings = LintersSettings{ @@ -308,10 +311,46 @@ 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"` +} + +// 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 +} + +// String is intentionally not called MarshalText because that led to infinite +// recursion when yaml.Marshal called MarshalText. +func (p *ForbidigoPattern) String() ([]byte, error) { + if p.patternString != "" { + return []byte(p.patternString), nil + } + return yaml.Marshal(p) } +var _ encoding.TextUnmarshaler = &ForbidigoPattern{} + type FunlenSettings struct { Lines int Statements int diff --git a/pkg/config/reader.go b/pkg/config/reader.go index 2dfd3c06c9b0..3e2a01967851 100644 --- a/pkg/config/reader.go +++ b/pkg/config/reader.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/mitchellh/go-homedir" + "github.com/mitchellh/mapstructure" "github.com/spf13/viper" "github.com/golangci/golangci-lint/pkg/exitcodes" @@ -83,7 +84,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) } diff --git a/pkg/golinters/forbidigo.go b/pkg/golinters/forbidigo.go index a3f5c56d226d..101b445e6df0 100644 --- a/pkg/golinters/forbidigo.go +++ b/pkg/golinters/forbidigo.go @@ -10,12 +10,12 @@ 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" ) const forbidigoName = "forbidigo" -//nolint:dupl func NewForbidigo(settings *config.ForbidigoSettings) *goanalysis.Linter { var mu sync.Mutex var resIssues []goanalysis.Issue @@ -40,6 +40,12 @@ func NewForbidigo(settings *config.ForbidigoSettings) *goanalysis.Linter { }, } + loadMode := goanalysis.LoadModeSyntax + if settings != nil && settings.AnalyzeTypes { + // Need more information for the analysis. + loadMode = goanalysis.LoadModeTypesInfo + } + return goanalysis.NewLinter( forbidigoName, "Forbids identifiers", @@ -47,7 +53,7 @@ func NewForbidigo(settings *config.ForbidigoSettings) *goanalysis.Linter { nil, ).WithIssuesReporter(func(*linter.Context) []goanalysis.Issue { return resIssues - }).WithLoadMode(goanalysis.LoadModeSyntax) + }).WithLoadMode(loadMode) } func runForbidigo(pass *analysis.Pass, settings *config.ForbidigoSettings) ([]goanalysis.Issue, error) { @@ -56,15 +62,36 @@ func runForbidigo(pass *analysis.Pass, settings *config.ForbidigoSettings) ([]go // disable "//permit" directives so only "//nolint" directives matters within golangci-lint forbidigo.OptionIgnorePermitDirectives(true), } + if settings != nil && settings.AnalyzeTypes { + options = append(options, forbidigo.OptionAnalyzeTypes(true)) + } - 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.String() + 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) } diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index 6f406f7d26e3..30138eba455d 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/pkg/lint/lintersdb/manager.go @@ -414,10 +414,17 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config { WithLoadForGoAnalysis(). WithURL("https://github.com/kyoh86/exportloopref"), - linter.NewConfig(golinters.NewForbidigo(forbidigoCfg)). - WithSince("v1.34.0"). - WithPresets(linter.PresetStyle). - WithURL("https://github.com/ashanbrown/forbidigo"), + func() *linter.Config { + l := linter.NewConfig(golinters.NewForbidigo(forbidigoCfg)). + WithSince("v1.34.0"). + WithPresets(linter.PresetStyle). + WithURL("https://github.com/ashanbrown/forbidigo") + if forbidigoCfg != nil && forbidigoCfg.AnalyzeTypes { + // Need more information for the analysis. + l = l.WithLoadForGoAnalysis() + } + return l + }(), linter.NewConfig(golinters.NewForceTypeAssert()). WithSince("v1.38.0"). diff --git a/pkg/logutils/logutils.go b/pkg/logutils/logutils.go index 62c521eac3e0..4282193ca711 100644 --- a/pkg/logutils/logutils.go +++ b/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" diff --git a/test/testdata/configs/forbidigo_struct.yml b/test/testdata/configs/forbidigo_struct.yml new file mode 100644 index 000000000000..84d05c579b64 --- /dev/null +++ b/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! diff --git a/test/testdata/forbidigo_example.go b/test/testdata/forbidigo_example.go index 0a2897883a56..094215a84953 100644 --- a/test/testdata/forbidigo_example.go +++ b/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!" } diff --git a/test/testdata/forbidigo_struct_config.go b/test/testdata/forbidigo_struct_config.go new file mode 100644 index 000000000000..c39ba208cee4 --- /dev/null +++ b/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!" +}