cmd/go: query each path only once in 'go get'

If we don't know whether a path is a module path or a package path,
previously we would first try a module query for it, then fall back to
a package query.

If we are using a sequence of proxies with fallback (as will be the
default in Go 1.13), and the path is not actually a module path, that
initial module query will fail against the first proxy, then
immediately fall back to the next proxy in the sequence — even if the
query could have been satisfied by some other (prefix) module
available from the first proxy.

Instead, we now query the requested path as only one kind of path.
If we query it as a package path but it turns out to only exist as a
module, we can detect that as a PackageNotInModuleError with an
appropriate module path — we do not need to issue a second query to
classify it.

Fixes #31785

Change-Id: I581d44279196e41d1fed27ec25489e75d62654e3
Reviewed-on: https://go-review.googlesource.com/c/go/+/189517
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

6 files changed, 66 insertions, 42 deletions

diff --git a/src/cmd/go/internal/modfetch/repo.go b/src/cmd/go/internal/modfetch/repo.go
index 95351269db..be52a8dc11 100644
--- a/src/cmd/go/internal/modfetch/repo.go
+++ b/src/cmd/go/internal/modfetch/repo.go
@@ -240,7 +240,7 @@ func lookup(proxy, path string) (r Repo, err error) {
 
 var (
 	errModVendor       = errors.New("module lookup disabled by -mod=vendor")
-	errProxyOff        = errors.New("module lookup disabled by GOPROXY=off")
+	errProxyOff        = notExistError("module lookup disabled by GOPROXY=off")
 	errNoproxy   error = notExistError("disabled by GOPRIVATE/GONOPROXY")
 	errUseProxy  error = notExistError("path does not match GOPRIVATE/GONOPROXY")
 )
diff --git a/src/cmd/go/internal/modget/get.go b/src/cmd/go/internal/modget/get.go
index 84b1ac1b01..1cae311c4c 100644
--- a/src/cmd/go/internal/modget/get.go
+++ b/src/cmd/go/internal/modget/get.go
@@ -735,7 +735,7 @@ func runQueries(cache map[querySpec]*query, queries []*query, modOnly map[string
 	return byPath
 }
 
-// getQuery evaluates the given package path, version pair
+// getQuery evaluates the given (package or module) path and version
 // to determine the underlying module version being requested.
 // If forceModulePath is set, getQuery must interpret path
 // as a module path.
@@ -753,40 +753,51 @@ func getQuery(path, vers string, prevM module.Version, forceModulePath bool) (mo
 		base.Fatalf("go get: internal error: prevM may be set if and only if forceModulePath is set")
 	}
 
-	if forceModulePath || !strings.Contains(path, "...") {
+	// If the query must be a module path, try only that module path.
+	if forceModulePath {
 		if path == modload.Target.Path {
 			if vers != "latest" {
 				return module.Version{}, fmt.Errorf("can't get a specific version of the main module")
 			}
 		}
 
-		// If the path doesn't contain a wildcard, try interpreting it as a module path.
 		info, err := modload.Query(path, vers, prevM.Version, modload.Allowed)
 		if err == nil {
 			return module.Version{Path: path, Version: info.Version}, nil
 		}
 
-		// If the query fails, and the path must be a real module, report the query error.
-		if forceModulePath {
-			// If the query was "upgrade" or "patch" and the current version has been
-			// replaced, check to see whether the error was for that same version:
-			// if so, the version was probably replaced because it is invalid,
-			// and we should keep that replacement without complaining.
-			if vers == "upgrade" || vers == "patch" {
-				var vErr *module.InvalidVersionError
-				if errors.As(err, &vErr) && vErr.Version == prevM.Version && modload.Replacement(prevM).Path != "" {
-					return prevM, nil
-				}
+		// If the query was "upgrade" or "patch" and the current version has been
+		// replaced, check to see whether the error was for that same version:
+		// if so, the version was probably replaced because it is invalid,
+		// and we should keep that replacement without complaining.
+		if vers == "upgrade" || vers == "patch" {
+			var vErr *module.InvalidVersionError
+			if errors.As(err, &vErr) && vErr.Version == prevM.Version && modload.Replacement(prevM).Path != "" {
+				return prevM, nil
 			}
-			return module.Version{}, err
 		}
+
+		return module.Version{}, err
 	}
 
-	// Otherwise, try a package path or pattern.
+	// If the query may be either a package or a module, try it as a package path.
+	// If it turns out to only exist as a module, we can detect the resulting
+	// PackageNotInModuleError and avoid a second round-trip through (potentially)
+	// all of the configured proxies.
 	results, err := modload.QueryPattern(path, vers, modload.Allowed)
 	if err != nil {
+		// If the path doesn't contain a wildcard, check whether it was actually a
+		// module path instead. If so, return that.
+		if !strings.Contains(path, "...") {
+			var modErr *modload.PackageNotInModuleError
+			if errors.As(err, &modErr) && modErr.Mod.Path == path {
+				return modErr.Mod, nil
+			}
+		}
+
 		return module.Version{}, err
 	}
+
 	return results[0].Mod, nil
 }
 
diff --git a/src/cmd/go/internal/modload/query.go b/src/cmd/go/internal/modload/query.go
index 8c5fdc8bf9..602bf47275 100644
--- a/src/cmd/go/internal/modload/query.go
+++ b/src/cmd/go/internal/modload/query.go
@@ -380,10 +380,10 @@ func QueryPattern(pattern, query string, allowed func(module.Version) bool) ([]Q
 			}
 			r.Packages = match(r.Mod, root, isLocal)
 			if len(r.Packages) == 0 {
-				return r, &packageNotInModuleError{
-					mod:     r.Mod,
-					query:   query,
-					pattern: pattern,
+				return r, &PackageNotInModuleError{
+					Mod:     r.Mod,
+					Query:   query,
+					Pattern: pattern,
 				}
 			}
 			return r, nil
@@ -446,30 +446,31 @@ func queryPrefixModules(candidateModules []string, queryModule func(path string)
 	wg.Wait()
 
 	// Classify the results. In case of failure, identify the error that the user
-	// is most likely to find helpful.
+	// is most likely to find helpful: the most useful class of error at the
+	// longest matching path.
 	var (
+		noPackage   *PackageNotInModuleError
 		noVersion   *NoMatchingVersionError
-		noPackage   *packageNotInModuleError
 		notExistErr error
 	)
 	for _, r := range results {
 		switch rErr := r.err.(type) {
 		case nil:
 			found = append(found, r.QueryResult)
+		case *PackageNotInModuleError:
+			if noPackage == nil {
+				noPackage = rErr
+			}
 		case *NoMatchingVersionError:
 			if noVersion == nil {
 				noVersion = rErr
 			}
-		case *packageNotInModuleError:
-			if noPackage == nil {
-				noPackage = rErr
-			}
 		default:
 			if errors.Is(rErr, os.ErrNotExist) {
 				if notExistErr == nil {
 					notExistErr = rErr
 				}
-			} else {
+			} else if err == nil {
 				err = r.err
 			}
 		}
@@ -515,31 +516,31 @@ func (e *NoMatchingVersionError) Error() string {
 	return fmt.Sprintf("no matching versions for query %q", e.query) + currentSuffix
 }
 
-// A packageNotInModuleError indicates that QueryPattern found a candidate
+// A PackageNotInModuleError indicates that QueryPattern found a candidate
 // module at the requested version, but that module did not contain any packages
 // matching the requested pattern.
 //
-// NOTE: packageNotInModuleError MUST NOT implement Is(os.ErrNotExist).
+// NOTE: PackageNotInModuleError MUST NOT implement Is(os.ErrNotExist).
 //
 // If the module came from a proxy, that proxy had to return a successful status
 // code for the versions it knows about, and thus did not have the opportunity
 // to return a non-400 status code to suppress fallback.
-type packageNotInModuleError struct {
-	mod     module.Version
-	query   string
-	pattern string
+type PackageNotInModuleError struct {
+	Mod     module.Version
+	Query   string
+	Pattern string
 }
 
-func (e *packageNotInModuleError) Error() string {
+func (e *PackageNotInModuleError) Error() string {
 	found := ""
-	if e.query != e.mod.Version {
-		found = fmt.Sprintf(" (%s)", e.mod.Version)
+	if e.Query != e.Mod.Version {
+		found = fmt.Sprintf(" (%s)", e.Mod.Version)
 	}
 
-	if strings.Contains(e.pattern, "...") {
-		return fmt.Sprintf("module %s@%s%s found, but does not contain packages matching %s", e.mod.Path, e.query, found, e.pattern)
+	if strings.Contains(e.Pattern, "...") {
+		return fmt.Sprintf("module %s@%s%s found, but does not contain packages matching %s", e.Mod.Path, e.Query, found, e.Pattern)
 	}
-	return fmt.Sprintf("module %s@%s%s found, but does not contain package %s", e.mod.Path, e.query, found, e.pattern)
+	return fmt.Sprintf("module %s@%s%s found, but does not contain package %s", e.Mod.Path, e.Query, found, e.Pattern)
 }
 
 // ModuleHasRootPackage returns whether module m contains a package m.Path.
diff --git a/src/cmd/go/testdata/script/mod_get_fallback.txt b/src/cmd/go/testdata/script/mod_get_fallback.txt
new file mode 100644
index 0000000000..a9834a324e
--- /dev/null
+++ b/src/cmd/go/testdata/script/mod_get_fallback.txt
@@ -0,0 +1,10 @@
+env GO111MODULE=on
+
+[!net] skip
+
+env GOPROXY=https://proxy.golang.org,direct
+env GOSUMDB=off
+
+go get -x -v -d golang.org/x/tools/cmd/goimports
+stderr '# get https://proxy.golang.org/golang.org/x/tools/@latest'
+! stderr '# get https://golang.org'
diff --git a/src/cmd/go/testdata/script/mod_get_newcycle.txt b/src/cmd/go/testdata/script/mod_get_newcycle.txt
index 9616863383..b1838f824a 100644
--- a/src/cmd/go/testdata/script/mod_get_newcycle.txt
+++ b/src/cmd/go/testdata/script/mod_get_newcycle.txt
@@ -1,6 +1,7 @@
 env GO111MODULE=on
 
 # Download modules to avoid stderr chatter
+go mod download example.com@v1.0.0
 go mod download example.com/newcycle/a@v1.0.0
 go mod download example.com/newcycle/a@v1.0.1
 go mod download example.com/newcycle/b@v1.0.0
@@ -10,5 +11,6 @@ go mod init m
 cmp stderr stderr-expected
 
 -- stderr-expected --
+go: finding example.com/newcycle v1.0.0
 go get: inconsistent versions:
 	example.com/newcycle/a@v1.0.0 requires example.com/newcycle/a@v1.0.1 (not example.com/newcycle/a@v1.0.0)
diff --git a/src/cmd/go/testdata/script/mod_sumdb.txt b/src/cmd/go/testdata/script/mod_sumdb.txt
index 8e1f3d7a7b..641b9e73bc 100644
--- a/src/cmd/go/testdata/script/mod_sumdb.txt
+++ b/src/cmd/go/testdata/script/mod_sumdb.txt
@@ -9,8 +9,8 @@ env dbname=localhost.localdev/sumdb
 cp go.mod.orig go.mod
 env GOSUMDB=$sumdb' '$proxy/sumdb-wrong
 ! go get -d rsc.io/quote
-stderr 'verifying rsc.io/quote@v1.5.2/go.mod: checksum mismatch'
-stderr 'downloaded: h1:LzX7'
+stderr 'verifying rsc.io/quote@v1.5.2: checksum mismatch'
+stderr 'downloaded: h1:3fEy'
 stderr 'localhost.localdev/sumdb: h1:wrong'
 stderr 'SECURITY ERROR\nThis download does NOT match the one reported by the checksum server.'
 ! go get -d rsc.io/sampler