Expand linters set

[notimetoname]

I would add some:

• containedctx: thoughts (see Add contextcheck linter neofs-node#1911)
• contextcheck: will help the linter above, it encourages passing context.Context (see Add contextcheck linter neofs-node#1911)
• funlen: thoughts
• gocognit: I see too much nesting code, sometimes it can be up to the 7th nesting level inside one function
• asciicheck: nspcc-dev/neofs-node@e5c6d18
• dogsled: https://github.com/nspcc-dev/neofs-node/blob/8d054f10c97d0e28703730c828381311ba2be929/pkg/local_object_storage/pilorama/boltdb.go#L281; I always ask on a review why someone adds 2 bool as a return value or even adds useful return values but do not use them; it is a red flag IMO
• exhaustruct: I do always vote for an explicit declaration, see also: fix/Panic in storagegroup put neofs-node#2399
• exportloopref: see [#2212] morph: Fix subscription restoration neofs-node#2220
• forcetypeassert: we have some places that rely on such a code, I would say that it is smth bad and should be reconsidered (moreover, generics are here now)
• goconst: seems like we already use it as a rule implicitly
• gomnd: the same as above
• nilerr: see and see
• reassign: we do have such a bug once, but can't find the PR; in general, I would say that reassigning on purpose is an antipattern
• unconvert: I have found 12 unnecessary type conversions currently

I have just run through the available but turned off linters. Not all of them are appliable to us (some of them may be already implemented via the others and I have not noticed that) but that is a discussion issue. @roman-khimov, @cthulhu-rider, @smallhive.

[roman-khimov]

• contextcheck is Add contextcheck linter neofs-node#1911 and let's add containedctx there as well, it's essentially the same problem for us
• funlen and gocognit (or gocyclo which is very similar) are a bit too vague for me, they're nice, but they can make you work just to please some linter and not really improving anything
• asciicheck can be enabled
• dogsled issues in many cases just can't be fixed, if you have a function returning 4 variables and you only need one, what are you going to do?
• exhaustruct is likely to lead to a lot of false positives, in many cases I'd prefer more concise code and ability to extend structure without touching every line initializing it
• exportloopref is enabled in NeoGo, should be enabled here as well
• forcetypeassert don't seem to be useful to me, if it's written this way then it's OK to panic here
• goconst and gomnd can be tried, but I fear there will be false positives
• nilerr can be tried
• reassign is enabled in NeoGo
• unconvert should be OK

Linters are great, but in many cases it's a balance between false positives/time to spend pleasing the linter and the effect of the problem it can prevent. At the same time looking at your list I see some new ones, so some periodic reshuffling of the linter set can be useful.

[notimetoname]

funlen and gocognit (or gocyclo which is very similar) are a bit too vague for me, they're nice, but they can make you work just to please some linter and not really improving anything

Mostly always I would say that 500 lines long functions are strange. If it is a huge switch or some other exceptions that really can't be done any other way, a few nolints in a 100K lines code base is OK to me.

if you have a function returning 4 variables and you only need one, what are you going to do?

Ask yourself what led you to the times you use func that returns N args and you do not need most of them (if the func is the internal one, of course, external libs can't be fixed but I do not think we use such).

exhaustruct is likely to lead to a lot of false positives

What do you call "false positives" in that context? My arg is the example I provided: panic in the master that was hard to predict on review.

forcetypeassert don't seem to be useful to me, if it's written this way then it's OK to panic here

I would force a dev to check asserting and panic explicitly, it (may) adds details, it saves reviewer's time, it does not allow mistakes when a dev writes asserting to check his code but forget to add a check (where it is needed, of course). Anyway, if panicing is OK in some scenarios, we usually write comment about that (~the same number of chars compared to panicing with some detailed message).

goconst and gomnd can be tried, but I fear there will be false positives

What do you call "false positives" in that context?

Linters are great, but in many cases it's a balance between false positives/time to spend pleasing the linter and the effect of the problem it can prevent.

Sure, that is why I filtered them, and not just copy-pasted all the linters golangci-lint allows. Almost every linter I suggested would have saved us from issuing, testing, fixing, and reviewing some stupid bugs. What takes more time? Well, that is an open discussion.

[roman-khimov]

dogsled

Ask yourself what led you to the times you use func that returns N args and you do not need most of them

But other users may use all of them anyway, so you'll waste some time, but gain nothing. It can be tried though, if there is a small number of places like this and all of them can be legitimately fixed, then maybe it's OK.

BTW, we have exactly one case of this (but tests: false, meh).

exhaustruct

What do you call "false positives" in that context?

pkg/network/address.go:54:11                                    exhaustruct  Opaque, User, Path, RawPath, OmitHost, ForceQuery, RawQuery, Fragment, RawFragment are missing in URL
...
pkg/morph/client/notary.go:563:11                               exhaustruct  Rules is missing in Signer
pkg/morph/client/notary.go:575:12                               exhaustruct  Rules is missing in Signer
...
pkg/innerring/processors/netmap/process_peers.go:67:10          exhaustruct  InvokePrmOptional is missing in AddPeerPrm
pkg/innerring/processors/netmap/process_peers.go:169:11         exhaustruct  InvokePrmOptional is missing in UpdatePeerPrm

forcetypeassert

I would force a dev to check asserting and panic explicitly, it (may) adds details

What detail can I add to https://github.com/nspcc-dev/neo-go/blob/9185820289ce942153bc5ba012e677b5278f0cb5/cli/server/server.go#L469? "If you see this panic, then whoever wrote this code is an idiot, but this is not likely to help you anyway, the system has failed"?

It's not always the case, but I'd expect the majority of them to be like that.

goconst and gomnd

What do you call "false positives" in that context?

pkg/morph/client/notary.go:417:35                             gomnd    mnd: Magic number: 2, in <argument> detected
pkg/morph/client/notary.go:552:35                             gomnd    mnd: Magic number: 2, in <argument> detected
...
pkg/util/os.go:9:32                                           gomnd    mnd: Magic number: 0110, in <argument> detected
...
pkg/innerring/processors/settlement/audit/calculate.go:45:27  gomnd    mnd: Magic number: 30, in <argument> detected

[notimetoname]

dogsled

But other users may use all of them anyway, so you'll waste some time, but gain nothing.

I meant if it is a private func for internals (like it is in my example), then you (may) want to split it into subfuncs and not do _, _, _, val := func().

exhaustruct

pkg/network/address.go:54:11

Nothing can be done, agree.

pkg/morph/client/notary.go:563:11

Do you mind Rules: nil? It would be ok for me.

pkg/innerring/processors/netmap/process_peers.go:67:10

Bad design, IMO. And the linter would highlight it. We are gonna remove it, aren't, we?

forcetypeassert

What detail can I add to https://github.com/nspcc-dev/neo-go/blob/9185820289ce942153bc5ba012e677b5278f0cb5/cli/server/server.go#L469? "If you see this panic, then whoever wrote this code is an idiot, but this is not likely to help you anyway, the system has failed"?

I am not sure about neo-go code. My ide says that GetStateModule always returns *corestate.Module and that is the only (exported) implementation of the StateRoot interface, so I can't say why it is done that way. I would add details that explain why that cast is necessary and why it should not be changed when another dev comes here and adds one more implementation. (I may be such a dev if I have 2 mins on fixing smth in that repo).
Also, it may add and may not add any details. As I already said, usually we add comments anyway, your example is not an exception.

goconst and gomnd

pkg/morph/client/notary.go:417:35
pkg/morph/client/notary.go:552:35

Well, they are real examples of the magics, no? We add comments to explain what is happening here. This is not smth obvious (even for a neofs dev payments and order details are tricky). Also, we reuse them (you added two links and both of them are the same numbers meaning the same).

pkg/util/os.go:9:32

Arguable, but agree that const for that may be too much (but having it does not bother me).

pkg/innerring/processors/settlement/audit/calculate.go:45:27

Magic, no? Sometimes we add comments (that also can be comments to some well-named constants) to such numbers.

[roman-khimov]

you (may) want to split it into subfuncs

Do we have an issue for our single example of this behavior? I'd leave it to that. Maybe this code is bad, maybe it can be improved, but I'd not add a linter for it.

Do you mind Rules: nil?

Yes, I do. I want some specific signer, this signer doesn't use Rules, so I don't want to think about many other fields that can be set. For some simple signers like None or CalledByEntry I also don't care about AllowedContracts or AllowedGroups (they won't work anyway), so I don't want to be setting them.

Now the problem would be if I'm to set CustomContracts and not specify AllowedContracts. It'd be a bug. But I can set it to nil as well and it'd also be a bug.

when another dev comes here and adds one more implementation

He won't. There is no need to, other than tests of some kind.

Well, they are real examples of the magics, no?

We have exactly this number of signers and all the code around works with exactly that number. You can't change a constant and expect all the other code to work, for example. Compare that to defaultNotaryValidTime, that's a nice constant that I can change independently of any other code. So 2/3 constants don't bring a lot of value (while they can make some things more readable), but you'll have to manage them in your code and there is some cost to it.

pkg/innerring/processors/settlement/audit/calculate.go:45:27

Magic, no?

Well, it's a GB, as the variable name suggests. We can write 1024*1024*1024 here, but then gomnd will ask us to have some constant for 1024. That's exactly the thing I'm talking about --- very soon you start spending time and effort just to have some positive feedback from the linting machine instead of solving real problems. And these examples were taken randomly (I just don't want to spend time digging into all of them), some will require more time and effort than others. Some of course will be seriously improved (goconst warnings (there are just three of them) looked fair to me, for example), but for a linter to be enabled by default it must have some serious SNR, these ones just don't have it.

[carpawell]

• dupword: sounds like a joke, but I have fixed so many duplicated comment words in that repo (and I just found one more example right now) so it is not that fun to me now. TBH, It is also strange to me, my IDE shouts at me when it sees duplications, but I can understand that not every editor does the same.