Go-tools: staticcheck: reimplement errcheck using our framework

[dominikh]

It should be rather straightforward to implement errcheck using our framework. Do it for fun, and also to experiment with some SSA aided features. For example, we could rather easily ignore unchecked errors on (*os.File) if the file is known to be opened read-only.

We may either try to upstream our ideas, assuming adding SSA there is feasible, or maintain our own errcheck check in staticcheck.

[mvdan]

This sounds like a great idea. The only reason I can't use the current errcheck regularly is how it complains about ignoring errors that don't matter, like the one you suggested. Errors that will always be nil also come to mind, like any write funcs on a bytes.Buffer.

[myitcv]

@mvdan @dominikh if we are creating a list of things to consider/implement for this issue, might I ask that we debate/consider the following:

// open myfile for reading
fi, err := os.Open("myfile") 
defer fi.Close()

I seem to recall reading somewhere some thoughts/guidance from @ianlancetaylor that in such situations it is safe enough to ignore the error from the close, because we were simply reading from a file (and if we can't close that file handle we've got bigger, more systemic problems). Contrast when you are writing to a file you must have checked the error on the write to be sure everything was flushed to disk (I'm badly paraphrasing here because I can't find the comment/message to reference)

Thoughts?

[mvdan]

@myitcv this is exactly what dominik mentioned above and what I agreed on - close errors on files that are open read-only aren't useful and need not be checked. Unless I misunderstand what you mean?

[myitcv]

@mvdan sorry, my brain totally skipped that when reading the headline issue... looking back I've no idea how I managed to miss that. Apologies for the noise and just to reiterate my 👍

[dominikh]

Errors that will always be nil also come to mind, like any write funcs on a bytes.Buffer

@mvdan Errcheck already ignores those errors, thanks to https://github.com/kisielk/errcheck/commit/4b72a619bb203aaab86628f64f328bbc2ad19e26

[mvdan]

Ah, hadn't noticed that was in place.

That's a bit limited though. For example, I assume it won't work with transitive funcs/types like io.WriteString or bufio.Writer. Perhaps that's too much to ask of the original tool if it makes no use of types/ssa.

[dominikh]

The errcheck branch tracks my development on this. It currently contains an implementation of errcheck's core logic, and the beginning of automatic nil error detection.

I'll expand on some of the choices I've made:

• Assigning an error to the blank identifier always silences the error. Upstream errcheck has a flag, we don't. It's not worth the extra UI.
• We do not complain about unchecked type assertions. Errcheck does (given a flag, I believe). I find this a) out of scope b) too noisy. Some type type assertions are known to always succeed.

• Upstream errcheck looks for all types that implement the error interface. We only care about functions that return actual error. To quote a9de59a32800f8b81efcd755da9f78a650f92a45:

  Our implementation will not check for types that implement the error
  interface. We only care about functions that return the actual error
  interface itself. Doing anything else is a code smell in itself that we
  don't need to support.

• This is still very much an experiment. I haven't yet decided whether I will upstream the SSA bits, or if I will include my own errcheck in staticcheck. I'm sort of leaning towards the latter, because that way my errcheck will be able to benefit from future improved analyses, such as my VRP work. But we'll see.

[mvdan]

Some type type assertions are known to always succeed.

Agreed. Perhaps this could be a separate staticcheck thing, but it would need to not have false positives.

[dominikh]

On another note, this check will have to come a long way before I can even consider merging it into master. While it is already performing better than errcheck, it is still flagging a lot of code that people will not care about – an issue that errcheck had since day one. Unlike errcheck, however, we care about only making useful suggestions.

This means that our "is this error irrelevant?" detection needs to get more advanced, looking through interfaces, understanding the relationship between function arguments and the returned errors, and so on. We may even need to improve other parts of staticcheck in the process – VRP comes to mind.

This will remain an experiment for the foreseeable future, but definitely one worth pursuing.

[joonas-fi]

At the risk of sounding like +1, still my two cents: I really really believe this to be an important check. All the arguments I've heard against this is the "too much noise" and "some errors don't matter". I agree, they would make this check unusable because of the bad UX.

However, if this check is done right, we should not underestimate the importance of this feature! It's not a major overstatement to call this a game changer. It is so easy to forget to check the error on a call and this is my only gripe against Golang vs. exceptions (you cannot forget to react to an exception in a way that it would go unnoticed).

[dominikh]

I'm tempted to put the check into its own tool (think errcheck-ng). Initially, I just wanted to make it opt-in with a flag, but that seems clumsy.

