Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: go list -e does not assign missing import errors to correct package #26909

Open
rsc opened this issue Aug 10, 2018 · 10 comments
Open

cmd/go: go list -e does not assign missing import errors to correct package #26909

rsc opened this issue Aug 10, 2018 · 10 comments
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Aug 10, 2018

See TODO in testdata/script/mod_lists_bad_import.txt.

@rsc
Copy link
Contributor Author

@rsc rsc commented Aug 17, 2018

It's complicated. The current behavior might even be almost correct. Leaving for Go 1.12.

@rsc rsc modified the milestones: Go1.11, Go1.12 Aug 17, 2018
@rsc rsc added NeedsDecision and removed NeedsFix labels Aug 17, 2018
@bcmills bcmills added the GoCommand label Nov 14, 2018
@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 20, 2018
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Feb 4, 2020

@bcmills, @matloob, and I discussed this last week in the context of #36188. There's currently some ambiguity about whether import errors should be attached to the importing package or the imported package in go list -e -json output.

Here's what we came up with:

  • If the package being imported cannot be built, for example, because it doesn't exist or has no .go files, the error should be attached to the imported package. The error may still have a position in a file in the importing package. If multiple packages import an unbuildable package with the same path, multiple packages with errors should be listed.
  • If the package can be built but the importing package is not allowed to import it, for example because it's an internal package, a main package, or has a different canonical path, the error should be attached to the importing package, and the imported package should be listed normally without error. If the importing package has multiple import errors, we'll only include the first one in go list -e -json output.
@jayconrod jayconrod modified the milestones: Backlog, Go1.15 Feb 4, 2020
@bcmills
Copy link
Member

@bcmills bcmills commented Feb 13, 2020

If the package being imported cannot be built, … the error may still have a position in a file in the importing package. If multiple packages import an unbuildable package with the same path, multiple packages with errors should be listed.

I don't think that's quite right: if the error for a given package is not dependent on the importer, the Error field of the go list -e -json entry for that package should not itself include information about importers. (However, the information in the DepsErrors field of each of those importers should populate the Pos field with the position of the corresponding import line.)

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 13, 2020

If the package can be built but the importing package is not allowed to import it, … the error should be attached to the importing package, and the imported package should be listed normally without error.

The details of this part are a bit subtle. I think in that case the error should be encoded in the Error field for the importing package (rather than the DepsErrors field), because the error did not strictly occur in the course of “loading” the dependency.

@bcmills
Copy link
Member

@bcmills bcmills commented May 7, 2020

@matloob, is this fixed by your recent import-stack CL?

I think the relevant TODO is here:

# TODO: go list creates a dummy package with the import-not-found
# but really the Error belongs on example.com/direct, and this package
# should not be printed.
# ! stdout example.com/notfound

@matloob
Copy link
Contributor

@matloob matloob commented May 26, 2020

I just checked, and this isn't fixed at head. I'll take a more in-depth look tomorrow.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 1, 2020

Change https://golang.org/cl/235925 mentions this issue: cmd/go: assign missing import errors to importing package

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 15, 2020

There is a CL out for this issue but nobody has reviewed it. Is this still expected for 1.15, or should we push it to 1.16? Thanks.

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 16, 2020

Michael tagged the CL as wait-release shortly after it was mailed, so looks like 1.16.

@bcmills bcmills modified the milestones: Go1.15, Go1.16 Jun 16, 2020
@jayconrod jayconrod added NeedsFix and removed NeedsDecision labels Aug 6, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 29, 2020

Change https://golang.org/cl/251445 mentions this issue: cmd/go/internal/modload: rework import resolution

gopherbot pushed a commit that referenced this issue Sep 9, 2020
modload.Import previously performed two otherwise-separable tasks:

1. Identify which module in the build list contains the requested
   package.

2. If no such module exists, search available modules to try to find
   the missing package.

This change splits those two tasks into two separate unexported
functions, and reports import-resolution errors by attaching them to
the package rather than emitting them directly to stderr. That allows
'list' to report the errors, but 'list -e' to ignore them.

With the two tasks now separate, it will be easier to avoid the
overhead of resolving missing packages during lazy loading if we
discover that some existing dependency needs to be promoted to the top
level (potentially altering the main module's selected versions, and
thus suppling packages that were previously missing).

For #36460
Updates #26909

Change-Id: I32bd853b266d7cd231d1f45f92b0650d95c4bcbd
Reviewed-on: https://go-review.googlesource.com/c/go/+/251445
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.