search

monogon-dev / monogon

The Monogon Monorepo. May contain traces of peanuts and a ✨pure Go Linux userland✨. Work in progress!
https://monogon.tech
Apache License 2.0
377 stars 9 forks source link

codestyle: Add more nogo checks #298

Open fionera opened 5 months ago

fionera commented 5 months ago

We should write/add some more checks to nogo. This is a small list I started on creating, feel free to add more.

q3k commented 5 months ago

Before we go writing our own, it would be prudent to trawl GitHub for analysis.Analyzer. There might already be some analyzers for this.

fionera commented 5 months ago

These are some checks that I found on Github

Imported from CockroachDB

Additionally I took some inspiration at Sourcegraph and found these:

Additionally Sourcegraph is using Staticcheck which sounds like another good idea

fionera commented 5 months ago

After running some of them I skipped unparam (always crashed when checking k8s code), bodyclose (not very useful/lots of false positives) and go-critic (also crashed all the time). I am using https://github.com/sluongng/nogo-analyzer/ now as it has a better staticcheck integration and am working on fixing the things it found

fionera commented 5 months ago

We obviously have to decide which checks to disable/add exceptions.

build/bazel_cc_fix/main.go:202:3: could use tagged switch on inclType (QF1003)
build/bazel_cc_fix/main.go:330:2: this value of err is never used (SA4006)
cloud/bmaas/bmdb/reflection/schema.go:88:10: the argument's underlying type is a string, should use a simple conversion instead of fmt.Sprintf (S1025)
cloud/bmaas/bmdb/reflection/schema.go:152:8: should not use underscores in Go names; var column_name should be columnName (ST1003)
cloud/bmaas/bmdb/reflection/schema.go:152:21: should not use underscores in Go names; var data_type should be dataType (ST1003)
cloud/bmaas/bmdb/reflection/schema.go:152:32: should not use underscores in Go names; var udt_name should be udtName (ST1003)
cloud/equinix/wrapngo/wrapngo_live_test.go:265:30: possible nil pointer dereference (SA5011)
cloud/equinix/wrapngo/wrapngo_live_test.go:266:30: possible nil pointer dereference (SA5011)
cloud/equinix/wrapngo/wrapngo_live_test.go:287:8: possible nil pointer dereference (SA5011)
go/net/psample/subscriber.go:15:6: type attrId should be attrID (ST1003)
go/net/psample/subscriber.go:126:6: var pktGrpId should be pktGrpID (ST1003)
go/types/mapsets/orderedmap.go:83:2: should use copy(to, from) instead of a loop (S1001)
metropolis/build/gotoolwrap/main.go:142:3: unnecessary use of fmt.Sprintf (S1039)
metropolis/node/build/fsspec/utils.go:24:3: should replace loop with mergedSpec.File = append(mergedSpec.File, spec.File...) (S1011)
metropolis/node/build/fsspec/utils.go:27:3: should replace loop with mergedSpec.Directory = append(mergedSpec.Directory, spec.Directory...) (S1011)
metropolis/node/build/fsspec/utils.go:30:3: should replace loop with mergedSpec.SymbolicLink = append(mergedSpec.SymbolicLink, spec.SymbolicLink...) (S1011)
metropolis/node/build/fsspec/utils.go:33:3: should replace loop with mergedSpec.SpecialFile = append(mergedSpec.SpecialFile, spec.SpecialFile...) (S1011)
metropolis/node/build/mkpayload/mkpayload.go:69:2: should not use underscores in Go names; var rootfs_dm_table should be rootfsDmTable (ST1003)
metropolis/node/core/consensus/client/unimplemented.go:15:2: error var UnimplementedInNamespaced should have name of the form ErrFoo (ST1012)
metropolis/node/core/curator/watcher/watch_node.go:77:1: comment on exported method Error should be of the form "Error ..." (ST1020)
metropolis/node/core/identity/certificates.go:47:1: comment on exported function CACertificate should be of the form "CACertificate ..." (ST1020)
metropolis/node/core/network/dhcp4c/transport/transport.go:27:5: error var DeadlineExceededErr should have name of the form ErrFoo (ST1012)
metropolis/node/ids.go:22:2: const RootUid should be RootUID (ST1003)
metropolis/node/ids.go:23:2: const TimeUid should be TimeUID (ST1003)
metropolis/pkg/bootparam/bootparam.go:36:1: comment on exported function TrimLeftSpace should be of the form "TrimLeftSpace ..." (ST1020)
metropolis/pkg/devicemapper/devicemapper.go:86:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:87:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:88:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:91:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:92:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:93:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:94:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:95:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:96:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:99:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:100:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:101:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:102:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:105:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:106:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:107:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:108:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:112:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:113:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/devicemapper/devicemapper.go:114:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/efivarfs/devicepath.go:214:24: could use strings.ReplaceAll instead (QF1004)
metropolis/pkg/fat32/fat32.go:273:7: should use strings.EqualFold instead (SA6005)
metropolis/pkg/fat32/fat32.go:434:10: error strings should not end with punctuation or newlines (ST1005)
metropolis/pkg/fileargs/fileargs.go:49:1: comment on exported function NewWithSize should be of the form "NewWithSize ..." (ST1020)
metropolis/pkg/fsquota/fsxattrs/fsxattrs.go:49:1: comment on exported const FS_IOC_FSGETXATTR should be of the form "FS_IOC_FSGETXATTR ..." (ST1022)
metropolis/pkg/fsquota/fsxattrs/fsxattrs.go:50:7: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/fsquota/fsxattrs/fsxattrs.go:51:7: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/fsquota/quotactl/quotactl.go:39:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/fsquota/quotactl/quotactl.go:40:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/fsquota/quotactl/quotactl.go:41:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/fsquota/quotactl/quotactl.go:42:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/fsquota/quotactl/quotactl.go:43:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/fsquota/quotactl/quotactl.go:44:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/fsquota/quotactl/quotactl.go:45:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/fsquota/quotactl/quotactl.go:46:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/fsquota/quotactl/quotactl.go:47:2: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/jsonpatch/jsonpatch.go:23:1: comment on exported type JsonPatchOp should be of the form "JsonPatchOp ..." (with optional leading article) (ST1021)
metropolis/pkg/jsonpatch/jsonpatch.go:24:6: type JsonPatchOp should be JSONPatchOp (ST1003)
metropolis/pkg/kmod/modinfo.go:68:4: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/kmod/modinfo.go:69:4: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/kmod/modinfo.go:70:4: should not use ALL_CAPS in Go names; use CamelCase instead (ST1003)
metropolis/pkg/kmod/modinfo.go:108:1: comment on exported method Licenses should be of the form "Licenses ..." (ST1020)
metropolis/pkg/kmod/radix.go:58:2: redundant return statement (S1023)
metropolis/pkg/logtree/journal.go:191:3: could lift into loop condition (QF1006)
metropolis/pkg/logtree/journal.go:227:3: could lift into loop condition (QF1006)
metropolis/pkg/logtree/journal_entry.go:163:4: could lift into loop condition (QF1006)
metropolis/pkg/logtree/leveled_payload.go:100:1: comment on exported method Messages should be of the form "Messages ..." (ST1020)
metropolis/pkg/logtree/logtree_entry.go:208:1: comment on exported method Proto should be of the form "Proto ..." (ST1020)
metropolis/pkg/logtree/logtree_entry.go:231:1: comment on exported function LogEntryFromProto should be of the form "LogEntryFromProto ..." (ST1020)
metropolis/pkg/logtree/logtree_test.go:69:3: should replace loop with lines = append(lines, e.Leveled.Messages()...) (S1011)
metropolis/pkg/pstore/pstore.go:55:1: comment on exported type KmsgDump should be of the form "KmsgDump ..." (with optional leading article) (ST1021)
metropolis/pkg/scsi/inquiry.go:202:1: comment on exported type VPDPageCode should be of the form "VPDPageCode ..." (with optional leading article) (ST1021)
metropolis/pkg/scsi/log.go:154:3: could lift into loop condition (QF1006)
metropolis/pkg/scsi/sensekeydata.go:137:2: const IdCrcOrEccError should be IDCrcOrEccError (ST1003)
metropolis/pkg/scsi/sensekeydata.go:165:2: const AddressMarkNotFoundForIdField should be AddressMarkNotFoundForIDField (ST1003)
metropolis/pkg/scsi/sensekeydata.go:188:2: const RecoveredDataUsingPreviousSectorId should be RecoveredDataUsingPreviousSectorID (ST1003)
metropolis/pkg/scsi/sensekeydata.go:213:2: const RecoveredIdWithEccCorrection should be RecoveredIDWithEccCorrection (ST1003)
metropolis/pkg/scsi/sensekeydata.go:218:2: const AccessDeniedInvalidMgmtIdKey should be AccessDeniedInvalidMgmtIDKey (ST1003)
metropolis/pkg/scsi/sensekeydata.go:225:2: const AccessDeniedAclLunConflict should be AccessDeniedACLLunConflict (ST1003)
metropolis/pkg/scsi/sensekeydata.go:449:2: const IscsiIpAddressAdded should be IscsiIPAddressAdded (ST1003)
metropolis/pkg/scsi/sensekeydata.go:450:2: const IscsiIpAddressRemoved should be IscsiIPAddressRemoved (ST1003)
metropolis/pkg/scsi/sensekeydata.go:451:2: const IscsiIpAddressChanged should be IscsiIPAddressChanged (ST1003)
metropolis/pkg/scsi/sensekeydata.go:458:2: const RamFailureshouldUse40Nn should be RAMFailureshouldUse40Nn (ST1003)
metropolis/pkg/scsi/sensekeydata.go:707:2: const DecompressionExceptionLongAlgorithmId should be DecompressionExceptionLongAlgorithmID (ST1003)
metropolis/pkg/smbios/structures.go:15:1: comment on exported const UEFISpecificationSupported should be of the form "UEFISpecificationSupported ..." (ST1022)
metropolis/pkg/socksproxy/socksproxy_test.go:161:3: could lift into loop condition (QF1006)
metropolis/pkg/socksproxy/socksproxy_test.go:170:3: could lift into loop condition (QF1006)
metropolis/pkg/tpm/eventlog/internal/events.go:71:2: const CpuMicrocode should be CPUMicrocode (ST1003)
metropolis/pkg/tpm/eventlog/internal/events.go:151:46: no value of type uint32 is less than 0 (SA4003)
metropolis/pkg/verity/encoder.go:520:2: this value of e is never used (SA4006)
metropolis/pkg/verity/encoder.go:535:16: error strings should not end with punctuation or newlines (ST1005)
metropolis/pkg/verity/encoder.go:537:15: error strings should not end with punctuation or newlines (ST1005)
third_party/sandboxroot/mirror/external.go:189:11: error strings should not be capitalized (ST1005)
third_party/sandboxroot/mirror/external.go:192:11: error strings should not be capitalized (ST1005)
version/stampgo/main.go:149:2: this value of err is never used (SA4006)
fionera commented 5 months ago

Done with fixing them up. Now working on https://github.com/uber-go/nilaway

leoluk commented 5 months ago
fionera commented 5 months ago

improper use of klog.Exitf (which is almost always, since it calls os.Exit without unwinding anything)

We cannot detect these reliably, but we can forbid the use of them.

Extend importsort to only allow one group per importClass and move all proto imports into one group