Go-tools: No replacement for megacheck -unused.exported in staticcheck 2019.1

[bconway]

I've checked both the web site docs and the changelog page, and can't seem to find a replacement for megacheck's -unused.exported (also unused -exported) in the new release. Previously this fell under the umbrella of U1000, but I'm not seeing much there referencing being exported.

If possible, can this functionality be re-added to staticcheck? If not and it's gone for good, calling it out in the changelog or docs may be prudent (unless I have missed something).

Thanks!

[dominikh]

There doesn't seem to be a way to do this currently. We don't want to get rid of the feature, though. Will have to think about how to make this option available again.

[ainar-g]

Just as a thought, you could rename U1000 to "unused unexported identifier in a package" and introduce a new U1001 check, disabled by default, to mean "unused exported identifier in the project".

[dominikh]

Implementing unused's whole program mode in the go/analysis framework is likely to become very hacky. There's a decent chance we won't be doing it. Whole program mode never behaved flawlessly, anyway, due to all the guessing that's required due to reflection.

[ainar-g]

@dominikh Hacky or not, this functionality has become essential to my workflow, and I don't think I am the only one. I am ready to sponsor the port, within reasonable limits, if it brings noticeable improvements in speed and RAM usage. I love the work you've been doing so far, and if go/analysis doesn't support the kind of whole-project analysis, it's probably an issue with the framework, that will probably come up later anyway. Otherwise I'll probably just use an old version of unused until it stops working, and then fork it.

(Edited to be nicer.)

[dominikh]

I will keep that in mind and invest some time in experimenting with this. As far as go/analysis being the issue: yes and no. It's designed for a specific thing (see https://docs.google.com/document/d/1-azPLXaLgTCKeKDNg0HVMq2ovMlD-e7n1ZHzZVzOlJk/edit) and for the majority of analyses, it makes sense. The things it prohibits tend to also be the things that are so expensive that they don't scale.

The specific thing we want to do should be able to scale, and is technically possible to implement within the constraints of the framework. It will, however, require custom logic in the analysis driver (at least that's the only solution I've come up with so far), which means something like vet or golangci-lint can't just reuse our unused check; they'd have to add some custom logic to their driver. Which is fine by me.

Edited to be nicer

I wasn't bothered by the original version :)

[ainar-g]

@dominikh Thanks for the response! I've noticed in the original paper that they define “modular” as

one that inspects one package at a time but can save information from a lower-level package and use it when inspecting a higher-level package, analogous to separate compilation in a toolchain.

This made me thing of my unused workflows. They usually centre around binaries: everything that is not used in one or several main packages should be marked. Would implementing (a subset of) this check be simpler if we only defined it as “find exported identifiers in this module that are not used, directly or indirectly, by these binaries”?

[dominikh]

The issue is the current API of go/analysis. You report findings (such as unused identifiers) while you process a package, and store information for the dependents. However, in whole program mode, we don't know if an identifier is unused until we have processed the dependents of the package. Essentially, the flow of information in go/analysis is in the opposite direction of what we need in unused.

My intended solution to this is to report both unused _and_ used identifiers, and do a final merging pass in the analysis runner itself. That is the hacky approach I mentioned earlier.

That aside, your simplification isn't much of a simplification, if any. That's pretty much how -exported currently works. main functions and tests are roots of the reachability graph, anything that isn't used by them is unused.

[dominikh]

This is fixed in the incr branch, which will eventually get merged into master.