Skip to content

Commit

Permalink
rules: support inverted path match
Browse files Browse the repository at this point in the history
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 (#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
  • Loading branch information
pohly committed Feb 21, 2023
1 parent defde13 commit c05858e
Show file tree
Hide file tree
Showing 12 changed files with 108 additions and 4 deletions.
7 changes: 7 additions & 0 deletions .golangci.reference.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
15 changes: 14 additions & 1 deletion docs/src/docs/usage/false-positives.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand Down
12 changes: 10 additions & 2 deletions pkg/config/issues.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)
}
Expand All @@ -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 != "" {
Expand All @@ -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
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/lint/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
})
Expand Down Expand Up @@ -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,
},
})
Expand Down
7 changes: 6 additions & 1 deletion pkg/result/processors/base_rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,20 @@ type BaseRule struct {
Text string
Source string
Path string
NotPath string
Linters []string
}

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 {
Expand All @@ -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
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/result/processors/exclude_rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 9 additions & 0 deletions pkg/result/processors/exclude_rules_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ func TestExcludeRulesMultiple(t *testing.T) {
Path: `_test\.go`,
},
},
{
BaseRule: BaseRule{
Text: "^nontestonly$",
NotPath: `_test\.go`,
},
},
{
BaseRule: BaseRule{
Source: "^//go:generate ",
Expand All @@ -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
Expand All @@ -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)
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/result/processors/severity_rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 11 additions & 0 deletions pkg/result/processors/severity_rules_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 ",
Expand Down Expand Up @@ -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"},
Expand All @@ -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"},
Expand Down
13 changes: 13 additions & 0 deletions test/testdata/configs/forbidigo_tests.yml
Original file line number Diff line number Diff line change
@@ -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
14 changes: 14 additions & 0 deletions test/testdata/forbidigo_exclude.go
Original file line number Diff line number Diff line change
@@ -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)
}
14 changes: 14 additions & 0 deletions test/testdata/forbidigo_exclude_test.go
Original file line number Diff line number Diff line change
@@ -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!"
}

0 comments on commit c05858e

Please sign in to comment.