🌟 Let's talk about exclusion options inside issues section #5297

ldez · 2025-01-06T11:21:16Z

Important

This is a proposal: I don't know if it is possible and what the impact could be inside the code.
The proposal may evolve.

The current configuration:

issues:
  # ...
  
  exclude-generated: strict
  
  # If set to true, `exclude` and `exclude-rules` regular expressions become case-sensitive.
  exclude-case-sensitive: false
  exclude:
    - abcdef
  exclude-rules:
    - path: _test\.go
      linters:
        - gocyclo
        - errcheck
        - dupl
        - gosec
    - path-except: _test\.go
      linters:
        - forbidigo
    - path: internal/hmac/
      text: "weak cryptographic primitive"
      linters:
        - gosec
    - linters:
        - staticcheck
      text: "SA9003:"
    - linters:
        - lll
      source: "^//go:generate "

  exclude-dirs-use-default: false
  exclude-dirs:
    - src/external_libs
    - autogenerated_by_my_lib
  exclude-files:
    - ".*\\.my\\.go$"
    - lib/bad.gO

  exclude-use-default: false
  include:
    - EXC0001
    - EXC0002
    - ...

  # ...

⛑️ The Problems

Useless Option

The option exclude-dirs-use-default comes from the "prehistorical" world of GOPATH.

vendor$, testdata$ are useless inside this default configuration because the Go tooling already excludes them.
Since go modules, Godeps$ is useless.
examples$: This is unexpected because it excludes all directories ending with examples.
third_party$, builtin$: we should not create "custom conventions", so those elements should not be excluded.

Duplicated Options

The goals of exclude-dirs and exclude-files are the same.

The matching can be complex in some cases:

exclude can be expressed with an exclude-rules.

Unclear Section

At the 1st level of the issues section, there are options about filtering (ex: max-issues-per-linter, new-from-rev), and about exclusions (ex: exclude-rules).

I don't think we should create a subsection for each topic but we should have at least one for exclusions.

Default Exclusions

There is a dedicated proposal for this topic:

🌟 Let's talk about default exclusions #5298

💭 Proposal

All the options related to exclusions should be grouped inside a dedicated subsection of issues:
- issues.exclude-generated -> issues.exclusions.generated
- issues.exclude-case-sensitive -> issues.exclusions.case-sensitive: Useful? Remove? Merge with rules? (it just add (?i) prefix) -> by default?
- issues.exclude-rules -> issues.exclusions.rules
Removes (deprecates) issues.exclude-use-default. Replaced by issues.exclusions.default.
Removes (deprecates) exclude-dirs-use-default.
Removes (deprecates) issues.exclude.
Removes (deprecates) issues.exclude-use-default (cf proposal about default exclusions)
Removes (deprecates) issues.exclude-dirs. Replaced by issues.exclusions.paths.
Removes (deprecates) issues.exclude-files. Replaced by issues.exclusions.paths.

Example

issues:
  # ...
  
  exclusions:
    default: # presets?
      - comments
      - stdErrorHandling
      - commonFalsePositives
    generated: strict # by default?
    case-sensitive: false # Useful? Remove? Merge with rules? (it just add `(?i)`)
    rules:
      - path: _test\.go
        linters:
          - gocyclo
          - errcheck
          - dupl
          - gosec
      - path-except: _test\.go
        linters:
          - forbidigo
      - path: internal/hmac/
        text: "weak cryptographic primitive"
        linters:
          - gosec
      - linters:
          - staticcheck
        text: "SA9003:"
      - linters:
          - lll
        source: "^//go:generate "
    paths:
      - src/external_libs
      - autogenerated_by_my_lib
      - ".*\\.my\\.go$"
      - lib/bad.go

  # ...

The text was updated successfully, but these errors were encountered:

bombsimon · 2025-01-06T20:39:42Z

Related to this topic, can we do something to clarify that this is purely about excluding reports/issues and not analyzing? Don't know from the top of my head what that would be and the setting is clearly already under the issues section but since it's a somewhat common question and misconception I felt I wanted to put it out there on this topic.

ldez · 2025-01-06T21:01:28Z

Currently, I don't know if the section will be inside issues or inside linters or at 1st level.
Because linters and formatters have different needs regarding exclusions.

ldez · 2025-01-08T20:15:24Z

After working on the formatters, I think my proposal should be updated to:

