From d4971274fe382404aee0e8c163af974f2bf738e6 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 18 Dec 2018 14:53:56 -0500 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- imports/fix.go | 39 +++++++++++++++++++++------------------ imports/fix_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/imports/fix.go b/imports/fix.go index 5a2ac120..8267f087 100644 --- a/imports/fix.go +++ b/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 } diff --git a/imports/fix_test.go b/imports/fix_test.go index 04035ae2..ef80384c 100644 --- a/imports/fix_test.go +++ b/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) {