]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: attribute direct imports from indirect dependencies to the importing package
authorBryan C. Mills <bcmills@google.com>
Tue, 23 Mar 2021 04:26:39 +0000 (00:26 -0400)
committerBryan C. Mills <bcmills@google.com>
Thu, 25 Mar 2021 03:17:51 +0000 (03:17 +0000)
For #36460
Updates #40775

Change-Id: I833ee8ee733151aafcff508bf91d0e6e37c50032
Reviewed-on: https://go-review.googlesource.com/c/go/+/303869
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
src/cmd/go/internal/load/pkg.go
src/cmd/go/internal/modload/import.go
src/cmd/go/internal/modload/load.go
src/cmd/go/testdata/script/mod_get_promote_implicit.txt

index c9619f1b288430283e798e37f436833e3becb9de..66b6d0dc4677e9f1ad622eba1264f31835fab28f 100644 (file)
@@ -29,6 +29,7 @@ import (
        "cmd/go/internal/base"
        "cmd/go/internal/cfg"
        "cmd/go/internal/fsys"
+       "cmd/go/internal/imports"
        "cmd/go/internal/modinfo"
        "cmd/go/internal/modload"
        "cmd/go/internal/par"
@@ -478,6 +479,7 @@ var (
        _ ImportPathError = (*importError)(nil)
        _ ImportPathError = (*modload.ImportMissingError)(nil)
        _ ImportPathError = (*modload.ImportMissingSumError)(nil)
+       _ ImportPathError = (*modload.DirectImportFromImplicitDependencyError)(nil)
 )
 
 type importError struct {
@@ -675,6 +677,8 @@ func loadImport(ctx context.Context, pre *preload, path, srcDir string, parent *
                        stk.Push(path)
                        defer stk.Pop()
                }
+               // TODO(bcmills): Why are we constructing Error inline here instead of
+               // calling setLoadPackageDataError?
                return &Package{
                        PackagePublic: PackagePublic{
                                ImportPath: path,
@@ -840,6 +844,27 @@ func loadPackageData(ctx context.Context, path, parentPath, parentDir, parentRoo
                                        data.p.Root = info.Dir
                                }
                        }
+                       if r.err != nil {
+                               if data.err != nil {
+                                       // ImportDir gave us one error, and the module loader gave us another.
+                                       // We arbitrarily choose to keep the error from ImportDir because
+                                       // that's what our tests already expect, and it seems to provide a bit
+                                       // more detail in most cases.
+                               } else if errors.Is(r.err, imports.ErrNoGo) {
+                                       // ImportDir said there were files in the package, but the module
+                                       // loader said there weren't. Which one is right?
+                                       // Without this special-case hack, the TestScript/test_vet case fails
+                                       // on the vetfail/p1 package (added in CL 83955).
+                                       // Apparently, imports.ShouldBuild biases toward rejecting files
+                                       // with invalid build constraints, whereas ImportDir biases toward
+                                       // accepting them.
+                                       //
+                                       // TODO(#41410: Figure out how this actually ought to work and fix
+                                       // this mess.
+                               } else {
+                                       data.err = r.err
+                               }
+                       }
                } else if r.err != nil {
                        data.p = new(build.Package)
                        data.err = r.err
index 508db2f247952c3e277968597a98673e5fc4a4ed..1d321bb24bb7313566b8a2542bc1dbf52594be8f 100644 (file)
@@ -128,6 +128,23 @@ func (e *AmbiguousImportError) Error() string {
        return buf.String()
 }
 
+// A DirectImportFromImplicitDependencyError indicates a package directly
+// imported by a package or test in the main module that is satisfied by a
+// dependency that is not explicit in the main module's go.mod file.
+type DirectImportFromImplicitDependencyError struct {
+       ImporterPath string
+       ImportedPath string
+       Module       module.Version
+}
+
+func (e *DirectImportFromImplicitDependencyError) Error() string {
+       return fmt.Sprintf("package %s imports %s from implicitly required module; to add missing requirements, run:\n\tgo get %s@%s", e.ImporterPath, e.ImportedPath, e.Module.Path, e.Module.Version)
+}
+
+func (e *DirectImportFromImplicitDependencyError) ImportPath() string {
+       return e.ImporterPath
+}
+
 // ImportMissingSumError is reported in readonly mode when we need to check
 // if a module contains a package, but we don't have a sum for its .zip file.
 // We might need sums for multiple modules to verify the package is unique.
index d89dc670283c97fbd151e94b7612ccb963e1bf06..386b53938c987c540e005fa27f90f5207e664bfd 100644 (file)
@@ -940,32 +940,51 @@ func loadFromRoots(ctx context.Context, params loaderParams) *loader {
        }
        base.ExitIfErrors() // TODO(bcmills): Is this actually needed?
 
+       rs := ld.requirements
+
        // Compute directly referenced dependency modules.
        direct := make(map[string]bool)
        for _, pkg := range ld.pkgs {
-               if pkg.mod == Target {
-                       for _, dep := range pkg.imports {
-                               if dep.mod.Path != "" && dep.mod.Path != Target.Path && index != nil {
-                                       _, explicit := index.require[dep.mod]
-                                       if allowWriteGoMod && cfg.BuildMod == "readonly" && !explicit {
-                                               // TODO(#40775): attach error to package instead of using
-                                               // base.Errorf. Ideally, 'go list' should not fail because of this,
-                                               // but today, LoadPackages calls WriteGoMod unconditionally, which
-                                               // would fail with a less clear message.
-                                               base.Errorf("go: %[1]s: package %[2]s imported from implicitly required module; to add missing requirements, run:\n\tgo get %[2]s@%[3]s", pkg.path, dep.path, dep.mod.Version)
+               if pkg.mod != Target {
+                       continue
+               }
+               for _, dep := range pkg.imports {
+                       if dep.mod.Path == "" || dep.mod.Path == Target.Path {
+                               continue
+                       }
+
+                       if pkg.err == nil && cfg.BuildMod != "mod" {
+                               if v, ok := rs.rootSelected(dep.mod.Path); !ok || v != dep.mod.Version {
+                                       // dep.mod is not an explicit dependency, but needs to be.
+                                       // Because we are not in "mod" mod, we will not be able to update it.
+                                       // Instead, mark the importing package with an error.
+                                       //
+                                       // TODO(#41688): The resulting error message fails to include the file
+                                       // position of the erroneous import (because that information is not
+                                       // tracked by the module loader). Figure out how to plumb the import
+                                       // position through.
+                                       pkg.err = &DirectImportFromImplicitDependencyError{
+                                               ImporterPath: pkg.path,
+                                               ImportedPath: dep.path,
+                                               Module:       dep.mod,
                                        }
-                                       direct[dep.mod.Path] = true
+                                       // cfg.BuildMod does not allow us to change dep.mod to be a direct
+                                       // dependency, so don't mark it as such.
+                                       continue
                                }
                        }
+
+                       // dep is a package directly imported by a package or test in the main
+                       // module and loaded from some other module (not the standard library).
+                       // Mark its module as a direct dependency.
+                       direct[dep.mod.Path] = true
                }
        }
-       base.ExitIfErrors()
 
        // If we didn't scan all of the imports from the main module, or didn't use
        // imports.AnyTags, then we didn't necessarily load every package that
        // contributes “direct” imports — so we can't safely mark existing
        // direct dependencies in ld.requirements as indirect-only. Propagate them as direct.
-       rs := ld.requirements
        if !ld.loadedDirect() {
                for mPath := range rs.direct {
                        direct[mPath] = true
index 10ca6594e4c6e33e8abd5c1b38624d50e04b9ff7..9eec2013210350fdd7b4234eeea6160abaa70ec7 100644 (file)
@@ -6,7 +6,7 @@ cp go.mod.orig go.mod
 go list -m indirect-with-pkg
 stdout '^indirect-with-pkg v1.0.0 => ./indirect-with-pkg$'
 ! go list ./use-indirect
-stderr '^go: m/use-indirect: package indirect-with-pkg imported from implicitly required module; to add missing requirements, run:\n\tgo get indirect-with-pkg@v1.0.0$'
+stderr '^package m/use-indirect imports indirect-with-pkg from implicitly required module; to add missing requirements, run:\n\tgo get indirect-with-pkg@v1.0.0$'
 
 # We can promote the implicit requirement by getting the importing package.
 # NOTE: the hint recommends getting the imported package (tested below) since