Makefile: drop gofmt in favor of golangci-lint

[robert-zaremba]

Description

It's better to explicitly disable default lint checkers rather then disable all of them. The main reason is that if we golangci-lint will have a very good and useful checker we may skip him.

Other changes:

• removed gofmt command from make lint. It's not needed because golangci-lint is handling it (and it's doing it faster because gofmt doesn't need to re-read files).
• removed .vendor patterns - we don't use patterns any more

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer

[codecov]

Codecov Report

Merging #7260 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #7260   +/-   ##
=======================================
  Coverage   55.83%   55.83%           
=======================================
  Files         586      586           
  Lines       35851    35851           
=======================================
  Hits        20017    20017           
  Misses      13871    13871           
  Partials     1963     1963           

[alessio]

@robert-zaremba dixit:

It's better to explicitly disable default lint checkers rather then disable all of them. The main reason is that if we golangci-lint will have a very good and useful checker we may skip him.

I used to think likewise, and I changed my mind since then. The problem is that if we enable all checks by default, new checks may be introduced unpromptedly, causing PRs to fail unexpectedly - been there, done that and developers felt annoyed.

[tac0turtle]

errcheck is the only needed here if you are not using disable-all. They practically do the same thing because the only linter from the defaults we keep disabled is errcheck. This is more of a preference thing.

[robert-zaremba]

One of the motivation of this PR is to not use disable-all. From my OP:

It's better to explicitly disable default lint checkers rather then disable all of them. The main reason is that if we golangci-lint will have a very good and useful checker we may skip him.

[tac0turtle]

Can we enable varcheck, also WSL is not needed here, it's not a default linter

[robert-zaremba]

BTW: we also disabled varcheck - it's slow and unused is faster and check functions as well.

[robert-zaremba]

I removed the line with wsl

[alessio]

It's better to explicitly disable default lint checkers rather then disable all of them.

I'd argue that no, that is not necessarily better, reason being:

if we enable all checks by default, new checks may be introduced unpromptedly, causing PRs to fail unexpectedly

[robert-zaremba]

I'm fine with rolling back to disable-all, just need clear confirmation for that.

[robert-zaremba]

@robert-zaremba dixit:

It's better to explicitly disable default lint checkers rather then disable all of them. The main reason is that if we golangci-lint will have a very good and useful checker we may skip him.

I used to think likewise, and I changed my mind since then. The problem is that if we enable all checks by default, new checks may be introduced unpromptedly, causing PRs to fail unexpectedly - been there, done that and developers felt annoyed.

@alessio - I think it's easier to disable a linter if needed (adding one line of code) than update for an some important one (because in the latter situation we will not see it).

[alessio]

Please see #5733, the disable/enable thing was reconciled that was to mitigate Goland's Golint plugin users' headache

[robert-zaremba]

Please see #5733, the disable/enable thing was reconciled that was to mitigate Goland's Golint plugin users' headache

Thanks for the link. I don't know if Goland's Golint plugin still have that issue. Any Goland user here?
If you want I can revert to use back disable-all.

[alessio]

On Tue, Sep 8, 2020 at 3:22 PM Robert Zaremba ***@***.***> wrote: Please see #5733 <#5733>, the disable/enable thing was reconciled that was to mitigate Goland's Golint plugin users' headache Thanks for the link. I don't know if Goland's Golint plugin still have that issue. Any Goland user here?

The majority AiB's engineers are Goland users :)

[robert-zaremba]

So, should I use disable-all? (please put 👍 for yes or 👎 for no).

[alessio]

So, should I use disable-all? (please put +1 for yes or -1 for no).

I don't think that that is the right way to go about it. Let's try build consensus around a decision first, shall we?

I've tried Goland's Golinter, and it seems to work just fine. @robert-zaremba I've just got a question: if we use both disable and enable lists, what happens if a new linter is introduced in golangci-lint? Would such new linter be enabled by default?

[robert-zaremba]

if we use both disable and enable lists, what happens if a new linter is introduced in golangci-lint? Would such new linter be enabled by default?

If the golangci-lint team will decide that the tool should be enabled by default then it will be enabled.

[alessio]

Thanks for clarifying! Two questions:

If the golangci-lint team will decide that the tool should be enabled by default then it will be enabled.

If a plugin enabled by default will be enabled regardless of whether it is included in the enable list or not, what's the whole point of the enable field then?

If the golangci-lint team will decide

