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 +```