Proposal: Option to not show lint errors if there are build errors

[atombender]

Here's an annoying problem. Say you have this:

type Result struct {
  Value int
}

func getValue() (string, error) {
  return "ok", nil
}

func doStuff() (Result, error) {
  foo, err := getValue()
  if err != nil {
    return nil, err
  }
  return &Result{
    Value: foo,  // Obviously invalid, since foo is a string
  }, nil
}

If you run gometalinter with all linters enabled, this will get a bunch of errors/warnings.

• cannot use foo (type string) as type int in field value
• ineffectual assignment to foo (ineffassign)
• error return value not checked (foo declared but not used) (errcheck)
• error return value not checked (cannot use foo (variable of type string) as int value in struct literal) (errcheck)

It's possible to get even more false positives — for example, if you call a function that ends up not being called because of the build error error, that will trigger the deadcode linter.

The only real error here is this one:

cannot use foo (type string) as type int in field value

Dropping these linter errors into the mix serves to obscure the real error; I use "jump to next marker" to find the next error to fix, but since they're intermingled, it's initially hard to see what to fix, especially when there are multiple errors on the same line (which is true in the above example).

VSCode underlines build errors with a red squiggle, as opposed to green for linter errors, but I have also seen cases where there are only linter errors. Here's one:

That's an actual build error, but it only has a green squiggle (latest vscode-go, 1.7.1).

My proposal is to build first, and don't run any linters if there are build errors. This will also speed up responsiveness to actual syntax errors, since only a build is needed initially. (It might be good to make this an optional setting, though.)

[ramya-rao-a]

What are your thoughts on running go vet? At the moment, build, vet, and linting are all run in parallel

[atombender]

I think that would be fine. A quick test seems to indicate that vet doesn't give false positives on lines that fail to compile; not sure if that is intentional, a fluke, or just my test case not testing enough problems. For example, change the example's last line to:

    return &Result{foo}, nil

vet should complain about an unkeyed field, but doesn't. So that's good.

[ramya-rao-a]

We won't be serializing the build and lint process. They run in parallel at the moment.

What we can do is add an option to show linting errors only if there are no build errors.

I'd like to see if there are more people interested in this feature. Our settings list is growing big real fast.

[Paxxi]

Heavy vscode user, light go plugin user.
I agree completely that displaying lint errors on build failures is counterproductive. Not a big fan of having it as a setting or drop down filter as it takes extra steps and likely to be hard to find for beginners that might benefit the most.
edited out
Failed at reading, I agree with with @ramya-rao-a

[atombender]

Looks like this issue is dead. I tried enabling some more linters — I had disabled errcheck because of the above — and was immediately hit by this issue again.

Just to reiterate, it seems nonsensical to offer a lint error (green squiggle) if a line also has a syntax error (red squiggle). Here's me hitting the "next marker" key to cycle:

How does (2/2) help me here? The line contains a syntax error and the linting error probably can't be correct. (And it isn't. When linters encounter build errors, they emit them as linter errors, so this is a false positive.)

My suggested fix is to ignore linter errors for lines that already have error markers on them. That will almost completely fix this problem. There's still the chance that false-positive linter errors can appear on other lines that aren't build errors, but I don't think that will be as damaging.

[ramya-rao-a]

ignore linter errors for lines that already have error markers on them

Now that's something I can do :) Will get it out for the next update

[ramya-rao-a]

@atombender I have pushed the changes to master, can you help with testing? All you have to do is

• Download https://github.com/Microsoft/vscode-go/blob/master/Go-latest.vsix (If you are on ubuntu, then clone the repo and get the file from the master branch)
• Run code --install-extension Go-latest.vsix and reload VS Code

[atombender]

@ramya-rao-a Thanks, sorry about the delay. Love the new behaviour. However, I also notice that error markers don't show up until Gometalinter has finished running, which, as you know, can take a while. (I've tried disabling the slowest linters, but it's still very resource-intensive.) In my opinion, with this new behaviour, there's no point in waiting for Gometalinter, since it won't override any of the error markers.

[atombender]

@ramya-rao-a The change caused format-on-save to no longer happen. I have editor.formatOnSave disabled and go.formatOnSave enabled. It worked before, is this a breaking change?

[atombender]

I just rebooted, and after this, VSCode is now linting Go itself!

Finished running tool: /usr/local/opt/go/libexec/bin/go build -i -o /var/folders/qs/wpmg19r12_9_nz7pvvs2_82r0000gn/T/go-code-check github.com/sanity-io/gradient/pkg/server

