Add severity to error messages ("warning" vs. "error")

[rliebz]

Currently, all messages are treated as errors, and some number of issues are excluded by default. To provide more granular detail, it may make sense to flag certain issues as "warning" or "info" level messages instead of "error" or instead of being on the default exclude list.

It may make sense to categorize certain linters as being warning or error level linters by default. For example, issues from gofmt are probably of severity "warning", while issues from go vet are probably of severity "error". Other linters like gas actually provide severities, which can be used directly.

[jirfag]

Hi, thank you! I agree, it's a cool idea and it's planned. Now there is something similar to differentiate between different kinds of issues: presets. You can enable a bugs preset only to exclude all minor issues.

All available presets:

Linters presets:
bugs: govet, errcheck, staticcheck, gas, typecheck, megacheck
unused: unused, structcheck, varcheck, ineffassign, deadcode, megacheck
format: gofmt, goimports
style: golint, gosimple, interfacer, unconvert, dupl, goconst, megacheck, depguard
complexity: gocyclo
performance: maligned

[jirfag]

it's planned but in another major version of golangci-lint: it needs big rewrite of code. Closing it now

[AlekSi]

Well, but we still want this feature. Why closing?

[jirfag]

we can reopen, but it's too much of rewriting. I'm not sure it will be made for golangci-lint because of big rewrite

[powerman]

Another example how this feature may be useful: turn deprecation warnings (staticcheck SA1019) into warnings instead of errors - I want linter to notify me about deprecated code, but such code is still correct so it's not nice to force me to rewrite it immediately or disable such warnings and then forget about this issue until it's too late (when deprecated API will be removed from external lib).

[AlekSi]

Well, but we still want this feature. Why closing?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[AlekSi]

We solved this problem by having two separate configuration files. But there are downsides: golangci-lint runs twice, and common settings between files should be synced manually.

[theckman]

Personally speaking, I think we should take the same stance as the Go compiler: there are no warnings and only things to be fixed. If folks want to treat different things as not as critical, I think the approach @AlekSi took is the right way to do it.

I can understand why they'd like us to solve that need for them, but based on the overall interest interest in this request in 2 years I'm not sure we should complicate golangci-lint to support this workflow.

[AlekSi]

I think we should take the same stance as the Go compiler: there are no warnings and only things to be fixed.

I disagree. I think golang/lint's README sums it up nicely:

The suggestions made by golint are exactly that: suggestions. Golint is not perfect, and has both false positives and false negatives. Do not treat its output as a gold standard. We will not be adding pragmas or other knobs to suppress specific warnings, so do not expect or require code to be completely "lint-free". In short, this tool is not, and will never be, trustworthy enough for its suggestions to be enforced automatically, for example as part of a build process.

golangci-lint packs a lot of different linters with different levels of quality, usefulness, and amount of false positives. The results of some of them are useful when reviewed by a human.

I think the approach @AlekSi took is the right way to do it.

I don't think this is the right approach due to the downsides I mentioned above.

but based on the overall interest interest in this request in 2 years I'm not sure we should complicate golangci-lint to support this workflow.

@jirfag seems to disagree with you:

I agree, it's a cool idea and it's planned.

Unless there is another way to track big feature requests, I don't think this issue should be closed.

[theckman]

@AlekSi I understand they disagree, which is why I said personally speaking.

[ryancurrah]

I have a need to separate lints into different categories for sonarqube bugs and style issues. Sonarqube considers checkstyle errors as bugs and info as style issues.

I was thinking of making a contribution to golangci to have a post processor that allows you categorize issues based on the linter name and a regex of the issue message.

So this would be a new section of the config that would be disabled by default

[powerman]

It looks like right now this issue can be worked around by having two different configs for golangci-lint, and running it twice, where one of runs won't used to break the build even if it fails. But managing these two configs may be hard enough and most likely will result in people just give up and just disable all useful warnings.

So, the actual question: is it valuable feature for golangci-lint to provide users with extra hints, which otherwise will be completely disabled? If the answer is yes, then there should be a way to separate warnings and errors, and while we can have some presets (e.g. newly added linter with high level of false positives might be set to warning level by default) users should be able to setup any message as a warning or an error, just like they are able to disable any message.

[ryancurrah]

@powerman running golangci with two different configs still does not categorize the issues in a way code quality tools can determine the type of lint issue.

