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: golint or govet should check for differently named conditional var #23313

Closed
purpleidea opened this issue Jan 3, 2018 · 12 comments

Comments

Projects
None yet
7 participants
@purpleidea
Copy link

commented Jan 3, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.9

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

GNU/Linux

What did you do?

Found a bug in my code. I wish the tools had found it for me.

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

Here is a snippet of my code:

if e := foo.Init(); err != nil {
        // whatever
}

It was an obvious typo that the used err var did not match. I think this should be a simple case to look for and report in either golint or govet. After a long time debugging, and tracing things down to this part of the code, I spotted the mistake.

I think this is a particularly relevant check to do, since a lot of security sensitive things might rely on checking the error carefully.

What did you expect to see?

golint or govet warning

What did you see instead?

Nothing.

Thanks!

@bradfitz bradfitz changed the title golint or govet should check for differently named conditional var cmd/vet: golint or govet should check for differently named conditional var Jan 3, 2018

@bradfitz bradfitz added this to the Go1.11 milestone Jan 3, 2018

@cespare

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2018

I've done this intentionally a few times. It's usually in code that wants to execute several statements but report the first error:

err := foo.X()
if e := foo.Close(); err == nil {
	err = e
}
return err

(Not saying this is definitely the best way to write this code, but the fact that the pattern exists intentionally out in the wild probably means that this is a no-go for a vet check.)

@purpleidea

This comment has been minimized.

Copy link
Author

commented Jan 3, 2018

@cespare If someone can come up with a sane use case for this, then that's why I suggested golint as well. But I've not found any. Either way, a warning should come out somewhere!

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

The specific snippet above already produces a compiler error (e defined and not used). As others pointed out, there are reasonable code snippets that would be caught by any attempt to catch this.

@rsc rsc closed this Jan 29, 2018

@purpleidea

This comment has been minimized.

Copy link
Author

commented Jan 29, 2018

@rsc Not true... Here's a longer example where the compiler doesn't catch this:

var err error
if err != nil {
        printf("blah")
}

if e := foo.Init(); err != nil {
        return errwrap.Wrapf(e, "some error")
}

Please re-open, thanks.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

What @rsc meant was: any attempt to report this error in vet is likely to cause false positives for correct code.

@purpleidea

This comment has been minimized.

Copy link
Author

commented Jan 29, 2018

@ianlancetaylor Can you provide one scenario where that sort of thing happens or run your fancy scripts on the stdlib and see if that's the case?

If so, please re-open and move to golint, otherwise re-open and leave here.

Thanks

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2018

@purpleidea See the comment by @cespare , above.

@purpleidea

This comment has been minimized.

Copy link
Author

commented Jan 30, 2018

@ianlancetaylor if you believe this is the case:

please re-open and move to golint

I know you folks probably have a lot of issues to wade through, but closing issues quickly without too much discussion and consensus is not awesome for a community.

@dominikh

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

please re-open and move to golint

golint has its own repository with its own issue tracker, and GitHub has no notion of moving issues across repositories. You're free to file the issue there yourself, but it will probably be similarly rejected there. Golint is primarily concerned with style, not correctness. And there is nothing inherently wrong with the code, as demonstrated by @cespare.

but closing issues quickly without too much discussion and consensus

There is consensus among the people responsible for vet, in that the proposed check would have false positives in valid code, and is therefore not suitable. This issue tracker isn't concerned with golint, which is maintained by other people, so any golint-specific discussion should be had on golint's issue tracker, see my note above regarding filing an issue there.

@purpleidea

This comment has been minimized.

Copy link
Author

commented Jan 30, 2018

@dominikh

  1. Didn't realize golint was elsewhere

  2. Since so much golang code consists of if err != ???, you'd think this common bug would be caught or warned about in some tool. I guess not!

AFAICT, this is literally one of the ways to get software in golang to "accidentally" miss a critical safety check, and as a result, I would have thought more folks would be interested in preventing this sort of potential security bug.

Thanks

@dominikh

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

One pattern that could be caught is the following:

if err != nil {
  return ...
}

if e := fn(); err != nil {
  ...
}

Because the second condition is known to be always false. It may still be out of scope for vet due to the increased complexity of that check (as it requires tracking path conditions and assignment to variables), but if it is of any consolation, a check like that will eventually make it into staticcheck.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2018

@purpleidea I'm sorry if it seems like we are closing issues too quickly. These issues are not new to us. We've seen and thought about them before. One of vet's guiding rules, as listed in cmd/vet/README, is that any tests must be precise. Any change in vet comes to close to being a language change. The question is not "do we want to warn about e := fn(); err != nil"; the question is whether we want to prohibit that construct. There is clearly no consensus about that.

I'm less familiar with golint.

@golang golang locked and limited conversation to collaborators Jan 30, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.