linters: # <-- Important
  # ...
  
  exclusions:
    default:
      - comments
      - stdErrorHandling
      - commonFalsePositives
      - legacy
    generated: strict # by default?
    rules:
      - path: _test\.go
        linters:
          - gocyclo
          - errcheck
          - dupl
          - gosec
      - path-except: _test\.go
        linters:
          - forbidigo
      - path: internal/hmac/
        text: "weak cryptographic primitive"
        linters:
          - gosec
      - linters:
          - staticcheck
        text: "SA9003:"
      - linters:
          - lll
        source: "^//go:generate "
    paths:
      - src/external_libs
      - autogenerated_by_my_lib
      - ".*\\.my\\.go$"
      - lib/bad.go

  # ...

Linters (run command) and formatters (fmt command) are too different, so I think we should have a dedicated exclusions section by "type".

linters: 
  # ...
  
  exclusions:
    #...

fommatters:
  # ...
  
  exclusions:
  # ...

ldez · 2025-01-09T11:52:49Z

Introducing exclusions[].paths is a bit more complex than I expected: #1178 (comment)

ldez · 2025-01-10T02:51:12Z

After going down the rabbit hole, and spending too much time on it, I think the problem with exclusions[].paths had no simple solution:

if the option is based on absolute paths some cases work but others not
if the option is based on relative paths some cases work but others not

This is impossible with a simple option to handle all cases, the 2 sets of constraints are incompatible.

My current thoughts are either to have 2 options and/or have a specific placeholder (like ${configDir} we introduced for ruleguard rules).

The extra hidden problem here is consistency: exclusions[].paths and exclusions[].rules[].path should have the same behavior.

ldez · 2025-01-12T18:52:51Z

Maybe it can feel the topic is not moving fast, but the sub-topic of exclusions[].paths is just the tip of an iceberg.

Thinking about the compatibility is a nightmare as I explained here

This option asks questions about absolute/relative path usage inside processors.
When digging into this, I can see different layers of code related to different periods of the golangci-lint life inside the processors.
Some inconsistencies, redundancies, unintentional complexities, dead codes appear.

I will share my current notes (draft) about processors.

Processors are mainly divided into 3 categories (this is a simplification there are variants):

Transformer: modifies reports (text, filepath, etc.).
Filter: filters reports based on criteria (text, filepath, file content, etc.).
Sorter: sorts the reports.

They are executed in this order:

Cgo: filters cgo artifacts (uses absolute path) (FILTER)
FilenameUnadjuster: fixes filename based on adjusted and unadjusted position (related to line directives and cgo) (uses and produces absolute path) (TRANSFORMER)
InvalidIssue: filters invalid reports (it can use rel or abs, no problem) (FILTER)
PathPrettifier: modify report filename with the shortest relative path (uses wd + filepath.EvalSymlinks) (TRANSFORMER)
SkipFiles: filters reports based on filename (uses the shortest relative paths and path-prefix option). (FILTER)
SkipDirs: filters reports based on filename (uses the shortest relative paths and path-prefix option). (FILTER)
AutogeneratedExclude: filters generated files (it can use rel or abs, no problem) (FILTER)
IdentifierMarker: modifies report messages, must be before Exclude and ExcludeRules. (TRANSFORMER)
Exclude: filters reports only based on report text. (FILTER)
ExcludeRules: filters reports (uses the shortest relative paths and path-prefix option). (FILTER)
Nolint: filters reports related to nolint directives. (FILTER/SORTER)
UniqByLine: filters reports to keep only one report by LOC. (FILTER)
Diff: related to options --new, new-from-rev, etc. Uses git. (uses paths relative to the path where git/golangci-lint is run) (FILTER)
MaxPerFileFromLinter: filters reports by file and by linter (it can use rel or abs, no problem) (FILTER)
MaxSameIssues: filters reports by report text. (FILTER)
MaxFromLinter: filters reports by linter. (FILTER)
SourceCode: modifies displayed information based on lineRange (position to LOC) -> rel/abs? (TRANSFORMER)
PathShortener: modifies text of the reports to reduce filepath inside the text (related to wd + filepath.EvalSymlinks) (TRANSFORMER)
Severity: modifies report severity (uses the shortest relative paths but not the path-prefix option). (TRANSFORMER)
Fixer: Fixes and so filter fixed reports (FILTER) -> rel/abs?
PathPrefixer: adds path-prefix to report filenames for user output (uses the shortest relative paths and path-prefix option). (TRANSFORMER)
SortResults: sorts reports. (SORTER)

