Add unchecked-type-assertion

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 |

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:/golang/go/wiki/CodeReviewComments#initialisms), [variable](https:/golang/go/wiki/CodeReviewComments#variable-names) or [package](https:/golang/go/wiki/CodeReviewComments#package-names) naming conventions are not followed.

config/config.go

@@ -1,4 +1,4 @@
 package config
 
 import (
  "errors"
@@ -80,6 +80,7 @@
  &rule.FunctionLength{},
  &rule.NestedStructs{},
  &rule.UselessBreak{},
+ &rule.UncheckedTypeAssertionRule{},
  &rule.TimeEqualRule{},
  &rule.BannedCharsRule{},
  &rule.OptimizeOperandsOrderRule{},

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=

rule/unchecked-type-assertion.go

@@ -0,0 +1,196 @@
+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) 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) {
+ 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) handleChannelSend(n *ast.SendStmt) {
+ w.requireNoTypeAssert(n.Value)
+}
+
+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)
+ case *ast.IfStmt:
+ w.handleIfStmt(n)
+ case *ast.CaseClause:
+ w.handleCaseClause(n)
+ case *ast.SendStmt:
+ w.handleChannelSend(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,
+ })
+}

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})
+}

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/
+}