Fix #331: fix errcheck "ignore" config directive.

Make tests for "ignore" and "exclude" directives.
Mark all hidden command-line options as deprecated.

Author: jirfag

.golangci.example.yml

@@ -69,6 +69,15 @@ linters-settings:
     # report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`;
     # default is false: such cases aren't reported by default.
     check-blank: false
+
+    # [deprecated] comma-separated list of pairs of the form pkg:regex
+    # the regex is used to ignore names within pkg. (default "fmt:.*").
+    # see https://github.com/kisielk/errcheck#the-deprecated-method for details
+    ignore: fmt,io/ioutil:^ReadF.*
+
+    # path to a file containing a list of functions to exclude from checking
+    # see https://github.com/kisielk/errcheck#excluding-functions for details
+    exclude: /path/to/file.txt
   govet:
     # report about shadowed variables
     check-shadowing: true

README.md

@@ -585,6 +585,15 @@ linters-settings:
     # report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`;
     # default is false: such cases aren't reported by default.
     check-blank: false
+
+    # [deprecated] comma-separated list of pairs of the form pkg:regex
+    # the regex is used to ignore names within pkg. (default "fmt:.*").
+    # see https://github.com/kisielk/errcheck#the-deprecated-method for details
+    ignore: fmt,io/ioutil:^ReadF.*
+
+    # path to a file containing a list of functions to exclude from checking
+    # see https://github.com/kisielk/errcheck#excluding-functions for details
+    exclude: /path/to/file.txt
   govet:
     # report about shadowed variables
     check-shadowing: true

go.mod

