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

x/tools/go/packages: LoadSyntax doesn't error with missing files #29280

Open
mvdan opened this Issue Dec 15, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@mvdan
Copy link
Member

mvdan commented Dec 15, 2018

$ 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 <bradfitz@golang.org>
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 label Dec 15, 2018

@gopherbot gopherbot added this to the Unreleased milestone Dec 15, 2018

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 17, 2018

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

@matloob

This comment has been minimized.

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

go/packages: suppress go list errors when ad-hoc package doesn't exist
Updates golang/go#29280

Change-Id: Ie5a5dc1fef8f3d989b3a5fffb6c2ca66e97c143a
Reviewed-on: https://go-review.googlesource.com/c/154517
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Dec 18, 2018

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

This comment has been minimized.

Copy link
Member

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 13ba8ad772dfbf0f451b5dd0679e9c5605afc05d 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

stop on package load errors too
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

This comment has been minimized.

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
@matloob

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

dominikh commented Jan 8, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment