-
Notifications
You must be signed in to change notification settings - Fork 277
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Dirk Faust
committed
Sep 6, 2023
1 parent
ca0678c
commit 652dc0a
Showing
8 changed files
with
269 additions
and
3 deletions.
There are no files selected for viewing
This file contains 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 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 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 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 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,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, | ||
}) | ||
} |
This file contains 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,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}) | ||
} |
This file contains 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,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/ | ||
} |
This file contains 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,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) | ||
} |