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

doc: vet check included in gc needs to be documented #26109

Closed
hyangah opened this issue Jun 28, 2018 · 10 comments
Closed

doc: vet check included in gc needs to be documented #26109

hyangah opened this issue Jun 28, 2018 · 10 comments

Comments

@hyangah
Copy link
Contributor

@hyangah hyangah commented Jun 28, 2018

vet check seems to be included in gc by default but not documented in go1.11 release note yet (sorry, I didn't search enough to find related issue/proposal/change to link yet).

This behavior change resulted in build-failures for codes that don't pass vet currently and could be misunderstood as a regression in go1.11. For example, with go1.11 (or tip), go test on
github.com/spf13/cobra will fail due to failure to pass vet check.

$ go version
go version devel +257d6c48e0 Thu Jun 28 03:12:01 2018 +0000 linux/amd64

$  go test github.com/spf13/cobra
# github.com/spf13/cobra
../github.com/spf13/cobra/command_test.go:1105:3: parentPersPreArgs declared but not used
../github.com/spf13/cobra/command_test.go:1109:3: parentPersPostArgs declared but not used
vet: typecheck failures
FAIL	github.com/spf13/cobra [build failed]

BTW, the vet error message is confusing because the parentPersPreArgs and parentPersPostArgs in the test are 'used' (literally). But for the purpose of vet check, the vars are 'not used' because they are never read.

@hyangah hyangah added this to the Go1.11 milestone Jun 28, 2018
@hyangah hyangah changed the title doc: vet check included in gc needs to be documented (go1.11) doc: vet check included in gc needs to be documented Jun 28, 2018
@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Jun 28, 2018

That's not a vet error message. That file is failing the type-checking phase that precedes the vet checks.

The reason the file fails typecheking is that this:

var parentPersPreArgs string

PersistentPreRun: func(_ *Command, args []string) {
    parentPersPreArgs = strings.Join(args, " ")
},

doesn't count as using the parentPersPreArgs variable. The program should not compile, but it does because of #3059. But go/types, which is used by vet to type-check, correctly marks the variable as unused.

@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Jun 28, 2018

Root cause is the decision taken on #21287 (cmd/vet: decide how to handle type checking failure). I quote:

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.

This happened in go1.10, but AFAIK it wasn't documented in the release notes. I don't see it here:

https://golang.org/doc/go1.10#vet

We could document this in the 1.11 release notes(?)

@hyangah

This comment has been minimized.

Copy link
Contributor Author

@hyangah hyangah commented Jun 28, 2018

Is the error message misleading?

vet: typecheck failures

anyway, regardless of where the origin of this change is, the changed behavior should be documented clearly. It's a newly observed behavior from 1.11.

@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Jun 28, 2018

Is the error message misleading?

I think it's correct, but too brief. Maybe it should be changed to something like "vet: the package did not typecheck"

the changed behavior should be documented clearly

Agree, but what changed? vet typechecks since 1.10, and test runs vet since 1.10. Why is this failing now on tip and not on 1.10?

@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Jun 28, 2018

Ah, my bad. This is false:

vet typechecks since 1.10

it was put in, disabled to fix a go/types issue, and then re-enabled again during the 1.11 cycle, here:

https://go-review.googlesource.com/c/go/+/108555

So it should be documented.

@ALTree ALTree added NeedsFix and removed NeedsInvestigation labels Jun 28, 2018
hyangah added a commit to hyangah/cobra that referenced this issue Jun 28, 2018
As discussed in golang/go#26109, vet typecheck is enabled
in go1.11 and the command_test.go can't be compiled any
more with go1.11 due to the unread variables in the
command_test.go.

Instead of removing the unused variables, this CL reads the
variables and compares the values against the current
behavior so when the related issue is fixed, this test can
be updated accordingly.
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 28, 2018

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

eparis added a commit to spf13/cobra that referenced this issue Jun 29, 2018
As discussed in golang/go#26109, vet typecheck is enabled
in go1.11 and the command_test.go can't be compiled any
more with go1.11 due to the unread variables in the
command_test.go.

Instead of removing the unused variables, this CL reads the
variables and compares the values against the current
behavior so when the related issue is fixed, this test can
be updated accordingly.
@gopherbot gopherbot closed this in bccbf59 Jul 17, 2018
@therealplato

This comment has been minimized.

Copy link

@therealplato therealplato commented Nov 11, 2018

Sorry, I'm experiencing this error and I'd like to know what I'm supposed to be doing instead of

var parentPersPreArgs string

PersistentPreRun: func(_ *Command, args []string) {
    parentPersPreArgs = strings.Join(args, " ")
},

Why is this considered a failure?

@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Nov 11, 2018

Sorry, I'm experiencing this error

Where? When?

I'd like to know what I'm supposed to be doing

Use the parentPersPreArgs variable in some way, or remove it from the code.

Why is this considered a failure?

Because unused variables are not allowed in Go (at least in gc and in go/types), and parentPersPreArgs is an unused variable.

@therealplato

This comment has been minimized.

Copy link

@therealplato therealplato commented Nov 12, 2018

@ALTree I'm experiencing it with the same closure usecase as you posted. By my reading, parentPersPreArgs is "used" when strings.Join assigns its result to it. I don't understand what makes it "unused."
Until I can refactor the code to improve the poor scoping, I have worked around this "typecheck" issue with _ = parentPersPreArgs.

@dominikh

This comment has been minimized.

Copy link
Member

@dominikh dominikh commented Nov 12, 2018

A value that is only ever written to but never read from is effectively unused. go/types (which vet uses) is more strict about this than the compiler.

@golang golang locked and limited conversation to collaborators Nov 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.