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: decide how to handle type checking failure #21287

Closed
ianlancetaylor opened this issue Aug 3, 2017 · 5 comments
Closed

cmd/vet: decide how to handle type checking failure #21287

ianlancetaylor opened this issue Aug 3, 2017 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

This issue is to decide how cmd/vet should handle type checking failures.

In https://golang.org/cl/7393052 (pre-1.1) type checking was added to cmd/vet. A type checking error would issue a warning and cause vet to exit with status 1.

Issue #4895 complained about this behavior, because it meant that the type checker had to be able to find imported packages. In that issue Russ said "It would definitely be nice for 'go vet' to be able to run in degraded mode. I ran it on a large uncompiled tree over the weekend and was pretty annoyed by all the import messages."

https://golang.org/cl/7399051 (also pre-1.1) addressed this problem by only issuing the warning, and setting the exit status, if vet were run with the -v option.

People rarely ran vet with the -v option, because there was no way to do so when using go vet; it could only be done by using go tool vet.

https://golang.org/cl/40112 (pre-1.9) changed cmd/go to pass more flags to vet. In particular, after that change, go vet -v invoked cmd/vet with the -v option.

This led to issue #21188 complaining that go vet -v now caused unexpected errors when a package was not available (for example, the special "C" package used by cgo). This was essentially a repeat of #4895, only with the -v option that is now easier to use.

https://golang.org/cl/52851 (also pre-1.9) changed cmd/vet so that a type checking error just prints a message with -v, rather than issuing a warning and thus changing the exit status to 1.

How do we want to handle this going forward?

  • Should a type checker error cause vet to exit with status 1?
  • Should vet change its exit status depending on whether -v is used?
  • Does it matter if the type checker error is specifically a missing import?
  • Does it matter if the missing import is "C"?

CC @robpike

@ianlancetaylor ianlancetaylor added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Aug 3, 2017
@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Aug 3, 2017
@mvdan
Copy link
Member

mvdan commented Aug 3, 2017

Thanks for the thorough explanation of the history of the issue, Ian.

Does it matter if the missing import is "C"?

Something to add is #6774 - eventually, the "C" import will no longer be an error. Perhaps that's an answer to your question. If the issue won't be fixed in time for 1.10 it might not be the right answer, though.

Should vet change its exit status depending on whether -v is used?

I strongly believe this is a bad idea, because it's counter-intuitive. It's also worth noting that since -v was useless in go vet up until 1.9, the flag has never actually changed the exit status.

@robpike
Copy link
Contributor

robpike commented Aug 3, 2017

@robpike cc'ing myself.

@robpike
Copy link
Contributor

robpike commented Aug 3, 2017

I suggested we check in the CL that restores prior behavior, however awkwardly, and then address this in 1.10. It does not need to be a release blocker.

@rsc
Copy link
Contributor

rsc commented Oct 30, 2017

https://go-review.googlesource.com/c/go/+/74356 is for #18084 but the idea was to also hook it up to #16086, so that when run as "go vet", vet can expect that it will have complete type information and no magic import "C" to worry about. Then it should be a fatal error in that mode for vet to fail to typecheck.

So the decision here is "go vet" will be fixed so that it can always typecheck, and then failure to typecheck will be a fatal error. Given that decision, this is a duplicate of #16086.

@gopherbot
Copy link

Change https://golang.org/cl/121455 mentions this issue: doc: document new vet behaviour for typechecking failures

gopherbot pushed a commit that referenced this issue Jul 17, 2018
Since Go1.10, go test runs vet on the tests before executing them.

Moreover, the vet tool typechecks the package under analysis with
go/types before running. In Go1.10, a typechecking failure just caused
a warning to be printed. In Go1.11, a typechecking failure will cause
vet to exit with a fatal error (see Issue #21287).

This means that starting with Go1.11, tests that don't typecheck will
fail immediately. This would not normally be an issue, since a test
that doesn't typecheck shouldn't even compile, and it should already
be broken.

Unfortunately, there's a bug in gc that makes it accept programs with
unused variables inside a closure (Issue #3059). This means that a
test with an unused variable inside a closure, that compiled and
passed in Go1.10, will fail in the typechecking step of vet starting
with Go1.11.

Explain this in the 1.11 release notes.

Fixes #26109

Change-Id: I970c1033ab6bc985d8c64bd24f56e854af155f96
Reviewed-on: https://go-review.googlesource.com/121455
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Jun 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants