Add unchecked-type-assertion (#889)

Co-authored-by: Dirk Faust <d.faust@mittwald.de>

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://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.

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{},

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

testdata/unchecked-type-assertion.go

@@ -0,0 +1,108 @@
+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 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 1 == foo.(int) { // 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/
+	}
+}
+
+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/
+}