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..ca4a89b07 --- /dev/null +++ b/rule/unchecked-type-assertion.go @@ -0,0 +1,155 @@ +package rule + +import ( + "fmt" + "go/ast" + "go/types" + "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, _ = v.(bool) + 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 (w *lintUnchekedTypeAssertion) isBoolType(e ast.Expr) bool { + t := w.pkg.TypeOf(e) + ut, ok := t.Underlying().(*types.Basic) + return (ok && ut != nil) && ut.Info()&types.IsBoolean == 0 +} + +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 && !w.isBoolType(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) +}