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/vet: line numbers don't show up for "internal package not allowed" errors from _test files #36173

Closed
ghost opened this issue Dec 17, 2019 · 14 comments
Milestone

Comments

@ghost
Copy link

@ghost ghost commented Dec 17, 2019

What version of Go are you using (go version)?

$ go version
go version go1.13.4 

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

darwin/amd64

go env Output
$ go env

What did you do?

  1. Set the env variable in .zshrc:
export GOPATH=$HOME/Github/service
  1. Open my test file:
// $HOME/Github/service/src/github.com/myName/project/internal/user/user_test.go
package user_test

import (
    ...
    "github.com/myName/project/internal/platform/web"
)

...
  1. It report a suspicious state from go-vet checker, the warm message is:
imports github.com/myName/project/internal/platform/web:
use of internal package github.com/myName/project/internal/platform/web not allowed
@cagedmantis cagedmantis changed the title Suspicious state from syntax checker go-vet cmd/go: suspicious state from syntax checker go-vet Dec 23, 2019
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Dec 23, 2019

Hi @YUANLAI. I've attempted to reproduce the scenario posted above. go vet did not return any errors in my sample scenario. Can you post what command you are running that is producing these results?

@ghost
Copy link
Author

@ghost ghost commented Dec 24, 2019

I using emacs flycheck wrap the go-vet, the error occurs seems happen between flycheck and go-vet. flycheck/flycheck#1659 (comment)

@agnivade
Copy link
Contributor

@agnivade agnivade commented Dec 25, 2019

If the error comes from flycheck and not go-vet, then I'm afraid there is nothing we can do here.

But the issue you linked to shows that the issue is with vet. Could you please clarify ?

Please post the exact instructions that we can run from the command line to reproduce this. Thank you.

@ghost ghost closed this Dec 25, 2019
@ghost ghost reopened this Dec 25, 2019
@nicks
Copy link

@nicks nicks commented Jan 29, 2020

I think there are two bugs here (one in flycheck and one in the go tool) and they stack in weird ways.

The go tool bug is that when it finds a bad "internal package" use in a test file, it doesn't report line numbers properly.

For example:

$ go vet /tmp/scratch/podmonitor.go
/tmp/scratch/podmonitor.go:11:2: use of internal package github.com/windmilleng/tilt/internal/k8s not allowed
$ go vet /tmp/scratch/podmonitor_test.go 
package command-line-arguments (test)
	imports github.com/windmilleng/tilt/internal/container: use of internal package github.com/windmilleng/tilt/internal/container not allowed

Notice that the first invocation reports line numbers, and the second invocation does not.

This confuses flycheck, because flycheck's Go mode expects all Go error messages to have line numbers attached.

How hard is it to fix this error message to have the right line numbers?

@cpitclaudel
Copy link

@cpitclaudel cpitclaudel commented Jan 31, 2020

To be clear, Flycheck is failing because go-vet is producing error messages that don't match the usual pattern (which includes a line number). Fixing that is just a matter of making that message conform to the usual template.

Now, the reason users are running into this message is that we create a temporary copy of their file (including unsaved changes) in a temporary directory before running go-vet; this breaks imports.

The ideal solution for us would be a way to feed file contents to go vet on stdin; then we wouldn't have to create temporary files: we'd just pass unsaved file contents on stdin and tell go-vet what the original file name was, so it could look up the imports properly. (this is what we're doing with most other checkers).

@agnivade
Copy link
Contributor

@agnivade agnivade commented Feb 1, 2020

Thanks for the clarification ! I have a few follow-up questions:

  • Is this a regression or was it always there ?
  • Is "use of internal package not allowed" the only error that does not have line number or are there other errors too ?
@agnivade agnivade changed the title cmd/go: suspicious state from syntax checker go-vet cmd/vet: line numbers don't show up for "internal package not allowed" errors from _test files Feb 1, 2020
@agnivade agnivade added this to the Backlog milestone Feb 1, 2020
@cpitclaudel
Copy link

@cpitclaudel cpitclaudel commented Feb 1, 2020

Thanks for the clarification !

My pleasure! I'll let go users answer the two questions, since I don't know the answers myself :)

@nicks
Copy link

@nicks nicks commented Feb 2, 2020

Yes, this is a regression. The bug is in Go 1.13, but not in Go 1.12

Repro steps:

Create a Dockerfile like

