Enable previously disabled revive (lint) rules and fix up code

[alexandear]

This PR enables revive linter default rules that were implicitly disabled in #12197. Also, the PR fixes revive lint issues.

Explanation

By design, if no particular configuration is provided, revive will behave as golint does, i.e. all golint rules are enabled (see this default.toml). But when a configuration is provided, only rules in the configuration are enabled (see revive's doc). That's why, when PR #12197 disables unused-parameter in the configuration, it actually disables the whole default ruleset.

Thanks to @bboreham for pointing out revive's configuration issue here

Additional Logs

Below is the full list of revive issues that were disabled or fixed:

❯ golangci-lint run
cmd/promtool/main.go:414:5: error-naming: error var lintError should have name of the form errFoo (revive)
var lintError = fmt.Errorf("lint error")
    ^
discovery/openstack/instance.go:148:4: var-naming: var flavorId should be flavorID (revive)
                        flavorId, ok := s.Flavor["id"].(string)
                        ^
discovery/openstack/instance.go:155:4: var-naming: var imageId should be imageID (revive)
                        imageId, ok := s.Image["id"].(string)
                        ^
discovery/zookeeper/zookeeper.go:196:4: empty-block: this block is empty, you can remove it (revive)
                        for range pathUpdate {
                        }
model/histogram/float_histogram.go:344:1: receiver-naming: receiver name fh should be consistent with previous receiver name h for FloatHistogram (revive)
func (fh *FloatHistogram) Size() int {
        // Size of each slice separately.
        posSpanSize := len(fh.PositiveSpans) * 8     // 8 bytes (int32 + uint32).
        negSpanSize := len(fh.NegativeSpans) * 8     // 8 bytes (int32 + uint32).
        posBucketSize := len(fh.PositiveBuckets) * 8 // 8 bytes (float64).
        negBucketSize := len(fh.NegativeBuckets) * 8 // 8 bytes (float64).

        // Total size of the struct.

        // fh is 8 bytes.
        // fh.CounterResetHint is 4 bytes (1 byte bool + 3 bytes padding).
        // fh.Schema is 4 bytes.
        // fh.ZeroThreshold is 8 bytes.
        // fh.ZeroCount is 8 bytes.
        // fh.Count is 8 bytes.
        // fh.Sum is 8 bytes.
        // fh.PositiveSpans is 24 bytes.
        // fh.NegativeSpans is 24 bytes.
        // fh.PositiveBuckets is 24 bytes.
        // fh.NegativeBuckets is 24 bytes.
        structSize := 144

        return structSize + posSpanSize + negSpanSize + posBucketSize + negBucketSize
}
model/histogram/float_histogram.go:1021:22: var-declaration: should omit type int from declaration of var iSpan; it will be inferred from the right-hand side (revive)
                iSpan              int = -1
                                   ^
model/histogram/float_histogram.go:1022:22: var-declaration: should omit type int from declaration of var iBucket; it will be inferred from the right-hand side (revive)
                iBucket            int = -1
                                   ^
model/histogram/float_histogram.go:1055:13: superfluous-else: if block ends with a goto statement, so drop this else and outdent its block (revive)
                                        } else {
                                                spansA = append(spansA, Span{})
                                                copy(spansA[1:], spansA)
                                                spansA[0] = Span{Offset: indexB, Length: 1}
                                                if len(spansA) > 1 {
                                                        // Convert the absolute offset in the formerly
                                                        // first span to a relative offset.
                                                        spansA[1].Offset -= indexB + 1
                                                }
                                                goto nextLoop
                                        }
model/histogram/float_histogram.go:1083:12: superfluous-else: if block ends with a break statement, so drop this else and outdent its block (revive)
                                } else {
                                        deltaIndex -= remainingInSpan
                                        iBucket += int(remainingInSpan)
                                        iSpan++
                                        if iSpan == len(spansA) || deltaIndex < spansA[iSpan].Offset {
                                                // Bucket is in gap behind previous span (or there are no further spans).
                                                bucketsA = append(bucketsA, 0)
                                                copy(bucketsA[iBucket+1:], bucketsA[iBucket:])
                                                bucketsA[iBucket] = bucketB
                                                switch {
                                                case deltaIndex == 0:
                                                        // Directly after previous span, extend previous span.
                                                        if iSpan < len(spansA) {
                                                                spansA[iSpan].Offset--
                                                        }
                                                        iSpan--
                                                        iInSpan = int32(spansA[iSpan].Length)
                                                        spansA[iSpan].Length++
                                                        goto nextLoop
                                                case iSpan < len(spansA) && deltaIndex == spansA[iSpan].Offset-1:
                                                        // Directly before next span, extend next span.
                                                        iInSpan = 0
                                                        spansA[iSpan].Offset--
                                                        spansA[iSpan].Length++
                                                        goto nextLoop
                                                default:
                                                        // No next span, or next span is not directly adjacent to new bucket.
                                                        // Add new span.
                                                        iInSpan = 0
                                                        if iSpan < len(spansA) {
                                                                spansA[iSpan].Offset -= deltaIndex + 1
                                                        }
                                                        spansA = append(spansA, Span{})
                                                        copy(spansA[iSpan+1:], spansA[iSpan:])
                                                        spansA[iSpan] = Span{Length: 1, Offset: deltaIndex}
                                                        goto nextLoop
                                                }
                                        } else {
                                                // Try start of next span.
                                                deltaIndex -= spansA[iSpan].Offset
                                                iInSpan = 0
                                        }
                                }
