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: "could not import" warning message not visible without -v. #9439

Open
dmitshur opened this Issue Dec 24, 2014 · 6 comments

Comments

Projects
None yet
5 participants
@dmitshur
Member

dmitshur commented Dec 24, 2014

Using Go 1.4 and latest tools subrepo:

~ $ go version
go version go1.4 darwin/amd64
~ $ gostatus -v ...cmd/vet
     golang.org/x/tools/...

By default, vet is supposed to detect "Suspicious calls to functions in the Printf family." I've noticed that it sometimes did not do that reliably (it would detect some cases but not others). I wrote the following .go file that reproduces the issue for me:

package main

import (
    "log"
    "net/url"

    "golang.org/x/oauth2"
)

func main() {
    thirdParty := oauth2.Config{
        ClientID: "some-string", // A string.
    }
    stdLib := url.URL{
        Path: "some-string", // A string.
    }

    log.Printf("Hello, %d.\n", thirdParty.ClientID) // go vet does _not_ catch this.
    log.Printf("Hello, %d.\n", stdLib.Path)         // But it catches this.
}

What did you expect to see?

Both printf verb usage issues should be reported, not just one.

main.go:18: arg thirdParty.ClientID for printf verb %d of wrong type: string
main.go:19: arg stdLib.Path for printf verb %d of wrong type: string

What did you see instead?

Here is my output of vet on that file/package:

main.go:19: arg stdLib.Path for printf verb %d of wrong type: string

It detects the bad printf verb usage for a string type from standard library, but not external library.

It would be great if vet were more reliable, so I could depend on it to catch all printf verb misuse when possible (and it's certainly possible here).

@dmitshur

This comment has been minimized.

Member

dmitshur commented Dec 24, 2014

While writing this up (this is an issue I discovered long ago and added it to my TODO list to report it) just now, I investigated further and found out why it happens.

You may or may not be able to reproduce it in your setup if you follow the steps above. It may be an issue or it may be working as intended.

Cause

The reason vet did not catch the thirdParty.ClientID bad printf usage case is because I had the third party package source only, but it wasn't installed (no entry in $GOPATH/pkg). Running it with -v option displays a warning message:

vet: main.go:8:2: could not import golang.org/x/oauth2 (can't find import: golang.org/x/oauth2)
Checking file main.go
main.go:21: arg stdLib.Path for printf verb %d of wrong type: string

So, I could do go run main.go on that file because go run and go build will build all packages that are not installed from source on demand. (Due to unrelated complicated reasons, I tend to have very few packages installed and rely on go build always building everything from source.)

After I did go install golang.org/x/oauth2, running vet produced expected results.

Possible solutions

I see two possible solutions:

  1. Either make vet behave more intelligently like go build and go run, if the source is present but compiled package is missing, build it temporarily on demand or process the source instead. This way vet will always work reliably.

  2. Right now vet is not 100% reliable because it may or may not work depending on whether you have some packages installed. However, the exit code is 1 regardless whether:

    • vet ran successfully, processed all files and found some issues to report,
    • vet did not run successfully due to packages not being installed or missing source.

    There should be some (machine-readable) way to distinguish a failed vet run vs. a successful run that found issues.

I think fixing point 2 is easy and should be done. Fixing point 1 may be too complicated and not worth it.

In fact, running vet with the entire golang.org/x/oauth2 package missing (no source, no installed) - so that even go build would fail - still results in the exact same incomplete output and exit code 1.

@dmitshur

This comment has been minimized.

Member

dmitshur commented Dec 24, 2014

  1. Right now vet is not 100% reliable because it may or may not work depending on whether you have some packages installed. However, the exit code is 1 regardless whether:
  • vet ran successfully, processed all files and found some issues to report,
  • vet did not run successfully due to packages not being installed or missing source.

There should be some (machine-readable) way to distinguish a failed vet run vs. a successful run that found issues.

Actually, that might not be correct.

What's most important is that there is always a (machine-readable) way to distinguish between these 2 cases:

  1. A successful vet run that found 0 issues,
  2. Either successful run that detected 1 or more issues or failed to check everything because of missing packages.

And I think that's currently the case. The exit code from vet seems to be always 1 even if it found no issues, but failed to import some package (meaning there may be issues found once package is successfully imported).

So, as long as you get exit code 0 with no output, you can be sure there are no vet issues. If you get exit code 1, you need to investigate (even if there is no output, no errors caught) until the exit code becomes 0.

If that's really the case, then this is likely a non-issue. While it would be nice if vet could be more capable at loading/processing packages from source only and display issues right away, it's not critical as long as it can be reliably used to verify there are no issues. I'll let you decide what should be done or if this can be closed.

@minux

This comment has been minimized.

Member

minux commented Dec 25, 2014

I think it's working as intended.

However, I do want to propose we make the "couldn't load import" message visible
without -v, at least as a warning.

@mikioh mikioh added the repo-tools label Dec 25, 2014

@dmitshur

This comment has been minimized.

Member

dmitshur commented Dec 25, 2014

However, I do want to propose we make the "couldn't load import" message visible
without -v, at least as a warning.

I agree with that.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@rsc rsc changed the title from tools/cmd/vet: printf verb checking doesn't always work on non-standard package types to x/tools/cmd/vet: printf verb checking doesn't always work on non-standard package types Apr 14, 2015

@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015

@rsc rsc removed the repo-tools label Apr 14, 2015

@dmitshur dmitshur changed the title from x/tools/cmd/vet: printf verb checking doesn't always work on non-standard package types to x/tools/cmd/vet: "could not import" warning message not visible without -v. May 17, 2015

@dmitshur

This comment has been minimized.

Member

dmitshur commented May 17, 2015

I've updated the issue title to reflect the latest status of this issue.

Everything is working as intended, except that the "could not import" warning message should be displayed even if -v is not specified, otherwise the output can be visually misleading. The non-zero exit status is less visible to humans.

@minux minux changed the title from x/tools/cmd/vet: "could not import" warning message not visible without -v. to cmd/vet: "could not import" warning message not visible without -v. Sep 23, 2015

@seanrees

This comment has been minimized.

seanrees commented Jul 20, 2016

I'm interested in working on this as this behaviour of govet caused me some confusion recently.

I did some digging. The way this fails is:
cmd/vet/main.go: doPackage creates a local Package and calls check() on it
cmd/vet/types.go: (*Package).check() creates go/types.Config and calls Check(). Check ultimately returns an error when the import fails. There isn't a good way to determine an import failure from any other type of error from Check.

When check() creates types.Config, it builds it with importer.Default(). This provides a convenient hook pass a wrapped Importer/ImporterFrom, one that delegates all calls but collects failure evidence.

check() could then update the local Package with failed imports, which would then be available to go vet for error reporting. I would suggest keeping a map[string]bool in the package var and reporting any failed imports at the end. E.g:

if len(failedImports) > 0 {
warnf("unable to import the following packages, go install them and rerun: ...")
}

I could have a CL for this in a few days if this SG.

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