From 04b16cfc6e24a1dbd3bf6ba958a4ad44f18fde0e Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Sun, 18 Dec 2022 15:06:54 +0100 Subject: [PATCH 1/2] 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 | 47 +++++++++++++++++++++- pkg/config/reader.go | 10 ++++- pkg/golinters/forbidigo.go | 26 +++++++++++- pkg/lint/lintersdb/manager.go | 4 ++ 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, 129 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 29d4414c1a33..5744512ee4eb 100644 --- a/.golangci.reference.yml +++ b/.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. diff --git a/go.mod b/go.mod index 65013730a131..bc13160ddb50 100644 --- a/go.mod +++ b/go.mod @@ -153,7 +153,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 @@ -190,6 +190,6 @@ require ( golang.org/x/text v0.9.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 0b2327be0c1b..ce8496f5f8dc 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{ @@ -330,10 +333,50 @@ 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 } +// 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) +} + +var _ encoding.TextUnmarshaler = &ForbidigoPattern{} + type FunlenSettings struct { Lines int Statements int diff --git a/pkg/config/reader.go b/pkg/config/reader.go index 1fb995e853a6..de203876e969 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" "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) } diff --git a/pkg/golinters/forbidigo.go b/pkg/golinters/forbidigo.go index ab270f5e076c..6aced29226e1 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,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) } diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index 948bda227b4b..3c2394a4c88c 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/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"), diff --git a/pkg/logutils/logutils.go b/pkg/logutils/logutils.go index 3844aa64f36f..80c9fed7a959 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!" +} From ed7f93839c3607d4dede521bea48e5ac01232691 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Wed, 31 May 2023 15:33:59 +0200 Subject: [PATCH 2/2] review: fix dependencies and cosmetic changes --- .golangci.reference.yml | 8 ++++---- go.mod | 4 ++-- pkg/config/linters_settings.go | 31 ++++++++++++++----------------- 3 files changed, 20 insertions(+), 23 deletions(-) diff --git a/.golangci.reference.yml b/.golangci.reference.yml index 5744512ee4eb..ea866843e51c 100644 --- a/.golangci.reference.yml +++ b/.golangci.reference.yml @@ -370,10 +370,10 @@ linters-settings: # Exclude godoc examples from forbidigo checks. # Default: true 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. + # 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 diff --git a/go.mod b/go.mod index bc13160ddb50..005e266af53b 100644 --- a/go.mod +++ b/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 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 @@ -190,6 +190,6 @@ require ( golang.org/x/text v0.9.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 + gopkg.in/yaml.v2 v2.4.0 // indirect mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b // indirect ) diff --git a/pkg/config/linters_settings.go b/pkg/config/linters_settings.go index ce8496f5f8dc..b520ea4c6eeb 100644 --- a/pkg/config/linters_settings.go +++ b/pkg/config/linters_settings.go @@ -5,7 +5,7 @@ import ( "errors" "runtime" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" ) var defaultLintersSettings = LintersSettings{ @@ -338,17 +338,16 @@ type ForbidigoSettings struct { AnalyzeTypes bool `mapstructure:"analyze-types"` } -// ForbidigoPattern corresponds to forbidigo.pattern and adds -// mapstructure support. The YAML field names must match what -// forbidigo expects. +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. + // 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. + // If the entry is a map, then the other fields are set as usual by mapstructure. patternString string Pattern string `yaml:"p" mapstructure:"p"` @@ -362,21 +361,19 @@ func (p *ForbidigoPattern) UnmarshalText(text []byte) error { return nil } -// MarshalString converts the pattern into a string as needed by -// forbidigo.NewLinter. +// 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. +// 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) } -var _ encoding.TextUnmarshaler = &ForbidigoPattern{} - type FunlenSettings struct { Lines int Statements int