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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: 'go get -d' does not report line numbers in errors for bad import statements #41688

Open
bcmills opened this issue Sep 29, 2020 · 8 comments
Assignees
Milestone

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 29, 2020

The fix for #41315 has revealed another variant of issue #34393.

Adding the following test case to mod_build_info_err.txt

+go get -d ./main
+stderr 'bad[/\\]bad.go:3:8: malformed module path "馃惂.example.com/string": invalid char ''馃惂'''

gives the failure mode of #41315 at release-branch.go1.15. At head, it gives a different, but still not quite right, failure mode:

+ go test cmd/go '-run=TestScript/^mod_build_info_err$'
go test proxy running at GOPROXY=http://127.0.0.1:44957/mod
--- FAIL: TestScript (0.01s)
    --- FAIL: TestScript/mod_build_info_err (0.05s)
        script_test.go:211:
            # This test verifies that line numbers are included in module import errors.
            # Verifies golang.org/issue/34393. (0.036s)
            > go list -e -deps -f '{{with .Error}}{{.Pos}}: {{.Err}}{{end}}' ./main
            [stdout]
            bad/bad.go:3:8: malformed import path "馃惂.example.com/string": invalid char '馃惂'
            > stdout 'bad[/\\]bad.go:3:8: malformed import path "馃惂.example.com/string": invalid char ''馃惂'''
            > go get -d ./main
            [stderr]
            m/main imports
                m/bad imports
                馃惂.example.com/string: import missing
            [exit status 1]
            FAIL: testdata/script/mod_build_info_err.txt:7: unexpected command failure

FAIL

Notably, the import stack for this error message is again bereft of line numbers (as in #34393).

CC @jayconrod @matloob @rogpeppe

@bcmills bcmills added this to the Backlog milestone Sep 29, 2020
@bcmills bcmills changed the title cmd/go: "go get" does not report line numbers in errors for bad import statements cmd/go: 'go get -d' does not report line numbers in errors for bad import statements Sep 29, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 29, 2020

Change https://golang.org/cl/258219 mentions this issue: cmd/go: test more commands in mod_build_info_error

@bcmills
Copy link
Member Author

@bcmills bcmills commented Sep 29, 2020

I think this needs to be a blocker for 1.16: with -mod=readonly we're going to end up encouraging wider use of go get.

@bcmills bcmills modified the milestones: Backlog, Go1.16 Sep 29, 2020
@bcmills
Copy link
Member Author

@bcmills bcmills commented Sep 29, 2020

(The import missing error message in the above trace is a separate bug in go get, which I plan to fix as part of #37438.)

gopherbot pushed a commit that referenced this issue Sep 30, 2020
For #26909
For #41688

Change-Id: I22f28d426ce499fce6f0f1295dbde425998042aa
Reviewed-on: https://go-review.googlesource.com/c/go/+/258219
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
@toothrot
Copy link
Contributor

@toothrot toothrot commented Oct 15, 2020

@bcmills What else is left to do on this before the release?

@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Oct 22, 2020

@bcmills Is there anything left to do on this before the release?

@bcmills
Copy link
Member Author

@bcmills bcmills commented Oct 22, 2020

I'm not sure. I'm still in the process of overhauling the go get implementation for #36460.

@bcmills
Copy link
Member Author

@bcmills bcmills commented Jan 5, 2021

We should decide whether this is feasible to fix for the release, but if it turns out to be too invasive we can punt it to 1.17.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Jan 5, 2021

Looked into this a bit. In go get, we invoke modload.LoadPackages with AllowErrors = true and SilenceErrors = false. This means the stacks are currently formatted by modload.loadPkg.stackText. I see two solutions:

  1. modload.loadPkg doesn't currently track locations of import declarations. We could modify imports.ScanDir to return that information.
  2. We could use load.PackagesAndErrors instead. That would give us error messages identical to those printed by go build, but it would apply build constraints, which we probably don't want.

I think it's viable to implement (1) before 1.16, but there are a lot of open release blockers that should fix first. This shouldn't actually block the release, and it's probably too big to go in after the RC.

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
6 participants