From c05858e2b1395295e2f5058cd215dd737c56725f Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Sun, 19 Feb 2023 20:22:16 +0100 Subject: [PATCH] rules: support inverted path match It is not possible to use `exclude-rules: path` to exclude issues for non-test files because it is impossible to write a regexp that reliably matches those (https://github.com/golangci/golangci-lint/issues/2440). The new `not-path` check solves that by inverting the meaning: it excludes if the pattern does not match. Besides test files, this is also useful for large code bases where some specific code paths have additional constraints. For example, in Kubernetes the usage of certain functions is forbidden in test/e2e/framework but okay elsewhere. Without `not-path`, a regular expression that carefully matches all other paths has to be used, which is very cumbersome: - path: ^(api|cluster|cmd|hack|pkg|plugin|staging|test/(cmd|conformance|e2e/(apimachinery|apps|architecture|auth|autoscaling|chaosmonkey|cloud|common|dra|instrumentation|kubectl|lifecycle|network|node|perftype|reporters|scheduling|storage|testing-manifests|upgrades|windows)|e2e_kubeadm|e2e_node|fixtures|fuzz|images|instrumentation|integration|kubemark|list|soak|typecheck|utils)|third_party|vendor)/ msg: "E2E framework:" linters: - forbidigo With not-path, this becomes much simpler: - not-path: ^test/e2e/framework/ msg: "E2E framework:" linters: - forbidigo --- .golangci.reference.yml | 7 +++++++ docs/src/docs/usage/false-positives.mdx | 15 ++++++++++++++- pkg/config/issues.go | 12 ++++++++++-- pkg/lint/runner.go | 2 ++ pkg/result/processors/base_rule.go | 7 ++++++- pkg/result/processors/exclude_rules.go | 4 ++++ pkg/result/processors/exclude_rules_test.go | 9 +++++++++ pkg/result/processors/severity_rules.go | 4 ++++ pkg/result/processors/severity_rules_test.go | 11 +++++++++++ test/testdata/configs/forbidigo_tests.yml | 13 +++++++++++++ test/testdata/forbidigo_exclude.go | 14 ++++++++++++++ test/testdata/forbidigo_exclude_test.go | 14 ++++++++++++++ 12 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 test/testdata/configs/forbidigo_tests.yml create mode 100644 test/testdata/forbidigo_exclude.go create mode 100644 test/testdata/forbidigo_exclude_test.go diff --git a/.golangci.reference.yml b/.golangci.reference.yml index 7de210b98ba4..6cc33fdafe3f 100644 --- a/.golangci.reference.yml +++ b/.golangci.reference.yml @@ -2250,6 +2250,13 @@ issues: - dupl - gosec + + # Run some linter only for test files by excluding its issues + # for everything else. + - not-path: _test\.go + linters: + - forbidigo + # Exclude known linters from partially hard-vendored code, # which is impossible to exclude via `nolint` comments. # `/` will be replaced by current OS file path separator to properly work on Windows. diff --git a/docs/src/docs/usage/false-positives.mdx b/docs/src/docs/usage/false-positives.mdx index 354aee9f67fa..f2ed85d98347 100644 --- a/docs/src/docs/usage/false-positives.mdx +++ b/docs/src/docs/usage/false-positives.mdx @@ -66,7 +66,8 @@ issues: Exclude issues in path by `run.skip-dirs`, `run.skip-files` or `issues.exclude-rules` config options. -In the following example, all the reports from the linters (`linters`) that concerns the path (`path`) are excluded: +In the following example, all the reports from the linters (`linters`) that concerns the path (`path`) are excluded +and therefore test files do not get checked: ```yml issues: @@ -77,6 +78,18 @@ issues: - goconst ``` +The opposite, excluding reports *except* for specific paths, is also possible. +In the following example, only test files get checked: + +```yml +issues: + exclude-rules: + - not-path: '(.+)_test\.go' + linters: + - funlen + - goconst +``` + In the following example, all the reports related to the files (`skip-files`) are excluded: ```yml diff --git a/pkg/config/issues.go b/pkg/config/issues.go index b2437ec97471..201c2a41c814 100644 --- a/pkg/config/issues.go +++ b/pkg/config/issues.go @@ -132,6 +132,7 @@ func (e ExcludeRule) Validate() error { type BaseRule struct { Linters []string Path string + NotPath string `mapstructure:"not-path"` Text string Source string } @@ -140,6 +141,9 @@ func (b BaseRule) Validate(minConditionsCount int) error { if err := validateOptionalRegex(b.Path); err != nil { return fmt.Errorf("invalid path regex: %v", err) } + if err := validateOptionalRegex(b.NotPath); err != nil { + return fmt.Errorf("invalid not-path regex: %v", err) + } if err := validateOptionalRegex(b.Text); err != nil { return fmt.Errorf("invalid text regex: %v", err) } @@ -150,7 +154,11 @@ func (b BaseRule) Validate(minConditionsCount int) error { if len(b.Linters) > 0 { nonBlank++ } - if b.Path != "" { + // Filtering by path counts as one condition, regardless how it is done + // (one or both). Otherwise a rule with Path and NotPath set would + // pass validation whereas before the introduction of not-path that + // would't have been precise enough. + if b.Path != "" || b.NotPath != "" { nonBlank++ } if b.Text != "" { @@ -160,7 +168,7 @@ func (b BaseRule) Validate(minConditionsCount int) error { nonBlank++ } if nonBlank < minConditionsCount { - return fmt.Errorf("at least %d of (text, source, path, linters) should be set", minConditionsCount) + return fmt.Errorf("at least %d of (text, source, [not-]path, linters) should be set", minConditionsCount) } return nil } diff --git a/pkg/lint/runner.go b/pkg/lint/runner.go index f285b731b86b..75d48f5dfa43 100644 --- a/pkg/lint/runner.go +++ b/pkg/lint/runner.go @@ -268,6 +268,7 @@ func getExcludeRulesProcessor(cfg *config.Issues, log logutils.Log, lineCache *f Text: r.Text, Source: r.Source, Path: r.Path, + NotPath: r.NotPath, Linters: r.Linters, }, }) @@ -311,6 +312,7 @@ func getSeverityRulesProcessor(cfg *config.Severity, log logutils.Log, lineCache Text: r.Text, Source: r.Source, Path: r.Path, + NotPath: r.NotPath, Linters: r.Linters, }, }) diff --git a/pkg/result/processors/base_rule.go b/pkg/result/processors/base_rule.go index 6958b9f2f37d..a259ba4cb9d4 100644 --- a/pkg/result/processors/base_rule.go +++ b/pkg/result/processors/base_rule.go @@ -12,6 +12,7 @@ type BaseRule struct { Text string Source string Path string + NotPath string Linters []string } @@ -19,11 +20,12 @@ type baseRule struct { text *regexp.Regexp source *regexp.Regexp path *regexp.Regexp + notPath *regexp.Regexp linters []string } func (r *baseRule) isEmpty() bool { - return r.text == nil && r.source == nil && r.path == nil && len(r.linters) == 0 + return r.text == nil && r.source == nil && r.path == nil && r.notPath == nil && len(r.linters) == 0 } func (r *baseRule) match(issue *result.Issue, lineCache *fsutils.LineCache, log logutils.Log) bool { @@ -36,6 +38,9 @@ func (r *baseRule) match(issue *result.Issue, lineCache *fsutils.LineCache, log if r.path != nil && !r.path.MatchString(issue.FilePath()) { return false } + if r.notPath != nil && r.notPath.MatchString(issue.FilePath()) { + return false + } if len(r.linters) != 0 && !r.matchLinter(issue) { return false } diff --git a/pkg/result/processors/exclude_rules.go b/pkg/result/processors/exclude_rules.go index 62533b811533..fcd18e36ae23 100644 --- a/pkg/result/processors/exclude_rules.go +++ b/pkg/result/processors/exclude_rules.go @@ -47,6 +47,10 @@ func createRules(rules []ExcludeRule, prefix string) []excludeRule { path := fsutils.NormalizePathInRegex(rule.Path) parsedRule.path = regexp.MustCompile(path) } + if rule.NotPath != "" { + notPath := fsutils.NormalizePathInRegex(rule.NotPath) + parsedRule.notPath = regexp.MustCompile(notPath) + } parsedRules = append(parsedRules, parsedRule) } return parsedRules diff --git a/pkg/result/processors/exclude_rules_test.go b/pkg/result/processors/exclude_rules_test.go index c247e6ab0f84..f0d619a2f1a7 100644 --- a/pkg/result/processors/exclude_rules_test.go +++ b/pkg/result/processors/exclude_rules_test.go @@ -31,6 +31,12 @@ func TestExcludeRulesMultiple(t *testing.T) { Path: `_test\.go`, }, }, + { + BaseRule: BaseRule{ + Text: "^nontestonly$", + NotPath: `_test\.go`, + }, + }, { BaseRule: BaseRule{ Source: "^//go:generate ", @@ -46,6 +52,8 @@ func TestExcludeRulesMultiple(t *testing.T) { {Path: "e_Test.go", Text: "normal", Linter: "testlinter"}, {Path: "e_test.go", Text: "another", Linter: "linter"}, {Path: "e_test.go", Text: "testonly", Linter: "linter"}, + {Path: "e.go", Text: "nontestonly", Linter: "linter"}, + {Path: "e_test.go", Text: "nontestonly", Linter: "linter"}, {Path: filepath.Join("testdata", "exclude_rules.go"), Line: 3, Linter: "lll"}, } var issues []result.Issue @@ -66,6 +74,7 @@ func TestExcludeRulesMultiple(t *testing.T) { {Path: "e.go", Text: "some", Linter: "linter"}, {Path: "e_Test.go", Text: "normal", Linter: "testlinter"}, {Path: "e_test.go", Text: "another", Linter: "linter"}, + {Path: "e_test.go", Text: "nontestonly", Linter: "linter"}, } assert.Equal(t, expectedCases, resultingCases) } diff --git a/pkg/result/processors/severity_rules.go b/pkg/result/processors/severity_rules.go index 85c1866a21d9..5ce25af69fb8 100644 --- a/pkg/result/processors/severity_rules.go +++ b/pkg/result/processors/severity_rules.go @@ -52,6 +52,10 @@ func createSeverityRules(rules []SeverityRule, prefix string) []severityRule { path := fsutils.NormalizePathInRegex(rule.Path) parsedRule.path = regexp.MustCompile(path) } + if rule.NotPath != "" { + notPath := fsutils.NormalizePathInRegex(rule.NotPath) + parsedRule.notPath = regexp.MustCompile(notPath) + } parsedRules = append(parsedRules, parsedRule) } return parsedRules diff --git a/pkg/result/processors/severity_rules_test.go b/pkg/result/processors/severity_rules_test.go index b7407b531018..0f0c068807e2 100644 --- a/pkg/result/processors/severity_rules_test.go +++ b/pkg/result/processors/severity_rules_test.go @@ -37,6 +37,13 @@ func TestSeverityRulesMultiple(t *testing.T) { Path: `_test\.go`, }, }, + { + Severity: "info", + BaseRule: BaseRule{ + Text: "^nontestonly$", + NotPath: `_test\.go`, + }, + }, { BaseRule: BaseRule{ Source: "^//go:generate ", @@ -70,6 +77,8 @@ func TestSeverityRulesMultiple(t *testing.T) { {Path: "ssl.go", Text: "ssl", Linter: "gosec"}, {Path: "e.go", Text: "some", Linter: "linter"}, {Path: "e_test.go", Text: "testonly", Linter: "testlinter"}, + {Path: "e.go", Text: "nontestonly", Linter: "testlinter"}, + {Path: "e_test.go", Text: "nontestonly", Linter: "testlinter"}, {Path: filepath.Join("testdata", "exclude_rules.go"), Line: 3, Linter: "lll"}, {Path: filepath.Join("testdata", "severity_rules.go"), Line: 3, Linter: "invalidgo"}, {Path: "someotherlinter.go", Text: "someotherlinter", Linter: "someotherlinter"}, @@ -95,6 +104,8 @@ func TestSeverityRulesMultiple(t *testing.T) { {Path: "ssl.go", Text: "ssl", Linter: "gosec", Severity: "info"}, {Path: "e.go", Text: "some", Linter: "linter", Severity: "info"}, {Path: "e_test.go", Text: "testonly", Linter: "testlinter", Severity: "info"}, + {Path: "e.go", Text: "nontestonly", Linter: "testlinter", Severity: "info"}, // matched + {Path: "e_test.go", Text: "nontestonly", Linter: "testlinter", Severity: "error"}, // not matched {Path: filepath.Join("testdata", "exclude_rules.go"), Line: 3, Linter: "lll", Severity: "error"}, {Path: filepath.Join("testdata", "severity_rules.go"), Line: 3, Linter: "invalidgo", Severity: "info"}, {Path: "someotherlinter.go", Text: "someotherlinter", Linter: "someotherlinter", Severity: "info"}, diff --git a/test/testdata/configs/forbidigo_tests.yml b/test/testdata/configs/forbidigo_tests.yml new file mode 100644 index 000000000000..a53da6b4fe21 --- /dev/null +++ b/test/testdata/configs/forbidigo_tests.yml @@ -0,0 +1,13 @@ +linters-settings: + forbidigo: + forbid: + - fmt\.Print.* + - time.Sleep(# no sleeping!)? + +issues: + exclude-rules: + # Apply forbidigo only to test files, exclude + # it everywhere else. + - not-path: _test\.go + linters: + - forbidigo diff --git a/test/testdata/forbidigo_exclude.go b/test/testdata/forbidigo_exclude.go new file mode 100644 index 000000000000..23e014cf47bd --- /dev/null +++ b/test/testdata/forbidigo_exclude.go @@ -0,0 +1,14 @@ +//golangcitest:args -Eforbidigo +//golangcitest:config_path testdata/configs/forbidigo_tests.yml +//golangcitest:expected_exitcode 0 +package testdata + +import ( + "fmt" + "time" +) + +func Forbidigo() { + fmt.Printf("too noisy!!!") + time.Sleep(time.Nanosecond) +} diff --git a/test/testdata/forbidigo_exclude_test.go b/test/testdata/forbidigo_exclude_test.go new file mode 100644 index 000000000000..73466be1b22d --- /dev/null +++ b/test/testdata/forbidigo_exclude_test.go @@ -0,0 +1,14 @@ +//golangcitest:args -Eforbidigo +//golangcitest:config_path testdata/configs/forbidigo_tests.yml +package testdata + +import ( + "fmt" + "testing" + "time" +) + +func TestForbidigo(t *testing.T) { + fmt.Printf("too noisy!!!") // want "use of `fmt\\.Printf` forbidden by pattern `fmt\\\\.Print\\.\\*`" + time.Sleep(time.Nanosecond) // want "no sleeping!" +}