diff --git a/.golangci.reference.yml b/.golangci.reference.yml index 1864e5b11388..94edc49cdac4 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 8106806005a6..d3a1291824e7 100644 --- a/go.mod +++ b/go.mod @@ -152,7 +152,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.7.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 2199fdd2de33..a7866b38977e 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" @@ -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!" +}