From a105a2a5f741eceaee00a0a3ddaefde3e9085fad Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Fri, 14 Feb 2025 13:33:51 -0800 Subject: [PATCH] Refresh golangci-lint config and convert to YAML --- .golangci.toml | 641 ----------------------------------------------- .golangci.yml | 618 +++++++++++++++++++++++++++++++++++++++++++++ contributions.go | 6 +- parse.go | 2 +- parse_test.go | 2 +- 5 files changed, 623 insertions(+), 646 deletions(-) delete mode 100644 .golangci.toml create mode 100644 .golangci.yml diff --git a/.golangci.toml b/.golangci.toml deleted file mode 100644 index 1c812a3..0000000 --- a/.golangci.toml +++ /dev/null @@ -1,641 +0,0 @@ -[run] -# This is needed for precious, which may run multiple instances -# in parallel -allow-parallel-runners = true -go = "1.21" -tests = true -timeout = "10m" - -[linters] -enable-all = true -disable = [ - # The canonical form is not always the most common form for some headers - # and there is a small chance that switching existing strings could - # break something. - "canonicalheader", - - "cyclop", - "dogsled", - "dupl", - - # This is probably worthwhile, but there are a number of false positives - # that would need to be addressed. - "dupword", - - # We don't follow its policy about not defining dynamic errors. - "err113", - - # We often don't initialize all of the struct fields. This is fine - # generally - "exhaustruct", - - # We tried this linter but most places we do forced type asserts are - # pretty safe, e.g., an atomic.Value when everything is encapsulated - # in a small package. - "forcetypeassert", - - "funlen", - "gochecknoglobals", - "gochecknoinits", - - # Similar to the exhaustive linter and I don't know that we use these - # sorts of sum types - "gochecksumtype", - - "gocognit", - "godox", - - # This only "caught" one thing, and it seemed like a reasonable use - # of Han script. Generally, I don't think we want to prevent the use - # of particular scripts. The time.Local checks might be useful, but - # this didn't actually catch anything of note there. - "gosmopolitan", - - # Seems too opinionated or at least would require going through all the - # interfaces we have. - "inamedparam", - - "ireturn", - - # We don't use these loggers - "loggercheck", - - # Maintainability Index. Seems like it could be a good idea, but a - # lot of things fail and we would need to make some decisions about - # what to allow. - "maintidx", - - # Using a const for every number doesn't necessarily increase code clarity, - # and it would be a ton of work to move everything to that. - "mnd", - - # Causes panics, e.g., when processing mmerrors - "musttag", - - "nestif", - - # Perhaps too opinionated. We do have some legitimate uses of "return nil, nil" - "nilnil", - - "nlreturn", - - # We occasionally use named returns for documentation, which is helpful. - # Named returns are only really a problem when used in conjunction with - # a bare return statement. I _think_ Revive's bare-return covers that - # case. - "nonamedreturns", - - "paralleltest", - "prealloc", - - # We have very few structs with multiple tags and for the couple we had, this - # actually made it harder to read. - "tagalign", - - # We probably _should_ be doing this! - "thelper", - - # Deprecated since golangci-lint 1.64.0. The usetesting linter replaces it. - "tenv", - - # We don't follow this. Sometimes we test internal code. - "testpackage", - - "varnamelen", - - # This would probably be good, but we would need to configure it. - "wsl", - - # Require Go 1.22 - "copyloopvar", - "intrange", -] - -# Please note that we only use depguard for blocking packages and -# gomodguard for blocking modules. -# supports modules currently. See https://github.com/ryancurrah/gomodguard/issues/12 -[[linters-settings.depguard.rules.main.deny]] -pkg = "golang.org/x/exp/slog" -desc = "Use log/slog instead." - -[[linters-settings.depguard.rules.main.deny]] -pkg = "io/ioutil" -desc = "Deprecated. Functions have been moved elsewhere." - -[[linters-settings.depguard.rules.main.deny]] -pkg = "k8s.io/utils/strings/slices" -desc = "Use slices" - -[[linters-settings.depguard.rules.main.deny]] -# slices has better alternatives. -pkg = "sort" -desc = "Use slices instead" - -[linters-settings.errcheck] -# Don't allow setting of error to the blank identifier. If there is a legitimate -# reason, there should be a nolint with an explanation. -check-blank = true - -exclude-functions = [ - # If we are rolling back a transaction, we are often already in an error - # state. - '(*database/sql.Tx).Rollback', - - # It is reasonable to ignore errors if Cleanup fails in most cases. - '(*github.com/google/renameio/v2.PendingFile).Cleanup', - - # We often don't care if removing a file failed (e.g., it doesn't exist) - 'os.Remove', - 'os.RemoveAll', -] - -[linters-settings.errorlint] -errorf = true -asserts = true -comparison = true - -[linters-settings.exhaustive] -default-signifies-exhaustive = true - -[linters-settings.forbidigo] -# Forbid the following identifiers -forbid = [ - { p = "Geoip", msg = "you should use `GeoIP`" }, - { p = "geoIP", msg = "you should use `geoip`" }, - { p = "^hubSpot", msg = "you should use `hubspot`" }, - { p = "Maxmind", msg = "you should use `MaxMind`" }, - { p = "^maxMind", msg = "you should use `maxmind`" }, - { p = "Minfraud", msg = "you should use `MinFraud`" }, - { p = "^minFraud", msg = "you should use `minfraud`" }, - { p = "[Uu]ser[iI][dD]", msg = "you should use `accountID` or `AccountID`" }, - { p = "WithEnterpriseURLs", msg = "Use ghe.NewClient instead." }, - { p = "^bigquery.NewClient", msg = "you should use mmgcloud.NewBigQueryClient instead." }, - { p = "^cloudresourcemanager.NewService", msg = "you should use mmgcloud.NewCloudResourceManagerService instead." }, - { p = "^compute.NewService", msg = "you should use mmgcloud.NewComputeService instead." }, - { p = "^drive.NewService", msg = "you should use mmgdrive.NewGDrive instead." }, - { p = "^math.Max$", msg = "you should use the max built-in instead." }, - { p = "^math.Min$", msg = "you should use the min built-in instead." }, - { p = "^net.ParseCIDR", msg = "you should use netip.ParsePrefix unless you really need a *net.IPNet" }, - { p = "^net.ParseIP", msg = "you should use netip.ParseAddr unless you really need a net.IP" }, - { p = "^pgtype.NewMap", msg = "you should use mmdatabase.NewTypeMap instead" }, - { p = "^serviceusage.NewService", msg = "you should use mmgcloud.NewServiceUsageService instead." }, - { p = "^sheets.NewService", msg = "you should use mmgcloud.NewSheetsService instead." }, - { p = "^storage.NewClient", msg = "you should use gstorage.NewClient instead. This sets the HTTP client settings that we need for internal use." }, - { p = "^os.IsNotExist", msg = "As per their docs, new code should use errors.Is(err, fs.ErrNotExist)." }, - { p = "^os.IsExist", msg = "As per their docs, new code should use errors.Is(err, fs.ErrExist)" }, - { p = "^net.LookupIP", msg = "You should use net.Resolver functions instead." }, - { p = "^net.LookupCNAME", msg = "You should use net.Resolver functions instead." }, - { p = "^net.LookupHost", msg = "You should use net.Resolver functions instead." }, - { p = "^net.LookupPort", msg = "You should use net.Resolver functions instead." }, - { p = "^net.LookupTXT", msg = "You should use net.Resolver functions instead." }, - { p = "^net.LookupAddr", msg = "You should use net.Resolver functions instead." }, - { p = "^net.LookupMX", msg = "You should use net.Resolver functions instead." }, - { p = "^net.LookupNS", msg = "You should use net.Resolver functions instead." }, - { p = "^net.LookupSRV", msg = "You should use net.Resolver functions instead." }, -] - -[linters-settings.gci] -sections = ["standard", "default", "prefix(github.com/maxmind/xgbshap)"] - -[linters-settings.gocritic] -enable-all = true -disabled-checks = [ - # Revive's defer rule already captures this. This caught no extra cases. - "deferInLoop", - # Given that all of our code runs on Linux and the / separate should - # work fine, this seems less important. - "filepathJoin", - # This seems like it could be good, but we would need to update current - # uses. It supports "--fix", but the fixing is a bit broken. - "httpNoBody", - # This might be good, but we would have to revisit a lot of code. - "hugeParam", - # This might be good, but I don't think we want to encourage - # significant changes to regexes as we port stuff from Perl. - "regexpSimplify", - # This seems like it might also be good, but a lot of existing code - # fails. - "sloppyReassign", - # I am not sure we would want this linter and a lot of existing - # code fails. - "unnamedResult", - # Covered by nolintlint - "whyNoLint", -] - -[linters-settings.gofumpt] -extra-rules = true - -# IMPORTANT: gomodguard blocks _modules_, not arbitrary packages. Be -# sure to use the module path from the go.mod file for these. -[linters-settings.gomodguard] -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/avct/uasurfer"] -recommendations = ["github.com/xavivars/uasurfer"] -reason = "The original avct module appears abandoned." - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/BurntSushi/toml"] -recommendations = ["github.com/pelletier/go-toml/v2"] -reason = "This library panics frequently on invalid input." - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/pelletier/go-toml"] -recommendations = ["github.com/pelletier/go-toml/v2"] -reason = "This is an outdated version." - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/gofrs/uuid"] -recommendations = ["github.com/google/uuid"] - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/gofrs/uuid/v5"] -recommendations = ["github.com/google/uuid"] - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/satori/go.uuid"] -recommendations = ["github.com/google/uuid"] - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/lib/pq"] -recommendations = ["github.com/jackc/pgx"] -reason = "This library is no longer actively maintained." - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/neilotoole/errgroup"] -recommendations = ["golang.org/x/sync/errgroup"] -reason = "This library can lead to subtle deadlocks in certain use cases." - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/pariz/gountries"] -reason = "This library's data is not actively maintained. Use GeoInfo data." - -[linters-settings.gomodguard.blocked.modules."github.com/pkg/errors"] -recommendations = ["github.maxmind.com/maxmind/mm_website/go/pkg/mmerrors"] -reason = "pkg/errors is no longer maintained." - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/RackSec/srslog"] -recommendations = ["log/syslog"] -reason = "This library's data is not actively maintained." - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/ua-parser/uap-go"] -recommendations = ["github.com/xavivars/uasurfer"] -reason = "The performance of this library is absolutely abysmal." - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."github.com/ugorji/go"] -recommendations = ["encoding/json", "github.com/mailru/easyjson"] -reason = "This library is poorly maintained. We should default to using encoding/json and use easyjson where performance really matters." - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."gotest.tools/v3"] -recommendations = ["github.com/stretchr/testify/assert"] -reason = "Use github.com/stretchr/testify/assert" - -[[linters-settings.gomodguard.blocked.modules]] -[linters-settings.gomodguard.blocked.modules."inet.af/netaddr"] -recommendations = ["net/netip", "go4.org/netipx"] -reason = "inet.af/netaddr has been deprecated." - -[[linters-settings.gomodguard.blocked.versions]] -[linters-settings.gomodguard.blocked.versions."github.com/jackc/pgconn"] -reason = "Use github.com/jackc/pgx/v5" - -[[linters-settings.gomodguard.blocked.versions]] -[linters-settings.gomodguard.blocked.versions."github.com/jackc/pgtype"] -reason = "Use github.com/jackc/pgx/v5" - -[[linters-settings.gomodguard.blocked.versions]] -[linters-settings.gomodguard.blocked.versions."github.com/jackc/pgx"] -version = "< 5.0.0" -reason = "Use github.com/jackc/pgx/v5" - -[linters-settings.gosec] -excludes = [ - # G104 - "Audit errors not checked." We use errcheck for this. - "G104", - - # G306 - "Expect WriteFile permissions to be 0600 or less". - "G306", - - # Prohibits defer (*os.File).Close, which we allow when reading from file. - "G307", - - # no longer relevant with 1.22 - "G601", - - # xgbshap uses md5 - "G401", - "G501", -] - -[linters-settings.govet] -"enable-all" = true - -# Although it is very useful in particular cases where we are trying to -# use as little memory as possible, there are even more cases where -# other organizations may make more sense. -disable = ["fieldalignment"] - -[linters-settings.govet.settings.shadow] -strict = true - -[linters-settings.lll] -line-length = 120 -tab-width = 4 - -[linters-settings.misspell] -locale = "US" - -[[linters-settings.misspell.extra-words]] -typo = "marshall" -correction = "marshal" - -[[linters-settings.misspell.extra-words]] -typo = "marshalling" -correction = "marshaling" - -[[linters-settings.misspell.extra-words]] -typo = "marshalls" -correction = "marshals" - -[[linters-settings.misspell.extra-words]] -typo = "unmarshall" -correction = "unmarshal" - -[[linters-settings.misspell.extra-words]] -typo = "unmarshalling" -correction = "unmarshaling" - -[[linters-settings.misspell.extra-words]] -typo = "unmarshalls" -correction = "unmarshals" - -[linters-settings.nolintlint] -allow-unused = false -allow-no-explanation = ["lll", "misspell"] -require-explanation = true -require-specific = true - -[linters-settings.revive] -enable-all-rules = true -ignore-generated-header = true -severity = "warning" - -# This might be nice but it is so common that it is hard -# to enable. -[[linters-settings.revive.rules]] -name = "add-constant" -disabled = true - -[[linters-settings.revive.rules]] -name = "argument-limit" -disabled = true - -[[linters-settings.revive.rules]] -name = "cognitive-complexity" -disabled = true - -[[linters-settings.revive.rules]] -name = "comment-spacings" -arguments = ["easyjson", "nolint"] -disabled = false - -# Probably a good rule, but we have a lot of names that -# only have case differences. -[[linters-settings.revive.rules]] -name = "confusing-naming" -disabled = true - -[[linters-settings.revive.rules]] -name = "cyclomatic" -disabled = true - -# Although being consistent might be nice, I don't know that it -# is worth the effort enabling this rule. It doesn't have an -# autofix option. -[[linters-settings.revive.rules]] -name = "enforce-repeated-arg-type-style" -arguments = ["short"] -disabled = true - -[[linters-settings.revive.rules]] -name = "enforce-map-style" -arguments = ["literal"] -disabled = false - -# We have very few of these as we force nil slices in most places, -# but there are a couple of cases. -[[linters-settings.revive.rules]] -name = "enforce-slice-style" -arguments = ["literal"] -disabled = false - -[[linters-settings.revive.rules]] -name = "file-header" -disabled = true - -# We have a lot of flag parameters. This linter probably makes -# a good point, but we would need some cleanup or a lot of nolints. -[[linters-settings.revive.rules]] -name = "flag-parameter" -disabled = true - -[[linters-settings.revive.rules]] -name = "function-length" -disabled = true - -[[linters-settings.revive.rules]] -name = "function-result-limit" -disabled = true - -[[linters-settings.revive.rules]] -name = "line-length-limit" -disabled = true - -[[linters-settings.revive.rules]] -name = "max-public-structs" -disabled = true - -# We frequently use nested structs, particularly in tests. -[[linters-settings.revive.rules]] -name = "nested-structs" -disabled = true - -# This doesn't make sense with 1.22 loop var changes. -[[linters-settings.revive.rules]] -name = "range-val-address" -disabled = true - -# This causes a ton of failures. Many are fairly safe. It might be nice to -# enable, but probably not worth the effort. -[[linters-settings.revive.rules]] -name = "unchecked-type-assertion" -disabled = true - -# This is covered elsewhere and we want to ignore some -# functions such as fmt.Fprintf. -[[linters-settings.revive.rules]] -name = "unhandled-error" -disabled = true - -# We generally have unused receivers in tests for meeting the -# requirements of an interface. -[[linters-settings.revive.rules]] -name = "unused-receiver" -disabled = true - -[linters-settings.tagliatelle.case.rules] -avro = "snake" -bson = "snake" -env = "upperSnake" -envconfig = "upperSnake" -json = "snake" -mapstructure = "snake" -xml = "snake" -yaml = "snake" - -[linters-settings.unparam] -check-exported = true - -[linters-settings.wrapcheck] -"ignoreSigs" = [ - ".Errorf(", - "errgroup.NewMultiError(", - "errors.Join(", - "errors.New(", - ".Wait(", - ".WithStack(", - ".Wrap(", - ".Wrapf(", - "v4.Retry(", - "v4.RetryNotify(", -] - -[issues] -exclude-use-default = false - -exclude-dirs = [ - "geoip-build/mmcsv", -] - -exclude-files = [ - "_easyjson\\.go$", - "_easyjson_test\\.go$", - "_xgb2code\\.go$", - "_json2vector\\.go$", -] - -[[issues.exclude-rules]] -linters = [ - "bodyclose", -] -# This rule doesn't really make sense for tests where we don't have an open -# connection and we might be passing around the response for other reasons. -path = "_test.go" - -[[issues.exclude-rules]] -linters = [ - "forbidigo", -] -# This refers to a minFraud field, not the MaxMind Account ID -source = "AccountUserID|Account\\.UserID" - -# we include both a source and text exclusion as the source exclusion -# misses matches where forbidigo reports the error on the first line -# of a chunk of a function call even though the use is on a later line. -[[issues.exclude-rules]] -linters = [ - "forbidigo", -] -text = "AccountUserID|Account\\.UserID" - -[[issues.exclude-rules]] -linters = [ - "gocritic", -] -# For some reason the imports stuff in ruleguard doesn't work in golangci-lint. -# Perhaps it has an outdated version or something -path = "_test.go" -text = "ruleguard: Prefer the alternative Context method instead" - -[[issues.exclude-rules]] -linters = [ - "gocritic", -] -# The nolintlint linter behaves oddly with ruleguard rules -source = "// *no-ruleguard" - -[[issues.exclude-rules]] -linters = [ - "nolintlint", -] -# The contextcheck linter also uses "nolint" in a slightly different way, -# leading to falso positives from nolintlint. -source = "//nolint:contextcheck //.*" - -[[issues.exclude-rules]] -linters = [ - "govet", -] -# These are usually fine to shadow and not allowing shadowing for them can -# make the code unnecessarily verbose. -text = 'shadow: declaration of "(ctx|err|ok)" shadows declaration' - -[[issues.exclude-rules]] -linters = [ - "contextcheck", - # With recent changes to the linter, there were a lot of failures in - # the tests and it wasn't clear to me that fixing them would actually - # improve the readability. - "goconst", - "nilerr", - "wrapcheck", -] -path = "_test.go" - -[[issues.exclude-rules]] -linters = [ - "errcheck", -] -# There are many cases where we want to just close resources and ignore the -# error (e.g., for defer f.Close on a read). errcheck removed its built-in -# wildcard ignore. I tried listing all of the cases, but it was too many -# and some were very specific. -source = "\\.Close" - -[[issues.exclude-rules]] -linters = [ - "stylecheck", -] -# ST1016 - methods on the same type should have the same receiver name. -# easyjson doesn't interact well with this. -text = "ST1016" - -[[issues.exclude-rules]] -linters = [ - "staticcheck", -] -# SA5008: unknown JSON option "intern" - easyjson specific option. -text = 'SA5008: unknown JSON option "intern"' - -[[issues.exclude-rules]] -linters = [ - "wrapcheck", -] -text = "github.com/maxmind/xgbshap" - -[[issues.exclude-rules]] -linters = [ - "wrapcheck", -] -path = "_easyjson.go" - -[[issues.exclude-rules]] -linters = [ - "gocritic", -] -source = "Chmod|WriteFile" -text = "octalLiteral" diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..8f37a94 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,618 @@ +run: + # This is needed for precious, which may run multiple instances + # in parallel + allow-parallel-runners: true + go: "1.23" + tests: true + timeout: "30m" + +linters: + enable-all: true + disable: + # The canonical form is not always the most common form for some headers + # and there is a small chance that switching existing strings could + # break something. + - canonicalheader + + - cyclop + - dogsled + - dupl + + # This is probably worthwhile, but there are a number of false positives + # that would need to be addressed. + - dupword + + # We don't follow its policy about not defining dynamic errors. + - err113 + + # We often don't initialize all of the struct fields. This is fine + # generally + - exhaustruct + + # We tried this linter but most places we do forced type asserts are + # pretty safe, e.g., an atomic.Value when everything is encapsulated + # in a small package. + - forcetypeassert + + - funlen + - gochecknoglobals + - gochecknoinits + + # Similar to the exhaustive linter and I don't know that we use these + # sorts of sum types + - gochecksumtype + + - gocognit + - godox + + # This only "caught" one thing, and it seemed like a reasonable use + # of Han script. Generally, I don't think we want to prevent the use + # of particular scripts. The time.Local checks might be useful, but + # this didn't actually catch anything of note there. + - gosmopolitan + + # Seems too opinionated or at least would require going through all the + # interfaces we have. + - inamedparam + + - ireturn + + # We don't use these loggers + - loggercheck + + # Maintainability Index. Seems like it could be a good idea, but a + # lot of things fail and we would need to make some decisions about + # what to allow. + - maintidx + + # Using a const for every number doesn't necessarily increase code clarity, + # and it would be a ton of work to move everything to that. + - mnd + + # Causes panics, e.g., when processing mmerrors + - musttag + + - nestif + + # Perhaps too opinionated. We do have some legitimate uses of "return nil, nil" + - nilnil + + - nlreturn + + # We occasionally use named returns for documentation, which is helpful. + # Named returns are only really a problem when used in conjunction with + # a bare return statement. I _think_ Revive's bare-return covers that + # case. + - nonamedreturns + + - paralleltest + - prealloc + + # We have very few structs with multiple tags and for the couple we had, this + # actually made it harder to read. + - tagalign + + # Deprecated since golangci-lint 1.64.0. The usetesting linter replaces it. + - tenv + + # We probably _should_ be doing this! + - thelper + + # We don't follow this. Sometimes we test internal code. + - testpackage + + - varnamelen + + # This would probably be good, but we would need to configure it. + - wsl + +linters-settings: + # Please note that we only use depguard for blocking packages and + # gomodguard for blocking modules. + depguard: + rules: + main: + deny: + - pkg: github.com/likexian/gokit/assert + desc: Use github.com/stretchr/testify/assert + + - pkg: golang.org/x/exp/maps + desc: Use maps instead. + + - pkg: golang.org/x/exp/slices + desc: Use slices instead. + + - pkg: golang.org/x/exp/slog + desc: Use log/slog instead. + + - pkg: google.golang.org/api/compute/v1 + desc: Use cloud.google.com/go/compute/apiv1 instead. + + - pkg: google.golang.org/api/cloudresourcemanager/v1 + desc: Use cloud.google.com/go/resourcemanager/apiv3 instead. + + - pkg: google.golang.org/api/serviceusage/v1 + desc: Use cloud.google.com/go/serviceusage/apiv1 instead. + + - pkg: io/ioutil + desc: Deprecated. Functions have been moved elsewhere. + + - pkg: k8s.io/utils/strings/slices + desc: Use slices + + - pkg: math/rand$ + desc: Use math/rand/v2 or crypto/rand as appropriate. + + - pkg: sort + desc: Use slices instead + + errcheck: + # Don't allow setting of error to the blank identifier. If there is a legitimate + # reason, there should be a nolint with an explanation. + check-blank: true + + exclude-functions: + # If we are rolling back a transaction, we are often already in an error + # state. + - (*database/sql.Tx).Rollback + + # It is reasonable to ignore errors if Cleanup fails in most cases. + - (*github.com/google/renameio/v2.PendingFile).Cleanup + + # We often do not care if unlocking failed as we are exiting anyway. + - (*github.com/gofrs/flock.Flock).Unlock + + # We often don't care if removing a file failed (e.g., it doesn't exist) + - os.Remove + - os.RemoveAll + + # This is a workaround for https://github.com/golangci/golangci-lint/issues/4733 + ignore: "" + + errorlint: + errorf: true + asserts: true + comparison: true + + exhaustive: + default-signifies-exhaustive: true + + forbidigo: + # Forbid the following identifiers + forbid: + - p: Geoip + msg: you should use the `GeoIP` qualifier instead + - p: geoIP + msg: you should use the `geoip` qualifier instead + - p: ^hubSpot + msg: you should use the `hubspot` qualifier instead + - p: Maxmind + msg: you should use the `MaxMind` qualifier instead + - p: ^maxMind + msg: you should use the `maxmind` qualifier instead + - p: Minfraud + msg: you should use the `MinFraud` qualifier instead + - p: ^minFraud + msg: you should use the `minfraud` qualifier instead + - p: "[Uu]ser[iI][dD]" + msg: you should use the `accountID` or the `AccountID` qualifier instead + - p: WithEnterpriseURLs + msg: Use ghe.NewClient instead. + - p: ^bigquery.NewClient + msg: you should use mmgcloud.NewBigQueryClient instead. + - p: ^drive.NewService + msg: you should use mmgdrive.NewGDrive instead. + - p: ^filepath.Walk$ + msg: you should use filepath.WalkDir instead as it doesn't call os.Lstat on every entry. + - p: ^math.Max$ + msg: you should use the max built-in instead. + - p: ^math.Min$ + msg: you should use the min built-in instead. + - p: ^mux.Vars$ + msg: use req.PathValue instead. + - p: ^net.ParseCIDR + msg: you should use netip.ParsePrefix unless you really need a *net.IPNet + - p: ^net.ParseIP + msg: you should use netip.ParseAddr unless you really need a net.IP + - p: ^pgtype.NewMap + msg: you should use mmdatabase.NewTypeMap instead + - p: ^sheets.NewService + msg: you should use mmgcloud.NewSheetsService instead. + - p: ^storage.NewClient + msg: you should use gstorage.NewClient instead. This sets the HTTP client settings that we need for internal use. + - p: ^os.IsNotExist + msg: As per their docs, new code should use errors.Is(err, fs.ErrNotExist). + - p: ^os.IsExist + msg: As per their docs, new code should use errors.Is(err, fs.ErrExist) + - p: ^net.LookupIP + msg: You should use net.Resolver functions instead. + - p: ^net.LookupCNAME + msg: You should use net.Resolver functions instead. + - p: ^net.LookupHost + msg: You should use net.Resolver functions instead. + - p: ^net.LookupPort + msg: You should use net.Resolver functions instead. + - p: ^net.LookupTXT + msg: You should use net.Resolver functions instead. + - p: ^net.LookupAddr + msg: You should use net.Resolver functions instead. + - p: ^net.LookupMX + msg: You should use net.Resolver functions instead. + - p: ^net.LookupNS + msg: You should use net.Resolver functions instead. + - p: ^net.LookupSRV + msg: You should use net.Resolver functions instead. + + gci: + sections: + - standard + - default + - prefix(github.com/maxmind/xgbshap) + + gocritic: + enable-all: true + disabled-checks: + # Revive's defer rule already captures this. This caught no extra cases. + - deferInLoop + # Given that all of our code runs on Linux and the / separate should + # work fine, this seems less important. + - filepathJoin + # This seems like it could be good, but we would need to update current + # uses. It supports "--fix", but the fixing is a bit broken. + - httpNoBody + # This might be good, but we would have to revisit a lot of code. + - hugeParam + # This might be good, but I don't think we want to encourage + # significant changes to regexes as we port stuff from Perl. + - regexpSimplify + # This seems like it might also be good, but a lot of existing code + # fails. + - sloppyReassign + # I am not sure we would want this linter and a lot of existing + # code fails. + - unnamedResult + # Covered by nolintlint + - whyNoLint + + gofumpt: + extra-rules: true + + # IMPORTANT: gomodguard blocks _modules_, not arbitrary packages. Be + # sure to use the module path from the go.mod file for these. + # See https://github.com/ryancurrah/gomodguard/issues/12 + gomodguard: + blocked: + modules: + - github.com/avct/uasurfer: + recommendations: + - github.com/xavivars/uasurfer + reason: The original avct module appears abandoned. + - github.com/BurntSushi/toml: + recommendations: + - github.com/pelletier/go-toml/v2 + reason: This library panics frequently on invalid input. + - github.com/pelletier/go-toml: + recommendations: + - github.com/pelletier/go-toml/v2 + reason: This is an outdated version. + - github.com/gofrs/uuid: + recommendations: + - github.com/google/uuid + - github.com/gofrs/uuid/v5: + recommendations: + - github.com/google/uuid + - github.com/satori/go.uuid: + recommendations: + - github.com/google/uuid + - github.com/lib/pq: + recommendations: + - github.com/jackc/pgx + reason: This library is no longer actively maintained. + - github.com/neilotoole/errgroup: + recommendations: + - golang.org/x/sync/errgroup + reason: This library can lead to subtle deadlocks in certain use cases. + - github.com/pariz/gountries: + reason: This library's data is not actively maintained. Use GeoInfo data. + github.com/pkg/errors: + reason: pkg/errors is no longer maintained. + - github.com/RackSec/srslog: + recommendations: + - log/syslog + reason: This library's data is not actively maintained. + - github.com/ua-parser/uap-go: + recommendations: + - github.com/xavivars/uasurfer + reason: The performance of this library is absolutely abysmal. + - github.com/ugorji/go: + recommendations: + - encoding/json + - github.com/mailru/easyjson + reason: This library is poorly maintained. We should default to using encoding/json and use easyjson where performance really matters. + - github.com/zeebo/assert: + recommendations: + - github.com/stretchr/testify/assert + reason: Use github.com/stretchr/testify/assert + - gotest.tools/v3: + recommendations: + - github.com/stretchr/testify/assert + reason: Use github.com/stretchr/testify/assert + - inet.af/netaddr: + recommendations: + - net/netip + - go4.org/netipx + reason: inet.af/netaddr has been deprecated. + versions: + - github.com/jackc/pgconn: + reason: Use github.com/jackc/pgx/v5 + - github.com/jackc/pgtype: + reason: Use github.com/jackc/pgx/v5 + - github.com/jackc/pgx: + version: < 5.0.0 + reason: Use github.com/jackc/pgx/v5 + + gosec: + excludes: + # G104 - "Audit errors not checked." We use errcheck for this. + - G104 + + # G306 - "Expect WriteFile permissions to be 0600 or less". + - G306 + + # Prohibits defer (*os.File).Close, which we allow when reading from file. + - G307 + + # no longer relevant with 1.22 + - G601 + + govet: + enable-all: true + # Although it is very useful in particular cases where we are trying to + # use as little memory as possible, there are even more cases where + # other organizations may make more sense. + disable: + - fieldalignment + settings: + shadow: + strict: true + lll: + line-length: 120 + tab-width: 4 + + misspell: + locale: "US" + extra-words: + - typo: "marshall" + correction: "marshal" + - typo: "marshalling" + correction: "marshaling" + - typo: "marshalls" + correction: "marshals" + - typo: "unmarshall" + correction: "unmarshal" + - typo: "unmarshalling" + correction: "unmarshaling" + - typo: "unmarshalls" + correction: "unmarshals" + + nolintlint: + allow-unused: false + allow-no-explanation: ["lll", "misspell"] + require-explanation: true + require-specific: true + + revive: + enable-all-rules: true + ignore-generated-header: true + severity: "warning" + + rules: + # This might be nice but it is so common that it is hard + # to enable. + - name: "add-constant" + disabled: true + + - name: "argument-limit" + disabled: true + + - name: "cognitive-complexity" + disabled: true + + - name: "comment-spacings" + arguments: ["easyjson", "nolint"] + disabled: false + + # Probably a good rule, but we have a lot of names that + # only have case differences. + - name: "confusing-naming" + disabled: true + + - name: "cyclomatic" + disabled: true + + # Although being consistent might be nice, I don't know that it + # is worth the effort enabling this rule. It doesn't have an + # autofix option. + - name: "enforce-repeated-arg-type-style" + arguments: ["short"] + disabled: true + + - name: "enforce-map-style" + arguments: ["literal"] + disabled: false + + # We have very few of these as we force nil slices in most places, + # but there are a couple of cases. + - name: "enforce-slice-style" + arguments: ["literal"] + disabled: false + + - name: "file-header" + disabled: true + + # We have a lot of flag parameters. This linter probably makes + # a good point, but we would need some cleanup or a lot of nolints. + - name: "flag-parameter" + disabled: true + + - name: "function-length" + disabled: true + + - name: "function-result-limit" + disabled: true + + - name: "line-length-limit" + disabled: true + + - name: "max-public-structs" + disabled: true + + # We frequently use nested structs, particularly in tests. + - name: "nested-structs" + disabled: true + + # This doesn't make sense with 1.22 loop var changes. + - name: "range-val-address" + disabled: true + + # This flags things that do not seem like a problem, e.g. "sixHours". + - name: "time-naming" + disabled: true + + # This causes a ton of failures. Many are fairly safe. It might be nice to + # enable, but probably not worth the effort. + - name: "unchecked-type-assertion" + disabled: true + + # This seems to give many false positives. + - name: "unconditional-recursion" + disabled: true + + # This is covered elsewhere and we want to ignore some + # functions such as fmt.Fprintf. + - name: "unhandled-error" + disabled: true + + # We generally have unused receivers in tests for meeting the + # requirements of an interface. + - name: "unused-receiver" + disabled: true + + tagliatelle: + case: + rules: + avro: "snake" + bson: "snake" + env: "upperSnake" + envconfig: "upperSnake" + json: "snake" + mapstructure: "snake" + xml: "snake" + yaml: "snake" + + unparam: + check-exported: true + + wrapcheck: + ignoreSigs: + - ".Errorf(" + - "errgroup.NewMultiError(" + - "errors.Join(" + - "errors.New(" + - ".Wait(" + - ".WithStack(" + - ".Wrap(" + - ".Wrapf(" + - "v5.Retry[T any](" + +issues: + exclude-use-default: false + + exclude-dirs: + - "geoip-build/mmcsv" + + exclude-files: + - "_easyjson\\.go$" + - "_easyjson_test\\.go$" + - "_xgb2code\\.go$" + - "_json2vector\\.go$" + + exclude-rules: + - linters: + - "bodyclose" + # This rule doesn't really make sense for tests where we don't have an open + # connection and we might be passing around the response for other reasons. + path: "_test.go" + + - linters: + - "errcheck" + # There are many cases where we want to just close resources and ignore the + # error (e.g., for defer f.Close on a read). errcheck removed its built-in + # wildcard ignore. I tried listing all of the cases, but it was too many + # and some were very specific. + source: "\\.Close" + + - linters: + - "forbidigo" + # This refers to a minFraud field, not the MaxMind Account ID + source: "AccountUserID|Account\\.UserID" + + # we include both a source and text exclusion as the source exclusion + # misses matches where forbidigo reports the error on the first line + # of a chunk of a function call even though the use is on a later line. + - linters: + - "forbidigo" + text: "AccountUserID|Account\\.UserID" + + - linters: + - "gocritic" + # For some reason the imports stuff in ruleguard doesn't work in golangci-lint. + # Perhaps it has an outdated version or something + path: "_test.go" + text: "ruleguard: Prefer the alternative Context method instead" + + - linters: + - "gocritic" + # The nolintlint linter behaves oddly with ruleguard rules + source: "// *no-ruleguard" + + - linters: + - "nolintlint" + # The contextcheck linter also uses "nolint" in a slightly different way, + # leading to falso positives from nolintlint. + source: "//nolint:contextcheck //.*" + + - linters: + - "govet" + # These are usually fine to shadow and not allowing shadowing for them can + # make the code unnecessarily verbose. + text: 'shadow: declaration of "(ctx|err|ok)" shadows declaration' + + - linters: + - "contextcheck" + # With recent changes to the linter, there were a lot of failures in + # the tests and it wasn't clear to me that fixing them would actually + # improve the readability. + - "goconst" + - "nilerr" + - "wrapcheck" + path: "_test.go" + + - linters: + - "stylecheck" + # ST1016 - methods on the same type should have the same receiver name. + # easyjson doesn't interact well with this. + text: "ST1016" + + - linters: + - "wrapcheck" + text: "github.com/maxmind/xgbshap" + + - linters: + - "wrapcheck" + path: "_easyjson.go" + + - linters: + - "gocritic" + source: "Chmod|WriteFile" + text: "octalLiteral" diff --git a/contributions.go b/contributions.go index b7010a4..52058dc 100644 --- a/contributions.go +++ b/contributions.go @@ -76,7 +76,7 @@ func predictContributions( // Initialize tree node mean values. meanValues := make([][]float32, ntreeLimit) - for i := 0; i < ntreeLimit; i++ { + for i := range ntreeLimit { meanValues[i] = make([]float32, trees[i].NumNodes) nodeIndex := 0 @@ -93,7 +93,7 @@ func predictContributions( // If ngroup was not 1, then we'd need an additional loop here. - for i := 0; i < ntreeLimit; i++ { + for i := range ntreeLimit { treeMeanValues := meanValues[i] treeContribs := make([]float32, nColumns) @@ -114,7 +114,7 @@ func predictContributions( return nil, err } - for ci := 0; ci < nColumns; ci++ { + for ci := range nColumns { // tree_weights is null in my testing, so I'm ignoring it. contribs[ci] += treeContribs[ci] } diff --git a/parse.go b/parse.go index 626065f..bee5b48 100644 --- a/parse.go +++ b/parse.go @@ -137,7 +137,7 @@ func parseTree( } nodes := make([]Node, numNodes) - for i := 0; i < int(numNodes); i++ { + for i := range numNodes { nodes[i].Data = NodeData{ BaseWeight: xt.BaseWeights[i], DefaultLeft: xt.DefaultLeft[i] == 1, diff --git a/parse_test.go b/parse_test.go index 69f2ae8..9d9eee1 100644 --- a/parse_test.go +++ b/parse_test.go @@ -7,7 +7,7 @@ import ( ) func BenchmarkParseModel(b *testing.B) { - for i := 0; i < b.N; i++ { + for range b.N { _, _, err := parseModel("testdata/small-model/model.json") require.NoError(b, err) }