imports: don't remove imports that conflict with globals

When a package contains both a var and an import that produce the same
identifier, the compiler will complain. I had thought that it made sense
to remove the redundant imports in that case. However, we can't reliably
tell whether a global from one file is in scope in another file. Most
notably, having multiple main packages in the same directory is pretty
common, and if someone declares a var in one that matches an import in
another, we don't want to remove the import.

Change-Id: I49f58fccdb8a8542ec85cf4d80d3e20d3159d2c7
Reviewed-on: https://go-review.googlesource.com/c/154740
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

imports/fix.go

@@ -242,8 +242,8 @@ type pass struct {
 
 	// Intermediate state, generated by load.
 	existingImports map[string]*importInfo
-	missing         map[string]map[string]bool
-	used            map[*importInfo]bool
+	allRefs         map[string]map[string]bool
+	missingRefs     map[string]map[string]bool
 
 	// Inputs to fix. These can be augmented between successive fix calls.
 	lastTry       bool                    // indicates that this is the last call and fix should clean up as best it can.
@@ -308,16 +308,15 @@ func (p *pass) importIdentifier(imp *importInfo) string {
 }
 
 // load reads in everything necessary to run a pass, and reports whether the
-// file already has all the imports it needs. It fills in p.missing with the
+// file already has all the imports it needs. It fills in p.missingRefs with the
 // file's missing symbols, if any, or removes unused imports if not.
 func (p *pass) load() bool {
 	p.knownPackages = map[string]*packageInfo{}
-	p.missing = map[string]map[string]bool{}
-	p.used = map[*importInfo]bool{}
+	p.missingRefs = map[string]map[string]bool{}
 	p.existingImports = map[string]*importInfo{}
 
 	// Load basic information about the file in question.
-	allReferences := collectReferences(p.f)
+	p.allRefs = collectReferences(p.f)
 
 	// Load stuff from other files in the same package:
 	// global variables so we know they don't need resolving, and imports
@@ -340,19 +339,18 @@ func (p *pass) load() bool {
 		p.existingImports[p.importIdentifier(imp)] = imp
 	}
 
-	// Find missing references and mark used imports used.
-	for left, rights := range allReferences {
+	// Find missing references.
+	for left, rights := range p.allRefs {
 		if globals[left] {
 			continue
 		}
-		imp, ok := p.existingImports[left]
+		_, ok := p.existingImports[left]
 		if !ok {
-			p.missing[left] = rights
+			p.missingRefs[left] = rights
 			continue
 		}
-		p.used[imp] = true
 	}
-	if len(p.missing) != 0 {
+	if len(p.missingRefs) != 0 {
 		return false
 	}
 
@@ -365,19 +363,24 @@ func (p *pass) load() bool {
 func (p *pass) fix() bool {
 	// Find missing imports.
 	var selected []*importInfo
-	for left, rights := range p.missing {
+	for left, rights := range p.missingRefs {
 		if imp := p.findMissingImport(left, rights); imp != nil {
 			selected = append(selected, imp)
 		}
 	}
 
-	if !p.lastTry && len(selected) != len(p.missing) {
+	if !p.lastTry && len(selected) != len(p.missingRefs) {
 		return false
 	}
 
 	// Found everything, or giving up. Add the new imports and remove any unused.
 	for _, imp := range p.existingImports {
-		if !p.used[imp] {
+		// We deliberately ignore globals here, because we can't be sure
+		// they're in the same package. People do things like put multiple
+		// main packages in the same directory, and we don't want to
+		// remove imports if they happen to have the same name as a var in
+		// a different package.
+		if _, ok := p.allRefs[p.importIdentifier(imp)]; !ok {
 			astutil.DeleteNamedImport(p.fset, p.f, imp.name, imp.importPath)
 		}
 	}
@@ -484,7 +487,7 @@ func fixImportsDefault(fset *token.FileSet, f *ast.File, filename string) error
 
 	// Now we can try adding imports from the stdlib.
 	p.assumeSiblingImportsValid()
-	addStdlibCandidates(p, p.missing)
+	addStdlibCandidates(p, p.missingRefs)
 	if p.fix() {
 		return nil
 	}
@@ -502,7 +505,7 @@ func fixImportsDefault(fset *token.FileSet, f *ast.File, filename string) error
 		return nil
 	}
 
-	addStdlibCandidates(p, p.missing)
+	addStdlibCandidates(p, p.missingRefs)
 	p.assumeSiblingImportsValid()
 	if p.fix() {
 		return nil
@@ -510,7 +513,7 @@ func fixImportsDefault(fset *token.FileSet, f *ast.File, filename string) error
 
 	// Go look for candidates in $GOPATH, etc. We don't necessarily load
 	// the real exports of sibling imports, so keep assuming their contents.
-	if err := addExternalCandidates(p, p.missing, filename); err != nil {
+	if err := addExternalCandidates(p, p.missingRefs, filename); err != nil {
 		return err
 	}
 

imports/fix_test.go

@@ -1931,6 +1931,35 @@ var _ = fmt.Printf
 	}.processTest(t, "foo.com", "pkg/uses.go", nil, nil, want)
 }
 
+func TestGlobalImports_MultipleMains(t *testing.T) {
+	const declaresGlobal = `package main
+var fmt int
+`
+	const input = `package main
+import "fmt"
+var _, _ = fmt.Printf, bytes.Equal
+`
+	const want = `package main
+
+import (
+	"bytes"
+	"fmt"
+)
+
+var _, _ = fmt.Printf, bytes.Equal
+`
+
+	testConfig{
+		module: packagestest.Module{
+			Name: "foo.com",
+			Files: fm{
+				"pkg/main.go": declaresGlobal,
+				"pkg/uses.go": input,
+			},
+		},
+	}.processTest(t, "foo.com", "pkg/uses.go", nil, nil, want)
+}
+
 // Tests that sibling files - other files in the same package - can provide an
 // import that may not be the default one otherwise.
 func TestSiblingImports(t *testing.T) {