Enable all golangci-lint linters

[Sean-Der]

What do you think? I prefer being aggressive with these (less to worry about with contributors)

But I don't want to make things miserable for people

[tarrencev]

Hard to say without using them, but generally for more consistent code 👍

[Sean-Der]

@at-wat @tarrencev Here is another pass!

https://gist.github.com/dbd355c702abbbf8ae818856ef50fa72 this is what it gives us on pion/webrtc. I think the stuff it complains about is valid!

I think it is better to disable things selectively. These linters really push newer programmers the right way I think.

[Sean-Der]

@daenney @at-wat would any of these linters make life for you miserable? Just want to get some light consensus before merging.

It drives me crazy when code massively needs to change, and I personally don't see the value :)

[daenney]

I'm a bit confused. The PR says "enable all" but there's a bunch we disable?

[daenney]

There's a few places in DTLS that I think the linters are a bit too enthusiastic about

record_layer_header.go:28:5: `protocolVersion1_0` is a global variable (gochecknoglobals)
var protocolVersion1_0 = protocolVersion{dtls1_0Major, dtls1_0Minor}
    ^
record_layer_header.go:29:5: `protocolVersion1_2` is a global variable (gochecknoglobals)
var protocolVersion1_2 = protocolVersion{dtls1_2Major, dtls1_2Minor}
    ^