Finished running tool: /Users/alex/.go/bin/gometalinter --concurrency=2 --disable-all --enable=unused --enable=vet --enable=vetshadow --enable=structcheck --enable=goconst --enable=gotypex --enable=ineffassign --enable=golint --enable=deadcode --enable=unconvert --enable=gotype --enable=interfacer --exclude=\.pb\.go --exclude=or be unexported --exclude=that.stutters --exclude=don't use underscores --exclude=\.nolint\.go --tests --aggregate
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:47: unused struct field undeclared name: lookupGroup (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/net/lookup_unix.go:190: unused struct field undeclared name: cgoLookupPTR (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:47: undeclared name: lookupGroup (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/net/lookup_unix.go:123: undeclared name: cgoLookupCNAME (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:58: unused struct field undeclared name: listGroups (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:53: unused struct field undeclared name: lookupGroupId (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/net/lookup_unix.go:107: unused struct field undeclared name: cgoLookupPort (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/crypto/x509/cert_pool.go:38: unused struct field undeclared name: loadSystemRoots (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/crypto/x509/root.go:21: unused struct field undeclared name: loadSystemRoots (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:58: undeclared name: listGroups (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/crypto/x509/cert_pool.go:38: undeclared name: loadSystemRoots (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:32: unused struct field undeclared name: lookupUser (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:41: unused struct field undeclared name: lookupUserId (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/net/lookup_unix.go:95: unused struct field undeclared name: cgoLookupIP (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/net/lookup_unix.go:123: unused struct field undeclared name: cgoLookupCNAME (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:32: undeclared name: lookupUser (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/net/lookup_unix.go:95: undeclared name: cgoLookupIP (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/net/lookup_unix.go:190: undeclared name: cgoLookupPTR (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:11: unused struct field undeclared name: current (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/net/lookup_unix.go:80: unused struct field undeclared name: cgoLookupHost (structcheck)
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:11: undeclared name: current (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:41: undeclared name: lookupUserId (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/os/user/lookup.go:53: undeclared name: lookupGroupId (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/net/lookup_unix.go:80: undeclared name: cgoLookupHost (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/net/lookup_unix.go:107: undeclared name: cgoLookupPort (interfacer, unconvert)
/usr/local/Cellar/go/1.9.2/libexec/src/crypto/x509/root.go:21: undeclared name: loadSystemRoots (interfacer, unconvert)
[...]

I've verified that Gometalinter is run from the right directory. If I cd to that directory and run the exact same command line, I get the right linter output. Reverting to the released version fixes the problem, so this must be an unreleased breakage.

[ramya-rao-a]

That's weird. There was no changes made to how the linter is run. The changes were only made to how we treat the results from the linter.

[ramya-rao-a]

About the formatting, we are deprecating go.formatOnSave in favor of editor.formatOnSave, so if you are using the vsix, set

"[go]": {
"editor.formatOnSave": true
}

[atombender]

Got it, thanks.

[atombender]

I just installed the .vix and restarted VSCode again. It's back to linting the right things. Weird!

[ramya-rao-a]

Ok, you gave me quite a scare there :)

[atombender]

@ramya-rao-a Ugh, it happened again, and I figured out why. It turns out VSCode is (sometimes!) sets PATH=/usr/bin:/bin:/usr/sbin:/sbin, which doesn't include Go itself (I use Homebrew, and I have a much longer PATH). Gometalinter lints the wrong things when it doesn't have Go, and I've created an issue about that, but of course VSCode should also set the right $PATH. It just happened again, and I restarted VSCode, and the $PATH is right again. Could this be caused by the Go extension?

[ramya-rao-a]

The Go extension doesn't touch the $PATH
It only updates the $GOPATH as described in https://github.com/Microsoft/vscode-go/wiki/GOPATH-in-the-VS-Code-Go-extension

If you have set the go.toolsGopath setting and gometalinter is being run from there, then this go.toolsGopath is appended to whatever GOPATH previously deciphered because gometalinter expects all its linter to be available under GOPATH

[atombender]

Is there another explanation for why PATH is sometimes wrong? It only seems to affect Gometalinter. It also only started happening after I installed your .vix file. I've confirmed that the subprocess gets the wrong environment; it's not a bug in Gometalinter. Want me to create a separate issue?

[ramya-rao-a]

Yes, please create a new issue.
Also, since I cannot repro, I'd appreciate if you can help me debug this by running the Go extension from source

[ramya-rao-a]

In the latest update to the Go extension (0.6.70), linting/vetting errors will not show up on a line that has build errors