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.

Sign up for GitHub

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

Jump to bottom

cmd/go: go list -e should list a package containing an error if files named on the command line do not exist #29280

Closed
mvdan opened this issue Dec 15, 2018 · 15 comments
Closed

cmd/go: go list -e should list a package containing an error if files named on the command line do not exist #29280

mvdan opened this issue Dec 15, 2018 · 15 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Backlog

Comments

@mvdan
Copy link
Member

mvdan commented Dec 15, 2018 •
edited
Loading 

$ go version
go version devel +47fb1fbd55 Fri Dec 14 16:18:51 2018 +0000 linux/amd64
$ cd go/src/golang.org/x/tools
$ git show HEAD --no-patch
commit 3c39ce7b61056afe4473b651789da5f89d4aeb20 (HEAD -> master, origin/master, origin/HEAD)
Author: Brad Fitzpatrick <[email protected]>
Date:   Fri Dec 14 14:45:46 2018 +0000

    tip: fix, update tip.golang.org
[...]
$ go install ./go/packages/gopackages
$ gopackages missing.go; echo $?
gopackages: go [list -e -json -compiled -test=false -export=false -deps=true -- missing.go]: exit status 1: stat missing.go: no such file or directory
1
$ gopackages -mode syntax missing.go; echo $?
0

Seems to me like the LoadSyntax mode should error too. Found while porting https://github.com/mvdan/gogrep from go/loader to go/packages, since some error test cases stopped erroring as expected.

/cc @ianthehat @matloob
@mvdan mvdan added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 15, 2018
@gopherbot gopherbot added this to the Unreleased milestone Dec 15, 2018
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/154517 mentions this issue: go/packages: suppress go list errors when ad-hoc package doesn't exist

@matloob
Copy link
Contributor

matloob commented Dec 17, 2018

The underlying problem is that go list -e doesn't exit 0 (as it should) when an ad-hoc package doesn't exist. I've sent a cl to suppress the error in go packages, and then I'll re-purpose this bug to target that issue.

gopherbot pushed a commit to golang/tools that referenced this issue Dec 17, 2018
@matloob
go/packages: suppress go list errors when ad-hoc package doesn't exist
ae5b881 
Updates golang/go#29280

Change-Id: Ie5a5dc1fef8f3d989b3a5fffb6c2ca66e97c143a
Reviewed-on: https://go-review.googlesource.com/c/154517
Reviewed-by: Ian Cottrell <[email protected]>
@mvdan
Copy link
Member Author

mvdan commented Dec 18, 2018 •
edited
Loading

Thanks for the quick reply. It sounds to me like most tools out there want to check for most or all errors, and the fact that Load returns an error seems to tell that it's enough to check that one. So it can be easy to forget to check each package's Errors field.

Perhaps this should be clearer in the docs, e.g. via an example that most static analysis tools should follow.

Edit: I just realised that the package example does include packages.PrintErrors(pkgs) - I had missed that.

@mvdan
Copy link
Member Author

mvdan commented Dec 18, 2018

Here's another bug - missing files in syntax mode produce no command-line-arguments package at all, which seems like a bug to me too:

$ gopackages -mode syntax empty.go
Go package "command-line-arguments":
        package
        has complete exported type info
        -:
empty.go:1:1: expected 'package', found 'EOF'
$ gopackages -mode syntax missing.go

This is on x/tools 13ba8ad and go version devel +47fb1fbd55 Fri Dec 14 16:18:51 2018 +0000 linux/amd64.

mvdan added a commit to mvdan/gogrep that referenced this issue Dec 18, 2018
@mvdan
stop on package load errors too
b332c55 
I had forgotten to check those. Otherwise, we might be ignoring parse or
typecheck errors.

We can't fix the TODO in load_test.go yet, because it looks like there's
another bug in go/packages. See golang/go#29280.
@dominikh
Copy link
Member

dominikh commented Jan 4, 2019

It should be noted that this issue is (no longer?) limited to LoadSyntax. It occurs in all modes.