An example of the sub-topics of the sub-topic: initially the Fixer was the last processor, due to a side effect with path-prefix this processor was moved before the PathPrefixer processor.
But when you have the full overview, and know the implementation of each processor, the best place for the Fixer is after Nolint or Diff processor.

Another example is the output.path-prefix option: what are the needs behind this? Can we replace it with something else?

There are too many rabbit holes 😸

I opened a first PR with just some processor optimizations: #5316
The PR about processor order: #5322

ldez · 2025-01-13T23:41:30Z

The option output.path-prefix has been introduced due to the option working-directory of the GitHub Action.

The goal is to handle the filepaths when golangci-lint is run inside a subdirectory but the reports must be related to a parent.

.
├── frontend
└── backend
    ├── go.mod
    └── a
        └── b
            └── c.go

The main goal is to be used inside PathPrefixer to modify filepaths inside reports for user-facing.

The usage has been extended SkipFiles, SkipDirs, and ExcludeRules to fit the output of reports and the configuration.

This option requires the usage of relative paths (to the binary).

References:

The path-prefix is used inside:

SkipFiles
SkipDirs
ExcludeRules
PathPrefixer
Severity

Note: Diff uses paths relative to the binary (wd) without output.path-prefix.

I wonder if the placement of this option inside output is right 🤔

I feel that output.path-prefix was not the right solution to the initial problem.

ldez · 2025-01-14T17:04:54Z

Classic project

.
├── .golangci.yml
├── go.mod
└── a
    └── b
        └── c.go

Currently:

Runs inside the root .
- file paths inside the configuration are right if relative to the root.
Runs inside a, or b, or c
- file paths inside the configuration are wrong if relative to the root.

Monorepo

.
├── .git
├── .golangci.yml
├── projectA
│   ├── go.mod
│   └── a
│       └── b
│           └── c.go
└── projectB
    ├── go.mod
    └── a
        └── b
            └── c.go

Currently:

Runs inside the projectA or projectB
- file paths inside the configuration are right if relative to the projectA/projectB.
- file paths inside the configuration are wrong if relative to the root.
Runs inside a, or b, or c
- file paths inside the configuration are wrong if relative to the projectA/projectB or the root.
Inside a CI (ex: GitHub Actions)
- the output needs to be related to the root or git root.

Local group of projects

.
├── .golangci.yml
├── projectA
│   ├── .git
│   ├── go.mod
│   └── a
│       └── b
│           └── c.go
└── projectB
    ├── .git
    ├── go.mod
    └── a
        └── b
            └── c.go

Currently:

Runs inside the projectA or projectB
- file paths inside the configuration are right if relative to the projectA/projectB.
- file paths inside the configuration are wrong if relative to the root.
Runs inside a, or b, or c
- the file paths inside the configuration are wrong if relative to the projectA/projectB or the root.

Mixed content

.
├── .git
├── frontend
└── backend
    ├── .golangci.yml
    ├── go.mod
    └── a
        ├── b
        └── c.go

Currently:

Runs inside the backend
- file paths inside the configuration are right if relative to backend.
- file paths inside the configuration are wrong if relative to the root.
Runs inside a, or b, or c
- file paths inside the configuration are wrong if relative to backend or the root.
Inside a CI (ex: GitHub Actions)
- the output needs to be related to the root or git root.

ldez · 2025-01-14T17:09:32Z

absolute path:

It cannot be used by users (for exclusions) because absolute paths are absolute, so dependent on local fs.
Internally, the Go tooling produces absolute paths, and we need it for some operations.

relative path:

The only way for the user to express configuration based on paths.
Required by the output.path-prefix option.
Currently related to the binary wd.
Inside repo with multiple projects with a global configuration file, it doesn't work well when golangci-lint is run from a subdirectory.

My conclusions are we need several ways to match:

related to binary wd
related to a parent: git root? manual (output.path-prefix)?
related to the configuration file?
related to go.mod?

There are 5 topics behind relative paths:

the report output
the matching (exclusions, severities)
the diff processor (--new, --new-from-rev, etc.)
the plugins (custom.*.path)
the linters (gocritic/ruleguard, depguard, errcheck, goheader)

