dev: refactor .golangci.yml configuration and fix up nolintlint issues (#4537)

Author: alexandear

.golangci.yml

@@ -17,13 +17,15 @@ linters-settings:
       logger:
         deny:
           # logging is allowed only by logutils.Log,
-          # logrus is allowed to use only in logutils package.
           - pkg: "github.com/sirupsen/logrus"
             desc: logging is allowed only by logutils.Log.
           - pkg: "github.com/pkg/errors"
             desc: Should be replaced by standard lib errors package.
           - pkg: "github.com/instana/testify"
             desc: It's a fork of github.com/stretchr/testify.
+        files:
+          # logrus is allowed to use only in logutils package.
+          - "!**/pkg/logutils/**.go"
   dupl:
     threshold: 100
   funlen:
@@ -86,10 +88,12 @@ linters-settings:
     line-length: 140
   misspell:
     locale: US
+    ignore-words:
+      - "importas" # linter name
   nolintlint:
     allow-unused: false # report any unused nolint directives
-    require-explanation: false # don't require an explanation for nolint directives
-    require-specific: false # don't require nolint directives to be specific about which linter is being skipped
+    require-explanation: true # require an explanation for nolint directives
+    require-specific: true # require nolint directives to be specific about which linter is being skipped
   revive:
     rules:
       - name: unexported-return
@@ -144,7 +148,9 @@ issues:
   exclude-rules:
     - path: _test\.go
       linters:
+        - dupl
         - gomnd
+        - lll
 
     - path: pkg/golinters/errcheck.go
       linters: [staticcheck]
@@ -158,6 +164,10 @@ issues:
     - path: pkg/golinters/govet.go
       text: "SA1019: cfg.CheckShadowing is deprecated: the linter should be enabled inside `Enable`."
 
+    - path: pkg/golinters
+      linters:
+        - dupl
+
     - path: pkg/golinters/gofumpt.go
       linters: [staticcheck]
       text: "SA1019: settings.LangVersion is deprecated: use the global `run.go` instead."

pkg/commands/flagsets.go

@@ -80,7 +80,7 @@ func setupOutputFlagSet(v *viper.Viper, fs *pflag.FlagSet) {
 	internal.AddFlagAndBind(v, fs, fs.Bool, "show-stats", "output.show-stats", false, color.GreenString("Show statistics per linter"))
 }
 
-//nolint:gomnd
+//nolint:gomnd // magic numbers here is ok
 func setupIssuesFlagSet(v *viper.Viper, fs *pflag.FlagSet) {
 	internal.AddHackedStringSliceP(fs, "exclude", "e", color.GreenString("Exclude issue by regexp"))
 	internal.AddFlagAndBind(v, fs, fs.Bool, "exclude-use-default", "issues.exclude-use-default", true,

pkg/golinters/dogsled.go

@@ -16,7 +16,6 @@ import (
 
 const dogsledName = "dogsled"
 
-//nolint:dupl
 func NewDogsled(settings *config.DogsledSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/dupl.go

@@ -17,7 +17,6 @@ import (
 
 const duplName = "dupl"
 
-//nolint:dupl
 func NewDupl(settings *config.DuplSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/forbidigo.go

@@ -16,7 +16,6 @@ import (
 
 const forbidigoName = "forbidigo"
 
-//nolint:dupl
 func NewForbidigo(settings *config.ForbidigoSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/funlen.go

@@ -16,7 +16,6 @@ import (
 
 const funlenName = "funlen"
 
-//nolint:dupl
 func NewFunlen(settings *config.FunlenSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/goanalysis/runner.go

@@ -177,10 +177,9 @@ func (r *runner) buildActionFactDeps(act *action, a *analysis.Analyzer, pkg *pac
 	}
 }
 
-//nolint:gocritic
 func (r *runner) prepareAnalysis(pkgs []*packages.Package,
 	analyzers []*analysis.Analyzer,
-) (map[*packages.Package]bool, []*action, []*action) {
+) (initialPkgs map[*packages.Package]bool, allActions, roots []*action) {
 	// Construct the action graph.
 
 	// Each graph node (action) is one unit of analysis.
@@ -200,13 +199,13 @@ func (r *runner) prepareAnalysis(pkgs []*packages.Package,
 	actions := make(map[actKey]*action, totalActionsCount)
 	actAlloc := newActionAllocator(totalActionsCount)
 
-	initialPkgs := make(map[*packages.Package]bool, len(pkgs))
+	initialPkgs = make(map[*packages.Package]bool, len(pkgs))
 	for _, pkg := range pkgs {
 		initialPkgs[pkg] = true
 	}
 
 	// Build nodes for initial packages.
-	roots := make([]*action, 0, len(pkgs)*len(analyzers))
+	roots = make([]*action, 0, len(pkgs)*len(analyzers))
 	for _, a := range analyzers {
 		for _, pkg := range pkgs {
 			root := r.makeAction(a, pkg, initialPkgs, actions, actAlloc)
@@ -215,7 +214,7 @@ func (r *runner) prepareAnalysis(pkgs []*packages.Package,
 		}
 	}
 
-	allActions := maps.Values(actions)
+	allActions = maps.Values(actions)
 
 	debugf("Built %d actions", len(actions))
 
@@ -281,7 +280,6 @@ func (r *runner) analyze(pkgs []*packages.Package, analyzers []*analysis.Analyze
 	return rootActions
 }
 
-//nolint:nakedret
 func extractDiagnostics(roots []*action) (retDiags []Diagnostic, retErrors []error) {
 	extracted := make(map[*action]bool)
 	var extract func(*action)
@@ -338,5 +336,5 @@ func extractDiagnostics(roots []*action) (retDiags []Diagnostic, retErrors []err
 		}
 	}
 	visitAll(roots)
-	return
+	return retDiags, retErrors
 }

pkg/golinters/goanalysis/runners.go

@@ -184,10 +184,9 @@ func saveIssuesToCache(allPkgs []*packages.Package, pkgsFromCache map[*packages.
 	issuesCacheDebugf("Saved %d issues from %d packages to cache in %s", savedIssuesCount, len(allPkgs), time.Since(startedAt))
 }
 
-//nolint:gocritic
 func loadIssuesFromCache(pkgs []*packages.Package, lintCtx *linter.Context,
 	analyzers []*analysis.Analyzer,
-) ([]result.Issue, map[*packages.Package]bool) {
+) (issuesFromCache []result.Issue, pkgsFromCache map[*packages.Package]bool) {
 	startedAt := time.Now()
 
 	lintResKey := getIssuesCacheKey(analyzers)
@@ -221,17 +220,18 @@ func loadIssuesFromCache(pkgs []*packages.Package, lintCtx *linter.Context,
 				}
 
 				issues := make([]result.Issue, 0, len(pkgIssues))
-				for _, i := range pkgIssues {
+				for i := range pkgIssues {
+					issue := &pkgIssues[i]
 					issues = append(issues, result.Issue{
-						FromLinter:           i.FromLinter,
-						Text:                 i.Text,
-						Severity:             i.Severity,
-						Pos:                  i.Pos,
-						LineRange:            i.LineRange,
-						Replacement:          i.Replacement,
+						FromLinter:           issue.FromLinter,
+						Text:                 issue.Text,
+						Severity:             issue.Severity,
+						Pos:                  issue.Pos,
+						LineRange:            issue.LineRange,
+						Replacement:          issue.Replacement,
 						Pkg:                  pkg,
-						ExpectNoLint:         i.ExpectNoLint,
-						ExpectedNoLintLinter: i.ExpectedNoLintLinter,
+						ExpectNoLint:         issue.ExpectNoLint,
+						ExpectedNoLintLinter: issue.ExpectedNoLintLinter,
 					})
 				}
 				cacheRes.issues = issues
@@ -246,21 +246,20 @@ func loadIssuesFromCache(pkgs []*packages.Package, lintCtx *linter.Context,
 	wg.Wait()
 
 	loadedIssuesCount := 0
-	var issues []result.Issue
-	pkgsFromCache := map[*packages.Package]bool{}
+	pkgsFromCache = map[*packages.Package]bool{}
 	for pkg, cacheRes := range pkgToCacheRes {
 		if cacheRes.loadErr == nil {
 			loadedIssuesCount += len(cacheRes.issues)
 			pkgsFromCache[pkg] = true
-			issues = append(issues, cacheRes.issues...)
+			issuesFromCache = append(issuesFromCache, cacheRes.issues...)
 			issuesCacheDebugf("Loaded package %s issues (%d) from cache", pkg, len(cacheRes.issues))
 		} else {
 			issuesCacheDebugf("Didn't load package %s issues from cache: %s", pkg, cacheRes.loadErr)
 		}
 	}
 	issuesCacheDebugf("Loaded %d issues from cache in %s, analyzing %d/%d packages",
 		loadedIssuesCount, time.Since(startedAt), len(pkgs)-len(pkgsFromCache), len(pkgs))
-	return issues, pkgsFromCache
+	return issuesFromCache, pkgsFromCache
 }
 
 func analyzersHashID(analyzers []*analysis.Analyzer) string {

pkg/golinters/gocognit.go

@@ -16,7 +16,6 @@ import (
 
 const gocognitName = "gocognit"
 
-//nolint:dupl
 func NewGocognit(settings *config.GocognitSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/goconst.go

@@ -15,7 +15,6 @@ import (
 
 const goconstName = "goconst"
 
-//nolint:dupl
 func NewGoconst(settings *config.GoConstSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/gocyclo.go

@@ -15,7 +15,6 @@ import (
 
 const gocycloName = "gocyclo"
 
-//nolint:dupl
 func NewGocyclo(settings *config.GoCycloSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/godox.go

@@ -16,7 +16,6 @@ import (
 
 const godoxName = "godox"
 
-//nolint:dupl
 func NewGodox(settings *config.GodoxSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/gofmt_common.go

@@ -133,8 +133,8 @@ func (p *hunkChangesParser) handleDeletedLines(deletedLines []diffLine, addedLin
 	}
 
 	if len(addedLines) != 0 {
-		//nolint:gocritic
-		change.Replacement.NewLines = append(p.replacementLinesToPrepend, addedLines...)
+		change.Replacement.NewLines = append([]string{}, p.replacementLinesToPrepend...)
+		change.Replacement.NewLines = append(change.Replacement.NewLines, addedLines...)
 		if len(p.replacementLinesToPrepend) != 0 {
 			p.replacementLinesToPrepend = nil
 		}

pkg/golinters/importas.go

@@ -5,7 +5,7 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/julz/importas" //nolint:misspell
+	"github.com/julz/importas"
 	"golang.org/x/tools/go/analysis"
 
 	"github.com/golangci/golangci-lint/pkg/config"
@@ -26,7 +26,7 @@ func NewImportAs(settings *config.ImportAsSettings) *goanalysis.Linter {
 			return
 		}
 		if len(settings.Alias) == 0 {
-			lintCtx.Log.Infof("importas settings found, but no aliases listed. List aliases under alias: key.") //nolint:misspell
+			lintCtx.Log.Infof("importas settings found, but no aliases listed. List aliases under alias: key.")
 		}
 
 		if err := analyzer.Flags.Set("no-unaliased", strconv.FormatBool(settings.NoUnaliased)); err != nil {

pkg/golinters/lll.go

@@ -22,7 +22,6 @@ const lllName = "lll"
 
 const goCommentDirectivePrefix = "//go:"
 
-//nolint:dupl
 func NewLLL(settings *config.LllSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/makezero.go

@@ -15,7 +15,6 @@ import (
 
 const makezeroName = "makezero"
 
-//nolint:dupl
 func NewMakezero(settings *config.MakezeroSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/nestif.go

@@ -15,7 +15,6 @@ import (
 
 const nestifName = "nestif"
 
-//nolint:dupl
 func NewNestif(settings *config.NestifSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/nolintlint.go

@@ -16,7 +16,6 @@ import (
 
 const NoLintLintName = "nolintlint"
 
-//nolint:dupl
 func NewNoLintLint(settings *config.NoLintLintSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/nolintlint/nolintlint.go

@@ -156,7 +156,7 @@ var (
 	trailingBlankExplanation = regexp.MustCompile(`\s*(//\s*)?$`)
 )
 
-//nolint:funlen,gocyclo
+//nolint:funlen,gocyclo // the function is going to be refactored in the future
 func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
 	var issues []Issue
 

pkg/golinters/nolintlint/nolintlint_test.go

@@ -131,7 +131,7 @@ func foo() {
   good() //nolint: linter1, linter2
 }`,
 			expected: []issueWithReplacement{
-				{issue: "directive `//nolint:linter1 linter2` should match `//nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9"}, //nolint:lll // this is a string
+				{issue: "directive `//nolint:linter1 linter2` should match `//nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9"},
 			},
 		},
 		{

pkg/golinters/prealloc.go

@@ -15,7 +15,6 @@ import (
 
 const preallocName = "prealloc"
 
-//nolint:dupl
 func NewPreAlloc(settings *config.PreallocSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/golinters/unconvert.go

@@ -14,7 +14,6 @@ import (
 
 const unconvertName = "unconvert"
 
-//nolint:dupl // This is not a duplicate of dogsled.
 func NewUnconvert(settings *config.UnconvertSettings) *goanalysis.Linter {
 	var mu sync.Mutex
 	var resIssues []goanalysis.Issue

pkg/logutils/stderr_log.go

@@ -5,7 +5,7 @@ import (
 	"os"
 	"time"
 
-	"github.com/sirupsen/logrus" //nolint:depguard
+	"github.com/sirupsen/logrus"
 
 	"github.com/golangci/golangci-lint/pkg/exitcodes"
 )

pkg/packages/util_test.go

@@ -6,7 +6,6 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-//nolint:lll
 func Test_stackCrusher(t *testing.T) {
 	testCases := []struct {
 		desc     string

pkg/printers/checkstyle_test.go

@@ -49,7 +49,6 @@ func TestCheckstyle_Print(t *testing.T) {
 	err := printer.Print(issues)
 	require.NoError(t, err)
 
-	//nolint:lll
 	expected := "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n\n<checkstyle version=\"5.0\">\n  <file name=\"path/to/filea.go\">\n    <error column=\"4\" line=\"10\" message=\"some issue\" severity=\"warning\" source=\"linter-a\"></error>\n  </file>\n  <file name=\"path/to/fileb.go\">\n    <error column=\"9\" line=\"300\" message=\"another issue\" severity=\"error\" source=\"linter-b\"></error>\n  </file>\n</checkstyle>\n"
 
 	assert.Equal(t, expected, strings.ReplaceAll(buf.String(), "\r", ""))

pkg/printers/codeclimate_test.go

@@ -63,7 +63,6 @@ func TestCodeClimate_Print(t *testing.T) {
 	err := printer.Print(issues)
 	require.NoError(t, err)
 
-	//nolint:lll
 	expected := `[{"description":"linter-a: some issue","severity":"warning","fingerprint":"BA73C5DF4A6FD8462FFF1D3140235777","location":{"path":"path/to/filea.go","lines":{"begin":10}}},{"description":"linter-b: another issue","severity":"error","fingerprint":"0777B4FE60242BD8B2E9B7E92C4B9521","location":{"path":"path/to/fileb.go","lines":{"begin":300}}},{"description":"linter-c: issue c","severity":"critical","fingerprint":"BEE6E9FBB6BFA4B7DB9FB036697FB036","location":{"path":"path/to/filec.go","lines":{"begin":200}}}]
 `
 

pkg/printers/github_test.go

@@ -1,4 +1,3 @@
-//nolint:dupl
 package printers
 
 import (

pkg/printers/html_test.go

@@ -11,7 +11,6 @@ import (
 	"github.com/golangci/golangci-lint/pkg/result"
 )
 
-//nolint:lll
 const expectedHTML = `<!doctype html>
 <html lang="en">
 <head>

pkg/printers/json_test.go

@@ -49,7 +49,6 @@ func TestJSON_Print(t *testing.T) {
 	err := printer.Print(issues)
 	require.NoError(t, err)
 
-	//nolint:lll
 	expected := `{"Issues":[{"FromLinter":"linter-a","Text":"some issue","Severity":"warning","SourceLines":null,"Replacement":null,"Pos":{"Filename":"path/to/filea.go","Offset":2,"Line":10,"Column":4},"ExpectNoLint":false,"ExpectedNoLintLinter":""},{"FromLinter":"linter-b","Text":"another issue","Severity":"error","SourceLines":["func foo() {","\tfmt.Println(\"bar\")","}"],"Replacement":null,"Pos":{"Filename":"path/to/fileb.go","Offset":5,"Line":300,"Column":9},"ExpectNoLint":false,"ExpectedNoLintLinter":""}],"Report":null}
 `
 

pkg/printers/junitxml_test.go

@@ -1,4 +1,3 @@
-//nolint:dupl
 package printers
 
 import (