cmd/go: when resolving packages, try all module paths before falling back to the next proxy

Since we're mucking with error-propagation in modload.Query* anyway,
simplify the classification logic. Ensure that “module not found”
errors are reported as such in various places, since non-“not found”
errors terminate the module search.

Fixes #31785

Change-Id: Ie3ca5f4eec10a5f2a6037ec7e1c2cf47bd37a232
Reviewed-on: https://go-review.googlesource.com/c/go/+/177958
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>

src/cmd/go/internal/modconv/convert_test.go

@@ -28,7 +28,7 @@ func TestMain(m *testing.M) {
 }
 
 func testMain(m *testing.M) int {
-	modfetch.SetProxy("direct")
+	cfg.GOPROXY = "direct"
 
 	if _, err := exec.LookPath("git"); err != nil {
 		fmt.Fprintln(os.Stderr, "skipping because git binary not found")

src/cmd/go/internal/modfetch/cache.go

@@ -258,12 +258,12 @@ func (r *cachingRepo) Zip(dst io.Writer, version string) error {
 // Stat is like Lookup(path).Stat(rev) but avoids the
 // repository path resolution in Lookup if the result is
 // already cached on local disk.
-func Stat(path, rev string) (*RevInfo, error) {
+func Stat(proxy, path, rev string) (*RevInfo, error) {
 	_, info, err := readDiskStat(path, rev)
 	if err == nil {
 		return info, nil
 	}
-	repo, err := Lookup(path)
+	repo, err := Lookup(proxy, path)
 	if err != nil {
 		return nil, err
 	}
@@ -276,9 +276,22 @@ func InfoFile(path, version string) (string, error) {
 	if !semver.IsValid(version) {
 		return "", fmt.Errorf("invalid version %q", version)
 	}
-	if _, err := Stat(path, version); err != nil {
+
+	if file, _, err := readDiskStat(path, version); err == nil {
+		return file, nil
+	}
+
+	err := TryProxies(func(proxy string) error {
+		repo, err := Lookup(proxy, path)
+		if err == nil {
+			_, err = repo.Stat(version)
+		}
+		return err
+	})
+	if err != nil {
 		return "", err
 	}
+
 	// Stat should have populated the disk cache for us.
 	file, _, err := readDiskStat(path, version)
 	if err != nil {
@@ -294,21 +307,39 @@ func GoMod(path, rev string) ([]byte, error) {
 	// Convert commit hash to pseudo-version
 	// to increase cache hit rate.
 	if !semver.IsValid(rev) {
-		info, err := Stat(path, rev)
-		if err != nil {
-			return nil, err
+		if _, info, err := readDiskStat(path, rev); err == nil {
+			rev = info.Version
+		} else {
+			err := TryProxies(func(proxy string) error {
+				repo, err := Lookup(proxy, path)
+				if err != nil {
+					return err
+				}
+				info, err := repo.Stat(rev)
+				if err == nil {
+					rev = info.Version
+				}
+				return err
+			})
+			if err != nil {
+				return nil, err
+			}
 		}
-		rev = info.Version
 	}
+
 	_, data, err := readDiskGoMod(path, rev)
 	if err == nil {
 		return data, nil
 	}
-	repo, err := Lookup(path)
-	if err != nil {
-		return nil, err
-	}
-	return repo.GoMod(rev)
+
+	err = TryProxies(func(proxy string) error {
+		repo, err := Lookup(proxy, path)
+		if err == nil {
+			data, err = repo.GoMod(rev)
+		}
+		return err
+	})
+	return data, err
 }
 
 // GoModFile is like GoMod but returns the name of the file containing

src/cmd/go/internal/modfetch/coderepo_test.go

@@ -25,7 +25,7 @@ func TestMain(m *testing.M) {
 }
 
 func testMain(m *testing.M) int {
-	SetProxy("direct")
+	cfg.GOPROXY = "direct"
 
 	// The sum database is populated using a released version of the go command,
 	// but this test may include fixes for additional modules that previously
@@ -360,7 +360,7 @@ func TestCodeRepo(t *testing.T) {
 				return func(t *testing.T) {
 					t.Parallel()
 
-					repo, err := Lookup(tt.path)
+					repo, err := Lookup("direct", tt.path)
 					if tt.lookerr != "" {
 						if err != nil && err.Error() == tt.lookerr {
 							return
@@ -561,7 +561,7 @@ func TestCodeRepoVersions(t *testing.T) {
 				tt := tt
 				t.Parallel()
 
-				repo, err := Lookup(tt.path)
+				repo, err := Lookup("direct", tt.path)
 				if err != nil {
 					t.Fatalf("Lookup(%q): %v", tt.path, err)
 				}
@@ -616,7 +616,7 @@ func TestLatest(t *testing.T) {
 				tt := tt
 				t.Parallel()
 
-				repo, err := Lookup(tt.path)
+				repo, err := Lookup("direct", tt.path)
 				if err != nil {
 					t.Fatalf("Lookup(%q): %v", tt.path, err)
 				}

src/cmd/go/internal/modfetch/fetch.go

@@ -205,13 +205,16 @@ func downloadZip(mod module.Version, zipfile string) (err error) {
 		}
 	}()
 
-	repo, err := Lookup(mod.Path)
+	err = TryProxies(func(proxy string) error {
+		repo, err := Lookup(proxy, mod.Path)
+		if err != nil {
+			return err
+		}
+		return repo.Zip(f, mod.Version)
+	})
 	if err != nil {
 		return err
 	}
-	if err := repo.Zip(f, mod.Version); err != nil {
-		return err
-	}
 
 	// Double-check that the paths within the zip file are well-formed.
 	//

src/cmd/go/internal/modfetch/proxy.go

@@ -84,16 +84,6 @@ cached module versions with GOPROXY=https://example.com/proxy.
 `,
 }
 
-var proxyURL = cfg.Getenv("GOPROXY")
-
-// SetProxy sets the proxy to use when fetching modules.
-// It accepts the same syntax as the GOPROXY environment variable,
-// which also provides its default configuration.
-// SetProxy must not be called after the first module fetch has begun.
-func SetProxy(url string) {
-	proxyURL = url
-}
-
 var proxyOnce struct {
 	sync.Once
 	list []string
@@ -102,13 +92,25 @@ var proxyOnce struct {
 
 func proxyURLs() ([]string, error) {
 	proxyOnce.Do(func() {
-		for _, proxyURL := range strings.Split(proxyURL, ",") {
+		if cfg.GONOPROXY != "" && cfg.GOPROXY != "direct" {
+			proxyOnce.list = append(proxyOnce.list, "noproxy")
+		}
+		for _, proxyURL := range strings.Split(cfg.GOPROXY, ",") {
+			proxyURL = strings.TrimSpace(proxyURL)
 			if proxyURL == "" {
 				continue
 			}
+			if proxyURL == "off" {
+				// "off" always fails hard, so can stop walking list.
+				proxyOnce.list = append(proxyOnce.list, "off")
+				break
+			}
 			if proxyURL == "direct" {
 				proxyOnce.list = append(proxyOnce.list, "direct")
-				continue
+				// For now, "direct" is the end of the line. We may decide to add some
+				// sort of fallback behavior for them in the future, so ignore
+				// subsequent entries for forward-compatibility.
+				break
 			}
 
 			// Check that newProxyRepo accepts the URL.
@@ -125,32 +127,30 @@ func proxyURLs() ([]string, error) {
 	return proxyOnce.list, proxyOnce.err
 }
 
-func lookupProxy(path string) (Repo, error) {
-	list, err := proxyURLs()
+// TryProxies iterates f over each configured proxy (including "noproxy" and
+// "direct" if applicable) until f returns an error that is not
+// equivalent to os.ErrNotExist.
+//
+// TryProxies then returns that final error.
+//
+// If GOPROXY is set to "off", TryProxies invokes f once with the argument
+// "off".
+func TryProxies(f func(proxy string) error) error {
+	proxies, err := proxyURLs()
 	if err != nil {
-		return nil, err
+		return err
+	}
+	if len(proxies) == 0 {
+		return f("off")
 	}
 
-	var repos listRepo
-	for _, u := range list {
-		var r Repo
-		if u == "direct" {
-			// lookupDirect does actual network traffic.
-			// Especially if GOPROXY="http://mainproxy,direct",
-			// avoid the network until we need it by using a lazyRepo wrapper.
-			r = &lazyRepo{setup: lookupDirect, path: path}
-		} else {
-			// The URL itself was checked in proxyURLs.
-			// The only possible error here is a bad path,
-			// so we can return it unconditionally.
-			r, err = newProxyRepo(u, path)
-			if err != nil {
-				return nil, err
-			}
+	for _, proxy := range proxies {
+		err = f(proxy)
+		if !errors.Is(err, os.ErrNotExist) {
+			break
 		}
-		repos = append(repos, r)
 	}
-	return repos, nil
+	return err
 }
 
 type proxyRepo struct {
@@ -342,117 +342,3 @@ func (p *proxyRepo) Zip(dst io.Writer, version string) error {
 func pathEscape(s string) string {
 	return strings.ReplaceAll(url.PathEscape(s), "%2F", "/")
 }
-
-// A lazyRepo is a lazily-initialized Repo,
-// constructed on demand by calling setup.
-type lazyRepo struct {
-	path  string
-	setup func(string) (Repo, error)
-	once  sync.Once
-	repo  Repo
-	err   error
-}
-
-func (r *lazyRepo) init() {
-	r.repo, r.err = r.setup(r.path)
-}
-
-func (r *lazyRepo) ModulePath() string {
-	return r.path
-}
-
-func (r *lazyRepo) Versions(prefix string) ([]string, error) {
-	if r.once.Do(r.init); r.err != nil {
-		return nil, r.err
-	}
-	return r.repo.Versions(prefix)
-}
-
-func (r *lazyRepo) Stat(rev string) (*RevInfo, error) {
-	if r.once.Do(r.init); r.err != nil {
-		return nil, r.err
-	}
-	return r.repo.Stat(rev)
-}
-
-func (r *lazyRepo) Latest() (*RevInfo, error) {
-	if r.once.Do(r.init); r.err != nil {
-		return nil, r.err
-	}
-	return r.repo.Latest()
-}
-
-func (r *lazyRepo) GoMod(version string) ([]byte, error) {
-	if r.once.Do(r.init); r.err != nil {
-		return nil, r.err
-	}
-	return r.repo.GoMod(version)
-}
-
-func (r *lazyRepo) Zip(dst io.Writer, version string) error {
-	if r.once.Do(r.init); r.err != nil {
-		return r.err
-	}
-	return r.repo.Zip(dst, version)
-}
-
-// A listRepo is a preference list of Repos.
-// The list must be non-empty and all Repos
-// must return the same result from ModulePath.
-// For each method, the repos are tried in order
-// until one succeeds or returns a non-ErrNotExist (non-404) error.
-type listRepo []Repo
-
-func (l listRepo) ModulePath() string {
-	return l[0].ModulePath()
-}
-
-func (l listRepo) Versions(prefix string) ([]string, error) {
-	for i, r := range l {
-		v, err := r.Versions(prefix)
-		if i == len(l)-1 || !errors.Is(err, os.ErrNotExist) {
-			return v, err
-		}
-	}
-	panic("no repos")
-}
-
-func (l listRepo) Stat(rev string) (*RevInfo, error) {
-	for i, r := range l {
-		info, err := r.Stat(rev)
-		if i == len(l)-1 || !errors.Is(err, os.ErrNotExist) {
-			return info, err
-		}
-	}
-	panic("no repos")
-}
-
-func (l listRepo) Latest() (*RevInfo, error) {
-	for i, r := range l {
-		info, err := r.Latest()
-		if i == len(l)-1 || !errors.Is(err, os.ErrNotExist) {
-			return info, err
-		}
-	}
-	panic("no repos")
-}
-
-func (l listRepo) GoMod(version string) ([]byte, error) {
-	for i, r := range l {
-		data, err := r.GoMod(version)
-		if i == len(l)-1 || !errors.Is(err, os.ErrNotExist) {
-			return data, err
-		}
-	}
-	panic("no repos")
-}
-
-func (l listRepo) Zip(dst io.Writer, version string) error {
-	for i, r := range l {
-		err := r.Zip(dst, version)
-		if i == len(l)-1 || !errors.Is(err, os.ErrNotExist) {
-			return err
-		}
-	}
-	panic("no repos")
-}

src/cmd/go/internal/modfetch/repo.go

@@ -5,6 +5,7 @@
 package modfetch
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -172,20 +173,32 @@ type RevInfo struct {
 
 var lookupCache par.Cache
 
-// Lookup returns the module with the given module path.
+type lookupCacheKey struct {
+	proxy, path string
+}
+
+// Lookup returns the module with the given module path,
+// fetched through the given proxy.
+//
+// The distinguished proxy "direct" indicates that the path should be fetched
+// from its origin, and "noproxy" indicates that the patch should be fetched
+// directly only if GONOPROXY matches the given path.
+//
+// For the distinguished proxy "off", Lookup always returns a non-nil error.
+//
 // A successful return does not guarantee that the module
 // has any defined versions.
-func Lookup(path string) (Repo, error) {
+func Lookup(proxy, path string) (Repo, error) {
 	if traceRepo {
-		defer logCall("Lookup(%q)", path)()
+		defer logCall("Lookup(%q, %q)", proxy, path)()
 	}
 
 	type cached struct {
 		r   Repo
 		err error
 	}
-	c := lookupCache.Do(path, func() interface{} {
-		r, err := lookup(path)
+	c := lookupCache.Do(lookupCacheKey{proxy, path}, func() interface{} {
+		r, err := lookup(proxy, path)
 		if err == nil {
 			if traceRepo {
 				r = newLoggingRepo(r)
@@ -199,19 +212,39 @@ func Lookup(path string) (Repo, error) {
 }
 
 // lookup returns the module with the given module path.
-func lookup(path string) (r Repo, err error) {
+func lookup(proxy, path string) (r Repo, err error) {
 	if cfg.BuildMod == "vendor" {
-		return nil, fmt.Errorf("module lookup disabled by -mod=%s", cfg.BuildMod)
+		return nil, errModVendor
 	}
-	if proxyURL == "off" {
-		return nil, fmt.Errorf("module lookup disabled by GOPROXY=%s", proxyURL)
+
+	if str.GlobsMatchPath(cfg.GONOPROXY, path) {
+		switch proxy {
+		case "noproxy", "direct":
+			return lookupDirect(path)
+		default:
+			return nil, errNoproxy
+		}
 	}
-	if proxyURL != "" && proxyURL != "direct" && !str.GlobsMatchPath(cfg.GONOPROXY, path) {
-		return lookupProxy(path)
+
+	switch proxy {
+	case "off":
+		return nil, errProxyOff
+	case "direct":
+		return lookupDirect(path)
+	case "noproxy":
+		return nil, errUseProxy
+	default:
+		return newProxyRepo(proxy, path)
 	}
-	return lookupDirect(path)
 }
 
+var (
+	errModVendor       = errors.New("module lookup disabled by -mod=vendor")
+	errProxyOff        = errors.New("module lookup disabled by GOPROXY=off")
+	errNoproxy   error = notExistError("disabled by GONOPROXY")
+	errUseProxy  error = notExistError("path does not match GONOPROXY")
+)
+
 func lookupDirect(path string) (Repo, error) {
 	security := web.SecureOnly
 	if get.Insecure {
@@ -220,7 +253,7 @@ func lookupDirect(path string) (Repo, error) {
 	rr, err := get.RepoRootForImportPath(path, get.PreferMod, security)
 	if err != nil {
 		// We don't know where to find code for a module with this path.
-		return nil, err
+		return nil, notExistError(err.Error())
 	}
 
 	if rr.VCS == "mod" {
@@ -362,3 +395,13 @@ func (l *loggingRepo) Zip(dst io.Writer, version string) error {
 	defer logCall("Repo[%s]: Zip(%s, %q)", l.r.ModulePath(), dstName, version)()
 	return l.r.Zip(dst, version)
 }
+
+// A notExistError is like os.ErrNotExist, but with a custom message
+type notExistError string
+
+func (e notExistError) Error() string {
+	return string(e)
+}
+func (notExistError) Is(target error) bool {
+	return target == os.ErrNotExist
+}

src/cmd/go/internal/modfetch/sumdb.go

@@ -142,6 +142,9 @@ func (c *dbClient) initBase() {
 		return
 	}
 	for _, proxyURL := range urls {
+		if proxyURL == "noproxy" {
+			continue
+		}
 		if proxyURL == "direct" {
 			break
 		}

src/cmd/go/internal/modload/import.go

@@ -18,7 +18,6 @@ import (
 
 	"cmd/go/internal/cfg"
 	"cmd/go/internal/modfetch"
-	"cmd/go/internal/modfetch/codehost"
 	"cmd/go/internal/module"
 	"cmd/go/internal/par"
 	"cmd/go/internal/search"
@@ -188,10 +187,13 @@ func Import(path string) (m module.Version, dir string, err error) {
 
 	candidates, err := QueryPackage(path, "latest", Allowed)
 	if err != nil {
-		if _, ok := err.(*codehost.VCSError); ok {
+		if errors.Is(err, os.ErrNotExist) {
+			// Return "cannot find module providing package […]" instead of whatever
+			// low-level error QueryPackage produced.
+			return module.Version{}, "", &ImportMissingError{ImportPath: path}
+		} else {
 			return module.Version{}, "", err
 		}
-		return module.Version{}, "", &ImportMissingError{ImportPath: path}
 	}
 	m = candidates[0].Mod
 	newMissingVersion := ""

src/cmd/go/internal/modload/import_test.go

@@ -21,7 +21,7 @@ var importTests = []struct {
 	},
 	{
 		path: "golang.org/x/net",
-		err:  "cannot find module providing package golang.org/x/net",
+		err:  "module golang.org/x/net@.* found, but does not contain package golang.org/x/net",
 	},
 	{
 		path: "golang.org/x/text",

src/cmd/go/internal/modload/load.go

@@ -1208,11 +1208,15 @@ func (*mvsReqs) Upgrade(m module.Version) (module.Version, error) {
 func versions(path string) ([]string, error) {
 	// Note: modfetch.Lookup and repo.Versions are cached,
 	// so there's no need for us to add extra caching here.
-	repo, err := modfetch.Lookup(path)
-	if err != nil {
-		return nil, err
-	}
-	return repo.Versions("")
+	var versions []string
+	err := modfetch.TryProxies(func(proxy string) error {
+		repo, err := modfetch.Lookup(proxy, path)
+		if err == nil {
+			versions, err = repo.Versions("")
+		}
+		return err
+	})
+	return versions, err
 }
 
 // Previous returns the tagged version of m.Path immediately prior to

src/cmd/go/internal/modload/query_test.go

@@ -25,7 +25,7 @@ func TestMain(m *testing.M) {
 }
 
 func testMain(m *testing.M) int {
-	modfetch.SetProxy("direct")
+	cfg.GOPROXY = "direct"
 
 	dir, err := ioutil.TempDir("", "modload-test-")
 	if err != nil {

src/cmd/go/proxy_test.go

@@ -8,6 +8,7 @@ import (
 	"archive/zip"
 	"bytes"
 	"encoding/json"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
@@ -253,7 +254,11 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) {
 		if !quiet {
 			fmt.Fprintf(os.Stderr, "go proxy: no archive %s %s: %v\n", path, vers, err)
 		}
-		http.Error(w, "cannot load archive", 500)
+		if errors.Is(err, os.ErrNotExist) {
+			http.NotFound(w, r)
+		} else {
+			http.Error(w, "cannot load archive", 500)
+		}
 		return
 	}
 

src/cmd/go/testdata/script/mod_gobuild_import.txt

@@ -6,11 +6,11 @@ go build -o $WORK/testimport.exe ./testimport
 
 # GO111MODULE=off
 env GO111MODULE=off
-! exec $WORK/testimport.exe x/y/z/w .
+! exec $WORK/testimport.exe gobuild.example.com/x/y/z/w .
 
 # GO111MODULE=auto in GOPATH/src
 env GO111MODULE=auto
-exec $WORK/testimport.exe x/y/z/w .
+exec $WORK/testimport.exe gobuild.example.com/x/y/z/w .
 
 # GO111MODULE=auto outside GOPATH/src
 cd $GOPATH/other
@@ -18,8 +18,8 @@ env GO111MODULE=auto
 exec $WORK/testimport.exe other/x/y/z/w .
 stdout w2.go
 
-! exec $WORK/testimport.exe x/y/z/w .
-stderr 'cannot find module providing package x/y/z/w'
+! exec $WORK/testimport.exe gobuild.example.com/x/y/z/w .
+stderr 'cannot find module providing package gobuild.example.com/x/y/z/w'
 
 cd z
 exec $WORK/testimport.exe other/x/y/z/w .
@@ -36,17 +36,17 @@ stdout w2.go
 # GO111MODULE=on in GOPATH/src
 cd $GOPATH/src
 env GO111MODULE=
-exec $WORK/testimport.exe x/y/z/w .
+exec $WORK/testimport.exe gobuild.example.com/x/y/z/w .
 stdout w1.go
 env GO111MODULE=on
-exec $WORK/testimport.exe x/y/z/w .
+exec $WORK/testimport.exe gobuild.example.com/x/y/z/w .
 stdout w1.go
 cd w
-exec $WORK/testimport.exe x/y/z/w ..
+exec $WORK/testimport.exe gobuild.example.com/x/y/z/w ..
 stdout w1.go
 
 -- go.mod --
-module x/y/z
+module gobuild.example.com/x/y/z
 
 -- z.go --
 package z