U1000: The "unused" linting rule has misleading behaviour when interacting with generated code

[adamroyjones]

staticcheck has, in my judgement, some misleading behaviour when it interacts with generated code.

Under some conditions, it produces an error message that doesn't suffice to resolve the underlying problem. (This was first reported here, which led me to here.)

Reproduction

The following example was produced with staticcheck 2022.1.3 (v0.3.3).

The "details" block at the bottom of this section describes the following directory structure (which I've also attached as a tarball here):

.
├── bar.go
├── Dockerfile
├── foo.go
├── go.mod
└── rc.sh

First, recreate this directory structure and set rc.sh to be executable.

1. If you run ./rc.sh, then staticcheck will report that the function foo is unused.

2. If you remove the first line from bar.go (that says that the file is generated) and run ./rc.sh, then staticcheck will report that both foo and bar are unused.

3. If you replace bar with Bar, then, irrespective of the presence of the "first line" in bar.go, no errors are reported.

In my view, the messages in 1 and 2 are misleading. In both cases, the issue is that bar isn't public (and therefore isn't used); foo is only unused as a consequence of bar not being used. This is especially a problem in 1, because there's no way to resolve the problem from the error message alone.

(Ed.: I've changed my mind about 2, so I've struck this through and rewritten it. See the comments below.)

In my view, the message in option 1 is misleading; there's no way to resolve the problem from the error message alone.

Details

// bar.go
// Code generated by hand. DO NOT EDIT.
package foo

func bar() {
	foo()
}

# Dockerfile
FROM golang:1.19-bullseye
RUN go install honnef.co/go/tools/cmd/staticcheck@latest
RUN mkdir /reprex
WORKDIR /reprex

// foo.go
package foo

func foo() {}

// go.mod
module staticcheck-reprex

go 1.19

# rc.sh
#! /usr/bin/env sh

docker build --tag staticcheck-reprex .

docker container run \
  --mount type=bind,source=$(pwd),destination=/reprex \
  -it \
  staticcheck-reprex \
  staticcheck ./...

[dominikh]

This is especially a problem in 1, because there's no way to resolve the problem from the error message alone.

I haven't yet verified that behavior, but that would indeed be a bug. If bar gets special treatment because it's defined in auto-generated code, then anything it uses should also be used, and foo shouldn't get flagged.

I can see why case 2 might be confusing, but I disagree with the conclusion that

In both cases, the issue is that bar isn't public (and therefore isn't used); foo is only unused as a consequence of bar not being used.

That is not a conclusion that U1000 could make automatically. We don't know whether the unexported function (bar) has become genuinely unused over time, or if it was meant to be exported. More often than not, a web of unexported functions that have used each other become unused over time, and the correct solution would be to delete both foo and bar.

[adamroyjones]

Thanks for the quick and thoughtful response! Thanks, as well, for all of your work on staticcheck—I find it such a valuable tool.

I haven't yet verified that behavior, but that would indeed be a bug. If bar gets special treatment because it's defined in auto-generated code, then anything it uses should also be used, and foo shouldn't get flagged.

The special treatment bar gets is that its U1000-related error is not reported because it's in generated code. I think in the example there are three reasonable options:

1. to neither flag foo nor bar, as foo is only called by bar and staticcheck chooses, at least by default, to not make determinations about generated code;
2. to flag both foo and bar, since it is probably an error—even in generated code—for there to be non-exported functions that aren't called; or
3. to flag only foo, but to note in the error message that it's called in generated code that isn't exported (which is perhaps a bit clumsy).

My instinct would be to have a piece of configuration that toggles between options 1 and 2, since both seem reasonable in different contexts. In the private example that triggered this investigation, we have control of the code generation, so option 2 would have (correctly) flagged an issue in the code generator. If we were at the mercy of uncontrollable code generation, then I think option 1 might feel a bit better.

Those are just feelings, though. You'll have a much better sense than I have of what would be best for staticcheck as a tool.

That is not a conclusion that U1000 could make automatically. We don't know whether the unexported function (bar) has become genuinely unused over time, or if it was meant to be exported. More often than not, a web of unexported functions that have used each other become unused over time, and the correct solution would be to delete both foo and bar.

Yes, quite right—if there's a whole subgraph that's unused (as in this case), it's quite right of staticcheck to report not just any leaf nodes but the whole set of nodes in the subgraph. It absolutely should do this: if foo and bar mutually depended on each other, then staticcheck should tell us that we can delete both, not that we can delete neither.

I'll edit the OP to strike that through. Thanks for setting me straight!

[dominikh]

to flag both foo and bar, since it is probably an error—even in generated code—for there to be non-exported functions that aren't called

We've added the exception when generated code had good reasons for having unused functions (or rather objects in general). For example, cgo involves code generation that unconditionally emits helpers that may not be needed. We can't really undo that, and it's a useful pattern to use in a lot of code generation.

My instinct would be to have a piece of configuration that toggles between options 1 and 2, since both seem reasonable in different contexts

We have a flag, -show-ignored, but it currently doesn't work with U1000: #1123

to neither flag foo nor bar, as foo is only called by bar and staticcheck chooses, at least by default, to not make determinations about generated code;

I think this is the only valid option. Internally, U1000 builds a graph of which objects use which; some objects are roots (exported functions, for example). The current way of ignoring unused objects in generated code incorrectly assumes that generated code is self-contained, and simply suppresses the output of a diagnostic; it doesn't affect the shape of the graph. This is why bar doesn't get flagged, but foo does: There is no path from a root to foo, so we flag foo. There is also no path to bar, but bar is in generated code, so we drop the diagnostic on the floor. Instead, we should ignore generated code by making it reachable from a root. That behavior could depend on the -show-ignored flag.

to flag only foo, but to note in the error message that it's called in generated code that isn't exported (which is perhaps a bit clumsy).

That feels like an overly specific solution to me, and I hope that it shouldn't be needed once we improve how ignoring in generated code works.