Conf reason rule disabling (#193)

* adds support for comments when enabling/disabling

* adds config flag to require disabling reason

* Update lint/file.go

adds code fmt suggestion by @mgechev

Co-Authored-By: Minko Gechev <mgechev@gmail.com>

* moves regexp compilation out of the function
fix typo in condition

* adds support for comments when enabling/disabling

* skips incomplete directives and generate a failure

* adds _directive_ concept to cope with specify-disable-reason

* adds doc
gofmt

* fixes severity is ignored

README.md

@@ -140,7 +140,7 @@ revive -config revive.toml -exclude file1.go -exclude file2.go -formatter friend
 - The output will be formatted with the `friendly` formatter
 - The linter will analyze `github.com/mgechev/revive` and the files in `package`
 
-### Comment Annotations
+### Comment Directives
 
 Using comments, you can disable the linter for the entire file or only range of lines:
 
@@ -167,6 +167,28 @@ func Public() private {
 
 This way, `revive` will not warn you for that you're returning an object of an unexported type, from an exported function.
 
+You can document why you disable the linter by adding a trailing text in the directive, for example
+
+```go
+//revive:disable Until the code is stable
+```
+```go
+//revive:disable:cyclomatic High complexity score but easy to understand 
+```
+
+You can also configure `revive` to enforce documenting linter disabling directives by adding
+
+```toml
+[directive.specify-disable-reason]
+```
+
+in the configuration. You can set the severity (defaults to _warning_) of the violation of this directive
+
+```toml
+[directive.specify-disable-reason]
+    severity = "error"
+```
+
 ### Configuration
 
 `revive` can be configured with a TOML file. Here's a sample configuration with explanation for the individual properties:
@@ -399,7 +421,7 @@ Each formatter needs to implement the following interface:
 
 ```go
 type Formatter interface {
-	Format(<-chan Failure, RulesConfig) (string, error)
+	Format(<-chan Failure, Config) (string, error)
 	Name() string
 }
 ```

config.go

@@ -142,6 +142,12 @@ func normalizeConfig(config *lint.Config) {
 			}
 			config.Rules[k] = v
 		}
+		for k, v := range config.Directives {
+			if v.Severity == "" {
+				v.Severity = severity
+			}
+			config.Directives[k] = v
+		}
 	}
 }
 

fixtures/disable-annotations2.go

@@ -6,6 +6,13 @@ func foo1() {
 	var invalid_name2 = 1 //revive:disable-line:var-naming
 }
 
