[linter-miner] feat(linters): add jsonmarshalignoredeerror linter to catch discarded json.Marshal/Unmarshal errors #35767
+126
−0
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| // Package jsonmarshalignoredeerror implements a Go analysis linter that flags | ||
| // json.Marshal and json.Unmarshal calls where the error return is discarded with _. | ||
| package jsonmarshalignoredeerror | ||
|
|
||
| import ( | ||
| "go/ast" | ||
| "go/types" | ||
|
|
||
| "golang.org/x/tools/go/analysis" | ||
| "golang.org/x/tools/go/analysis/passes/inspect" | ||
| "golang.org/x/tools/go/ast/inspector" | ||
| ) | ||
|
|
||
| // 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 _", | ||
|
Contributor
Author
There was a problem hiding this comment. [/grill-with-docs] Typo in the public analyzer 💡 Suggested fixRename throughout (directory, package declaration,
Precisely: |
||
| URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/jsonmarshalignoredeerror", | ||
| Requires: []*analysis.Analyzer{inspect.Analyzer}, | ||
| Run: run, | ||
| } | ||
|
Comment on lines
+14
to
+21
|
||
|
|
||
| func run(pass *analysis.Pass) (any, error) { | ||
| insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) | ||
| nodeFilter := []ast.Node{(*ast.AssignStmt)(nil)} | ||
|
Contributor
Author
There was a problem hiding this comment. [/tdd] The linter only inspects 💡 Suggested fixAdd 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`
} |
||
| insp.Preorder(nodeFilter, func(n ast.Node) { | ||
| assign, ok := n.(*ast.AssignStmt) | ||
| if !ok { | ||
| return | ||
| } | ||
|
|
||
| // Pattern: val, _ := json.Marshal(x) — 2 lhs, 1 rhs, Lhs[1] is blank | ||
| if len(assign.Lhs) == 2 && len(assign.Rhs) == 1 { | ||
| blank, ok := assign.Lhs[1].(*ast.Ident) | ||
| if ok && blank.Name == "_" { | ||
| call, ok := assign.Rhs[0].(*ast.CallExpr) | ||
| if ok { | ||
|
Contributor
Author
There was a problem hiding this comment. [/tdd] 💡 Suggested fixExtend // 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 |
||
| if isJSONFunc(pass, call, "Marshal") { | ||
| pass.ReportRangef(call, "error return from json.Marshal is discarded; marshal failures produce nil bytes silently") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment.
💡 Suggested fixExtend the Marshal branch to also check 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 func isJSONFunc(pass *analysis.Pass, call *ast.CallExpr, names ...string) bool {
...
for _, n := range names {
if sel.Sel.Name == n { return true }
}
return false
}
|
||
| // Pattern: _ = json.Unmarshal(data, &v) — 1 lhs, 1 rhs, Lhs[0] is blank | ||
| if len(assign.Lhs) == 1 && len(assign.Rhs) == 1 { | ||
| blank, ok := assign.Lhs[0].(*ast.Ident) | ||
| if ok && blank.Name == "_" { | ||
| call, ok := assign.Rhs[0].(*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") | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| return nil, nil | ||
| } | ||
|
|
||
| func isJSONFunc(pass *analysis.Pass, call *ast.CallExpr, name string) bool { | ||
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| if !ok { | ||
| return false | ||
| } | ||
| if sel.Sel.Name != name { | ||
| return false | ||
| } | ||
| ident, ok := sel.X.(*ast.Ident) | ||
| if !ok { | ||
| return false | ||
| } | ||
| obj := pass.TypesInfo.Uses[ident] | ||
| pkgName, ok := obj.(*types.PkgName) | ||
| if !ok { | ||
| return false | ||
| } | ||
| return pkgName.Imported().Path() == "encoding/json" | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| //go:build !integration | ||
|
|
||
| package jsonmarshalignoredeerror_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "golang.org/x/tools/go/analysis/analysistest" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror" | ||
| ) | ||
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, jsonmarshalignoredeerror.Analyzer, "jsonmarshalignoredeerror") | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| package jsonmarshalignoredeerror | ||
|
|
||
| import "encoding/json" | ||
|
|
||
| type Foo struct{ X int } | ||
|
|
||
| func Bad() { | ||
| f := Foo{X: 1} | ||
| val, _ := json.Marshal(f) // want `error return from json\.Marshal is discarded` | ||
| _ = val | ||
|
|
||
| var f2 Foo | ||
| _ = json.Unmarshal([]byte(`{}`), &f2) // want `error return from json\.Unmarshal is discarded` | ||
| } | ||
|
|
||
| func Good() error { | ||
| f := Foo{X: 1} | ||
| val, err := json.Marshal(f) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| _ = val | ||
|
|
||
| var f2 Foo | ||
| if err := json.Unmarshal([]byte(`{}`), &f2); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in analyzer
Name(and package/directory name):jsonmarshalignoredeerrorcontains a double-e—"ignoredeerror"should be"ignorederror".💡 Details
The misspelling is baked into:
jsonmarshalignoredeerror/packagedeclarationAnalyzer.Name(the string that appears in diagnostics and//nolint:jsonmarshalignoredeerrorsuppressions)Users writing
//nolintsuppressions 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.