FROM golang:1.13
WORKDIR /go/src/github.com/fake/pkg
RUN echo 'package pkg' > pkg.go
RUN echo 'package pkg\n\nimport "golang.org/x/tools/internal/gopathwalk"\n' > pkg_test.go
RUN go get golang.org/x/tools/internal/gopathwalk
RUN go test ./

Run

docker build .

Result:

# github.com/fake/pkg
package github.com/fake/pkg (test)
	imports golang.org/x/tools/internal/gopathwalk: use of internal package golang.org/x/tools/internal/gopathwalk not allowed

Note that if I change the go version to 1.12 in the Dockerfile I get:

# github.com/fake/pkg
pkg_test.go:3:8: use of internal package golang.org/x/tools/internal/gopathwalk not allowed

which has the correct error message

@nicks
Copy link

@nicks nicks commented Feb 2, 2020

I did a git bisect and confirmed the bug was introduced by this commit: f1d5ce0

The commit before that one does not have the bug

nicks added a commit to nicks/go that referenced this issue Feb 2, 2020
Fixes golang#36173

For reasons that are unclear to me, this commit:
golang@f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.
nicks added a commit to nicks/go that referenced this issue Feb 2, 2020
Fixes golang#36173

For reasons that are unclear to me, this commit:
golang@f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.
nicks added a commit to nicks/go that referenced this issue Feb 2, 2020
Fixes golang#36173

For reasons that are unclear to me, this commit:
golang@f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 2, 2020

Change https://golang.org/cl/217106 mentions this issue: cmd/go/internal/vet: print line numbers appropriately on list errors

nicks added a commit to nicks/go that referenced this issue Feb 4, 2020
Fixes golang#36173

For reasons that are unclear to me, this commit:
golang@f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.
nicks added a commit to nicks/go that referenced this issue Feb 4, 2020
Fixes golang#36173

For reasons that are unclear to me, this commit:
golang@f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.
nicks added a commit to nicks/go that referenced this issue Feb 5, 2020
Fixes golang#36173

For reasons that are unclear to me, this commit:
golang@f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.
nicks added a commit to nicks/go that referenced this issue Feb 6, 2020
Fixes golang#36173

For reasons that are unclear to me, this commit:
golang@f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.
nicks added a commit to nicks/go that referenced this issue Feb 11, 2020
Fixes golang#36173

For reasons that are unclear to me, this commit:
golang@f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.
nicks added a commit to nicks/go that referenced this issue Feb 11, 2020
Fixes golang#36173

For reasons that are unclear to me, this commit:
golang@f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.
nicks added a commit to nicks/go that referenced this issue Feb 14, 2020
Fixes golang#36173

For reasons that are unclear to me, this commit:
golang@f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.
nicks added a commit to nicks/go that referenced this issue Feb 17, 2020
Fixes golang#36173

For reasons that are unclear to me, this commit:
golang@f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 24, 2020

Change https://golang.org/cl/185345 mentions this issue: cmd/go: rationalize errors in internal/load and internal/modload

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 24, 2020

Change https://golang.org/cl/220718 mentions this issue: cmd/go/internal/vet: print line numbers appropriately on list errors

gopherbot pushed a commit that referenced this issue Feb 28, 2020
This change is a non-minimal fix for #32917, but incidentally fixes
several other bugs and makes the error messages much more ergonomic.

Updates #32917
Updates #27122
Updates #28459
Updates #29280
Updates #30590
Updates #37214
Updates #36173
Updates #36587
Fixes #36008
Fixes #30992

Change-Id: Iedb26d2e0963697c130df5d0f72e7f83ec2dcf06
Reviewed-on: https://go-review.googlesource.com/c/go/+/185345
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 24, 2020

Change https://golang.org/cl/224660 mentions this issue: cmd/go: avoid needing to manipulate ImportStack when constructing error

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 26, 2020

Change https://golang.org/cl/225677 mentions this issue: go/packages: add a workaround for different import stacks between go versions

gopherbot pushed a commit to golang/tools that referenced this issue Mar 27, 2020
…versions

Go 1.15+, as of CL 224660, puts the importing package on the top of the stack
(because it makes more sense in the errors). Look there by default and fall
back to second from top position if top of stack is the package itself.

Updates golang/go#36173

Change-Id: I1681089b4a18af9e535661668329ad32b1ba1936
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225677
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot gopherbot closed this in 9ceb1e5 Mar 27, 2020
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.

5 participants
You can’t perform that action at this time.