go-tools: U1000 error for unused functions which are used in skipped test

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.

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 4
Comments: 18 (7 by maintainers)

Most upvoted comments

Note that I am fully aware that the current behavior is bad, I don’t need to be convinced of that. This problem, and most of the other problems of U1000, are caused by us using our IR to implement it, even though it’s not an ideal fit for the problem.

The quick workaround on our end would be to give special treatment to the Skip function. But this will a) affect other checks negatively b) only fix this specific instance of the problem.

The proper solution is to reimplement the check and operate on a representation that is closer to the actual source code. Doing so is near the top of my todo list.

dominikh on Jan 24, 2022

Okay, I have two options:

rewrite the entire check to operate on the AST instead of the IR.
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.

dominikh on Apr 16, 2020

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.

feldgendler on Mar 31, 2020

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.

dominikh on Oct 28, 2019