ldez · 2025-01-14T17:22:42Z

So IMO output.path-prefix is not a good option, because this option was here to "fix" the problem about relative paths but without fixing it. It just fixes one case in one context.
Also, the problem is not only related to the output of the reports.

The compatibility is complex mainly due to this option 😢

ldez · 2025-01-14T17:45:17Z

I searched public repositories inside GitHub: https://github.com/search?q=path%3A%2F.golangci*+%22output%3A%22+%2Fpath-prefix%3A+%28%27%7C%22%29.%2B%28%27%7C%22%29%2F+-path%3A*reference*+&type=code

Conclusion: only one real usage of output.path-prefix and some unexpected usage with ./ as a prefix (this is weird)
Maybe the majority of the usages are inside private monorepo 🤔

ldez · 2025-01-14T17:55:44Z

I searched for GitHub action: https://github.com/search?q=%2Fgolangci%5C%2Fgolangci-lint-action%40%28.%2B%29%3A%2F+%22working-directory%3A%22+path%3A%2F.github%2F&type=code

A few more results (<20) but not many.

ldez · 2025-01-16T14:42:34Z

I need to share my thoughts.

I can see several possibilities for the path problem:

A global option like path-relative-to: wd|cfg|git|... (this is just a name example) will be complex/impossible to propagate to linters.
An option run.path-relative-to: wd|cfg|git|... will be complex/impossible to propagate to linters.
An option only for *.exclusions will not change the file paths for user-facing.
An option only for output will not allow configuring *.exclusions.
An option inside each section linters.exclusions.path-relative-to, formatters.exclusions.path-relative-to, output.path-relative-to, output.path-relative-to but linters.exclusions.path-relative-to and formatters.exclusions.path-relative-to should use the same mode.
placeholders (${wd}, ${cfg}, ${git}) but this will require to be used in all path definitions (not user friendly).

Also, the management of file paths is inconsistent inside linters configurations:

errcheck (errcheck.exclude): uses a searching system with a relative path to find a file. But the option errcheck.exclude is deprecated.
gocritic (gocritic.ruleguard.rules): uses regexp with absolute path and ${configDir}.
depguard (rules.*.files): uses regexp with absolute path.
goheader (goheader.template-path): uses a relative path to wd

errcheck is a minor problem as the option has been deprecated since golangci-lint v1.42.

In all cases, linters cannot be easily handled with a configuration option (because the regexp matching is inside the linters) so a placeholder is the only solution for linters.

ldez · 2025-01-16T21:55:12Z

There were not a lot of reactions, I know this topic is not "sexy" but this is an important topic.

I have done my best to provide information because this is not a trivial problem, and it tortured my mind for 1 week.

For now, I choose run.path-relative-to: wd|cfg|git|... + linters placeholders (${wd}, ${cfg}, ${git}).

I will implement that to check if everything works as expected.

The previous placeholder ${configDir} will be deprecated.

ldez · 2025-01-18T16:07:21Z

I have a working POC:

I can keep the compatibility with output.path-prefix for SKipFiles, SKipDirs, ExcludeRules, Severity
Diff works are expected
The default behavior will still be the same (with the same current bugs)
The option relative-path-mode fixes all problems with relative paths (except with the run mode wd but this is expected by the nature of wd).
there are 4 modes: gomod, gitroot, cfg, wd.

About linters, I think having multiple placeholders (${wd}, ${cfg}, ${git}) is a bad idea because it requires doing all calls to get the value for each placeholder.
For example, to have ${git} placeholder we need to use git but if a project doesn't use git this can fail.

So only one placeholder should exist: something like ${root} or ${base-path} related to relative-path-mode.

About plugins (custom.*.path), I need to think about what is the right approach.

After that, I will implement again the new linters.exclusions section.

In v2, output.path-prefix will be used only for output and not for matching, and the default relative path mode will be gomod or cfg (not wd like today).

I see the end of this topic, I am relieved.

ldez · 2025-01-18T23:15:50Z

I named the placeholder ${base-path}, it will be usable inside:

gocritic (gocritic.ruleguard.rules)
depguard (rules.*.files)
goheader (goheader.template-path)

About errcheck: the option errcheck.exclude has been deprecated since golangci-lint v1.42 then I didn't add the support of the placeholder.

