U1000 error for unused functions which are used in skipped test

[leslie-qiwa]

Thanks for the great tool, and we used it in our CI heavily. Just from last Friday, our CI got broken because that static check starts complaining U1000 error for unused functions which are used in skipped test. Below are sample codes which are reproducible. The running commands is staticcheck .. Every CI run we get the latest tool through go get -u honnef.co/go/tools/cmd/staticcheck

package main

import "testing"

func test() {
}

func TestSum(t *testing.T) {
  t.Skip("skipping for test")
  go test()
}

For now, we have to add lint:file-ignore U1000 Ignore report to skip the error. Please let me know if something is changed recently. Thanks again.

[dominikh]

The reason this changed is because staticcheck is now "smarter", recognizing that no code after t.Skip will execute. This effectively makes the rest of TestSum invisible to U1000, which means test isn't used.

I'll have to think about this specific case and whether we need to work around it.

[leslie-qiwa]

Thanks for replying. I think changing test case to skip may happen quite often because of some other dependency failure, and developers don't have time to investigate temporarily. In this case, people probably don't want to see other static check failure because of the change.

Just my 2 cents.

[cespare]

This flags a couple of things in our repo too.

[cespare]

(but that is very clever analysis!)

[feldgendler]

Hi! Do you think you can do something about this? We are using golangci-lint and had to disable unused for *_test.go files because of this issue.

[dominikh]

Unfortunately there isn't a quick and easy fix for this. It will require fundamental changes to the implementation of the check. I do have these changes on my todo list, but it will take months until I get to it.

In the meantime, you will have to make use of the various ways of ignoring problems – disabling the check, ignoring the specific instances you consider wrong (via //lint:ignore in staticcheck, or //nolint in golangci-lint), or explicitly marking the entity used by assigning it to _ on the package level.

[feldgendler]

Marking every dependency of a skipped test with nolint is too much trouble (and then you'll have to remember to remove those marks later). Instead, I'll share a hack that we found, in case other subscribers to this issue are interested:

var skip = func(t *testing.T) {
    t.SkipNow()
}

func TestCurrentlyDisabled(t *testing T) {
    skip(t)
    ...
}

This way, unused doesn't detect that further code in TestCurrentlyDisabled is unreachable, and doesn't tag the stuff it calls as unused.

[dominikh]

Okay, I have two options:

1. rewrite the entire check to operate on the AST instead of the IR.
2. pretend that t.SkipNow does not actually abort execution

Option 1 would be an annoying amount of work, and constitute the third (or fourth? I can't remember) full rewrite of unused. I'd rather do anything else.

Option 2 is a quick and easy fix, but it is not localized to unused. It affects the IR as a whole. A side-effect of this is that, for example, we might flag nil dereferences in skipped code.

Option 2 is also specific to SkipNow. If you have other patterns of skipping over code, such as calling runtime.Goexit yourself, or panicking, possibly not even in test code, then we would still remove the unreachable code and mark things as unused. In other words, the check would still be based on the effective IR, not the code's syntactic form.

I believe limiting it to SkipNow is fine. I've only ever seen people complain because of SkipNow, but it's also why I'm writing this comment: so anyone who ran into this issue, but not because of t.SkipNow, can speak up.

[cespare]

I've only experienced this problem with SkipNow.

Option 2 is a quick and easy fix, but it is not localized to unused. It affects the IR as a whole. A side-effect of this is that, for example, we might flag nil dereferences in skipped code.

This would be fine for me, because I don't use SkipNow to guard completely broken code (I would comment it out or delete it). Other people might, I suppose.

I didn't expect unused to understand that SkipNow prevents subsequent code from running and make inferences based on that, so I think the flip side of that is that if it starts flagging new stuff in skipped tests, it shouldn't be too surprising for people or hard to understand.

[feldgendler]

Option 3 would be to invent a way to declare that a certain function does not return, e.g. in a special comment.