Skip to content

Commit

Permalink
fix: Also apply allowed list to value switch statements
Browse files Browse the repository at this point in the history
This also moves the reported AST position of switch statements from the
switch keyword to the first problematic case.
  • Loading branch information
polyfloyd committed May 30, 2024
1 parent e24df99 commit c0e6cac
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 26 deletions.
4 changes: 2 additions & 2 deletions errorlint/allowed.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,13 @@ func isAllowedErrAndFunc(err, fun string) bool {
return false
}

func isAllowedErrorComparison(pass *TypesInfoExt, binExpr *ast.BinaryExpr) bool {
func isAllowedErrorComparison(pass *TypesInfoExt, a, b ast.Expr) bool {
var errName string // `<package>.<name>`, e.g. `io.EOF`
var callExprs []*ast.CallExpr

// Figure out which half of the expression is the returned error and which
// half is the presumed error declaration.
for _, expr := range []ast.Expr{binExpr.X, binExpr.Y} {
for _, expr := range []ast.Expr{a, b} {
switch t := expr.(type) {
case *ast.SelectorExpr:
// A selector which we assume refers to a staticaly declared error
Expand Down
49 changes: 29 additions & 20 deletions errorlint/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,15 +172,15 @@ func LintErrorComparisons(info *TypesInfoExt) []analysis.Diagnostic {
continue
}
// Comparing errors with nil is okay.
if isNilComparison(binExpr) {
if isNil(binExpr.X) || isNil(binExpr.Y) {
continue
}
// Find comparisons of which one side is a of type error.
if !isErrorComparison(info.TypesInfo, binExpr) {
if !isErrorType(info.TypesInfo, binExpr.X) && !isErrorType(info.TypesInfo, binExpr.Y) {
continue
}
// Some errors that are returned from some functions are exempt.
if isAllowedErrorComparison(info, binExpr) {
if isAllowedErrorComparison(info, binExpr.X, binExpr.Y) {
continue
}
// Comparisons that happen in `func (type) Is(error) bool` are okay.
Expand All @@ -201,43 +201,52 @@ func LintErrorComparisons(info *TypesInfoExt) []analysis.Diagnostic {
continue
}
// Check whether the switch operates on an error type.
if switchStmt.Tag == nil {
if !isErrorType(info.TypesInfo, switchStmt.Tag) {
continue
}
tagType := info.TypesInfo.Types[switchStmt.Tag]
if tagType.Type.String() != "error" {

var problematicCaseClause *ast.CaseClause
outer:
for _, stmt := range switchStmt.Body.List {
caseClause := stmt.(*ast.CaseClause)
for _, caseExpr := range caseClause.List {
if isNil(caseExpr) {
continue
}
// Some errors that are returned from some functions are exempt.
if !isAllowedErrorComparison(info, switchStmt.Tag, caseExpr) {
problematicCaseClause = caseClause
break outer
}
}
}
if problematicCaseClause == nil {
continue
}
// Comparisons that happen in `func (type) Is(error) bool` are okay.
if isNodeInErrorIsFunc(info, switchStmt) {
continue
}

if switchComparesNonNil(switchStmt) {
lints = append(lints, analysis.Diagnostic{
Message: "switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors",
Pos: switchStmt.Pos(),
Pos: problematicCaseClause.Pos(),
})
}

}

return lints
}

func isNilComparison(binExpr *ast.BinaryExpr) bool {
if ident, ok := binExpr.X.(*ast.Ident); ok && ident.Name == "nil" {
return true
}
if ident, ok := binExpr.Y.(*ast.Ident); ok && ident.Name == "nil" {
return true
}
return false
func isNil(ex ast.Expr) bool {
ident, ok := ex.(*ast.Ident)
return ok && ident.Name == "nil"
}

func isErrorComparison(info *types.Info, binExpr *ast.BinaryExpr) bool {
tx := info.Types[binExpr.X]
ty := info.Types[binExpr.Y]
return tx.Type.String() == "error" || ty.Type.String() == "error"
func isErrorType(info *types.Info, ex ast.Expr) bool {
t := info.Types[ex].Type
return t != nil && t.String() == "error"
}

func isNodeInErrorIsFunc(info *TypesInfoExt, node ast.Node) bool {
Expand Down
8 changes: 4 additions & 4 deletions errorlint/testdata/src/errorsis/errorsis.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ func NotEqualOperatorYoda() {

func CompareSwitch() {
err := doThing()
switch err { // want `switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors`
switch err {
case nil:
fmt.Println("nil")
case ErrFoo:
case ErrFoo: // want `switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors`
fmt.Println("ErrFoo")
}
}
Expand All @@ -95,8 +95,8 @@ func CompareSwitchSafe() {
}

func CompareSwitchInline() {
switch doThing() { // want `switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors`
case ErrFoo:
switch doThing() {
case ErrFoo: // want `switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors`
fmt.Println("ErrFoo")
}
}
Expand Down
18 changes: 18 additions & 0 deletions errorlint/testdata/src/issues/github-54.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package issues

import (
"fmt"

"golang.org/x/sys/unix"
)

func SwitchOnUnixErrors() {
err := unix.Rmdir("somepath")
switch err {
case unix.ENOENT:
return
case unix.EPERM:
return
}
fmt.Println(err)
}

0 comments on commit c0e6cac

Please sign in to comment.