+func foo11() {
+	//revive:disable-next-line:var-naming I'm an Eiffel programmer thus I like underscores
+	var invalid_name = 0
+	var invalid_name2 = 1 //revive:disable-line:var-naming I'm an Eiffel programmer thus I like underscores
+}
+
+
 func foo2() {
 	// 		revive:disable-next-line:var-naming
 	//revive:disable
@@ -22,3 +29,10 @@ func foo3() {
 	/* revive:disable-next-line:var-naming */
 	var invalid_name3 = 0 // MATCH /don't use underscores in Go names; var invalid_name3 should be invalidName3/
 }
+
+func foo2p1() {
+	//revive:disable Underscores are fine
+	var invalid_name = 0
+	//revive:enable No! Underscores are not nice!
+	var invalid_name2 = 1 // MATCH /don't use underscores in Go names; var invalid_name2 should be invalidName2/
+}

formatter/checkstyle.go

@@ -28,7 +28,7 @@ type issue struct {
 }
 
 // Format formats the failures gotten from the lint.
-func (f *Checkstyle) Format(failures <-chan lint.Failure, config lint.RulesConfig) (string, error) {
+func (f *Checkstyle) Format(failures <-chan lint.Failure, config lint.Config) (string, error) {
 	var issues = map[string][]issue{}
 	for failure := range failures {
 		buf := new(bytes.Buffer)

formatter/default.go

@@ -18,7 +18,7 @@ func (f *Default) Name() string {
 }
 
 // Format formats the failures gotten from the lint.
-func (f *Default) Format(failures <-chan lint.Failure, _ lint.RulesConfig) (string, error) {
+func (f *Default) Format(failures <-chan lint.Failure, _ lint.Config) (string, error) {
 	for failure := range failures {
 		fmt.Printf("%v: %s\n", failure.Position.Start, failure.Failure)
 	}

formatter/friendly.go

@@ -37,7 +37,7 @@ func (f *Friendly) Name() string {
 }
 
 // Format formats the failures gotten from the lint.
-func (f *Friendly) Format(failures <-chan lint.Failure, config lint.RulesConfig) (string, error) {
+func (f *Friendly) Format(failures <-chan lint.Failure, config lint.Config) (string, error) {
 	errorMap := map[string]int{}
 	warningMap := map[string]int{}
 	totalErrors := 0

formatter/json.go

@@ -24,7 +24,7 @@ type jsonObject struct {
 }
 
 // Format formats the failures gotten from the lint.
-func (f *JSON) Format(failures <-chan lint.Failure, config lint.RulesConfig) (string, error) {
+func (f *JSON) Format(failures <-chan lint.Failure, config lint.Config) (string, error) {
 	var slice []jsonObject
 	for failure := range failures {
 		obj := jsonObject{}

formatter/ndjson.go

@@ -19,7 +19,7 @@ func (f *NDJSON) Name() string {
 }
 
 // Format formats the failures gotten from the lint.
-func (f *NDJSON) Format(failures <-chan lint.Failure, config lint.RulesConfig) (string, error) {
+func (f *NDJSON) Format(failures <-chan lint.Failure, config lint.Config) (string, error) {
 	enc := json.NewEncoder(os.Stdout)
 	for failure := range failures {
 		obj := jsonObject{}

formatter/plain.go

@@ -18,7 +18,7 @@ func (f *Plain) Name() string {
 }
 
 // Format formats the failures gotten from the lint.
-func (f *Plain) Format(failures <-chan lint.Failure, _ lint.RulesConfig) (string, error) {
+func (f *Plain) Format(failures <-chan lint.Failure, _ lint.Config) (string, error) {
 	for failure := range failures {
 		fmt.Printf("%v: %s %s\n", failure.Position.Start, failure.Failure, "https://revive.run/r#"+failure.RuleName)
 	}

formatter/severity.go

@@ -2,8 +2,11 @@ package formatter
 
 import "github.com/mgechev/revive/lint"
 
-func severity(config lint.RulesConfig, failure lint.Failure) lint.Severity {
+func severity(config lint.Config, failure lint.Failure) lint.Severity {
-	if config, ok := config[failure.RuleName]; ok && config.Severity == lint.SeverityError {
+	if config, ok := config.Rules[failure.RuleName]; ok && config.Severity == lint.SeverityError {
+		return lint.SeverityError
+	}
+	if config, ok := config.Directives[failure.RuleName]; ok && config.Severity == lint.SeverityError {
 		return lint.SeverityError
 	}
 	return lint.SeverityWarning

formatter/stylish.go

@@ -32,7 +32,7 @@ func formatFailure(failure lint.Failure, severity lint.Severity) []string {
 }
 
 // Format formats the failures gotten from the lint.
-func (f *Stylish) Format(failures <-chan lint.Failure, config lint.RulesConfig) (string, error) {
+func (f *Stylish) Format(failures <-chan lint.Failure, config lint.Config) (string, error) {
 	var result [][]string
 	var totalErrors = 0
 	var total = 0

formatter/unix.go

@@ -19,7 +19,7 @@ func (f *Unix) Name() string {
 }
 
 // Format formats the failures gotten from the lint.
-func (f *Unix) Format(failures <-chan lint.Failure, _ lint.RulesConfig) (string, error) {
+func (f *Unix) Format(failures <-chan lint.Failure, _ lint.Config) (string, error) {
 	for failure := range failures {
 		fmt.Printf("%v: [%s] %s\n", failure.Position.Start, failure.RuleName, failure.Failure)
 	}

lint/config.go

@@ -12,6 +12,14 @@ type RuleConfig struct {
 // RulesConfig defines the config for all rules.
 type RulesConfig = map[string]RuleConfig
 
+// DirectiveConfig is type used for the linter directive configuration.
+type DirectiveConfig struct {
+	Severity Severity
+}
+
+// DirectivesConfig defines the config for all directives.
+type DirectivesConfig = map[string]DirectiveConfig
+
 // Config defines the config of the linter.
 type Config struct {
 	IgnoreGeneratedHeader bool `toml:"ignoreGeneratedHeader"`
@@ -20,4 +28,5 @@ type Config struct {
 	Rules                 RulesConfig      `toml:"rule"`
 	ErrorCode             int              `toml:"errorCode"`
 	WarningCode           int              `toml:"warningCode"`
+	Directives            DirectivesConfig `toml:"directive"`
 }

lint/file.go

@@ -97,9 +97,12 @@ func (f *File) isMain() bool {
 	return false
 }
 
+const directiveSpecifyDisableReason = "specify-disable-reason"
+
 func (f *File) lint(rules []Rule, config Config, failures chan Failure) {
 	rulesConfig := config.Rules
-	disabledIntervals := f.disabledIntervals(rules)
+	_, mustSpecifyDisableReason := config.Directives[directiveSpecifyDisableReason]
+	disabledIntervals := f.disabledIntervals(rules, mustSpecifyDisableReason, failures)
 	for _, currentRule := range rules {
 		ruleConfig := rulesConfig[currentRule.Name()]
 		currentFailures := currentRule.Apply(f, ruleConfig.Arguments)
@@ -126,9 +129,15 @@ type enableDisableConfig struct {
 	position int
 }
 
-func (f *File) disabledIntervals(rules []Rule) disabledIntervalsMap {
+const directiveRE = `^//[\s]*revive:(enable|disable)(?:-(line|next-line))?(?::([^\s]+))?[\s]*(?: (.+))?$`
-	re := regexp.MustCompile(`^//[\s]*revive:(enable|disable)(?:-(line|next-line))?(?::([^\s]+))?[\s]*$`)
+const directivePos = 1
+const modifierPos = 2
+const rulesPos = 3
+const reasonPos = 4
 
+var re = regexp.MustCompile(directiveRE)
+
+func (f *File) disabledIntervals(rules []Rule, mustSpecifyDisableReason bool, failures chan Failure) disabledIntervalsMap {
 	enabledDisabledRulesMap := make(map[string][]enableDisableConfig)
 
 	getEnabledDisabledIntervals := func() disabledIntervalsMap {
@@ -202,14 +211,24 @@ func (f *File) disabledIntervals(rules []Rule) disabledIntervalsMap {
 			}
 
 			ruleNames := []string{}
-			if len(match) > 2 {
+			tempNames := strings.Split(match[rulesPos], ",")
-				tempNames := strings.Split(match[3], ",")
 			for _, name := range tempNames {
 				name = strings.Trim(name, "\n")
 				if len(name) > 0 {
 					ruleNames = append(ruleNames, name)
 				}
 			}
+
+			mustCheckDisablingReason := mustSpecifyDisableReason && match[directivePos] == "disable"
+			if mustCheckDisablingReason && strings.Trim(match[reasonPos], " ") == "" {
+				failures <- Failure{
+					Confidence: 1,
+					RuleName:   directiveSpecifyDisableReason,
+					Failure:    "reason of lint disabling not found",
+					Position:   ToFailurePosition(c.Pos(), c.End(), f),
+					Node:       c,
+				}
+				continue // skip this linter disabling directive
 			}
 
 			// TODO: optimize
@@ -219,7 +238,7 @@ func (f *File) disabledIntervals(rules []Rule) disabledIntervalsMap {
 				}
 			}
 
-			handleRules(filename, match[2], match[1] == "enable", line, ruleNames)
+			handleRules(filename, match[modifierPos], match[directivePos] == "enable", line, ruleNames)
 		}
 	}
 

lint/formatter.go

@@ -9,6 +9,6 @@ type FormatterMetadata struct {
 
 // Formatter defines an interface for failure formatters
 type Formatter interface {
-	Format(<-chan Failure, RulesConfig) (string, error)
+	Format(<-chan Failure, Config) (string, error)
 	Name() string
 }

main.go

@@ -44,7 +44,7 @@ func main() {
 
 	var output string
 	go (func() {
-		output, err = formatter.Format(formatChan, config.Rules)
+		output, err = formatter.Format(formatChan, *config)
 		if err != nil {
 			fail(err.Error())
 		}
@@ -62,6 +62,10 @@ func main() {
 		if c, ok := config.Rules[f.RuleName]; ok && c.Severity == lint.SeverityError {
 			exitCode = config.ErrorCode
 		}
+		if c, ok := config.Directives[f.RuleName]; ok && c.Severity == lint.SeverityError {
+			exitCode = config.ErrorCode
+		}
+
 		formatChan <- f
 	}