🌟 Let's talk about default exclusions

[ldez]

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-use-default: true
  include:
    - EXC0001
    - EXC0002
    - EXC0003
    - EXC0004

⛑️ Problems

Useful vs Good Practice

I agree with Denis: most projects don't add docs and don't want to add docs.
#456 (comment)

But I also agree that is not a good practice.
#456 (comment)

The Names of the Embed Exclusions

The codes of the default exclusions are machine-oriented but we are humans.

• EXC0001: (errcheck) std error handling (ex: os.Close)
• EXC0002: (golint) comment
• EXC0003: (golint) func name will be used as test.Test.* by other packages
• EXC0004: (govet) possible misuse of unsafe.Pointer|should have signature
• EXC0005: (staticcheck/SA4011) C-style with an explicit 'break' in a 'switch'
• EXC0006: (gosec/G103) Use of unsafe calls should be audited
• EXC0007: (gosec/G204) Subprocess launched with variable
• EXC0008: (gosec/G104) Duplicated errcheck checks.
• EXC0009: (gosec/G301|G302|G307) (G301|G302|G307): Expect (directory permissions to be 0750|file permissions to be 0600) or less
• EXC0010: (gosec/G304) False positive is triggered by 'src, err := ioutil.ReadFile(filename)'.
• EXC0011: (stylecheck/ST1000|ST1020|ST1021|ST1022) comments
• EXC0012: (revive) comments
• EXC0013: (revive) comments
• EXC0014: (revive) comments
• EXC0015: (revive) comments

Include?

I think that only a few people are understanding how the current configuration works.

For example, what those configurations are doing?

linters:
  disable-all: true
  enable:
    - revive

issues:
  exclude-use-default: true # <-- the difference is here
  include:
    - EXC0012 # related to revive and comments
    - EXC0013 # related to revive and comments
    - EXC0014 # related to revive and comments
    - EXC0015 # related to revive and comments

linters:
  disable-all: true
  enable:
    - revive

issues:
  exclude-use-default: false # <-- the difference is here
  include:
    - EXC0012 # related to revive and comments
    - EXC0013 # related to revive and comments
    - EXC0014 # related to revive and comments
    - EXC0015 # related to revive and comments

answers

The first configuration enables all the default exclusions but disables EXC0012, EXC0013, etc.

The second configuration disables all the default exclusions and that's all.
The include section is ignored because this is only a filter on default exclusions.

Existing Issues

• Cannot change settings disabled by default in staticcheck and stylecheck to enabled #5043 (EXC0011) -> about comments
• detects whether declarations of exported variables, constants, functions, etc. have comments.  #4828 (EXC0012, EXC0013, EXC0014, EXC0015) -> about comments
• errcheck does not work on Close #4262 (EXC0001) -> about std error handling
• Revive linters "exported" and "package-comments" do not work #3987 (EXC0012, EXC0013, EXC0014, EXC0015) -> about comments
• ignores unhandled errors of methods #3976 (EXC0001) -> about std error handling
• unsafeptr analyzer of govet is not reporting errors #3820 (EXC004) -> about govet rule
• Unable to detect exported codes, such as function and struct. #2827 (EXC0012, EXC0013, EXC0014, EXC0015) -> about comments
• Remove --exclude-use-default, default to false, or trim it WAY down #2239 (Ask to remove default exclusions)
• Stylecheck does not respect checks set in settings #2162 (EXC0011) -> about comments
• staticchecks errors are not reported #2049 (EXC0011) -> about comments
• revive: exported rule does not work #1937 (EXC0012, EXC0013, EXC0014, EXC0015) -> about comments
• golangci-lint does not return errors #1753 (EXC0002) -> about comments
• DefaultExcludePatterns should only be used for specified linter #1474 (side effect on plugin reports)
• golangci-lint run does not report issues. #1051 (EXC0002) -> about comments
• Error checking when the client is closed is not captured #609 (EXC0001) -> about std error handling

💭 The Proposal

• No default exclusions
• Removes (deprecates) exclude-use-default
• Groups exclusions by topic with human-oriented names:
  • comments -> EXC0011, EXC0012, EXC0013, EXC0014, EXC0015
  • stdErrorHandling -> EXC0001
  • commonFalsePositives -> EXC0006, EXC0007, EXC0010
• Removes (deprecates) useless exclusions:
  • related to golint: EXC0002 (comments), EXC0003
• Removes (deprecates) "too specific" exclusions:
  • EXC0005, EXC0008, EXC0009, EXC0010
• EXC0004?

# ...
  exclusions:
    default: # presets?
      - comments
      - stdErrorHandling
      - commonFalsePositives

[bombsimon]

This is great, I think it is in the spirit of us not dictating what should be linted or not. I fully see the original argument by Denis but I think there might be other approaches to it. Having a separate exclusion set that's not linter config always felt weird to me. If the linters are there, enabled by the user or default, I think its rules should also be run. If the rule isn't of interest, the linter setting should be setup to disable it. Having exclusions for non default linters feels even more strange to me.

Sadly not all linters have settings for these things so using exclusions is fine - but I think it should be setup or opted in by the user and not us.

These things are hard because we never know how users would use or approach the issues. Maybe all those guesstimated code bases without comments would actually have them if the default linter setup flagged it?

Sorry for off topic but...

I think this also leaks in the sharable presets territory (which should not be addressed now) but if we imagine a future where you can share a setup that's not only a set of enabled linters but also how they are configured and a set up exclusions, we could completely remove this and replace it with something like that. As with ESLint, that would just be a standard preset (by us or someone else) extended with any or all of these patterns. That is close to your suggestion, but the current suggestion splits the concept of sharable linters and exclusions into two topics (and doesn't address sharable or pre configured linter settings).

[bombsimon]

Should we have a legacy preset that excludes exactly what we do now to make upgrading easier? Maybe extra important if we don't plan to move all current exclusions into one of the new presets?

exclusions:
  default:
    - legacy # or v1 or whatever

[ldez]

Should we have a legacy preset that excludes exactly what we do now to make upgrading easier?

If we do that we will deprecate this option (legacy) at the same time because users will mix this value with the new one.

But maybe we can add legacy only with the exclusions we will remove (EXC0005, EXC0008, EXC0009, EXC0010). 🤔

Also, we will provide a migration command.

About exclusion section position: #5297 (comment)