From de3b67ba273564fd16a94c76b6ba1b3d06960ce7 Mon Sep 17 00:00:00 2001 From: Dirk Faust Date: Wed, 6 Sep 2023 09:38:09 +0200 Subject: [PATCH 1/3] Add unchecked-type-assertion --- README.md | 1 + RULES_DESCRIPTIONS.md | 23 ++- config/config.go | 1 + go.sum | 2 - rule/unchecked-type-assertion.go | 160 ++++++++++++++++++ test/unchecked_type_assertion_test.go | 20 +++ ...unchecked-type-assertion-accept-ignored.go | 12 ++ testdata/unchecked-type-assertion.go | 58 +++++++ 8 files changed, 274 insertions(+), 3 deletions(-) create mode 100644 rule/unchecked-type-assertion.go create mode 100644 test/unchecked_type_assertion_test.go create mode 100644 testdata/unchecked-type-assertion-accept-ignored.go create mode 100644 testdata/unchecked-type-assertion.go diff --git a/README.md b/README.md index daadbc168..a4fb4911a 100644 --- a/README.md +++ b/README.md @@ -464,6 +464,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`context-keys-type`](./RULES_DESCRIPTIONS.md#context-key-types) | n/a | Disallows the usage of basic types in `context.WithValue`. | yes | yes | | [`time-equal`](./RULES_DESCRIPTIONS.md#time-equal) | n/a | Suggests to use `time.Time.Equal` instead of `==` and `!=` for equality check time. | no | yes | | [`time-naming`](./RULES_DESCRIPTIONS.md#time-naming) | n/a | Conventions around the naming of time variables. | yes | yes | +| [`unchecked-type-assertions`](./RULES_DESCRIPTIONS.md#unchecked-type-assertions) | n/a | Disallows type assertions without checking the result. | no | yes | | [`var-declaration`](./RULES_DESCRIPTIONS.md#var-declaration) | n/a | Reduces redundancies around variable declaration. | yes | yes | | [`unexported-return`](./RULES_DESCRIPTIONS.md#unexported-return) | n/a | Warns when a public return is from unexported type. | yes | yes | | [`errorf`](./RULES_DESCRIPTIONS.md#errorf) | n/a | Should replace `errors.New(fmt.Sprintf())` with `fmt.Errorf()` | yes | yes | diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 4561e200e..a6bee3c9b 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -62,6 +62,7 @@ List of all available rules. - [string-format](#string-format) - [superfluous-else](#superfluous-else) - [time-equal](#time-equal) + - [unchecked-type-assertion](#unchecked-type-assertion) - [time-naming](#time-naming) - [var-naming](#var-naming) - [var-declaration](#var-declaration) @@ -170,8 +171,8 @@ Example: [rule.cognitive-complexity] arguments =[7] ``` - ## comment-spacings + _Description_: Spots comments of the form: ```go //This is a malformed comment: no space between // and the start of the sentence @@ -683,6 +684,26 @@ _Description_: Using unit-specific suffix like "Secs", "Mins", ... when naming v _Configuration_: N/A +## unchecked-type-assertion + +_Description_: This rule checks whether a type assertion result is checked (the `ok` value), preventing unexpected `panic`s. + +_Configuration_: list of key-value-pair-map (`[]map[string]any`). + +- `acceptIgnoredAssertionResult` : (bool) default `false`, set it to `true` to accept ignored type assertion results like this: + +```go +foo, _ := bar(.*Baz). +// ^ +``` + +Example: + +```yaml +[rule.unchecked-type-assertion] +arguments = [{acceptIgnoredAssertionResult=true}] +``` + ## var-naming _Description_: This rule warns when [initialism](https://github.com/golang/go/wiki/CodeReviewComments#initialisms), [variable](https://github.com/golang/go/wiki/CodeReviewComments#variable-names) or [package](https://github.com/golang/go/wiki/CodeReviewComments#package-names) naming conventions are not followed. diff --git a/config/config.go b/config/config.go index 438545942..de429136c 100644 --- a/config/config.go +++ b/config/config.go @@ -80,6 +80,7 @@ var allRules = append([]lint.Rule{ &rule.FunctionLength{}, &rule.NestedStructs{}, &rule.UselessBreak{}, + &rule.UncheckedTypeAssertionRule{}, &rule.TimeEqualRule{}, &rule.BannedCharsRule{}, &rule.OptimizeOperandsOrderRule{}, diff --git a/go.sum b/go.sum index 6eb658163..79b970c63 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,5 @@ github.com/BurntSushi/toml v1.3.2 h1:o7IhLm0Msx3BaB+n3Ag7L8EVlByGnpq14C4YWiu/gL8= github.com/BurntSushi/toml v1.3.2/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= -github.com/chavacava/garif v0.0.0-20230608123814-4bd63c2919ab h1:5JxePczlyGAtj6R1MUEFZ/UFud6FfsOejq7xLC2ZIb0= -github.com/chavacava/garif v0.0.0-20230608123814-4bd63c2919ab/go.mod h1:XMyYCkEL58DF0oyW4qDjjnPWONs2HBqYKI+UIPD+Gww= github.com/chavacava/garif v0.1.0 h1:2JHa3hbYf5D9dsgseMKAmc/MZ109otzgNFk5s87H9Pc= github.com/chavacava/garif v0.1.0/go.mod h1:XMyYCkEL58DF0oyW4qDjjnPWONs2HBqYKI+UIPD+Gww= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/rule/unchecked-type-assertion.go b/rule/unchecked-type-assertion.go new file mode 100644 index 000000000..f5801083b --- /dev/null +++ b/rule/unchecked-type-assertion.go @@ -0,0 +1,160 @@ +package rule + +import ( + "fmt" + "go/ast" + "sync" + + "github.com/mgechev/revive/lint" +) + +const ( + ruleUTAMessagePanic = "type assertion will panic if not matched" + ruleUTAMessageIgnored = "type assertion result ignored" +) + +// UncheckedTypeAssertionRule lints missing or ignored `ok`-value in danymic type casts. +type UncheckedTypeAssertionRule struct { + sync.Mutex + acceptIgnoredAssertionResult bool +} + +func (u *UncheckedTypeAssertionRule) configure(arguments lint.Arguments) { + u.Lock() + defer u.Unlock() + + if len(arguments) == 0 { + return + } + + args, ok := arguments[0].(map[string]any) + if !ok { + panic("Unable to get arguments. Expected object of key-value-pairs.") + } + + for k, v := range args { + switch k { + case "acceptIgnoredAssertionResult": + u.acceptIgnoredAssertionResult, ok = v.(bool) + if !ok { + panic(fmt.Sprintf("Unable to parse argument '%s'. Expected boolean.", k)) + } + default: + panic(fmt.Sprintf("Unknown argument: %s", k)) + } + } +} + +// Apply applies the rule to given file. +func (u *UncheckedTypeAssertionRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure { + u.configure(args) + + var failures []lint.Failure + + walker := &lintUnchekedTypeAssertion{ + pkg: file.Pkg, + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + acceptIgnoredTypeAssertionResult: u.acceptIgnoredAssertionResult, + } + + file.Pkg.TypeCheck() + ast.Walk(walker, file.AST) + + return failures +} + +// Name returns the rule name. +func (*UncheckedTypeAssertionRule) Name() string { + return "unchecked-type-assertion" +} + +type lintUnchekedTypeAssertion struct { + pkg *lint.Package + onFailure func(lint.Failure) + acceptIgnoredTypeAssertionResult bool +} + +func isIgnored(e ast.Expr) bool { + ident, ok := e.(*ast.Ident) + if !ok { + return false + } + + return ident.Name == "_" +} + +func isTypeSwitch(e *ast.TypeAssertExpr) bool { + return e.Type == nil +} + +func (w *lintUnchekedTypeAssertion) requireNoTypeAssert(expr ast.Expr) { + e, ok := expr.(*ast.TypeAssertExpr) + if ok && !isTypeSwitch(e) { + w.addFailure(e, ruleUTAMessagePanic) + } +} + +func (w *lintUnchekedTypeAssertion) handleSwitch(n *ast.SwitchStmt) { + w.requireNoTypeAssert(n.Tag) +} + +func (w *lintUnchekedTypeAssertion) handleAssignment(n *ast.AssignStmt) { + if len(n.Rhs) == 0 { + return + } + + e, ok := n.Rhs[0].(*ast.TypeAssertExpr) + if !ok || e == nil { + return + } + + if isTypeSwitch(e) { + return + } + + if len(n.Lhs) == 1 { + w.addFailure(e, ruleUTAMessagePanic) + } + + if !w.acceptIgnoredTypeAssertionResult && len(n.Lhs) == 2 && isIgnored(n.Lhs[1]) { + w.addFailure(e, ruleUTAMessageIgnored) + } +} + +// handles "return foo(.*bar)" - one of them is enough to fail as golang does not forward the type cast tuples in return statements +func (w *lintUnchekedTypeAssertion) handleReturn(n *ast.ReturnStmt) { + for _, r := range n.Results { + w.requireNoTypeAssert(r) + } +} + +func (w *lintUnchekedTypeAssertion) handleRange(n *ast.RangeStmt) { + w.requireNoTypeAssert(n.X) +} + +func (w *lintUnchekedTypeAssertion) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.RangeStmt: + w.handleRange(n) + case *ast.SwitchStmt: + w.handleSwitch(n) + case *ast.ReturnStmt: + w.handleReturn(n) + case *ast.AssignStmt: + w.handleAssignment(n) + } + + return w +} + +func (w *lintUnchekedTypeAssertion) addFailure(n *ast.TypeAssertExpr, why string) { + s := fmt.Sprintf("type cast result is unchecked in %v - %s", gofmt(n), why) + w.onFailure(lint.Failure{ + Category: "bad practice", + Confidence: 1, + Node: n, + Failure: s, + }) +} diff --git a/test/unchecked_type_assertion_test.go b/test/unchecked_type_assertion_test.go new file mode 100644 index 000000000..454923c96 --- /dev/null +++ b/test/unchecked_type_assertion_test.go @@ -0,0 +1,20 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestUncheckedDynamicCast(t *testing.T) { + testRule(t, "unchecked-type-assertion", &rule.UncheckedTypeAssertionRule{}) +} + +func TestUncheckedDynamicCastWithAcceptIgnored(t *testing.T) { + args := []any{map[string]any{ + "acceptIgnoredAssertionResult": true, + }} + + testRule(t, "unchecked-type-assertion-accept-ignored", &rule.UncheckedTypeAssertionRule{}, &lint.RuleConfig{Arguments: args}) +} diff --git a/testdata/unchecked-type-assertion-accept-ignored.go b/testdata/unchecked-type-assertion-accept-ignored.go new file mode 100644 index 000000000..be38c83ab --- /dev/null +++ b/testdata/unchecked-type-assertion-accept-ignored.go @@ -0,0 +1,12 @@ +package fixtures + +var foo any = "foo" + +func handleIgnoredIsOKByConfig() { + // No lint here bacuse `acceptIgnoredAssertionResult` is set to `true` + r, _ := foo.(int) +} + +func handleSkippedStillFails() { + r := foo.(int) // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ +} diff --git a/testdata/unchecked-type-assertion.go b/testdata/unchecked-type-assertion.go new file mode 100644 index 000000000..b52ed176f --- /dev/null +++ b/testdata/unchecked-type-assertion.go @@ -0,0 +1,58 @@ +package fixtures + +import "fmt" + +var ( + foo any = "foo" + bars = []any{1, 42, "some", "thing"} +) + +func handleIgnored() { + r, _ := foo.(int) // MATCH /type cast result is unchecked in foo.(int) - type assertion result ignored/ +} + +func handleSkipped() { + r := foo.(int) // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ +} + +func handleReturn() int { + return foo.(int) // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ +} + +func handleSwitch() { + switch foo.(int) { // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ + case 0: + case 1: + // + } +} + +func handleRange() { + var some any = bars + for _, x := range some.([]string) { // MATCH /type cast result is unchecked in some.([]string) - type assertion will panic if not matched/ + fmt.Println(x) + } +} + +func handleTypeSwitch() { + // Should not be a lint + switch foo.(type) { + case string: + case int: + // + } +} + +func handleTypeSwitchWithAssignment() { + // Should not be a lint + switch n := foo.(type) { + case string: + case int: + // + } +} + +func handleTypeSwitchReturn() { + // Should not be a lint + return foo.(type) +} From 4b5873bb6850a7096cafd34bab7fcb639face1f4 Mon Sep 17 00:00:00 2001 From: Dirk Faust Date: Thu, 14 Sep 2023 07:55:43 +0200 Subject: [PATCH 2/3] Add some more cases to lint (PR feedback) --- rule/unchecked-type-assertion.go | 30 +++++++++++++++++ testdata/unchecked-type-assertion.go | 50 ++++++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 3 deletions(-) diff --git a/rule/unchecked-type-assertion.go b/rule/unchecked-type-assertion.go index f5801083b..10b9ff060 100644 --- a/rule/unchecked-type-assertion.go +++ b/rule/unchecked-type-assertion.go @@ -96,8 +96,34 @@ func (w *lintUnchekedTypeAssertion) requireNoTypeAssert(expr ast.Expr) { } } +func (w *lintUnchekedTypeAssertion) handleIfStmt(n *ast.IfStmt) { + ifCondition, ok := n.Cond.(*ast.BinaryExpr) + if !ok { + return + } + + w.requireNoTypeAssert(ifCondition.X) + w.requireNoTypeAssert(ifCondition.Y) +} + +func (w *lintUnchekedTypeAssertion) requireBinaryExpressionWithoutTypeAssertion(expr ast.Expr) { + binaryExpr, ok := expr.(*ast.BinaryExpr) + if ok { + w.requireNoTypeAssert(binaryExpr.X) + w.requireNoTypeAssert(binaryExpr.Y) + } +} + +func (w *lintUnchekedTypeAssertion) handleCaseClause(n *ast.CaseClause) { + for _, expr := range n.List { + w.requireNoTypeAssert(expr) + w.requireBinaryExpressionWithoutTypeAssertion(expr) + } +} + func (w *lintUnchekedTypeAssertion) handleSwitch(n *ast.SwitchStmt) { w.requireNoTypeAssert(n.Tag) + w.requireBinaryExpressionWithoutTypeAssertion(n.Tag) } func (w *lintUnchekedTypeAssertion) handleAssignment(n *ast.AssignStmt) { @@ -144,6 +170,10 @@ func (w *lintUnchekedTypeAssertion) Visit(node ast.Node) ast.Visitor { w.handleReturn(n) case *ast.AssignStmt: w.handleAssignment(n) + case *ast.IfStmt: + w.handleIfStmt(n) + case *ast.CaseClause: + w.handleCaseClause(n) } return w diff --git a/testdata/unchecked-type-assertion.go b/testdata/unchecked-type-assertion.go index b52ed176f..f14b624ed 100644 --- a/testdata/unchecked-type-assertion.go +++ b/testdata/unchecked-type-assertion.go @@ -52,7 +52,51 @@ func handleTypeSwitchWithAssignment() { } } -func handleTypeSwitchReturn() { - // Should not be a lint - return foo.(type) +func handleTypeComparison() { + if foo.(int) == 1 { // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ + return + } +} + +func handleTypeComparisonReverse() { + if foo.(int) == 1 { // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ + return + } +} + +func handleTypeAssignmentComparison() { + var value any + value = 42 // int + + if v := value.(int); v == 42 { // MATCH /type cast result is unchecked in value.(int) - type assertion will panic if not matched/ + fmt.Printf("Value is an integer: %d\n", v) + } +} + +func handleSwitchComparison() { + switch foo.(int) == 1 { // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ + case true: + case false: + } +} + +func handleSwitchComparisonReverse() { + switch 1 == foo.(int) { // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ + case true: + case false: + } +} + +func handleInnerSwitchAssertion() { + switch { + case foo.(int) == 1: // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ + case bar.(int) == 1: // MATCH /type cast result is unchecked in bar.(int) - type assertion will panic if not matched/ + } +} + +func handleInnerSwitchAssertionReverse() { + switch { + case 1 == foo.(int): // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ + case 1 == bar.(int): // MATCH /type cast result is unchecked in bar.(int) - type assertion will panic if not matched/ + } } From aacc34a6dc4a690613d8482c22af3bc6a5eac78c Mon Sep 17 00:00:00 2001 From: Dirk Faust Date: Thu, 14 Sep 2023 10:26:43 +0200 Subject: [PATCH 3/3] Also handle type assertions in channel send (PR feedback) --- rule/unchecked-type-assertion.go | 6 ++++++ testdata/unchecked-type-assertion.go | 8 +++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/rule/unchecked-type-assertion.go b/rule/unchecked-type-assertion.go index 10b9ff060..ec4b5ed72 100644 --- a/rule/unchecked-type-assertion.go +++ b/rule/unchecked-type-assertion.go @@ -160,6 +160,10 @@ func (w *lintUnchekedTypeAssertion) handleRange(n *ast.RangeStmt) { w.requireNoTypeAssert(n.X) } +func (w *lintUnchekedTypeAssertion) handleChannelSend(n *ast.SendStmt) { + w.requireNoTypeAssert(n.Value) +} + func (w *lintUnchekedTypeAssertion) Visit(node ast.Node) ast.Visitor { switch n := node.(type) { case *ast.RangeStmt: @@ -174,6 +178,8 @@ func (w *lintUnchekedTypeAssertion) Visit(node ast.Node) ast.Visitor { w.handleIfStmt(n) case *ast.CaseClause: w.handleCaseClause(n) + case *ast.SendStmt: + w.handleChannelSend(n) } return w diff --git a/testdata/unchecked-type-assertion.go b/testdata/unchecked-type-assertion.go index f14b624ed..184008c77 100644 --- a/testdata/unchecked-type-assertion.go +++ b/testdata/unchecked-type-assertion.go @@ -59,7 +59,7 @@ func handleTypeComparison() { } func handleTypeComparisonReverse() { - if foo.(int) == 1 { // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ + if 1 == foo.(int) { // MATCH /type cast result is unchecked in foo.(int) - type assertion will panic if not matched/ return } } @@ -100,3 +100,9 @@ func handleInnerSwitchAssertionReverse() { case 1 == bar.(int): // MATCH /type cast result is unchecked in bar.(int) - type assertion will panic if not matched/ } } + +func handleChannelWrite() { + c := make(chan any) + var a any = "foo" + c <- a.(int) // MATCH /type cast result is unchecked in a.(int) - type assertion will panic if not matched/ +}