From a5cb733e0926e1810f29ac0b9de26d3acd3bd6ca 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 | 4 +- pkg/config/linters_settings.go | 43 +++++++++++++++++++++- pkg/config/reader.go | 10 ++++- pkg/golinters/forbidigo.go | 28 +++++++++++++- pkg/lint/lintersdb/manager.go | 7 ++++ 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 +++++++ 10 files changed, 130 insertions(+), 11 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 f7950919689e..49d6840ca3c7 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. + - 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. diff --git a/go.mod b/go.mod index 4e3c96ec3158..1e3e06a6c41b 100644 --- a/go.mod +++ b/go.mod @@ -148,7 +148,7 @@ 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 @@ -185,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/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 4879cca027a0..b46c8698ffa0 100644 --- a/pkg/golinters/forbidigo.go +++ b/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,10 @@ func NewForbidigo(settings *config.ForbidigoSettings) *goanalysis.Linter { }, } + // Without AnalyzeTypes, LoadModeSyntax is enough. But we cannot make + // this depend on the settings and have mirror the mode chosen in + // GetAllSupportedLinterConfigs, therefore we have to use + // LoadModeTypesInfo in all cases. return goanalysis.NewLinter( forbidigoName, "Forbids identifiers", @@ -55,16 +60,35 @@ 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.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 7cc64e588e0f..20ca520b1dfe 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/pkg/lint/lintersdb/manager.go @@ -417,6 +417,13 @@ 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"), 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!" +}