We may disagree with decisions made by golangci-lint (as a matter of fact, we have disagreed in the past and tweaked our config file accordingly), and their opinions on which plugin should be on and which off might would quite certainly impact our build sanity (again, 'been there, done that). This is exactly why I'd tend to favor for the SDK team to retain control on which checks should be enabled and which shouldn't.
In order to do so, we should keep disable-all and selectively enable only those linters that emit warnings that we reckon are of value for static analysis.

My 2p

[robert-zaremba]

If a plugin enabled by default will be enabled regardless of whether it is included in the enable list or not, what's the whole point of the enable field then?

Because only few linters are enabled by default. To enable other plugins we need to use enable section.

[robert-zaremba]

We may disagree with decisions made by golangci-lint

Correct. That's why disable is not empty. If using disable-all is a conclusion from this thread then I'm happy to use it. OK?

[alessio]

If using disable-all is a conclusion from this thread then I'm happy to use it. OK?

Indeed yet I think we should continue discussing as one question is yet to be answered:

If a plugin enabled by default will be enabled regardless of whether it is included in the enable list or not, what's the whole point of the enable field then?

Now, let's pretend that we have plugins A, B, C in the enable list, and D, E in the disable list. Then golangci-lint is (unpromptedly) upgraded to a new release, in which upstream enables a new plugin F by default.
Now, what would happen to our build? Would we get F plugin enabled?

• If the answer is yes, then it means that the enable field would have beeen ignored.
• If the answer is no, then it means that both the enable and disable fields would have beeen ignored.

Mind elaborating on this please @robert-zaremba?

[alessio]

(Another point I'm trying to make is that we should always aim to get to a deterministic state of things, i.e. that is easily done by using either disable-all). Alternatively, we could adopt using presets, see:

alessio@phoenix:~/work/cosmos-sdk$ golangci-lint run --help | grep presets
  -p, --presets strings                Enable presets (bugs|complexity|format|performance|style|unused) of linters. Run 'golangci-lint linters' to see them. This option implies option --disable-all

[robert-zaremba]

@alessio sure, let me elaborate.

Would we get F plugin enabled?

Yes.

what's the whole point of the enable field

The point of using enable is to selectively being able to enable more linters. To customize the default setting, you can use the following strategies.

1. Use disable to mark certain linters which are enabled by default (list of enabled linters). Then you use enable to enable more linters, which are not in the default list.
2. Use disable-all and then use enable to select linters we want to enable. This way you build a static list, and you make sure that it won't change even if golangci-lint get updated. The pros for this approach is that there won't be a situation, when there will be a new linter which will raise an alarm in our code and we won't have a surprise that a PR won't get merged. The cons is that the alarm could be useful and will not see it. It's worth to note that even if this will be a "false" alarm, it's extremely easy to turn it off by adding one line to .golangci-linter file (to disable that new linter).

[alessio]

The pros for this approach is that there won't be a situation, when there will be a new linter which will raise an alarm in our code and we won't have a surprise that a PR won't get merged.

Linters tend to ring alarm bells for stylistic mistakes. If we enable everything that is disabled by default by golangci-lint we'll risk to be flooded by warnings that we couldn't care less about. Many golangci linters have plenty of false positives that get fixed over time.

The cons is that the alarm could be useful and will not see it.

This is exactly the reason why we've come up with a custom disable-all-based configuration. There are warnings that add no value for us and just make unnecessary noise.

It's worth to note that even if this will be a "false" alarm, it's extremely easy to turn it off by adding one line to .golangci-linter file (to disable that new linter).

Yeah, and raise another PR, or add the file to your local .git/info/exclude: more work for very little gain.

In all honesty, this makes very little sense to me. I'd propose to leave the default configuration as it is so that we don't lose control over our builds, create another .extended-golangci.yml and let people customise their IDE by passing -c .extended-golangci.yml so that they get more (or less) warnings.

[robert-zaremba]

Any decision here? Shall I move back to disable-all?

[alessio]

How about create another .extended-golangci.yml and let people customise their IDE by passing -c .extended-golangci.yml so that they get more (or less) warnings?

[robert-zaremba]

How about create another .extended-golangci.yml and let people customise their IDE by passing -c .extended-golangci.yml so that they get more (or less) warnings?

I don't think this is helpful. We need to be everyone on the same page. We don't want that someone will see more errors and will fix them for others.

So, shall I use disable-all?

[robert-zaremba]

It seams that we stuck here.

I've rolled back to use disable-all. Can we go forward now?

[alessio]

Can we go forward now?

Looks good to me. Just a small note: lint-fix should be incorporated in the format rule. Looks good otherwise!

[robert-zaremba]

Looks good to me. Just a small note: lint-fix should be incorporated in the form at rule. Looks good otherwise!

Thanks. Re format: this will run linters with --fix option, and in some cases (when a linter can't automatically fix) will return an error. So it's more than format. Besides that format-linter-fix sounds odd.

[alessio]

make format is and has always been the place for fixing linter warnings. You don't want to to give users two commands instead of one to fix linter warnings, do you? That's not a DevX improvement.

Re format: this will run linters with --fix option

We've tried that in the past, and no warnings were actually fixed. It seems odd to me to add a Makefile run when we don't even know what is the expected output. Besides, this change is out of scope of this PR.

[robert-zaremba]

If a tool can sort out some issues for me that it's a DevX improvement.

So I checked for you which linters have an option to automatically fix issues:

% golangci-lint help linters | grep "auto-fix: true"
gci: Gci control golang package import order and make it always deterministic. [fast: true, auto-fix: true]
godot: Check if comments end in a period [fast: true, auto-fix: true]
gofmt: Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification [fast: true, auto-fix: true]
gofumpt: Gofumpt checks whether code was gofumpt-ed. [fast: true, auto-fix: true]
goimports: Goimports does everything that gofmt does. Additionally it checks unused imports [fast: true, auto-fix: true]
misspell: Finds commonly misspelled English words in comments [fast: true, auto-fix: true]
whitespace: Tool for detection of leading and trailing whitespace [fast: true, auto-fix: true]

[alessio]

If a tool can sort out some issues for me that it's a DevX improvement.

Adding more commands on top of other commands is not a DevX improvement. Very much like adding flags to commands is reckoned to worsen DevX (incidentally this is also how Golang goes about commands development).

Please integrate this in the format rule and we are good to go.

[alessio]

Some historical concerns wrt using golangci-lint on developer's local machine follows.
We have removed and disabled integration with golangci-lint in the past due to performance hit that could be felt when running it. Because of how golangci-lint and its caching mechanism works, we've decided to enable it only in the continuous integration environment (i.e. github actions). Considering that we can fix the warnings by running the commands listed in make format very quickly, we reckoned it was unnecessary to run golangci-lint as well.

[blushi]

@alessio from what I understood, you want to integrate the lint-fix command into the format one but lint-fix uses golangci-lint which is a bit inconsistent with #7260 (comment), or am I missing something?

[alessio]

@blushi format is Makefile's rule that developers run to fix formatting and linters warnings. I've been thinking about this a bit, so I actually see the benefit of having lint-fix separate from format.