Plugin reporting same error from many linters

[daenney]

When an issue is detected in a file it is reported in the problems tab by a number of linters gometalinter uses. This results in a tremendous amount of issues reported for what is only one thing:

The issue should be reproducible with a snippet like this:

package main

import "flag"

func main() {
    flag.Bool()
}

And settings:

   "go.lintTool": "gometalinter"

Though it matches with the output of every CLI reporting the issue it seems incorrect from the standpoint of the user, I should only be told about this error once. I would expect multiple errors for the same line at the same offset to get rolled up into one, especially if the error message is identical.

[daenney]

Interestingly the spurious output in this specific case disappears if one sets go.lintFlags to --fast but it might be that it just happens to disable every other linter that tripped over this.

[ramya-rao-a]

@daenney I'd prefer to get a fix for this from gometalinter itself.
Maybe a flag --removeDuplicates that would return only unique errors.

@alecthomas thoughts?

[ramya-rao-a]

This issue was moved to alecthomas/gometalinter#286

[ramya-rao-a]

Note: Try the --aggregate flag and see if it can be added as a default

[daenney]

That seems to work, with --aggregate we're now down to:

Still a few duplicates b/c of how those linters are outputting/wrapping the error, but better than before.

[bradleyfalzon]

Personally, I'd recommend running go build or similar, before running any linters. The errors you are seeing are from the linter's use of the https://golang.org/pkg/go/ packages - these aren't compiler errors, they are similar, but not exactly the same.

I'm not sure how this plugin works, but I'd recommend the best approach to only run linters if the Go compiler was able to build the program.

[ramya-rao-a]

@bradleyfalzon Skipping linting when there are build errors is tracked in #600

Regardless of this, it is still useful to use --aggregate on gometalinter

[bradleyfalzon]

@bradleyfalzon Skipping linting when there are build errors is tracked in #600

Ah excellent.

Regardless of this, it is still useful to use --aggregate on gometalinter

Yes, agreed, I was more referring to Still a few duplicates b/c of how those linters are outputting/wrapping the error.

Sorry I didn't mean to appear rude at all in any of my comments, I was more saying once the compiler errors are removed in #600, I don't believe any linter should share the exact same error text.

But this all sounds good to me, I've heard nothing but great things from this plugin 👍

[daenney]

I'm not sure how this plugin works, but I'd recommend the best approach to only run linters if the Go compiler was able to build the program.

I disagree. There's plenty of things the linters can already tell me to fix before I even attempt to run a go build which would build my package and all of its dependencies. Depending on the size of the project this can easily take 5-10s or more and not getting any feedback in the UI until then that something might be wrong is not great from my perspective. One of the advantages of, especially the --fast, linters in gometalinter is that they can run continuously providing much more real time feedback then what's possible if I'd need to continuously run go build for any results.

For example, it takes gotype about 3ms to find that error, whereas for go build to surface that error it takes 1.5s. Now I can immediately fix the problem before I even attempt a build and not get stuck on that issue. That slow-down can be fixed if you run a go build -i the first time around and every time you add a non-stdlib dependency. go get would take care of it but that's not necessarily how a repository is checked out for development. Even so, running a build after that is still about a factor 4 slower than running gometalinter for a very trivial main.go, to surface the same error.

Linters can also be used to catch things that don't matter at all to a compiler, nor require a compiler to have run to transform the source code. They can even catch things that the compiler would get upset about. Forcing serialisation of compiler then linters, though in this case would result in the most correct error reporting, slows things down.

[ramya-rao-a]

@bradleyfalzon I didn't notice any rudeness at all :)

@daenney Thanks for the detailed analysis. Note that the Go extension already uses go build -i on non main packages. Does that change your stand?

Including @atombender for his thoughts in this discussion as he originally opened #600

Instead of serializing the build and linting process, another approach for #600 can be to not show linting results if there are build errors to begin with. And to control this with a setting. Serializing can result in delayed linting results in bigger projects.

[daenney]

@daenney Thanks for the detailed analysis. Note that the Go extension already uses go build -i on non main packages. Does that change your stand?

I'm not quite sure I follow. So as long as I'm not working on something that's declared as package main vscode-go would invoke build with -i? Lets say I'm hacking on Moby and change anything in https://github.com/moby/moby/tree/master/cmd/dockerd. That would still get a regular go build? What's the reason for not running with -i by default?

My reason for opening up this ticket is that I wanted a way to try and deduplicate the linter errors. I think --aggregate gets us a long way there. It's already reduced the noise by 50% and it is reasonable to have that as a default flag since it doesn't change any part of the workflow. At this point, even though there's still a few duplicates, it gets in the way much less while still providing valuable feedback. As far as I'm concerned that resolves this issue.

