From ccac35a87e3c5d7d19b0d1b3fbdc971e2fb7eb71 Mon Sep 17 00:00:00 2001
From: Denis Isaev <denis@golangci.com>
Date: Mon, 5 Nov 2018 14:00:48 +0300
Subject: [PATCH] Fix false positives with unused identifiers

Issue #265: don't report unused warning when an identifier is
used only in tests.
---
 .github/ISSUE_TEMPLATE.md                  |  5 +-
 pkg/lint/load.go                           | 73 ++++++++++++++++++----
 test/run_test.go                           |  6 ++
 test/testdata/used_only_in_tests/a.go      |  5 ++
 test/testdata/used_only_in_tests/a_test.go |  9 +++
 5 files changed, 84 insertions(+), 14 deletions(-)
 create mode 100644 test/testdata/used_only_in_tests/a.go
 create mode 100644 test/testdata/used_only_in_tests/a_test.go

diff --git a/.github/ISSUE_TEMPLATE.md b/.github/ISSUE_TEMPLATE.md
index f79ba401..a55080e0 100644
--- a/.github/ISSUE_TEMPLATE.md
+++ b/.github/ISSUE_TEMPLATE.md
@@ -2,5 +2,6 @@ Thank you for creating the issue!
 
 Please include the following information:
 1. Version of golangci-lint: `golangci-lint --version` (or git commit if you don't use binary distribution)
-2. Go environment: `go version && go env`
-3. Verbose output of running: `golangci-lint run -v`
\ No newline at end of file
+2. Config file: `cat .golangci.yml`
+3. Go environment: `go version && go env`
+4. Verbose output of running: `golangci-lint run -v`
\ No newline at end of file
diff --git a/pkg/lint/load.go b/pkg/lint/load.go
index 9bd136cd..91ae9f81 100644
--- a/pkg/lint/load.go
+++ b/pkg/lint/load.go
@@ -7,6 +7,7 @@ import (
 	"go/types"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"time"
 
@@ -25,18 +26,20 @@ import (
 )
 
 type ContextLoader struct {
-	cfg    *config.Config
-	log    logutils.Log
-	debugf logutils.DebugFunc
-	goenv  *goutil.Env
+	cfg         *config.Config
+	log         logutils.Log
+	debugf      logutils.DebugFunc
+	goenv       *goutil.Env
+	pkgTestIDRe *regexp.Regexp
 }
 
 func NewContextLoader(cfg *config.Config, log logutils.Log, goenv *goutil.Env) *ContextLoader {
 	return &ContextLoader{
-		cfg:    cfg,
-		log:    log,
-		debugf: logutils.Debug("loader"),
-		goenv:  goenv,
+		cfg:         cfg,
+		log:         log,
+		debugf:      logutils.Debug("loader"),
+		goenv:       goenv,
+		pkgTestIDRe: regexp.MustCompile(`^(.*) \[(.*)\.test\]`),
 	}
 }
 
@@ -222,19 +225,65 @@ func (cl ContextLoader) loadPackages(ctx context.Context, loadMode packages.Load
 			i, pkg.ID, pkg.GoFiles, pkg.CompiledGoFiles, syntaxFiles)
 	}
 
-	var retPkgs []*packages.Package
 	for _, pkg := range pkgs {
 		for _, err := range pkg.Errors {
 			if strings.Contains(err.Msg, "no Go files") {
 				return nil, errors.Wrapf(exitcodes.ErrNoGoFiles, "package %s", pkg.PkgPath)
 			}
 		}
-		if !shouldSkipPkg(pkg) {
-			retPkgs = append(retPkgs, pkg)
+	}
+
+	return cl.filterPackages(pkgs), nil
+}
+
+func (cl ContextLoader) tryParseTestPackage(pkg *packages.Package) (name, testName string, isTest bool) {
+	matches := cl.pkgTestIDRe.FindStringSubmatch(pkg.ID)
+	if matches == nil {
+		return "", "", false
+	}
+
+	return matches[1], matches[2], true
+}
+
+func (cl ContextLoader) filterPackages(pkgs []*packages.Package) []*packages.Package {
+	packagesWithTests := map[string]bool{}
+	for _, pkg := range pkgs {
+		name, testName, isTest := cl.tryParseTestPackage(pkg)
+		if !isTest {
+			continue
+		}
+		packagesWithTests[name] = true
+
+		if name != testName {
+			cl.log.Infof("pkg ID=%s: %s != %s: %#v", pkg.ID, name, testName, pkg)
 		}
 	}
 
-	return retPkgs, nil
+	cl.debugf("package with tests: %#v", packagesWithTests)
+
+	var retPkgs []*packages.Package
+	for _, pkg := range pkgs {
+		if shouldSkipPkg(pkg) {
+			cl.debugf("skip pkg ID=%s", pkg.ID)
+			continue
+		}
+
+		_, _, isTest := cl.tryParseTestPackage(pkg)
+		if !isTest && packagesWithTests[pkg.PkgPath] {
+			// If tests loading is enabled,
+			// for package with files a.go and a_test.go go/packages loads two packages:
+			// 1. ID=".../a" GoFiles=[a.go]
+			// 2. ID=".../a [.../a.test]" GoFiles=[a.go a_test.go]
+			// We need only the second package, otherwise we can get warnings about unused variables/fields/functions
+			// in a.go if they are used only in a_test.go.
+			cl.debugf("skip pkg ID=%s because we load it with test package", pkg.ID)
+			continue
+		}
+
+		retPkgs = append(retPkgs, pkg)
+	}
+
+	return retPkgs
 }
 
 //nolint:gocyclo
diff --git a/test/run_test.go b/test/run_test.go
index 18fa429a..4cb2371e 100644
--- a/test/run_test.go
+++ b/test/run_test.go
@@ -163,6 +163,12 @@ func TestDeadcodeNoFalsePositivesInMainPkg(t *testing.T) {
 	checkNoIssuesRun(t, out, exitCode)
 }
 
+func TestIdentifierUsedOnlyInTests(t *testing.T) {
+	out, exitCode := runGolangciLint(t, "--no-config", "--disable-all", "-Eunused",
+		filepath.Join(testdataDir, "used_only_in_tests"))
+	checkNoIssuesRun(t, out, exitCode)
+}
+
 func TestConfigFileIsDetected(t *testing.T) {
 	checkGotConfig := func(out string, exitCode int) {
 		assert.Equal(t, exitcodes.Success, exitCode, out)
diff --git a/test/testdata/used_only_in_tests/a.go b/test/testdata/used_only_in_tests/a.go
new file mode 100644
index 00000000..31d9a9f4
--- /dev/null
+++ b/test/testdata/used_only_in_tests/a.go
@@ -0,0 +1,5 @@
+package p
+
+func f() bool {
+	return true
+}
diff --git a/test/testdata/used_only_in_tests/a_test.go b/test/testdata/used_only_in_tests/a_test.go
new file mode 100644
index 00000000..69930f68
--- /dev/null
+++ b/test/testdata/used_only_in_tests/a_test.go
@@ -0,0 +1,9 @@
+package p
+
+import "testing"
+
+func TestF(t *testing.T) {
+	if !f() {
+		t.Fail()
+	}
+}