[linter-miner] feat(linters): add jsonmarshalignoredeerror linter to catch discarded json.Marshal/Unmarshal errors#35767
Conversation
…e bodies (#35752) * Initial plan * strengthen spdd daily planner output contract Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> * clarify spdd output contract requirements Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Catches json.Marshal and json.Unmarshal calls where the error return is explicitly discarded with _, silently producing nil bytes or leaving the target value in a partial state. Patterns flagged: val, _ := json.Marshal(x) // error dropped _ = json.Unmarshal(data, &v) // error dropped Evidence: 15+ call-sites found in pkg/workflow (copilot_logs.go, mcp_config_playwright_renderer.go, safe_outputs_config_generation.go, cache.go, args.go, and more). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
pelikhan
deleted the
linter-miner/json-marshal-ignored-error-457d00c274fb69a6
branch
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
There was a problem hiding this comment.
Pull request overview
Adds a new custom Go analyzer jsonmarshalignoredeerror that flags json.Marshal and json.Unmarshal calls whose error return is discarded via the blank identifier (val, _ := json.Marshal(x) or _ = json.Unmarshal(data, &v)). It follows the design of the existing strconvparseignorederror linter, registers the analyzer in the multichecker cmd/linters/main.go, and ships a small analysistest fixture.
Changes:
- New analyzer package with
inspect.Analyzer-based walker that resolves the receiver viapass.TypesInfo.Usesto confirmencoding/json. - Analysistest with a positive
Bad()and negativeGood()fixture covering basic Marshal/Unmarshal cases. - Registration of the new analyzer in
cmd/linters/main.go.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/jsonmarshalignoredeerror/jsonmarshalignoredeerror.go | New analyzer implementing the discard-detection logic for json.Marshal / json.Unmarshal. |
| pkg/linters/jsonmarshalignoredeerror/jsonmarshalignoredeerror_test.go | analysistest-based test entry point for the analyzer. |
| pkg/linters/jsonmarshalignoredeerror/testdata/src/jsonmarshalignoredeerror/jsonmarshalignoredeerror.go | Fixture with positive Marshal/Unmarshal discards and properly-handled negatives. |
| cmd/linters/main.go | Registers the new analyzer in the multichecker binary. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
| // Analyzer is the json-marshal-ignored-error analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "jsonmarshalignoredeerror", | ||
| Doc: "reports json.Marshal and json.Unmarshal calls where the error return is discarded with _", | ||
| URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/jsonmarshalignoredeerror", | ||
| Requires: []*analysis.Analyzer{inspect.Analyzer}, | ||
| Run: run, | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Non-blocking observations on the new linter — two issues worth a follow-up.
### Findings summary
1. Typo in package/analyzer name (medium) — jsonmarshalignoredeerror has a double-e; should be jsonmarshalignorederror. This propagates into the directory name, the package declaration, and Analyzer.Name. Suppression directives (//nolint:...) will silently do nothing if someone types the correctly-spelled name.
2. json.MarshalIndent not covered (medium) — same ([]byte, error) signature, same failure modes; the linter misses this common companion function.
Dropped findings from sub-agent:
- Nil-pointer on
pass.TypesInfo.Uses[ident]: the code uses the two-value type-assertion form, which safely returnsnil, falseon a nil interface — not a panic. - Unmarshal return-type guard:
isJSONFuncalready resolves to the exactencoding/json.Unmarshalsymbol via the type-checker; signature is fixed.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.4M
| // Analyzer is the json-marshal-ignored-error analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "jsonmarshalignoredeerror", | ||
| Doc: "reports json.Marshal and json.Unmarshal calls where the error return is discarded with _", |
There was a problem hiding this comment.
Typo in analyzer Name (and package/directory name): jsonmarshalignoredeerror contains a double-e — "ignoredeerror" should be "ignorederror".
💡 Details
The misspelling is baked into:
- The directory name
jsonmarshalignoredeerror/ - The
packagedeclaration Analyzer.Name(the string that appears in diagnostics and//nolint:jsonmarshalignoredeerrorsuppressions)
Users writing //nolint suppressions will hit a silent no-op if they spell the analyzer name correctly (jsonmarshalignorederror), because the registered name is the typo'd form. A rename now (before widespread suppression comments accumulate) is cheaper than a later migration.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
json.MarshalIndent is not covered: the linter silently ignores discarded errors from json.MarshalIndent, which has the same ([]byte, error) signature and the same failure modes.
💡 Suggested fix
Extend the Marshal branch to also check MarshalIndent:
if isJSONFunc(pass, call, "Marshal") || isJSONFunc(pass, call, "MarshalIndent") {
pass.ReportRangef(call, "error return from json.Marshal/MarshalIndent is discarded; marshal failures produce nil bytes silently")
}Or make isJSONFunc accept a variadic list of names:
func isJSONFunc(pass *analysis.Pass, call *ast.CallExpr, names ...string) bool {
...
for _, n := range names {
if sel.Sel.Name == n { return true }
}
return false
}json.MarshalIndent can fail for the same reasons (json.Marshal does (cyclic values, unsupported types) and is used widely in formatting and logging paths.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on three issues: a permanent typo in the analyzer name, a missing detection pattern, and a missing function variant.
📋 Key Themes & Highlights
Issues
- Typo in
Analyzer.Name(jsonmarshalignoredeerrorhas a doublee—ignored+error=ignorederror, but the name hasignoredeerror). This is a permanent lint diagnostic identifier. - Bare
json.Unmarshal(data, &v)not caught — calling it as a statement expression (no LHS at all) silently discards the error but is not anAssignStmt, so the linter skips it. json.MarshalIndentnot covered — same return signature asMarshal, also commonly used.
Positive Highlights
- ✅ Correct package-path resolution via
pass.TypesInfo.Usesprevents false positives from local variables namedjson - ✅ Follows the established
strconvparseignorederrorpattern exactly — easy to review and maintain - ✅ Build tag present in test file;
analysistestharness used correctly - ✅ Clear PR description with concrete evidence of 15+ real call-sites
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.4M
| // Analyzer is the json-marshal-ignored-error analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "jsonmarshalignoredeerror", | ||
| Doc: "reports json.Marshal and json.Unmarshal calls where the error return is discarded with _", |
There was a problem hiding this comment.
[/grill-with-docs] Typo in the public analyzer Name: the package is spelled jsonmarshalignoredeerror (double e: ignored → d + error → e = de but the package has dee). This string appears in lint output and is a permanent API identifier once merged.
💡 Suggested fix
Rename throughout (directory, package declaration, Analyzer.Name, Analyzer.Doc, and URL):
- ❌
jsonmarshalignoredeerror(current — doublee) - ✅
jsonmarshalignoredererroror more readablyjsonmarshalignoredeerror
Precisely: json + marshal + ignored + error → jsonmarshalignoredererror. Check: ignored = i-g-n-o-r-e-d, error = e-r-r-o-r, so joined = ignorederror. Current name ignoredeerror inserts an extra e.
|
|
||
| func run(pass *analysis.Pass) (any, error) { | ||
| insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) | ||
| nodeFilter := []ast.Node{(*ast.AssignStmt)(nil)} |
There was a problem hiding this comment.
[/tdd] The linter only inspects *ast.AssignStmt nodes, so it misses json.Unmarshal(data, &v) called as a bare expression statement — which is also a discarded error and arguably more common.
💡 Suggested fix
Add (*ast.ExprStmt)(nil) to the nodeFilter and handle it:
nodeFilter := []ast.Node{(*ast.AssignStmt)(nil), (*ast.ExprStmt)(nil)}
// in the callback:
exprStmt, ok := n.(*ast.ExprStmt)
if ok {
call, ok := exprStmt.X.(*ast.CallExpr)
if ok {
if isJSONFunc(pass, call, "Unmarshal") {
pass.ReportRangef(call, "error return from json.Unmarshal is discarded; unmarshal failures leave the target value in a partial state")
}
}
}Add a fixture in the test data:
func BadBare() {
var f Foo
json.Unmarshal([]byte(`{}`), &f) // want `error return from json\.Unmarshal is discarded`
}| blank, ok := assign.Lhs[1].(*ast.Ident) | ||
| if ok && blank.Name == "_" { | ||
| call, ok := assign.Rhs[0].(*ast.CallExpr) | ||
| if ok { |
There was a problem hiding this comment.
[/tdd] json.MarshalIndent also returns ([]byte, error) and is used throughout the codebase, but the linter only checks for "Marshal" by exact name — MarshalIndent calls with a discarded error will be silently skipped.
💡 Suggested fix
Extend isJSONFunc to accept a list of names, or add a second check:
// Check for both Marshal and MarshalIndent
for _, fn := range []string{"Marshal", "MarshalIndent"} {
if isJSONFunc(pass, call, fn) {
pass.ReportRangef(call, "error return from json.%s is discarded; marshal failures produce nil bytes silently", fn)
}
}Add a fixture:
val2, _ := json.MarshalIndent(f, "", " ") // want `error return from json\.MarshalIndent is discarded`
_ = val2
🏗️ Design Decision Gate — ADR RequiredThis PR adds >100 new lines in business-logic directories (124 additions in
📄 Draft ADR — copy to
|
Summary
Adds a new
jsonmarshalignoredeerrorstatic analysis linter that detectsjson.Marshalandjson.Unmarshalcalls where the error return value is explicitly discarded with_. The linter is registered with the linter binary and ships with full test coverage and testdata fixtures.Changes
New:
jsonmarshalignoredeerroranalyzer (pkg/linters/jsonmarshalignoredeerror/)jsonmarshalignoredeerror.go— Corego/analysispass. Inspects call expressions forjson.Marshal/json.Unmarshaland reports a diagnostic whenever the error return is assigned to_.jsonmarshalignoredeerror_test.go— Wires the analyzer throughanalysistestto validate diagnostics against the testdata fixture.testdata/src/jsonmarshalignoredeerror/jsonmarshalignoredeerror.go— Fixture file containingBadfunctions (expected diagnostic hits) andGoodfunctions (clean patterns) that serve as ground truth for the analyzer tests.Modified:
cmd/linters/main.gojsonmarshalignoredeerroranalyzer with the linter binary so it participates in all linter runs.Motivation
Silently discarding errors from
json.Marshal/json.Unmarshal(val, _ := json.Marshal(...)or_ = json.Unmarshal(...)) is a common source of subtle bugs — marshalling failures go undetected and callers operate on zero-value or stale data. This linter makes that class of mistake a compile-time-equivalent error in CI.Testing
golang.org/x/tools/go/analysis/analysistestwith explicit// wantannotations in the testdata fixture.Risk
_-discarded errors