desktop ~ $ for mode in files imports types syntax allsyntax; do gopackages -mode $mode /does/definitely/not/exist.go; echo $?; done
0
0
0
0
0

@dominikh dominikh mentioned this issue Jan 4, 2019
Closed
@matloob
Copy link
Contributor

matloob commented Jan 8, 2019

@dominikh I think the 0 exit status is working as intended. In these cases, go/packages should return a package with a non-empty Errors field. Is it not doing that?

@dominikh
Copy link
Member

dominikh commented Jan 8, 2019

@matloob for me, it returns zero packages and no error.

@mvdan
Copy link
Member Author

mvdan commented Apr 5, 2019

@matloob I think you forgot to repurpose this issue?

@matloob matloob changed the title x/tools/go/packages: LoadSyntax doesn't error with missing files cmd/go: go list -e should exit 0 with a package containing an error when ad-hoc package doesn't exist Apr 5, 2019
@matloob
Copy link
Contributor

matloob commented Apr 5, 2019

@mvdan I retitled the issue, sorry about that!
@dominikh Hmm... let me look into that...

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/171018 mentions this issue: go/packages: add a work around for go list behavior for missing ad-hoc package

gopherbot pushed a commit to golang/tools that referenced this issue Apr 8, 2019
@matloob
go/packages: add a work around for go list behavior for missing ad-ho…
cb42bbe 
…c package

If a file in an ad-hoc package doesn't exist, go list should exit 0 and
return an dummy package with an error set on it. Since it doesn't do that
yet, add a work-around.

Updates golang/go#29280

Change-Id: I6019f28ce4770582f274919d1aa35d85a634687e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/171018
Run-TryBot: Michael Matloob <[email protected]>
Reviewed-by: Ian Cottrell <[email protected]>
@bcmills
Copy link
Contributor

bcmills commented Apr 19, 2019

@matloob, could you clarify the issue with cmd/go? I don't know what an “ad-hoc” package is or what it means for such a thing not to exist.

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 19, 2019
@ianthehat
Copy link

An ad-hoc package is the term used throughout the tools for one formed when you pass the go command a list of files.
@matloob means that one or more of the files did not exist, which should result in an error on the package in the output, not go list failing.

@bcmills bcmills changed the title cmd/go: go list -e should exit 0 with a package containing an error when ad-hoc package doesn't exist cmd/go: go list -e should list a package containing an error if files named on the command line do not exist Apr 19, 2019
@bcmills bcmills removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 19, 2019
@bcmills bcmills modified the milestones: Unreleased, Go1.13 Apr 19, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@gopherbot
Copy link
Contributor

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

@bcmills bcmills mentioned this issue Sep 13, 2019
Closed
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@bcmills
Copy link
Contributor

bcmills commented Dec 6, 2019

@mvdan, this appears to be fixed at head. Can you confirm?

~/src/golang.org/x/tools$ go version
go version devel +c49fd86ad9 Fri Dec 6 15:13:20 2019 -0500 linux/amd64

~/src/golang.org/x/tools$ gopackages -mode syntax missing.go
Go package "missing.go":
        package
        has complete exported type info
        -: cannot find module providing package missing.go: unrecognized import path "missing.go": https fetch: Get "https://missing.go/?go-get=1": dial tcp: lookup missing.go on 127.0.0.1:53: no such host

CC @jayconrod

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 6, 2019
@mvdan
Copy link
Member Author

mvdan commented Dec 6, 2019

Yes, I no longer see this bug - I checked both module mode and GOPATH mode.

There were a number of CLs above, so it's hard to see exactly when this happened. I assume go list got a proper fix at some point :)

@mvdan mvdan closed this as completed Dec 6, 2019
gopherbot pushed a commit that referenced this issue Feb 28, 2020
cmd/go: rationalize errors in internal/load and internal/modload
5a61de3 
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 <[email protected]>
Reviewed-by: Jay Conrod <[email protected]>
@golang golang locked and limited conversation to collaborators Dec 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
8 participants
@dominikh @rsc @andybons @ianthehat @mvdan @bcmills @gopherbot @matloob