From 5cb1c80a83ef2a98bbc7b2fba1305239bb684fbe Mon Sep 17 00:00:00 2001
From: Dmitri Shuralyov <shurcooL@gmail.com>
Date: Sat, 4 Jun 2016 23:20:26 -0700
Subject: [PATCH] cmd/bundle: Support renaming imports.

Document a risk of generating invalid code due to shadowing between
identifiers in code files and the imported package names.

This risk was present before for any package with more than 1 .go file,
but it's increased when some files have renamed imports (since they're
typically used to resolve shadowing conflicts).

Resolves TODO(adonovan): support renaming imports.

Change-Id: Ie0e702345790fd2059c229623fb99fe645d688a4
Reviewed-on: https://go-review.googlesource.com/23785
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
---
 cmd/bundle/main.go                   | 70 +++++++++++++++++-----------
 cmd/bundle/main_test.go              |  1 +
 cmd/bundle/testdata/out.golden       | 10 ++++
 cmd/bundle/testdata/src/initial/c.go | 12 +++++
 4 files changed, 66 insertions(+), 27 deletions(-)
 create mode 100644 cmd/bundle/testdata/src/initial/c.go

diff --git a/cmd/bundle/main.go b/cmd/bundle/main.go
index 7cd31c98b6..b53ced4d02 100644
--- a/cmd/bundle/main.go
+++ b/cmd/bundle/main.go
@@ -96,6 +96,7 @@ import (
 	"log"
 	"os"
 	"path"
+	"strconv"
 	"strings"
 
 	"golang.org/x/tools/go/loader"
@@ -257,46 +258,61 @@ func bundle(src, dst, dstpkg, prefix string) ([]byte, error) {
 
 	fmt.Fprintf(&out, "package %s\n\n", dstpkg)
 
-	// Print a single declaration that imports all necessary packages.
-	// TODO(adonovan):
-	// - support renaming imports.
+	// BUG(adonovan,shurcooL): bundle may generate incorrect code
+	// due to shadowing between identifiers and imported package names.
+	//
+	// The generated code will either fail to compile or
+	// (unlikely) compile successfully but have different behavior
+	// than the original package. The risk of this happening is higher
+	// when the original package has renamed imports (they're typically
+	// renamed in order to resolve a shadow inside that particular .go file).
+
+	// TODO(adonovan,shurcooL):
+	// - detect shadowing issues, and either return error or resolve them
 	// - preserve comments from the original import declarations.
+
+	// pkgStd and pkgExt are sets of printed import specs. This is done
+	// to deduplicate instances of the same import name and path.
+	var pkgStd = make(map[string]bool)
+	var pkgExt = make(map[string]bool)
 	for _, f := range info.Files {
 		for _, imp := range f.Imports {
+			path, err := strconv.Unquote(imp.Path.Value)
+			if err != nil {
+				log.Fatalf("invalid import path string: %v", err) // Shouldn't happen here since conf.Load succeeded.
+			}
+			if path == dst {
+				continue
+			}
+			if newPath, ok := importMap[path]; ok {
+				path = newPath
+			}
+
+			var name string
 			if imp.Name != nil {
-				return nil, fmt.Errorf("%s: renaming imports not supported",
-					lprog.Fset.Position(imp.Pos()))
+				name = imp.Name.Name
+			}
+			spec := fmt.Sprintf("%s %q", name, path)
+			if isStandardImportPath(path) {
+				pkgStd[spec] = true
+			} else {
+				pkgExt[spec] = true
 			}
 		}
 	}
 
-	var pkgStd, pkgExt []string
-	for _, p := range info.Pkg.Imports() {
-		if p.Path() == dst {
-			continue
-		}
-		x, ok := importMap[p.Path()]
-		if !ok {
-			x = p.Path()
-		}
-		if isStandardImportPath(x) {
-			pkgStd = append(pkgStd, x)
-		} else {
-			pkgExt = append(pkgExt, x)
-		}
-	}
-
+	// Print a single declaration that imports all necessary packages.
 	fmt.Fprintln(&out, "import (")
-	for _, p := range pkgStd {
-		fmt.Fprintf(&out, "\t%q\n", p)
+	for p := range pkgStd {
+		fmt.Fprintf(&out, "\t%s\n", p)
 	}
 	if len(pkgExt) > 0 {
 		fmt.Fprintln(&out)
-		for _, p := range pkgExt {
-			fmt.Fprintf(&out, "\t%q\n", p)
-		}
 	}
-	fmt.Fprintln(&out, ")\n")
+	for p := range pkgExt {
+		fmt.Fprintf(&out, "\t%s\n", p)
+	}
+	fmt.Fprint(&out, ")\n\n")
 
 	// Modify and print each file.
 	for _, f := range info.Files {
diff --git a/cmd/bundle/main_test.go b/cmd/bundle/main_test.go
index 96c8cf707d..4db8aa7776 100644
--- a/cmd/bundle/main_test.go
+++ b/cmd/bundle/main_test.go
@@ -28,6 +28,7 @@ func TestBundle(t *testing.T) {
 		"initial": {
 			"a.go": load("testdata/src/initial/a.go"),
 			"b.go": load("testdata/src/initial/b.go"),
+			"c.go": load("testdata/src/initial/c.go"),
 		},
 		"domain.name/importdecl": {
 			"p.go": load("testdata/src/domain.name/importdecl/p.go"),
diff --git a/cmd/bundle/testdata/out.golden b/cmd/bundle/testdata/out.golden
index 59f98fe9d6..0ca96c9d59 100644
--- a/cmd/bundle/testdata/out.golden
+++ b/cmd/bundle/testdata/out.golden
@@ -8,6 +8,10 @@ package dest
 
 import (
 	"fmt"
+	. "fmt"
+	_ "fmt"
+	renamedfmt "fmt"
+	renamedfmt2 "fmt"
 
 	"domain.name/importdecl"
 )
@@ -31,3 +35,9 @@ const prefixc = 1
 func prefixfoo() {
 	fmt.Println(importdecl.F())
 }
+
+func prefixbaz() {
+	renamedfmt.Println()
+	renamedfmt2.Println()
+	Println()
+}
diff --git a/cmd/bundle/testdata/src/initial/c.go b/cmd/bundle/testdata/src/initial/c.go
new file mode 100644
index 0000000000..6a9cbc6914
--- /dev/null
+++ b/cmd/bundle/testdata/src/initial/c.go
@@ -0,0 +1,12 @@
+package initial
+
+import _ "fmt"
+import renamedfmt "fmt"
+import renamedfmt2 "fmt"
+import . "fmt"
+
+func baz() {
+	renamedfmt.Println()
+	renamedfmt2.Println()
+	Println()
+}