It should be possible to configure the severity of an issue very similar to how we can configure exclude rules for issues. This would allow us to assign severity based on personal/team preferences.

issues:
  severity-rules:
    - linters:
        - staticcheck
      text: "SA9003:"
      severity: info

The problem would be: How do we define a severity? Following checkstyle convention would be a good start. It has 4 severity types with the default severity being error. Meaning if no severity rule is defined the default is error. The benefit of this is it is not a breaking changing and follows a well defined convention.

From https://checkstyle.sourceforge.io/config.html#Severity:

Each module has a severity property that a Checkstyle audit assigns to all violations of the check. The default severity level of a check is error.

You can use the severity property to control the output of the plain formatter for the command line tool and the ANT task. The plain formatter does not report violations with severity level ignore, and notes violations with severity level warning. For example, according to the following configuration fragment, the plain formatter outputs warnings for translation violations.

From https://checkstyle.sourceforge.io/property_types.html#severity:

This type represents the severity level of a check violation. The valid options are:

• ignore
• info
• warning
• error

Here is how sonarqube determines the type of lint issues based on severity (Link):

  protected RuleType ruleType(String ruleKey, @Nullable String severity) {
    return "error".equals(severity) ? RuleType.BUG : RuleType.CODE_SMELL;
  }

  protected Severity severity(String ruleKey, @Nullable String severity) {
    return "info".equals(severity) ? Severity.MINOR : Severity.MAJOR;
  }

So if the issue severity is error then the issue type is a bug, else it's a code_smell and it also determines issue severity.

By having the option to categorize lint issues it would allow us to have finer grained quality gates and maybe enable some linters that we would not of normally because they all get lumped into the same category bug.

[powerman]

Well, I'm not sure about what @rliebz meant when opening the issue, but me and @ryancurrah are probably talking about different things.

I don't care about categorization and believe it's orthogonal to warning/error feature.

Tools like SonarQube (I was involved in desinging a similar one several years ago) needs own categories, use different tools as a data sources (which output never match these categories anyway), and their categories are defined by their own business-logic and might be changed in any way. This means there is not much sense in tuning some data source (golangci-lint) output to match "today's" categories of tool like SonarQube.

What I'm talking about is ability to notify user about some issues with code without failing the build. This feature affects only exit status of golangci-lint and won't imply any stable categorization of issues.

[ryancurrah]

Ah I see. @powerman you are probably looking for this issue then: https://github.com/golangci/golangci-lint/issues/1043

[powerman]

@ryancurrah No. By "failing the build" I didn't mean compilation errors, I mean non-zero exit code from golangci-lint while running on CI.

[rliebz]

I'm certainly not an authority on the correct behavior here, but I can elaborate a bit as to what I often see in linting tools for other languages, and what I'm most interested in here.

There are two ways that I use a linter, generally:

1. In CI, to enforce that code does not make it into a repo unless it passes a check
2. In my editor, to provide additional information for me as I am coding

I am aware that there are other workflows, like pulling up diagnostics to analyze an existing code base, but they are not part of what I regularly do. People who do use those workflows might have different needs than I do.

On classifications, my preferred behavior is:

• Some issues that the linter finds are classified as "error", and these cause the linter to exit with a non-zero exit code.
• Some issues that the linter finds are classified as "warning", and these exit with a zero status code by default. There may be a command-line flag, which when passed, causes warnings to exit with a non-zero status code.
• There are sometimes additional severity levels as well. The language server specification defines "Information" and "Hint" in addition to the two I listed. "Info" tends to be pretty common. These are informative, but have no bearing on whether or not a build passes.
• Issues may have default values as to what their severity is (even "everything is an error"), but I can specify the severity of each issue in the configuration file.

The advantage to having additional severity levels other than "error" for my workflow is that I can get information on code, whether I'm running the check manually or automatically in my editor, without breaking my build. Having two files for a single linter solves that to a degree, but it is not always trivial to set up in a code editor, and having a standard way to do it is certainly cleaner, in my opinion.

[ryancurrah]

@rliebz i have a wip pr here - https://github.com/golangci/golangci-lint/pull/1155

[ryancurrah]

Merged https://github.com/golangci/golangci-lint/pull/1155

[SVilgelm]

Closing due to fixed