I don't think it will ever be possible to have the check in staticcheck and enable it by default. Even if we make the check sufficiently intelligent, it will always report unchecked errors that don't need to be checked. The Go standard library alone has hundreds of those. Sticky errors, interfaces, closing a writer after a write error and others don't need error checking and can only heuristically be detected by static analysis. Considering that staticcheck's mantra is "no false positives, unless your code is incredibly weird", a check like errcheck doesn't fit in.

[dominikh]

We may also want to promote the idea of whole-program checking and use PTA, via go/pointer.

This would give us a lot of insight into interfaces and data flow in general, and only flag unchecked errors where they can actually occur in the specific program. For example, io.WriteString would only be flagged if the passed writer can return an error. We could approximately check for this ourselves, but we'd be reimplementing a fair bit of data flow analysis for that.

One could make the argument that unchecked errors in libraries that accept, for example, any io.Writer are always bad. Just because all current uses pass in a *bytes.Buffer it doesn't mean that in the future someone won't provide a Writer that can fail. On the other side of that argument, however, is the fact that one can easily run errcheck for each commit and for all programs using the library and be notified of errors that now need checking. Additionally, the whole-program mode would be optional, and authors of pure libraries wouldn't need/want to use it to begin with, as whole-program analysis requires entry points (mains and tests). For authors of complete systems such as Docker or Prometheus, however, this mode might suppress a lot of useless warnings, making actual problems stand out more.

[dmitshur]

From 063a687828ad7d9e5a0366e1c3b3bfe11b31109b , is errcheck-ng a WIP name, or final name?

I see that the README still has TODOs for Usage and Purpose. What's the stability status of the tool?

[dominikh]

@shurcooL errcheck-ng is the final name. The tool isn't officially released yet and a work in progress. Specifically, I want to implement whole-program analysis and better detection of false positives before I announce the tool.

The tool will be officially usable once I add it to the tools README. You can, however, already experiment with it if you want to.

[dominikh]

Going to close this issue. Development will continue in the errcheck-ng tool, and any specific tasks or bugs will be filed as issues for that tool.

[dmitshur]

Have you considered naming it something simpler, like errcheck or errorcheck? I worry the -ng suffix will be suboptimal in the long term.

[dominikh]

@shurcooL you never run out of red color, do you? :)

errcheck is already taken and would make it impossible to install errcheck and my tool in parallel. What makes you think that the suffix "will be suboptimal"?

[dmitshur]

As a user (who hasn't read this thread in full detail), I don't want to type -ng. I don't know what it means, and I don't know why I have to. It's not self-explanatory nor clear. It feels like random letters.

I think it's a good practice to optimize for the user experience, and having -ng suffix IMO does not add value to the user.

One exception:

errcheck is already taken and would make it impossible to install errcheck and my tool in parallel

If a user wishes to have both tools installed, then the -ng suffix adds value, by virtue of being different from errcheck, it makes having both tools installed possible and easy.

Using a name like errorcheck would also accomplish that goal without needing to add random letters after a dash.

Would users want to have both installed? I don't know. I wouldn't, aside from trying both, because as I understand this is an alternative implementation of the same tool. Would it be confusing to have two or more implementations of the same thing have same names? I'm not sure, but I don't think so. I wouldn't want to have two ls or tree implementations to have different names either.

Whatever decision you make, I think you should make sure to optimize for the user experience, other matters are secondary.

[dominikh]

I don't know what it means, and I don't know why I have to

So, should Firefox, Safari et al be called "browser"? And Sublime Text should really be called "editor"? And Dmitri should be called "human"? It's a name, people can remember names, and not every name has to be a dull amalgamation of the things it does.

Would users want to have both installed?

Yes, they would. When initially trying the alternative, when requiring a rarely used feature that only errcheck implements but I don't, etc. This isn't "ls", it's an opinionated tool that can have many alternatives. And we certainly won't hijack the name of kisielk's project.

[mvdan]

Hijacking or not aside (you could also argue that unused is too broad a name), I agree that errorcheck is a nicer name than errcheck-ng.

[dominikh]

How do errcheck and errorcheck differ? Are they really doing the same thing, just with different names? errcheck-ng encodes in its name that it is a more advanced, more modern version of errcheck. errorcheck might as well just be a knockoff.

I invite you to set up whatever local aliases that make you happy :-)

[dmitshur]

It sounds like you've thought this through, and you're doing what you think is best for end users in the long run. I don't have objections. Thanks for considering this.