signature_algorithm.go:12:5: `signatureAlgorithms` is a global variable (gochecknoglobals)
var signatureAlgorithms = map[signatureAlgorithm]bool{
    ^
srtp_protection_profile.go:11:5: `srtpProtectionProfiles` is a global variable (gochecknoglobals)
var srtpProtectionProfiles = map[SRTPProtectionProfile]bool{

True it's a global. But why is that bad? Structs can't be assigned to a const otherwise we would've done that. Same for the lookup maps.

There's some cognitive complexity issues too, but I'm not sure how to deal with those in the TLS flights. It also seems unhappy about anything with a todo comment?

Then we have stuff like:

cipher_suite.go:15:2: don't use ALL_CAPS in Go names; use CamelCase (golint)
    TLS_ECDHE_ECDSA_WITH_AES_128_CCM   CipherSuiteID = 0xc0ac
    ^
cipher_suite.go:16:2: don't use ALL_CAPS in Go names; use CamelCase (golint)
    TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8 CipherSuiteID = 0xc0ae
    ^
cipher_suite.go:19:2: don't use ALL_CAPS in Go names; use CamelCase (golint)
    TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 CipherSuiteID = 0xc02b

Which I get, but this is just how they're named to match with the IANA registry.

Few other places too where it raises some eyebrows for me:

e2e/e2e_lossy_test.go:123:18: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
            chosenLoss := rand.Intn(9) + test.LossChanceRange

Yes crypto/rand is better, but it's not necessary here.

Similarly, some of these:

errors_errno.go:17:3: missing cases in switch of type syscall.Errno: E2BIG, EACCES, EADDRINUSE, EADDRNOTAVAIL, EADV, EAFNOSUPPORT, EAGAIN, EALREADY, EBADE, EBADF, EBADFD, EBADMSG, EBADR, EBADRQC, EBADSLT, EBFONT, EBUSY, ECANCELED, ECHILD, ECHRNG, ECOMM, ECONNABORTED, ECONNRESET, EDEADLK, EDEADLOCK, EDESTADDRREQ, EDOM, EDOTDOT, EDQUOT, EEXIST, EFAULT, EFBIG, EHOSTDOWN, EHOSTUNREACH, EIDRM, EILSEQ, EINPROGRESS, EINTR, EINVAL, EIO, EISCONN, EISDIR, EISNAM, EKEYEXPIRED, EKEYREJECTED, EKEYREVOKED, EL2HLT, EL2NSYNC, EL3HLT, EL3RST, ELIBACC, ELIBBAD, ELIBEXEC, ELIBMAX, ELIBSCN, ELNRNG, ELOOP, EMEDIUMTYPE, EMFILE, EMLINK, EMSGSIZE, EMULTIHOP, ENAMETOOLONG, ENAVAIL, ENETDOWN, ENETRESET, ENETUNREACH, ENFILE, ENOANO, ENOBUFS, ENOCSI, ENODATA, ENODEV, ENOENT, ENOEXEC, ENOKEY, ENOLCK, ENOLINK, ENOMEDIUM, ENOMEM, ENOMSG, ENONET, ENOPKG, ENOPROTOOPT, ENOSPC, ENOSR, ENOSTR, ENOSYS, ENOTBLK, ENOTCONN, ENOTDIR, ENOTEMPTY, ENOTNAM, ENOTRECOVERABLE, ENOTSOCK, ENOTSUP, ENOTTY, ENOTUNIQ, ENXIO, EOPNOTSUPP, EOVERFLOW, EOWNERDEAD, EPERM, EPFNOSUPPORT, EPIPE, EPROTO, EPROTONOSUPPORT, EPROTOTYPE, ERANGE, EREMCHG, EREMOTE, EREMOTEIO, ERESTART, ERFKILL, EROFS, ESHUTDOWN, ESOCKTNOSUPPORT, ESPIPE, ESRCH, ESRMNT, ESTALE, ESTRPIPE, ETIME, ETIMEDOUT, ETOOMANYREFS, ETXTBSY, EUCLEAN, EUNATCH, EUSERS, EWOULDBLOCK, EXDEV, EXFULL (exhaustive)
        switch ne {
        ^

We have a default clause there, so I don't see how it's not exhaustive? There's a few other places where this happens too where we don't have defaults though, but you can't reach those due to earlier validation logic.

[daenney]

Also, can we override those exclude rules at the repo level? B/c there's a bunch of stuff in _test and examples in DTLS it's unhappy about too, but shouldn't be fixed, like these:

pkg/crypto/ccm/ccm_test.go:21:5: `aesKey1to12` is a global variable (gochecknoglobals)
var aesKey1to12 = mustHexDecode("c0c1c2c3c4c5c6c7c8c9cacbcccdcecf")
    ^
pkg/crypto/ccm/ccm_test.go:22:5: `aesKey13to24` is a global variable (gochecknoglobals)
var aesKey13to24 = mustHexDecode("d7828d13b2b0bdc325a76236df93cc6b")
    ^

[at-wat]

BTW, it's better to mark review comments resolved instead of deleting.

[Sean-Der]

@daenney sorry at first I turned them all on, but after a few rounds of review I turned some stuff off. I also made the mistake of deleting my comments (should have just hidden them)

The trade-off I am trying to make is to have the most restrictive linters possible, without hurting contributors/maintainers. Having to review PRs has taken up a lot of my time lately. It takes a lot of time to explain to people why certain things need to change and feels subjective.

I feel like when we go with a global we know when it is good (or bad) but is hard for new programmers. I really want to get them involved though!

overriding stuff at the repo level

You can drop a //nolint next to an individual line. I really want settings to be the same across all repos. The code quality on the 'lesser loved' repos like STUN and RTCP is pretty poor. I have disabled checks over their because things are synced. By forcing this it will help the LCD repos :)

For the linters you called out

• exhaustive I think we should drop that one! Especially if it ignores default. I can't think of a PR or bug that this came up.
• gochecknoglobals I like it personally. I would rather have functions that return stuff OR //nolint if it really makes sense.
• golint I think valid. I get a lot of PRs with wonky var names, would help to not have to go fix these things.
• gosec annoying, but might save our asses some day. dunno I am on the fence!

[daenney]

Exhaustive might be less annoying with this setting enabled:

exhaustive:
  default-signifies-exhaustive: true

Other than that I'm guess I'm OK with most of this. But someone's going to have to do a big PR to fix all the warnings on DTLS. There's a bunch of complexity warnings too for the different flight handlers, and a lot around the dynamic errors thing.