add ability to set issue severity (#1155)

* add ability to set issue severity for out formats that support it based on severity rules

* fix lint issues

* change log child name

* code climate omit severity if empty

* add tests for severity rules, add support for case sensitive rules, fix lint issues, better doc comments, share processor test

* deduplicated rule logic into a base rule that can be used by multiple rule types, moved severity config to it's own parent key named severity, reduced size of NewRunner function to make it easier to read

* put validate function under base rule struct

* better validation error wording

* add Fingerprint and Description methods to Issue struct, made codeclimate reporter easier to read, checkstyle output is now pretty printed

Author: ryancurrah

.gitignore

@@ -15,3 +15,5 @@
 /.vscode/
 *.test
 .DS_Store
+coverage.out
+coverage.xml

.golangci.example.yml

@@ -382,3 +382,28 @@ issues:
 
   # Show only new issues created in git patch with set file path.
   new-from-patch: path/to/patch/file
+
+severity:
+  # Default value is empty string.
+  # Set the default severity for issues. If severity rules are defined and the issues 
+  # do not match or no severity is provided to the rule this will be the default 
+  # severity applied. Severities should match the supported severity names of the 
+  # selected out format.
+  # - Code climate: https://docs.codeclimate.com/docs/issues#issue-severity
+  # -   Checkstyle: https://checkstyle.sourceforge.io/property_types.html#severity
+  # -       Github: https://help.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-an-error-message
+  default-severity: error
+
+  # The default value is false. 
+  # If set to true severity-rules regular expressions become case sensitive.
+  case-sensitive: false
+
+  # Default value is empty list.
+  # When a list of severity rules are provided, severity information will be added to lint
+  # issues. Severity rules have the same filtering capability as exclude rules except you 
+  # are allowed to specify one matcher per severity rule.
+  # Only affects out formats that support setting severity information.
+  rules:
+    - linters:
+      - dupl
+      severity: info

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/fatih/color v1.9.0
 	github.com/go-critic/go-critic v0.4.3
 	github.com/go-lintpack/lintpack v0.5.2
+	github.com/go-xmlfmt/xmlfmt v0.0.0-20191208150333-d5b6f63a941b
 	github.com/gofrs/flock v0.7.1
 	github.com/golangci/check v0.0.0-20180506172741-cfe4005ccda2
 	github.com/golangci/dupl v0.0.0-20180902072040-3e9179ac440a

pkg/config/config.go

@@ -407,51 +407,71 @@ type Linters struct {
 	Presets []string
 }
 
-type ExcludeRule struct {
+type BaseRule struct {
 	Linters []string
 	Path    string
 	Text    string
 	Source  string
 }
 
-func validateOptionalRegex(value string) error {
-	if value == "" {
-		return nil
-	}
-	_, err := regexp.Compile(value)
-	return err
-}
-
-func (e ExcludeRule) Validate() error {
-	if err := validateOptionalRegex(e.Path); err != nil {
+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(e.Text); err != nil {
+	if err := validateOptionalRegex(b.Text); err != nil {
 		return fmt.Errorf("invalid text regex: %v", err)
 	}
-	if err := validateOptionalRegex(e.Source); err != nil {
+	if err := validateOptionalRegex(b.Source); err != nil {
 		return fmt.Errorf("invalid source regex: %v", err)
 	}
 	nonBlank := 0
-	if len(e.Linters) > 0 {
+	if len(b.Linters) > 0 {
 		nonBlank++
 	}
-	if e.Path != "" {
+	if b.Path != "" {
 		nonBlank++
 	}
-	if e.Text != "" {
+	if b.Text != "" {
 		nonBlank++
 	}
-	if e.Source != "" {
+	if b.Source != "" {
 		nonBlank++
 	}
-	const minConditionsCount = 2
 	if nonBlank < minConditionsCount {
-		return errors.New("at least 2 of (text, source, path, linters) should be set")
+		return fmt.Errorf("at least %d of (text, source, path, linters) should be set", minConditionsCount)
 	}
 	return nil
 }
 
+const excludeRuleMinConditionsCount = 2
+
+type ExcludeRule struct {
+	BaseRule `mapstructure:",squash"`
+}
+
+func validateOptionalRegex(value string) error {
+	if value == "" {
+		return nil
+	}
+	_, err := regexp.Compile(value)
+	return err
+}
+
+func (e ExcludeRule) Validate() error {
+	return e.BaseRule.Validate(excludeRuleMinConditionsCount)
+}
+
+const severityRuleMinConditionsCount = 1
+
+type SeverityRule struct {
+	BaseRule `mapstructure:",squash"`
+	Severity string
+}
+
+func (s *SeverityRule) Validate() error {
+	return s.BaseRule.Validate(severityRuleMinConditionsCount)
+}
+
 type Issues struct {
 	IncludeDefaultExcludes []string      `mapstructure:"include"`
 	ExcludeCaseSensitive   bool          `mapstructure:"exclude-case-sensitive"`
@@ -469,6 +489,12 @@ type Issues struct {
 	NeedFix bool `mapstructure:"fix"`
 }
 
+type Severity struct {
+	Default       string         `mapstructure:"default-severity"`
+	CaseSensitive bool           `mapstructure:"case-sensitive"`
+	Rules         []SeverityRule `mapstructure:"rules"`
+}
+
 type Config struct {
 	Run Run
 
@@ -484,6 +510,7 @@ type Config struct {
 	LintersSettings LintersSettings `mapstructure:"linters-settings"`
 	Linters         Linters
 	Issues          Issues
+	Severity        Severity
 
 	InternalTest bool // Option is used only for testing golangci-lint code, don't use it
 }

pkg/config/reader.go

@@ -113,6 +113,14 @@ func (r *FileReader) validateConfig() error {
 			return fmt.Errorf("error in exclude rule #%d: %v", i, err)
 		}
 	}
+	if len(c.Severity.Rules) > 0 && c.Severity.Default == "" {
+		return errors.New("can't set severity rule option: no default severity defined")
+	}
+	for i, rule := range c.Severity.Rules {
+		if err := rule.Validate(); err != nil {
+			return fmt.Errorf("error in severity rule #%d: %v", i, err)
+		}
+	}
 	if err := c.LintersSettings.Govet.Validate(); err != nil {
 		return fmt.Errorf("error in govet config: %v", err)
 	}

pkg/lint/runner.go

@@ -31,24 +31,6 @@ type Runner struct {
 
 func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env, es *lintersdb.EnabledSet,
 	lineCache *fsutils.LineCache, dbManager *lintersdb.Manager, pkgs []*gopackages.Package) (*Runner, error) {
-	icfg := cfg.Issues
-	excludePatterns := icfg.ExcludePatterns
-	if icfg.UseDefaultExcludes {
-		excludePatterns = append(excludePatterns, config.GetExcludePatternsStrings(icfg.IncludeDefaultExcludes)...)
-	}
-
-	var excludeTotalPattern string
-	if len(excludePatterns) != 0 {
-		excludeTotalPattern = fmt.Sprintf("(%s)", strings.Join(excludePatterns, "|"))
-	}
-
-	var excludeProcessor processors.Processor
-	if cfg.Issues.ExcludeCaseSensitive {
-		excludeProcessor = processors.NewExcludeCaseSensitive(excludeTotalPattern)
-	} else {
-		excludeProcessor = processors.NewExclude(excludeTotalPattern)
-	}
-
 	skipFilesProcessor, err := processors.NewSkipFiles(cfg.Run.SkipFiles)
 	if err != nil {
 		return nil, err
@@ -63,22 +45,6 @@ func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env, es *lint
 		return nil, err
 	}
 
-	var excludeRules []processors.ExcludeRule
-	for _, r := range icfg.ExcludeRules {
-		excludeRules = append(excludeRules, processors.ExcludeRule{
-			Text:    r.Text,
-			Source:  r.Source,
-			Path:    r.Path,
-			Linters: r.Linters,
-		})
-	}
-	var excludeRulesProcessor processors.Processor
-	if cfg.Issues.ExcludeCaseSensitive {
-		excludeRulesProcessor = processors.NewExcludeRulesCaseSensitive(excludeRules, lineCache, log.Child("exclude_rules"))
-	} else {
-		excludeRulesProcessor = processors.NewExcludeRules(excludeRules, lineCache, log.Child("exclude_rules"))
-	}
-
 	enabledLinters, err := es.GetEnabledLintersMap()
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to get enabled linters")
@@ -101,17 +67,18 @@ func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env, es *lint
 			// Must be before exclude because users see already marked output and configure excluding by it.
 			processors.NewIdentifierMarker(),
 
-			excludeProcessor,
-			excludeRulesProcessor,
+			getExcludeProcessor(&cfg.Issues),
+			getExcludeRulesProcessor(&cfg.Issues, log, lineCache),
 			processors.NewNolint(log.Child("nolint"), dbManager, enabledLinters),
 
 			processors.NewUniqByLine(cfg),
-			processors.NewDiff(icfg.Diff, icfg.DiffFromRevision, icfg.DiffPatchFilePath),
+			processors.NewDiff(cfg.Issues.Diff, cfg.Issues.DiffFromRevision, cfg.Issues.DiffPatchFilePath),
 			processors.NewMaxPerFileFromLinter(cfg),
-			processors.NewMaxSameIssues(icfg.MaxSameIssues, log.Child("max_same_issues"), cfg),
-			processors.NewMaxFromLinter(icfg.MaxIssuesPerLinter, log.Child("max_from_linter"), cfg),
+			processors.NewMaxSameIssues(cfg.Issues.MaxSameIssues, log.Child("max_same_issues"), cfg),
+			processors.NewMaxFromLinter(cfg.Issues.MaxIssuesPerLinter, log.Child("max_from_linter"), cfg),
 			processors.NewSourceCode(lineCache, log.Child("source_code")),
 			processors.NewPathShortener(),
+			getSeverityRulesProcessor(&cfg.Severity, log, lineCache),
 		},
 		Log: log,
 	}, nil
@@ -254,3 +221,89 @@ func (r *Runner) processIssues(issues []result.Issue, sw *timeutils.Stopwatch, s
 
 	return issues
 }
+
+func getExcludeProcessor(cfg *config.Issues) processors.Processor {
+	excludePatterns := cfg.ExcludePatterns
+	if cfg.UseDefaultExcludes {
+		excludePatterns = append(excludePatterns, config.GetExcludePatternsStrings(cfg.IncludeDefaultExcludes)...)
+	}
+
+	var excludeTotalPattern string
+	if len(excludePatterns) != 0 {
+		excludeTotalPattern = fmt.Sprintf("(%s)", strings.Join(excludePatterns, "|"))
+	}
+
+	var excludeProcessor processors.Processor
+	if cfg.ExcludeCaseSensitive {
+		excludeProcessor = processors.NewExcludeCaseSensitive(excludeTotalPattern)
+	} else {
+		excludeProcessor = processors.NewExclude(excludeTotalPattern)
+	}
+
+	return excludeProcessor
+}
+
+func getExcludeRulesProcessor(cfg *config.Issues, log logutils.Log, lineCache *fsutils.LineCache) processors.Processor {
+	var excludeRules []processors.ExcludeRule
+	for _, r := range cfg.ExcludeRules {
+		excludeRules = append(excludeRules, processors.ExcludeRule{
+			BaseRule: processors.BaseRule{
+				Text:    r.Text,
+				Source:  r.Source,
+				Path:    r.Path,
+				Linters: r.Linters,
+			},
+		})
+	}
+
+	var excludeRulesProcessor processors.Processor
+	if cfg.ExcludeCaseSensitive {
+		excludeRulesProcessor = processors.NewExcludeRulesCaseSensitive(
+			excludeRules,
+			lineCache,
+			log.Child("exclude_rules"),
+		)
+	} else {
+		excludeRulesProcessor = processors.NewExcludeRules(
+			excludeRules,
+			lineCache,
+			log.Child("exclude_rules"),
+		)
+	}
+
+	return excludeRulesProcessor
+}
+
+func getSeverityRulesProcessor(cfg *config.Severity, log logutils.Log, lineCache *fsutils.LineCache) processors.Processor {
+	var severityRules []processors.SeverityRule
+	for _, r := range cfg.Rules {
+		severityRules = append(severityRules, processors.SeverityRule{
+			Severity: r.Severity,
+			BaseRule: processors.BaseRule{
+				Text:    r.Text,
+				Source:  r.Source,
+				Path:    r.Path,
+				Linters: r.Linters,
+			},
+		})
+	}
+
+	var severityRulesProcessor processors.Processor
+	if cfg.CaseSensitive {
+		severityRulesProcessor = processors.NewSeverityRulesCaseSensitive(
+			cfg.Default,
+			severityRules,
+			lineCache,
+			log.Child("severity_rules"),
+		)
+	} else {
+		severityRulesProcessor = processors.NewSeverityRules(
+			cfg.Default,
+			severityRules,
+			lineCache,
+			log.Child("severity_rules"),
+		)
+	}
+
+	return severityRulesProcessor
+}

pkg/printers/checkstyle.go

@@ -5,6 +5,8 @@ import (
 	"encoding/xml"
 	"fmt"
 
+	"github.com/go-xmlfmt/xmlfmt"
+
 	"github.com/golangci/golangci-lint/pkg/logutils"
 	"github.com/golangci/golangci-lint/pkg/result"
 )
@@ -28,7 +30,7 @@ type checkstyleError struct {
 	Source   string `xml:"source,attr"`
 }
 
-const defaultSeverity = "error"
+const defaultCheckstyleSeverity = "error"
 
 type Checkstyle struct{}
 
@@ -54,12 +56,17 @@ func (Checkstyle) Print(ctx context.Context, issues []result.Issue) error {
 			files[issue.FilePath()] = file
 		}
 
+		severity := defaultCheckstyleSeverity
+		if issue.Severity != "" {
+			severity = issue.Severity
+		}
+
 		newError := &checkstyleError{
 			Column:   issue.Column(),
 			Line:     issue.Line(),
 			Message:  issue.Text,
 			Source:   issue.FromLinter,
-			Severity: defaultSeverity,
+			Severity: severity,
 		}
 
 		file.Errors = append(file.Errors, newError)
@@ -75,6 +82,6 @@ func (Checkstyle) Print(ctx context.Context, issues []result.Issue) error {
 		return err
 	}
 
-	fmt.Fprintf(logutils.StdOut, "%s%s\n", xml.Header, data)
+	fmt.Fprintf(logutils.StdOut, "%s%s\n", xml.Header, xmlfmt.FormatXML(string(data), "", "  "))
 	return nil
 }