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.

[dominikh]

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

We already know when a function doesn't return. That's why this issue exists in the first place. During IR construction, we note that any code after a call to SkipNow (and variations that call SkipNow) is unreachable and remove it. Which is why the unused pass doesn't see uses of identifiers that occur after the call of SkipNow.

That is,

func bar() {}

func TestFoo(t *testing.T) {
    t.SkipNow()
    bar()
}

translates to

func TestFoo(t *testing.T):
b0: # entry
	t1 = Parameter <*testing.T> {t}
	t2 = FieldAddr <*testing.common> [0] (common) t1
	t3 = Call <()> (*testing.common).SkipNow t2
	Jump → b1

b1: ← b0 # exit
	Return

Subsequently, unused sees no call to bar. However, as this issue shows, people don't expect that behavior. Uses of SkipNow are ephemeral, and do not warrant deleting temporarily unused code.

Thus we arrive at the two options: implement the check in terms of the AST, or pretend that SkipNow returns.

[feldgendler]

How do you know that about SkipNow in the first place? I suppose there is a list of known non-returning functions somewhere. A special comment could override that knowledge, so we could take SkipNow out of the no-return list.

[dominikh]

There is a known list of functions that terminate the process or the goroutine, and we hard-code that list. This is syscall.Exit, runtime.Goexit, and some runtime-internal functions (such as runtime.throw). Almost all other information derives from those basis functions. If a function unconditionally calls runtime.Goexit, then we know that it won't return, and so on.

I say almost because there is a list of hand-coded rules for functions where we cannot detect that they call either due to some conditionals, mostly in logging libraries. It is trivial to extend this list to treat SkipNow as a function that does return.

The problem here isn't implementing option 2, but deciding whether option 2 is the right approach. The implementation is as simple as adding a new case to

go-tools/ir/exits.go

Line 9 in f5dad99

switch obj.Pkg().Path() {

and to claim that SkipNow does neither exit nor unwind.

[tuxslayer]

So, any good workaround for this?

[dominikh]

#633 (comment) contains one workaround for now.

[danielchatfield]

I would be supportive of landing the change to special case Skip here, that seems like the most pragmatic solution.

[seebs]

I just ran into this. The issue, I think, is that it seems weird to call a function "unused" if you can't compile without it. I think there's a distinction to be had here between "isn't used" in the sense of "doesn't matter to program output" and "isn't used" in the sense of "doesn't matter to program compilation".

[dominikh]

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.