Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

forbidigo: better support for configuring complex rules #3612

Merged
merged 2 commits into from May 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 21 additions & 4 deletions .golangci.reference.yml
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
44 changes: 42 additions & 2 deletions pkg/config/linters_settings.go
@@ -1,8 +1,11 @@
package config

import (
"encoding"
"errors"
"runtime"

"gopkg.in/yaml.v3"
)

var defaultLintersSettings = LintersSettings{
Expand Down Expand Up @@ -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 {
Expand Down
10 changes: 9 additions & 1 deletion pkg/config/reader.go
Expand Up @@ -8,6 +8,7 @@ import (
"strings"

"github.com/mitchellh/go-homedir"
"github.com/mitchellh/mapstructure"
"github.com/spf13/viper"
"golang.org/x/exp/slices"

Expand Down Expand Up @@ -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)
}

Expand Down
26 changes: 24 additions & 2 deletions pkg/golinters/forbidigo.go
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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",
Expand All @@ -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)
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/lint/lintersdb/manager.go
Expand Up @@ -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"),

Expand Down
1 change: 1 addition & 0 deletions pkg/logutils/logutils.go
Expand Up @@ -22,6 +22,7 @@ const (
DebugKeyExcludeRules = "exclude_rules"
DebugKeyExec = "exec"
DebugKeyFilenameUnadjuster = "filename_unadjuster"
DebugKeyForbidigo = "forbidigo"
DebugKeyGoEnv = "goenv"
DebugKeyLinter = "linter"
DebugKeyLintersContext = "linters_context"
Expand Down
8 changes: 8 additions & 0 deletions 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!
2 changes: 2 additions & 0 deletions test/testdata/forbidigo_example.go
Expand Up @@ -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!"
}
13 changes: 13 additions & 0 deletions 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!"
}