From 2bec10fc5421dbcc085dcdae3f831622a47daefe Mon Sep 17 00:00:00 2001 From: John Kerl Date: Fri, 3 Jul 2026 10:31:08 -0400 Subject: [PATCH] Fix govet lint findings; fix masked unset-on-array error path (#2129) * plans/lintfixes.md * plans/lintfixes.md * Fix remaining govet lint findings - Rename MarshalJSON -> FormatAsJSON on Mlrval and Mlrmap (govet stdmethods): the methods shadowed json.Marshaler with an incompatible signature. - Remove unreachable return after exhaustive if-else in pkg/mlrval/mlrval_collections.go (govet unreachable). - Update plans/lintfixes.md with current status: 84 findings remain (50 errcheck, 34 staticcheck). Part of #2109. Co-Authored-By: Claude Fable 5 * Return nil on successful single-index array unset removeIndexedOnArray removed the element on the in-bounds path but then fell through to return an "array index out of bounds for unset" error, so the success path never returned nil. Callers currently ignore the error, which masked this; return nil on success so that upcoming errcheck fixes can propagate the error meaningfully. This matches removeIndexedOnMap, which returns nil on success. Add unit tests for RemoveIndexed on arrays. Part of #2109. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- pkg/bifs/collections.go | 4 +- pkg/mlrval/mlrmap_json.go | 4 +- pkg/mlrval/mlrmap_print.go | 2 +- pkg/mlrval/mlrval_collections.go | 3 +- pkg/mlrval/mlrval_collections_test.go | 44 +++++++ pkg/mlrval/mlrval_json.go | 4 +- pkg/mlrval/mlrval_output.go | 4 +- pkg/output/record_writer_json_jsonl.go | 8 +- plans/lintfixes.md | 166 +++++++++++++++++++++++++ 9 files changed, 224 insertions(+), 15 deletions(-) create mode 100644 pkg/mlrval/mlrval_collections_test.go create mode 100644 plans/lintfixes.md diff --git a/pkg/bifs/collections.go b/pkg/bifs/collections.go index 9a6a9bbf0..8982ee2ab 100644 --- a/pkg/bifs/collections.go +++ b/pkg/bifs/collections.go @@ -788,7 +788,7 @@ func BIF_json_parse(input1 *mlrval.Mlrval) *mlrval.Mlrval { } func BIF_json_stringify_unary(input1 *mlrval.Mlrval) *mlrval.Mlrval { - outputBytes, err := input1.MarshalJSON(mlrval.JSON_SINGLE_LINE, false) + outputBytes, err := input1.FormatAsJSON(mlrval.JSON_SINGLE_LINE, false) if err != nil { return mlrval.FromError(err) } @@ -805,7 +805,7 @@ func BIF_json_stringify_binary(input1, input2 *mlrval.Mlrval) *mlrval.Mlrval { jsonFormatting = mlrval.JSON_MULTILINE } - outputBytes, err := input1.MarshalJSON(jsonFormatting, false) + outputBytes, err := input1.FormatAsJSON(jsonFormatting, false) if err != nil { return mlrval.FromError(err) } diff --git a/pkg/mlrval/mlrmap_json.go b/pkg/mlrval/mlrmap_json.go index 5d96a22e4..421759cea 100644 --- a/pkg/mlrval/mlrmap_json.go +++ b/pkg/mlrval/mlrmap_json.go @@ -9,7 +9,7 @@ import ( "github.com/johnkerl/miller/v6/pkg/lib" ) -func (mlrmap *Mlrmap) MarshalJSON( +func (mlrmap *Mlrmap) FormatAsJSON( jsonFormatting TJSONFormatting, outputIsStdout bool, ) (string, error) { @@ -147,7 +147,7 @@ func (mlrmap *Mlrmap) marshalJSONAuxSingleLine( func (entry *MlrmapEntry) JSONStringifyInPlace( jsonFormatting TJSONFormatting, ) { - outputBytes, err := entry.Value.MarshalJSON(jsonFormatting, false) + outputBytes, err := entry.Value.FormatAsJSON(jsonFormatting, false) if err != nil { entry.Value = FromError(err) } else { diff --git a/pkg/mlrval/mlrmap_print.go b/pkg/mlrval/mlrmap_print.go index e98fd8eb5..2d043f6b5 100644 --- a/pkg/mlrval/mlrmap_print.go +++ b/pkg/mlrval/mlrmap_print.go @@ -29,7 +29,7 @@ func (mlrmap *Mlrmap) ToNIDXString() string { // Must have non-pointer receiver in order to implement the fmt.Stringer // interface to make mlrmap printable via fmt.Println et al. func (mlrmap Mlrmap) String() string { - bytes, err := mlrmap.MarshalJSON(JSON_MULTILINE, false) + bytes, err := mlrmap.FormatAsJSON(JSON_MULTILINE, false) if err != nil { return "Mlrmap: could not not marshal self to JSON" } diff --git a/pkg/mlrval/mlrval_collections.go b/pkg/mlrval/mlrval_collections.go index 9aeb7bb7e..9a37c2b36 100644 --- a/pkg/mlrval/mlrval_collections.go +++ b/pkg/mlrval/mlrval_collections.go @@ -459,6 +459,7 @@ func removeIndexedOnArray( leftSlice := (*baseArray)[0:zindex] rightSlice := (*baseArray)[zindex+1 : len((*baseArray))] *baseArray = append(leftSlice, rightSlice...) + return nil } else if mindex.intf.(int64) == 0 { return errors.New("zero indices are not supported. Indices are 1-up") } @@ -474,8 +475,6 @@ func removeIndexedOnArray( // TODO: improve wording return errors.New("array index out of bounds for unset") } - - return nil } // Nominally for TopKeeper diff --git a/pkg/mlrval/mlrval_collections_test.go b/pkg/mlrval/mlrval_collections_test.go new file mode 100644 index 000000000..09c2310ca --- /dev/null +++ b/pkg/mlrval/mlrval_collections_test.go @@ -0,0 +1,44 @@ +package mlrval + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRemoveIndexedOnArrayInBounds(t *testing.T) { + array := FromArray([]*Mlrval{FromInt(10), FromInt(20), FromInt(30)}) + + err := array.RemoveIndexed([]*Mlrval{FromInt(2)}) + assert.Nil(t, err) + + elements := array.GetArray() + assert.Equal(t, 2, len(elements)) + assert.Equal(t, int64(10), elements[0].intf.(int64)) + assert.Equal(t, int64(30), elements[1].intf.(int64)) +} + +func TestRemoveIndexedOnArrayNegativeIndex(t *testing.T) { + array := FromArray([]*Mlrval{FromInt(10), FromInt(20), FromInt(30)}) + + // Negative indices are aliased from the end, so -1 removes the last element. + err := array.RemoveIndexed([]*Mlrval{FromInt(-1)}) + assert.Nil(t, err) + + elements := array.GetArray() + assert.Equal(t, 2, len(elements)) + assert.Equal(t, int64(10), elements[0].intf.(int64)) + assert.Equal(t, int64(20), elements[1].intf.(int64)) +} + +func TestRemoveIndexedOnArrayOutOfBounds(t *testing.T) { + array := FromArray([]*Mlrval{FromInt(10), FromInt(20), FromInt(30)}) + + err := array.RemoveIndexed([]*Mlrval{FromInt(4)}) + assert.NotNil(t, err) + assert.Equal(t, 3, len(array.GetArray())) + + err = array.RemoveIndexed([]*Mlrval{FromInt(0)}) + assert.NotNil(t, err) + assert.Equal(t, 3, len(array.GetArray())) +} diff --git a/pkg/mlrval/mlrval_json.go b/pkg/mlrval/mlrval_json.go index 9c9cda7fa..8c3a6e12b 100644 --- a/pkg/mlrval/mlrval_json.go +++ b/pkg/mlrval/mlrval_json.go @@ -1,5 +1,5 @@ // Mlrval implements the Unmarshaler and Marshaler interfaces needed for -// marshaling/unmarshaling to/from JSON, via the UnmarshalJSON and MarshalJSON +// marshaling/unmarshaling to/from JSON, via the UnmarshalJSON and FormatAsJSON // methods. // // Please see also https://golang.org/pkg/encoding/json/ @@ -259,7 +259,7 @@ func MlrvalDecodeFromJSON(decoder *json.Decoder) ( } } -func (mv *Mlrval) MarshalJSON( +func (mv *Mlrval) FormatAsJSON( jsonFormatting TJSONFormatting, outputIsStdout bool, ) (string, error) { diff --git a/pkg/mlrval/mlrval_output.go b/pkg/mlrval/mlrval_output.go index d5b3cdf1b..33e96b16a 100644 --- a/pkg/mlrval/mlrval_output.go +++ b/pkg/mlrval/mlrval_output.go @@ -92,7 +92,7 @@ func (mv *Mlrval) setPrintRep() { } case MT_ARRAY: - bytes, err := mv.MarshalJSON(JSON_MULTILINE, false) + bytes, err := mv.FormatAsJSON(JSON_MULTILINE, false) // maybe just InternalCodingErrorIf(err != nil) if err != nil { fmt.Fprintf(os.Stderr, "mlr: %v\n", err) @@ -101,7 +101,7 @@ func (mv *Mlrval) setPrintRep() { mv.printrep = string(bytes) case MT_MAP: - bytes, err := mv.MarshalJSON(JSON_MULTILINE, false) + bytes, err := mv.FormatAsJSON(JSON_MULTILINE, false) // maybe just InternalCodingErrorIf(err != nil) if err != nil { fmt.Fprintf(os.Stderr, "mlr: %v\n", err) diff --git a/pkg/output/record_writer_json_jsonl.go b/pkg/output/record_writer_json_jsonl.go index 0739a9067..eb40c5382 100644 --- a/pkg/output/record_writer_json_jsonl.go +++ b/pkg/output/record_writer_json_jsonl.go @@ -74,9 +74,9 @@ func (writer *RecordWriterJSON) writeWithListWrap( bufferedOutputStream.WriteString("[\n") } - // The Mlrmap MarshalJSON doesn't include the final newline, so that we + // The Mlrmap FormatAsJSON doesn't include the final newline, so that we // can place it neatly with commas here (if the user requested them). - s, err := outrec.MarshalJSON(writer.jsonFormatting, outputIsStdout) + s, err := outrec.FormatAsJSON(writer.jsonFormatting, outputIsStdout) if err != nil { fmt.Fprintf(os.Stderr, "mlr: %v\n", err) os.Exit(1) @@ -115,9 +115,9 @@ func (writer *RecordWriterJSON) writeWithoutListWrap( return } - // The Mlrmap MarshalJSON doesn't include the final newline, so that we + // The Mlrmap FormatAsJSON doesn't include the final newline, so that we // can place it neatly with commas here (if the user requested them). - s, err := outrec.MarshalJSON(writer.jsonFormatting, outputIsStdout) + s, err := outrec.FormatAsJSON(writer.jsonFormatting, outputIsStdout) if err != nil { fmt.Fprintf(os.Stderr, "mlr: %v\n", err) os.Exit(1) diff --git a/plans/lintfixes.md b/plans/lintfixes.md new file mode 100644 index 000000000..5f51c2405 --- /dev/null +++ b/plans/lintfixes.md @@ -0,0 +1,166 @@ +# golangci-lint fix tracking + +Tracking issue: https://github.com/johnkerl/miller/issues/2109 + +golangci-lint v2.12.2, run against `./cmd/mlr ./pkg/...` (same as CI). + +## Baseline (first run, branch `johnkerl/lintfixes` before any fixes) + +- **134 total issues** +- **64 unique source files** + +### By linter + +| Count | Linter | +|------:|--------| +| 50 | errcheck | +| 50 | staticcheck | +| 30 | ineffassign | +| 3 | govet | +| 1 | unused | + +### By semantic type + +| Count | Category | +|------:|---------| +| 30 | `ineffassign`: variable assigned but never used after | +| 29 | `staticcheck QF1003`: use typed switch instead of if-else chain | +| 9 | `errcheck`: ignore return from `fmt.Fprintf/Fprint/Fprintln` | +| 8 | `errcheck`: ignore return from map `RemoveIndexed`/`PutIndexed` | +| 8 | `staticcheck ST1023/QF1011`: omit explicit type (inferred from RHS) | +| 6 | `errcheck`: ignore return from `Write()`/`WriteString()` | +| 6 | `errcheck`: ignore return from DSL emit/execute/redirect funcs | +| 5 | `errcheck`: ignore return from `cli.Finalize{Reader,Writer}Options` | +| 4 | `errcheck`: ignore return from `Close()` | +| 4 | `errcheck`: ignore return from `Flush()` | +| 3 | `errcheck`: generic unchecked return (anonymous funcs) | +| 3 | `errcheck`: ignore return from other stream/process ops | +| 3 | `staticcheck S1009`: omit nil check before `len()` (always safe for nil slices) | +| 3 | `staticcheck SA9003`: empty `else if` branch | +| 3 | `staticcheck QF1007`: merge conditional assignment into declaration | +| 2 | `errcheck`: ignore return from `os.Setenv`/`os.Unsetenv` | +| 2 | `govet stdmethods`: `MarshalJSON` has wrong signature | +| 2 | `staticcheck QF1001`: apply De Morgan's law to simplify negation | +| 1 | `govet`: unreachable code | +| 1 | `staticcheck QF1006`: lift check into loop condition | +| 1 | `staticcheck S1031`: unnecessary nil check before `range` | +| 1 | `unused`: unused struct field | +| **134** | **TOTAL** | + +## PRs merged against issue #2109 + +| PR | Title | Merged | +|----|-------|--------| +| [#2076](https://github.com/johnkerl/miller/pull/2076) | ci: add golangci-lint workflow | 2026-06-25 | +| [#2108](https://github.com/johnkerl/miller/pull/2108) | Next batch of lint fixes | 2026-06-28 | +| [#2110](https://github.com/johnkerl/miller/pull/2110) | Next batch of lint fixes | 2026-06-28 | +| [#2112](https://github.com/johnkerl/miller/pull/2112) | Convert if/else-if chains to typed switch (staticcheck QF1003) | 2026-06-28 | + +PR #2112 alone fixed ~29 QF1003 findings across 72 files. PRs #2108 and #2110 addressed other batches. + +**Note:** The `johnkerl/lintfixes` branch listed commits `0f8b931bc` (MarshalJSON rename) and +`f57edc037` (unreachable code) in an earlier version of this file, but those were never pushed — +`origin/johnkerl/lintfixes` ends at the plan-file commit. Those fixes were redone on `johnkerl/lint4`. + +## Work done on branch `johnkerl/lint4` (2026-07-03) + +- Rename `MarshalJSON` → `FormatAsJSON` on `Mlrval` and `Mlrmap` (fixes `govet stdmethods`, 2 findings; + 13 references across 6 files). The old signature shadowed `json.Marshaler` with an incompatible one. +- Remove unreachable `return nil` after exhaustive if-else in `pkg/mlrval/mlrval_collections.go` + (fixes `govet unreachable`, 1 finding). +- The `unused` finding (`exeName` field in `pkg/terminals/script/types.go`) was already resolved on + `main` by an unrelated change (field renamed to `name` and now referenced). + +## Remaining after `johnkerl/lint4` govet batch (2026-07-03) + +**84 issues: 50 errcheck, 34 staticcheck.** All `govet`, `unused`, and `ineffassign` findings are resolved. + +Staticcheck breakdown: 16 ST1023 (omit explicit type), 3 SA9003 (empty branch), 3 S1009 (redundant +nil check before len), 3 QF1007 (merge conditional assignment), 3 QF1006 (lift into loop condition), +3 QF1001 (De Morgan), 2 QF1011 (omit explicit type), 1 S1031 (nil check before range). + +## Priority order for remaining fixes + +1. `errcheck` — 50 findings. Varies in seriousness: + - DSL emit/execute return values, `RemoveIndexed`/`PutIndexed`, `Flush`/`Close` — worth checking individually + - `fmt.Fprintf`/`fmt.Fprint` to stderr — genuinely safe to ignore; consider `//nolint:errcheck` + - `os.Setenv`/`os.Unsetenv` — low-risk but cheap to fix +2. `staticcheck` style — 34 findings, mostly mechanical (type inference, nil-check cleanup, etc.) + +## Bug noticed in passing (fixed on `johnkerl/lint4`) + +In `pkg/mlrval/mlrval_collections.go`, `removeIndexedOnArray` with a single in-bounds index removed +the element but then fell through to `return errors.New("array index out of bounds for unset")` — +the success path never returned nil. Callers ignored the error (several of the errcheck findings), +which masked it. Fixed by returning nil on the success path, matching `removeIndexedOnMap`; +unit tests added in `pkg/mlrval/mlrval_collections_test.go`. No observable behavior change today, +but errcheck fixes for `RemoveIndexed` call sites can now propagate the error meaningfully. + +## Unique source files with issues (baseline, 64 files) + +``` +pkg/auxents/hex.go +pkg/auxents/lecat.go +pkg/auxents/termcvt.go +pkg/auxents/unhex.go +pkg/bifs/random.go +pkg/cli/option_parse.go +pkg/climain/mlrcli_mlrrc.go +pkg/climain/mlrcli_shebang.go +pkg/dkvpx/dkvpx_reader.go +pkg/dsl/cst/blocks.go +pkg/dsl/cst/builtin_functions.go +pkg/dsl/cst/dump.go +pkg/dsl/cst/emit_emitp.go +pkg/dsl/cst/evaluable.go +pkg/dsl/cst/for.go +pkg/dsl/cst/if.go +pkg/dsl/cst/lvalues.go +pkg/dsl/cst/print.go +pkg/dsl/cst/statements.go +pkg/dsl/cst/udf.go +pkg/dsl/cst/uds.go +pkg/dsl/cst/validate.go +pkg/input/line_reader.go +pkg/input/record_reader_xtab.go +pkg/mlrval/mlrmap_accessors.go +pkg/mlrval/mlrmap_flatten_unflatten.go +pkg/mlrval/mlrmap_json.go +pkg/mlrval/mlrval_collections.go +pkg/mlrval/mlrval_copy.go +pkg/mlrval/mlrval_infer.go +pkg/mlrval/mlrval_json.go +pkg/mlrval/mlrval_new.go +pkg/mlrval/mlrval_yaml.go +pkg/stream/stream.go +pkg/terminals/help/entry.go +pkg/terminals/repl/dsl.go +pkg/terminals/repl/entry.go +pkg/terminals/repl/verbs.go +pkg/terminals/script/types.go +pkg/terminals/terminals.go +pkg/transformers/altkv.go +pkg/transformers/bar.go +pkg/transformers/bootstrap.go +pkg/transformers/case.go +pkg/transformers/cat.go +pkg/transformers/check.go +pkg/transformers/count.go +pkg/transformers/fill_down.go +pkg/transformers/flatten.go +pkg/transformers/fraction.go +pkg/transformers/group_by.go +pkg/transformers/having_fields.go +pkg/transformers/join.go +pkg/transformers/json_stringify.go +pkg/transformers/merge_fields.go +pkg/transformers/nest.go +pkg/transformers/put_or_filter.go +pkg/transformers/reshape.go +pkg/transformers/seqgen.go +pkg/transformers/sort.go +pkg/transformers/split.go +pkg/transformers/summary.go +pkg/transformers/tee.go +pkg/transformers/unspace.go +```