imports: special case rand.Read, prevent math/rand by chance

In Go 1.7, math/rand.Read was added. Previously, the only package
containing "rand.Read" was "crypto/rand".

goimports was updated to know that, and zstdlib.go contains a note
that it's ambiguous:

        "rand.Perm":        "math/rand",
        "rand.Prime":       "crypto/rand",
        "rand.Rand":        "math/rand",
        // "rand.Read" is ambiguous
        "rand.Reader":                   "crypto/rand",
        "rand.Seed":                     "math/rand",
        "rand.Source":                   "math/rand",

The intention originally was that such ambiguous things would
never be resolved, even randomly.

But a later change added support for build.Default.SrcDirs, which
meant GOROOT was also searched for ambiguous things. Or maybe I forget
the history.

In any case, when goimports tried to resolve "rand.Read", the
findImportStdlib check was returning nothing, which lead to the
$GOROOT being searched, where math/rand was picked by chance. That's a
dangerous default when the intentional might've been crypto/rand.

Special case it and prefer crypto/rand if there's no more specific
clue either way.

Change-Id: Ib5f8f297f72fa309d5ca9b15a37493df2e17567c
Reviewed-on: https://go-review.googlesource.com/24847
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>

imports/fix.go

@@ -369,6 +369,16 @@ func findImportGoPath(pkgName string, symbols map[string]bool, filename string)
 	if pkg, rename, ok := findImportStdlib(pkgName, symbols); ok {
 		return pkg, rename, nil
 	}
+	if pkgName == "rand" && symbols["Read"] {
+		// Special-case rand.Read.
+		//
+		// If findImportStdlib didn't find it above, don't go
+		// searching for it, lest it find and pick math/rand
+		// in GOROOT (new as of Go 1.6)
+		//
+		// crypto/rand is the safer choice.
+		return "", false, nil
+	}
 
 	// TODO(sameer): look at the import lines for other Go files in the
 	// local directory, since the user is likely to import the same packages
@@ -459,8 +469,12 @@ func (fn visitFn) Visit(node ast.Node) ast.Visitor {
 
 func findImportStdlib(shortPkg string, symbols map[string]bool) (importPath string, rename, ok bool) {
 	for symbol := range symbols {
-		path := stdlib[shortPkg+"."+symbol]
+		key := shortPkg + "." + symbol
+		path := stdlib[key]
 		if path == "" {
+			if key == "rand.Read" {
+				continue
+			}
 			return "", false, false
 		}
 		if importPath != "" && importPath != path {
@@ -469,5 +483,8 @@ func findImportStdlib(shortPkg string, symbols map[string]bool) (importPath stri
 		}
 		importPath = path
 	}
+	if importPath == "" && shortPkg == "rand" && symbols["Read"] {
+		return "crypto/rand", false, true
+	}
 	return importPath, false, importPath != ""
 }

imports/fix_test.go

@@ -1008,38 +1008,85 @@ type Buffer2 struct {}
 	}
 }
 
-func TestFindImportInternal(t *testing.T) {
+func withEmptyGoPath(fn func()) {
 	pkgIndexOnce = &sync.Once{}
 	oldGOPATH := build.Default.GOPATH
 	build.Default.GOPATH = ""
 	defer func() {
 		build.Default.GOPATH = oldGOPATH
 	}()
+	fn()
+}
 
-	// Check for src/internal/race, not just src/internal,
-	// so that we can run this test also against go1.5
-	// (which doesn't contain that file).
-	_, err := os.Stat(filepath.Join(runtime.GOROOT(), "src/internal/race"))
-	if err != nil {
-		t.Skip(err)
-	}
+func TestFindImportInternal(t *testing.T) {
+	withEmptyGoPath(func() {
+		// Check for src/internal/race, not just src/internal,
+		// so that we can run this test also against go1.5
+		// (which doesn't contain that file).
+		_, err := os.Stat(filepath.Join(runtime.GOROOT(), "src/internal/race"))
+		if err != nil {
+			t.Skip(err)
+		}
 
-	got, rename, err := findImportGoPath("race", map[string]bool{"Acquire": true}, filepath.Join(runtime.GOROOT(), "src/math/x.go"))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if got != "internal/race" || rename {
-		t.Errorf(`findImportGoPath("race", Acquire ...)=%q, %t, want "internal/race", false`, got, rename)
-	}
+		got, rename, err := findImportGoPath("race", map[string]bool{"Acquire": true}, filepath.Join(runtime.GOROOT(), "src/math/x.go"))
+		if err != nil {
+			t.Fatal(err)
+		}
+		if got != "internal/race" || rename {
+			t.Errorf(`findImportGoPath("race", Acquire ...)=%q, %t, want "internal/race", false`, got, rename)
+		}
 
-	// should not be able to use internal from outside that tree
-	got, rename, err = findImportGoPath("race", map[string]bool{"Acquire": true}, filepath.Join(runtime.GOROOT(), "x.go"))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if got != "" || rename {
-		t.Errorf(`findImportGoPath("race", Acquire ...)=%q, %t, want "", false`, got, rename)
-	}
+		// should not be able to use internal from outside that tree
+		got, rename, err = findImportGoPath("race", map[string]bool{"Acquire": true}, filepath.Join(runtime.GOROOT(), "x.go"))
+		if err != nil {
+			t.Fatal(err)
+		}
+		if got != "" || rename {
+			t.Errorf(`findImportGoPath("race", Acquire ...)=%q, %t, want "", false`, got, rename)
+		}
+	})
+}
+
+// rand.Read should prefer crypto/rand.Read, not math/rand.Read.
+func TestFindImportRandRead(t *testing.T) {
+	withEmptyGoPath(func() {
+		file := filepath.Join(runtime.GOROOT(), "src/foo/x.go") // dummy
+		tests := []struct {
+			syms []string
+			want string
+		}{
+			{
+				syms: []string{"Read"},
+				want: "crypto/rand",
+			},
+			{
+				syms: []string{"Read", "NewZipf"},
+				want: "math/rand",
+			},
+			{
+				syms: []string{"NewZipf"},
+				want: "math/rand",
+			},
+			{
+				syms: []string{"Read", "Prime"},
+				want: "crypto/rand",
+			},
+		}
+		for _, tt := range tests {
+			m := map[string]bool{}
+			for _, sym := range tt.syms {
+				m[sym] = true
+			}
+			got, _, err := findImportGoPath("rand", m, file)
+			if err != nil {
+				t.Errorf("for %q: %v", tt.syms, err)
+				continue
+			}
+			if got != tt.want {
+				t.Errorf("for %q, findImportGoPath = %q; want %q", tt.syms, got, tt.want)
+			}
+		}
+	})
 }
 
 func TestFindImportVendor(t *testing.T) {