promql/engine.go:2028:47: empty-block: this block is empty, you can remove it (revive)
                for drop = 0; floats[drop].T < mint; drop++ {
                }
promql/engine.go:2050:51: empty-block: this block is empty, you can remove it (revive)
                for drop = 0; histograms[drop].T < mint; drop++ {
                }
promql/parser/lex.go:569:12: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive)
                                } else {
                                        l.errorf("missing `:` for histogram descriptor")
                                }
promql/parser/parse.go:75:43: unexported-return: exported func NewParser returns unexported type *parser.parser, which can be annoying to use (revive)
func NewParser(input string, opts ...Opt) *parser {
                                          ^
promql/parser/parse.go:662:92: empty-block: this block is empty, you can remove it (revive)
                        for r.Start = n.LHS.PositionRange().End; isSpace(rune(p.lex.input[r.Start])); r.Start++ {
                        }
promql/parser/parse.go:664:92: empty-block: this block is empty, you can remove it (revive)
                        for r.End = n.RHS.PositionRange().Start - 1; isSpace(rune(p.lex.input[r.End])); r.End-- {
                        }
storage/buffer_test.go:262:36: empty-block: this block is empty, you can remove it (revive)
        for it.Next() != chunkenc.ValNone {
                // Scan everything.
        }
storage/memoized_iterator_test.go:115:36: empty-block: this block is empty, you can remove it (revive)
        for it.Next() != chunkenc.ValNone {
                // Scan everything.
        }
storage/remote/azuread/azuread.go:65:6: exported: type name will be used as azuread.AzureADConfig by other packages, and that stutters; consider calling this Config (revive)
type AzureADConfig struct {
     ^
storage/remote/azuread/azuread_test.go:286:4: increment-decrement: should replace mockGetTokenCallCounter += 1 with mockGetTokenCallCounter++ (revive)
                        mockGetTokenCallCounter += 1
                        ^
storage/remote/otlptranslator/prometheus/normalize_label.go:18:6: exported: func name will be used as normalize.NormalizeLabel by other packages, and that stutters; consider calling this Label (revive)
func NormalizeLabel(label string) string {
     ^
tsdb/chunkenc/float_histogram.go:106:37: empty-block: this block is empty, you can remove it (revive)
        for it.Next() == ValFloatHistogram {
        }
tsdb/chunkenc/float_histogram_test.go:160:38: empty-block: this block is empty, you can remove it (revive)
        for itX.Next() == ValFloatHistogram {
                // Just iterate through without accessing anything.
        }
tsdb/chunkenc/histogram.go:117:32: empty-block: this block is empty, you can remove it (revive)
        for it.Next() == ValHistogram {
        }
tsdb/chunkenc/histogram_test.go:165:33: empty-block: this block is empty, you can remove it (revive)
        for itX.Next() == ValHistogram {
                // Just iterate through without accessing anything.
        }
tsdb/chunkenc/xor.go:102:27: empty-block: this block is empty, you can remove it (revive)
        for it.Next() != ValNone {
        }
tsdb/errors/errors.go:28:30: unexported-return: exported func NewMulti returns unexported type errors.multiError, which can be annoying to use (revive)
func NewMulti(errs ...error) multiError {
                             ^
tsdb/head_wal.go:407:3: empty-block: this block is empty, you can remove it (revive)
                for range decoded {
                }
tsdb/head_wal.go:531:2: empty-block: this block is empty, you can remove it (revive)
        for range wp.output {
        }
tsdb/head_wal.go:533:2: empty-block: this block is empty, you can remove it (revive)
        for range wp.histogramsOutput {
        }
tsdb/head_wal.go:852:2: empty-block: this block is empty, you can remove it (revive)
        for range wp.output {
        }
tsdb/head_wal.go:1367:5: empty-block: this block is empty, you can remove it (revive)
                                for range rc {
                                }
tsdb/querier_test.go:2251:33: context-as-argument: context.Context should be the first parameter of a function (revive)
func appendSeries(t *testing.T, ctx context.Context, wg *sync.WaitGroup, h *Head) {
                                ^
tsdb/wlog/wlog.go:975:44: unexported-return: exported func NewSegmentBufReader returns unexported type *wlog.segmentBufReader, which can be annoying to use (revive)
func NewSegmentBufReader(segs ...*Segment) *segmentBufReader {
                                           ^
tsdb/wlog/wlog.go:986:71: unexported-return: exported func NewSegmentBufReaderWithOffset returns unexported type *wlog.segmentBufReader, which can be annoying to use (revive)
func NewSegmentBufReaderWithOffset(offset int, segs ...*Segment) (sbr *segmentBufReader, err error) {
                                                                      ^
util/annotations/annotations.go:102:2: error-naming: error var PromQLInfo should have name of the form ErrFoo (revive)
        PromQLInfo    = errors.New("PromQL info")
        ^
util/annotations/annotations.go:103:2: error-naming: error var PromQLWarning should have name of the form ErrFoo (revive)
        PromQLWarning = errors.New("PromQL warning")
        ^
util/annotations/annotations.go:105:2: error-naming: error var InvalidQuantileWarning should have name of the form ErrFoo (revive)
        InvalidQuantileWarning              = fmt.Errorf("%w: quantile value should be between 0 and 1", PromQLWarning)
        ^
util/annotations/annotations.go:106:2: error-naming: error var BadBucketLabelWarning should have name of the form ErrFoo (revive)
        BadBucketLabelWarning               = fmt.Errorf("%w: bucket label %q is missing or has a malformed value", PromQLWarning, model.BucketLabel)
        ^
util/annotations/annotations.go:107:2: error-naming: error var MixedFloatsHistogramsWarning should have name of the form ErrFoo (revive)
        MixedFloatsHistogramsWarning        = fmt.Errorf("%w: encountered a mix of histograms and floats for metric name", PromQLWarning)
        ^
util/annotations/annotations.go:108:2: error-naming: error var MixedClassicNativeHistogramsWarning should have name of the form ErrFoo (revive)
        MixedClassicNativeHistogramsWarning = fmt.Errorf("%w: vector contains a mix of classic and native histograms for metric name", PromQLWarning)
        ^
util/annotations/annotations.go:110:55: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
        PossibleNonCounterInfo                  = fmt.Errorf("%w: metric might not be a counter, name does not end in _total/_sum/_count/_bucket:", PromQLInfo)
                                                             ^
util/annotations/annotations.go:111:2: error-naming: error var HistogramQuantileForcedMonotonicityInfo should have name of the form ErrFoo (revive)
        HistogramQuantileForcedMonotonicityInfo = fmt.Errorf("%w: input to histogram_quantile needed to be fixed for monotonicity (and may give inaccurate results) for metric name", PromQLInfo)
        ^
util/annotations/annotations.go:133:71: unexported-return: exported func NewInvalidQuantileWarning returns unexported type annotations.annoErr, which can be annoying to use (revive)
func NewInvalidQuantileWarning(q float64, pos posrange.PositionRange) annoErr {
                                                                      ^
util/annotations/annotations.go:142:85: unexported-return: exported func NewBadBucketLabelWarning returns unexported type annotations.annoErr, which can be annoying to use (revive)
func NewBadBucketLabelWarning(metricName, label string, pos posrange.PositionRange) annoErr {
                                                                                    ^
util/annotations/annotations.go:152:85: unexported-return: exported func NewMixedFloatsHistogramsWarning returns unexported type annotations.annoErr, which can be annoying to use (revive)
func NewMixedFloatsHistogramsWarning(metricName string, pos posrange.PositionRange) annoErr {
                                                                                    ^
util/annotations/annotations.go:161:92: unexported-return: exported func NewMixedClassicNativeHistogramsWarning returns unexported type annotations.annoErr, which can be annoying to use (revive)
func NewMixedClassicNativeHistogramsWarning(metricName string, pos posrange.PositionRange) annoErr {
                                                                                           ^
util/annotations/annotations.go:170:79: unexported-return: exported func NewPossibleNonCounterInfo returns unexported type annotations.annoErr, which can be annoying to use (revive)
func NewPossibleNonCounterInfo(metricName string, pos posrange.PositionRange) annoErr {
                                                                              ^
util/annotations/annotations.go:179:96: unexported-return: exported func NewHistogramQuantileForcedMonotonicityInfo returns unexported type annotations.annoErr, which can be annoying to use (revive)
func NewHistogramQuantileForcedMonotonicityInfo(metricName string, pos posrange.PositionRange) annoErr {
                                                                                               ^
util/treecache/treecache.go:119:3: empty-block: this block is empty, you can remove it (revive)
                for range tc.head.events {
                }

[bboreham]

Thanks for this.
Couple of suggestions, plus notes to help the next reviewer.

[bboreham]

Guessing this is exported for users outside of Prometheus?

[alexandear]

Yep.

[bboreham]

I would like to bump the priority of this, since revive would have warned me about a serious bug I added to Prometheus recently. [EDIT: actually a bug in revive means it wouldn't have 😢 see https://github.com/mgechev/revive/pull/943]

If you can rebase and/or address any of the review comments please do; in the meantime I'll start addressing a couple of them myself.

[alexandear]

@bboreham I fixed most of the comments. Please re-review.

[bboreham]

Thanks, I guess I can close my PRs now.