I understand the reasoning behind "don't run linters if it doesn't compile" but I don't think that's a workflow that should be enforced, though having the option to do so or withhold the errors at first seems entirely reasonable. Linters can add value regardless of if the code is a valid construct according to the compiler; in this particular case they do an equally good yet far quicker job of finding the same problem. For me that improves the speed I can iterate on a code base, which I find valuable. I think this is mostly a matter of personal preference and therefor something that should be left up to the end user.

[ramya-rao-a]

What's the reason for not running with -i by default

For go build -i part follow the discussion in #673
For why we don't do that for "main" package, see #673 (comment) specifically.

@mattetti The docs for go build -i says "The -i flag installs the packages that are dependencies of the target". As in only the dependencies are installed (if changed), in which case, do we still have to skip the -i for the main package?

My reason for opening up this ticket is that I wanted a way to try and deduplicate the linter errors. I think --aggregate gets us a long way there.

Oh definitely, for the purpose of this ticket using --aggregate solves the problem, and I can add that as a default while running gometalinter.

I personally don't do a lot of Go coding, so when there is an opportunity like this to discuss and understand the process more, I don't let go of that :)

I understand the reasoning behind "don't run linters if it doesn't compile" but I don't think that's a workflow that should be enforced

Yes, when I do get around to working on #600, it would be via a setting and not enforced by default.

[mattetti]

I was under the impression that -i was also installing the executable when building a main package. I might be wrong and it would be worth testing (on my cellphone).

…

On Sat, Jun 3, 2017, 13:25 Ramya Rao ***@***.***> wrote: What's the reason for not running with -i by default For go build -i part follow the discussion in #673 <#673> For why we don't do that for "main" package, see #673 (comment) <#673 (comment)> specifically. @mattetti <https://github.com/mattetti> The docs for go build -i says "The -i flag installs the packages that are dependencies of the target". As in only the dependencies are installed (if changed), in which case, do we still have to skip the -i for the main package? My reason for opening up this ticket is that I wanted a way to try and deduplicate the linter errors. I think --aggregate gets us a long way there. Oh definitely, for the purpose of this ticket using --aggregate solves the problem, and I can add that as a default while running gometalinter. I personally don't do a lot of Go coding, so when there is an opportunity like this to discuss and understand the process more, I don't let go of that :) I understand the reasoning behind "don't run linters if it doesn't compile" but I don't think that's a workflow that should be enforced Yes, when I do get around to working on #600 <#600>, it would be via a setting and not enforced by default. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1013 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAAcefG-Lmqk5u6ptMCvg02M9KI7WwWks5sAcFRgaJpZM4Npnxb> .

[atombender]

There's some discussion about the "-i" flag here: #673. The documentation is pretty lacking.

…

On Jun 3, 2017, at 17:10, Matt Aimonetti ***@***.***> wrote: I was under the impression that -i was also installing the executable when building a main package. I might be wrong and it would be worth testing (on my cellphone). On Sat, Jun 3, 2017, 13:25 Ramya Rao ***@***.***> wrote: > What's the reason for not running with -i by default > > For go build -i part follow the discussion in #673 > <#673> > For why we don't do that for "main" package, see #673 (comment) > <#673 (comment)> > specifically. > > @mattetti <https://github.com/mattetti> The docs for go build -i says > "The -i flag installs the packages that are dependencies of the target". As > in only the dependencies are installed (if changed), in which case, do we > still have to skip the -i for the main package? > > My reason for opening up this ticket is that I wanted a way to try and > deduplicate the linter errors. I think --aggregate gets us a long way there. > > Oh definitely, for the purpose of this ticket using --aggregate solves > the problem, and I can add that as a default while running gometalinter. > > I personally don't do a lot of Go coding, so when there is an opportunity > like this to discuss and understand the process more, I don't let go of > that :) > > I understand the reasoning behind "don't run linters if it doesn't > compile" but I don't think that's a workflow that should be enforced > > Yes, when I do get around to working on #600 > <#600>, it would be via a > setting and not enforced by default. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1013 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAAAcefG-Lmqk5u6ptMCvg02M9KI7WwWks5sAcFRgaJpZM4Npnxb> > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[atombender]

Sorry, I now see that this issue was actually referenced above.

[ramya-rao-a]

The docs:

So the executable for a "main" pkg gets created no matter what. Using -o (which we do), puts
the executable created in the temp path.

This means that we can use -i on the main package as well

[mattetti]

Sounds good, @rakyll you might be interested in knowing that the flag documentation is a bit confusing :)

[daenney]

Thanks for all the work on this @ramya-rao-a! 💯

[ramya-rao-a]

In the latest update (0.6.62), --aggregate is used for gometalinter