False negatives on compilation errors

[atombender]

Errcheck currently returns errors unrelated to error checking. Is it possible to remove these false negatives? Here's an example:

pkg/config/logging.go:23:5:warning: error return value not checked (invalid operation: lv (variable of type LoggingLevel) has no field or method key) (errcheck)

The code:

func setLevel(lv LoggingLevel) {
  if lv.key == "debug" {
    // ...
  }
}

As you can see, the code has nothing to do with error checking. It doesn't make sense to get a compilation error from errcheck; it should only run on valid code, in my opinion.

This causes a lot of redundancy when running errcheck via gometalinter in Visual Studio Code, which combines compilation errors, linter errors and vet errors into one workflow. Every compilation error seems to also get its corresponding errcheck error.

[dmitshur]

It doesn't make sense to get a compilation error from errcheck; it should only run on valid code, in my opinion.

I want to agree, but if someone runs errcheck independently, wouldn't it be useful for it to print the reason why it failed to run (non-zero exit code) instead of no output?

Perhaps it's gometalinter that should not run any other tools when basic compilation fails? But that might not be ideal either, since it'd require trying to compile code before running any linters.

Just thinking out loud here.

[atombender]

Yeah, I have a separate issue in vscode-go about this behaviour: In my opinion, it makes no sense to lint if compilation fails. Currently, vscode-go runs compilation and linting in parallel, which results in this happening. If vscode-go didn't do this, then this issue would go away entirely.

That said, I still think the correct behaviour is to not report compilation errors. Maybe there should be a warning that gometalinter can ignore.

go vet seems to have two behaviours depending on whether it's a syntax error or a semantic error. Here's if I insert a syntax error into some file:

$ go vet ./pkg/server/grpcapi/server.go
vet: pkg/server/grpcapi/server.go: pkg/server/grpcapi/server.go:27:4: missing ',' before newline in argument list
vet: no files checked
exit status 1

So that's potentially machine-readable.

But if I do something syntatically correct, but semantically invalid, such as this:

    type x struct{ b int }
    y := x{B: 1"}

...then go vet says nothing, and exits with 0. Here's what errcheck says, for comparison:

$ errcheck ./pkg/server/grpcapi/server.go 
pkg/server/grpcapi/server.go:30:9: unknown field B in struct literal
pkg/server/grpcapi/server.go:30:2: y declared but not used
error: failed to check packages: could not type check: couldn't load packages due to errors: grpcapi

which is again different from by first comment, where it reported it as a linter error. Not sure why the discrepancy.

[dominikh]

gometalinter merges duplicate errors into a single line (if you pass it an optional flag, because deduplication disables streaming of output). If our output differs from that of other tools/the compiler, then we should fix that.

That would be an easier change than modifying errcheck's general behaviour for errors.

[kisielk]

I'm all for improving the error messages to make it easier for other tools to parse. I'm not sure much can be done to silence the type checking errors (they're not compilation errors, they're from go/types), it's useful to report why the tool failed.

[dominikh]

I'm not sure much can be done to silence the type checking errors (they're not compilation errors, they're from go/types)

We can completely silence them, but I feel it'd be too confusing to hide, or summarize, those errors, for the reason @shurcooL outlined.

[kisielk]

That's basically what I said in the second half of the sentence ;)