Currently, Go plugins (custom.*.path) are using another approach: if the path is not absolute the configuration directory is added.
It was not consistent with the other approaches, 🤷
I created a compatibility layer, in v2, we will remove this layer and use the base path only.

ldez · 2025-01-20T16:42:14Z

The PR is open: #5339

ldez added area: config Related to .golangci.yml and/or cli options proposal labels Jan 6, 2025

ldez added this to the v2 milestone Jan 6, 2025

ldez mentioned this issue Jan 6, 2025

🪩 It's time to create a v2 🚀 #5300

Open

ldez mentioned this issue Jan 6, 2025

🌟 Let's talk about default exclusions #5298

Open

ldez added the area: exclusions label Jan 8, 2025

ldez self-assigned this Jan 8, 2025

ldez mentioned this issue Jan 8, 2025

🌟 Let's talk about "formatters" #5296

Open

ldez mentioned this issue Jan 11, 2025

dev: processor optimizations #5316

Merged

ldez mentioned this issue Jan 14, 2025

dev: new processor order #5322

Merged

ldez mentioned this issue Jan 20, 2025

feat: new linter exclusions system #5339

Merged

ldez closed this as completed in #5339 Jan 28, 2025

ldez mentioned this issue Jan 31, 2025

feat: use run.relative-path-mode for output format paths #5363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌟 Let's talk about exclusion options inside issues section #5297

🌟 Let's talk about exclusion options inside issues section #5297

ldez commented Jan 6, 2025 •

edited

Loading

bombsimon commented Jan 6, 2025

ldez commented Jan 6, 2025 •

edited

Loading

ldez commented Jan 8, 2025 •

edited

Loading

ldez commented Jan 9, 2025

ldez commented Jan 10, 2025 •

edited

Loading

ldez commented Jan 12, 2025 •

edited

Loading

ldez commented Jan 13, 2025 •

edited

Loading

ldez commented Jan 14, 2025 •

edited

Loading

ldez commented Jan 14, 2025 •

edited

Loading

ldez commented Jan 14, 2025

ldez commented Jan 14, 2025

ldez commented Jan 14, 2025 •

edited

Loading

ldez commented Jan 16, 2025 •

edited

Loading

ldez commented Jan 16, 2025

ldez commented Jan 18, 2025 •

edited

Loading

ldez commented Jan 18, 2025 •

edited

Loading

ldez commented Jan 20, 2025

🌟 Let's talk about exclusion options inside issues section #5297

🌟 Let's talk about exclusion options inside issues section #5297

Comments

ldez commented Jan 6, 2025 • edited Loading

⛑️ The Problems

Useless Option

Duplicated Options

Unclear Section

Default Exclusions

💭 Proposal

bombsimon commented Jan 6, 2025

ldez commented Jan 6, 2025 • edited Loading

ldez commented Jan 8, 2025 • edited Loading

ldez commented Jan 9, 2025

ldez commented Jan 10, 2025 • edited Loading

ldez commented Jan 12, 2025 • edited Loading

ldez commented Jan 13, 2025 • edited Loading

ldez commented Jan 14, 2025 • edited Loading

Classic project

Monorepo

Local group of projects

Mixed content

ldez commented Jan 14, 2025 • edited Loading

ldez commented Jan 14, 2025

ldez commented Jan 14, 2025

ldez commented Jan 14, 2025 • edited Loading

ldez commented Jan 16, 2025 • edited Loading

ldez commented Jan 16, 2025

ldez commented Jan 18, 2025 • edited Loading

ldez commented Jan 18, 2025 • edited Loading

ldez commented Jan 20, 2025

ldez commented Jan 6, 2025 •

edited

Loading

ldez commented Jan 6, 2025 •

edited

Loading

ldez commented Jan 8, 2025 •

edited

Loading

ldez commented Jan 10, 2025 •

edited

Loading

ldez commented Jan 12, 2025 •

edited

Loading

ldez commented Jan 13, 2025 •

edited

Loading

ldez commented Jan 14, 2025 •

edited

Loading

ldez commented Jan 14, 2025 •

edited

Loading

ldez commented Jan 14, 2025 •

edited

Loading

ldez commented Jan 16, 2025 •

edited

Loading

ldez commented Jan 18, 2025 •

edited

Loading

ldez commented Jan 18, 2025 •

edited

Loading