@@ -19,7 +19,7 @@ require (
 	github.com/golang/mock v1.1.1
 	github.com/golangci/check v0.0.0-20180506172741-cfe4005ccda2
 	github.com/golangci/dupl v0.0.0-20180902072040-3e9179ac440a
-	github.com/golangci/errcheck v0.0.0-20181003203344-1765131d5be5
+	github.com/golangci/errcheck v0.0.0-20181003203344-ef45e06d44b6
 	github.com/golangci/go-misc v0.0.0-20180628070357-927a3d87b613
 	github.com/golangci/go-tools v0.0.0-20180902103155-93eecd106a0b
 	github.com/golangci/goconst v0.0.0-20180610141641-041c5f2b40f3

go.sum

@@ -45,8 +45,8 @@ github.com/golangci/check v0.0.0-20180506172741-cfe4005ccda2 h1:23T5iq8rbUYlhpt5
 github.com/golangci/check v0.0.0-20180506172741-cfe4005ccda2/go.mod h1:k9Qvh+8juN+UKMCS/3jFtGICgW8O96FVaZsaxdzDkR4=
 github.com/golangci/dupl v0.0.0-20180902072040-3e9179ac440a h1:w8hkcTqaFpzKqonE9uMCefW1WDie15eSP/4MssdenaM=
 github.com/golangci/dupl v0.0.0-20180902072040-3e9179ac440a/go.mod h1:ryS0uhF+x9jgbj/N71xsEqODy9BN81/GonCZiOzirOk=
-github.com/golangci/errcheck v0.0.0-20181003203344-1765131d5be5 h1:q7aCFiVt6gMxZseillOwgsyitdcxbqQz5oxA2rHIeMY=
-github.com/golangci/errcheck v0.0.0-20181003203344-1765131d5be5/go.mod h1:DbHgvLiFKX1Sh2T1w8Q/h4NAI8MHIpzCdnBUDTXU3I0=
+github.com/golangci/errcheck v0.0.0-20181003203344-ef45e06d44b6 h1:i2jIkQFb8RG45DuQs+ElyROY848cSJIoIkBM+7XXypA=
+github.com/golangci/errcheck v0.0.0-20181003203344-ef45e06d44b6/go.mod h1:DbHgvLiFKX1Sh2T1w8Q/h4NAI8MHIpzCdnBUDTXU3I0=
 github.com/golangci/go-misc v0.0.0-20180628070357-927a3d87b613 h1:WadunOE/TeHR8U7f0TXiJACHeU3cuFOXuKafw4rozqU=
 github.com/golangci/go-misc v0.0.0-20180628070357-927a3d87b613/go.mod h1:SyvUF2NxV+sN8upjjeVYr5W7tyxaT1JVtvhKhOn2ii8=
 github.com/golangci/go-tools v0.0.0-20180902103155-93eecd106a0b h1:FSrt9JBK7JINu5UobyIF6epfpjL66H+67KZoTbE0zwk=

pkg/commands/run.go

@@ -41,11 +41,19 @@ func wh(text string) string {
 	return color.GreenString(text)
 }
 
-func initFlagSet(fs *pflag.FlagSet, cfg *config.Config, m *lintersdb.Manager) {
+func initFlagSet(fs *pflag.FlagSet, cfg *config.Config, m *lintersdb.Manager, isFinalInit bool) {
 	hideFlag := func(name string) {
 		if err := fs.MarkHidden(name); err != nil {
 			panic(err)
 		}
+
+		// we run initFlagSet multiple times, but we wouldn't like to see deprecation message multiple times
+		if isFinalInit {
+			const deprecateMessage = "flag will be removed soon, please, use .golangci.yml config"
+			if err := fs.MarkDeprecated(name, deprecateMessage); err != nil {
+				panic(err)
+			}
+		}
 	}
 
 	// Output config
@@ -85,9 +93,11 @@ func initFlagSet(fs *pflag.FlagSet, cfg *config.Config, m *lintersdb.Manager) {
 	fs.BoolVar(&lsc.Errcheck.CheckAssignToBlank, "errcheck.check-blank", false,
 		"Errcheck: check for errors assigned to blank identifier: _ = errFunc()")
 	hideFlag("errcheck.check-blank")
-	fs.StringVar(&lsc.Errcheck.Exclude, "errcheck.exclude", "", "errcheck.exclude")
+	fs.StringVar(&lsc.Errcheck.Exclude, "errcheck.exclude", "",
+		"Path to a file containing a list of functions to exclude from checking")
 	hideFlag("errcheck.exclude")
-	fs.Var(&lsc.Errcheck.Ignore, "errcheck.ignore", "errcheck.ignore")
+	fs.StringVar(&lsc.Errcheck.Ignore, "errcheck.ignore", "fmt:.*",
+		`Comma-separated list of pairs of the form pkg:regex. The regex is used to ignore names within pkg`)
 	hideFlag("errcheck.ignore")
 
 	fs.BoolVar(&lsc.Govet.CheckShadowing, "govet.check-shadowing", false,
@@ -171,7 +181,7 @@ func initFlagSet(fs *pflag.FlagSet, cfg *config.Config, m *lintersdb.Manager) {
 func (e *Executor) initRunConfiguration(cmd *cobra.Command) {
 	fs := cmd.Flags()
 	fs.SortFlags = false // sort them as they are defined here
-	initFlagSet(fs, e.cfg, e.DBManager)
+	initFlagSet(fs, e.cfg, e.DBManager, true)
 }
 
 func (e Executor) getConfigForCommandLine() (*config.Config, error) {
@@ -184,7 +194,7 @@ func (e Executor) getConfigForCommandLine() (*config.Config, error) {
 	// `changed` variable inside string slice vars will be shared.
 	// Use another config variable here, not e.cfg, to not
 	// affect main parsing by this parsing of only config option.
-	initFlagSet(fs, &cfg, e.DBManager)
+	initFlagSet(fs, &cfg, e.DBManager, false)
 
 	// Parse max options, even force version option: don't want
 	// to get access to Executor here: it's error-prone to use

pkg/config/config.go

@@ -2,8 +2,6 @@ package config
 
 import (
 	"errors"
-	"fmt"
-	"regexp"
 	"strings"
 	"time"
 
@@ -174,10 +172,10 @@ type LintersSettings struct {
 }
 
 type ErrcheckSettings struct {
-	CheckTypeAssertions bool       `mapstructure:"check-type-assertions"`
-	CheckAssignToBlank  bool       `mapstructure:"check-blank"`
-	Ignore              IgnoreFlag `mapstructure:"ignore"`
-	Exclude             string     `mapstructure:"exclude"`
+	CheckTypeAssertions bool   `mapstructure:"check-type-assertions"`
+	CheckAssignToBlank  bool   `mapstructure:"check-blank"`
+	Ignore              string `mapstructure:"ignore"`
+	Exclude             string `mapstructure:"exclude"`
 }
 
 type LllSettings struct {
@@ -295,9 +293,6 @@ var defaultLintersSettings = LintersSettings{
 		RangeLoops: true,
 		ForLoops:   false,
 	},
-	Errcheck: ErrcheckSettings{
-		Ignore: IgnoreFlag{},
-	},
 	Gocritic: GocriticSettings{
 		SettingsPerCheck: map[string]GocriticCheckSettings{},
 	},
@@ -347,47 +342,3 @@ func NewDefault() *Config {
 		LintersSettings: defaultLintersSettings,
 	}
 }
-
-// IgnoreFlags was taken from errcheck in order to keep the API identical.
-// https://github.com/kisielk/errcheck/blob/1787c4bee836470bf45018cfbc783650db3c6501/main.go#L25-L60
-type IgnoreFlag map[string]*regexp.Regexp
-
-func (f IgnoreFlag) String() string {
-	pairs := make([]string, 0, len(f))
-	for pkg, re := range f {
-		prefix := ""
-		if pkg != "" {
-			prefix = pkg + ":"
-		}
-		pairs = append(pairs, prefix+re.String())
-	}
-	return fmt.Sprintf("%q", strings.Join(pairs, ","))
-}
-
-func (f IgnoreFlag) Set(s string) error {
-	if s == "" {
-		return nil
-	}
-	for _, pair := range strings.Split(s, ",") {
-		colonIndex := strings.Index(pair, ":")
-		var pkg, re string
-		if colonIndex == -1 {
-			pkg = ""
-			re = pair
-		} else {
-			pkg = pair[:colonIndex]
-			re = pair[colonIndex+1:]
-		}
-		regex, err := regexp.Compile(re)
-		if err != nil {
-			return err
-		}
-		f[pkg] = regex
-	}
-	return nil
-}
-
-// Type returns the type of the flag follow the pflag format.
-func (IgnoreFlag) Type() string {
-	return "stringToRegexp"
-}

pkg/golinters/errcheck.go

@@ -5,6 +5,8 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"regexp"
+	"strings"
 
 	errcheckAPI "github.com/golangci/errcheck/golangci"
 	"github.com/pkg/errors"
@@ -57,19 +59,55 @@ func (e Errcheck) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Is
 	return res, nil
 }
 
+// parseIgnoreConfig was taken from errcheck in order to keep the API identical.
+// https://github.com/kisielk/errcheck/blob/1787c4bee836470bf45018cfbc783650db3c6501/main.go#L25-L60
+func parseIgnoreConfig(s string) (map[string]*regexp.Regexp, error) {
+	if s == "" {
+		return nil, nil
+	}
+
+	cfg := map[string]*regexp.Regexp{}
+
+	for _, pair := range strings.Split(s, ",") {
+		colonIndex := strings.Index(pair, ":")
+		var pkg, re string
+		if colonIndex == -1 {
+			pkg = ""
+			re = pair
+		} else {
+			pkg = pair[:colonIndex]
+			re = pair[colonIndex+1:]
+		}
+		regex, err := regexp.Compile(re)
+		if err != nil {
+			return nil, err
+		}
+		cfg[pkg] = regex
+	}
+
+	return cfg, nil
+}
+
 func genConfig(errCfg *config.ErrcheckSettings) (*errcheckAPI.Config, error) {
+	ignoreConfig, err := parseIgnoreConfig(errCfg.Ignore)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to parse 'ignore' directive")
+	}
+
 	c := &errcheckAPI.Config{
-		Ignore:  errCfg.Ignore,
+		Ignore:  ignoreConfig,
 		Blank:   errCfg.CheckAssignToBlank,
 		Asserts: errCfg.CheckTypeAssertions,
 	}
+
 	if errCfg.Exclude != "" {
 		exclude, err := readExcludeFile(errCfg.Exclude)
 		if err != nil {
 			return nil, err
 		}
 		c.Exclude = exclude
 	}
+
 	return c, nil
 }
 

test/linters_test.go

@@ -85,7 +85,9 @@ func saveConfig(t *testing.T, cfg map[string]interface{}) (cfgPath string, finis
 
 	return cfgPath, func() {
 		assert.NoError(t, f.Close())
-		assert.NoError(t, os.Remove(cfgPath))
+		if os.Getenv("GL_KEEP_TEMP_FILES") != "1" {
+			assert.NoError(t, os.Remove(cfgPath))
+		}
 	}
 }
 
@@ -106,6 +108,8 @@ func testOneSource(t *testing.T, sourcePath string) {
 		p, finish := saveConfig(t, rc.config)
 		defer finish()
 		cfgPath = p
+	} else if rc.configPath != "" {
+		cfgPath = rc.configPath
 	}
 
 	for _, addArg := range []string{"", "-Etypecheck"} {
@@ -129,8 +133,9 @@ func testOneSource(t *testing.T, sourcePath string) {
 }
 
 type runContext struct {
-	args   []string
-	config map[string]interface{}
+	args       []string
+	config     map[string]interface{}
+	configPath string
 }
 
 func buildConfigFromShortRepr(t *testing.T, repr string, config map[string]interface{}) {
@@ -188,6 +193,15 @@ func extractRunContextFromComments(t *testing.T, sourcePath string) *runContext
 			buildConfigFromShortRepr(t, repr, rc.config)
 			continue
 		}
+
+		if strings.HasPrefix(line, "config_path: ") {
+			configPath := strings.TrimPrefix(line, "config_path: ")
+			assert.NotEmpty(t, configPath)
+			rc.configPath = configPath
+			continue
+		}
+
+		assert.Fail(t, "invalid prefix of comment line %s", line)
 	}
 
 	return rc

test/testdata/depguard.go

@@ -1,4 +1,6 @@
-//args: -Edepguard --depguard.include-go-root --depguard.packages='compress/*,log'
+//args: -Edepguard
+//config: linters-settings.depguard.include-go-root=true
+//config: linters-settings.depguard.packages=compress/*,log
 package testdata
 
 import (

test/testdata/dupl.go

@@ -1,4 +1,5 @@
-//args: -Edupl --dupl.threshold=20
+//args: -Edupl
+//config: linters-settings.dupl.threshold=20
 package testdata
 
 type DuplLogger struct{}
@@ -10,7 +11,7 @@ func (DuplLogger) level() int {
 func (DuplLogger) Debug(args ...interface{}) {}
 func (DuplLogger) Info(args ...interface{})  {}
 
-func (logger *DuplLogger) First(args ...interface{}) { // ERROR "13-22 lines are duplicate of `testdata/dupl.go:24-33`"
+func (logger *DuplLogger) First(args ...interface{}) { // ERROR "14-23 lines are duplicate of `testdata/dupl.go:25-34`"
 	if logger.level() >= 0 {
 		logger.Debug(args...)
 		logger.Debug(args...)
@@ -21,7 +22,7 @@ func (logger *DuplLogger) First(args ...interface{}) { // ERROR "13-22 lines are
 	}
 }
 
-func (logger *DuplLogger) Second(args ...interface{}) { // ERROR "24-33 lines are duplicate of `testdata/dupl.go:13-22`"
+func (logger *DuplLogger) Second(args ...interface{}) { // ERROR "25-34 lines are duplicate of `testdata/dupl.go:14-23`"
 	if logger.level() >= 1 {
 		logger.Info(args...)
 		logger.Info(args...)

test/testdata/errcheck/exclude.txt

@@ -0,0 +1 @@
+io/ioutil.ReadFile

test/testdata/errcheck/ignore_config.yml

@@ -0,0 +1,4 @@
+linters-settings:
+  errcheck:
+    check-blank: true
+    ignore: os:.*,io/ioutil:^ReadF.*

test/testdata/errcheck_exclude.go

@@ -0,0 +1,18 @@
+//args: -Eerrcheck
+//config: linters-settings.errcheck.check-blank=true
+//config: linters-settings.errcheck.exclude=testdata/errcheck/exclude.txt
+package testdata
+
+import (
+	"io/ioutil"
+)
+
+func TestErrcheckExclude() []byte {
+	ret, _ := ioutil.ReadFile("f.txt")
+	return ret
+}
+
+func TestErrcheckNoExclude() []byte {
+	ret, _ := ioutil.ReadAll(nil) // ERROR "Error return value of `ioutil.ReadAll` is not checked"
+	return ret
+}

test/testdata/errcheck_ignore.go

@@ -0,0 +1,28 @@
+//args: -Eerrcheck
+//config_path: testdata/errcheck/ignore_config.yml
+package testdata
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+)
+
+func TestErrcheckIgnoreOs() {
+	_, _ = os.Open("f.txt")
+}
+
+func TestErrcheckNoIgnoreFmt(s string) int {
+	n, _ := fmt.Println(s) // ERROR "Error return value of `fmt.Println` is not checked"
+	return n
+}
+
+func TestErrcheckIgnoreIoutil() []byte {
+	ret, _ := ioutil.ReadFile("f.txt")
+	return ret
+}
+
+func TestErrcheckNoIgnoreIoutil() []byte {
+	ret, _ := ioutil.ReadAll(nil) // ERROR "Error return value of `ioutil.ReadAll` is not checked"
+	return ret
+}