Skip to content

Commit

Permalink
refactor: Permit matching on full paths of allowed errors
Browse files Browse the repository at this point in the history
  • Loading branch information
polyfloyd committed Aug 1, 2023
1 parent ae40071 commit cd16050
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 42 deletions.
50 changes: 24 additions & 26 deletions errorlint/allowed.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ var allowedErrors = []struct {
{err: "io.EOF", fun: "(*bytes.Reader).ReadRune"},
{err: "io.EOF", fun: "(*bytes.Reader).ReadString"},
// pkg/database/sql
{err: "sql.ErrNoRows", fun: "(*database/sql.Row).Scan"},
{err: "database/sql.ErrNoRows", fun: "(*database/sql.Row).Scan"},
// pkg/debug/elf
{err: "io.EOF", fun: "elf.Open"},
{err: "io.EOF", fun: "elf.NewFile"},
{err: "io.EOF", fun: "debug/elf.Open"},
{err: "io.EOF", fun: "debug/elf.NewFile"},
// pkg/io
{err: "io.EOF", fun: "(io.Reader).Read"},
{err: "io.EOF", fun: "(io.ReaderAt).ReadAt"},
Expand All @@ -50,14 +50,14 @@ var allowedErrors = []struct {
{err: "io.EOF", fun: "io.ReadFull"},
{err: "io.ErrUnexpectedEOF", fun: "io.ReadFull"},
// pkg/net/http
{err: "http.ErrServerClosed", fun: "(*net/http.Server).ListenAndServe"},
{err: "http.ErrServerClosed", fun: "(*net/http.Server).ListenAndServeTLS"},
{err: "http.ErrServerClosed", fun: "(*net/http.Server).Serve"},
{err: "http.ErrServerClosed", fun: "(*net/http.Server).ServeTLS"},
{err: "http.ErrServerClosed", fun: "http.ListenAndServe"},
{err: "http.ErrServerClosed", fun: "http.ListenAndServeTLS"},
{err: "http.ErrServerClosed", fun: "http.Serve"},
{err: "http.ErrServerClosed", fun: "http.ServeTLS"},
{err: "net/http.ErrServerClosed", fun: "(*net/http.Server).ListenAndServe"},
{err: "net/http.ErrServerClosed", fun: "(*net/http.Server).ListenAndServeTLS"},
{err: "net/http.ErrServerClosed", fun: "(*net/http.Server).Serve"},
{err: "net/http.ErrServerClosed", fun: "(*net/http.Server).ServeTLS"},
{err: "net/http.ErrServerClosed", fun: "net/http.ListenAndServe"},
{err: "net/http.ErrServerClosed", fun: "net/http.ListenAndServeTLS"},
{err: "net/http.ErrServerClosed", fun: "net/http.Serve"},
{err: "net/http.ErrServerClosed", fun: "net/http.ServeTLS"},
// pkg/os
{err: "io.EOF", fun: "(*os.File).Read"},
{err: "io.EOF", fun: "(*os.File).ReadAt"},
Expand All @@ -80,7 +80,7 @@ func isAllowedErrAndFunc(err, fun string) bool {
return false
}

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

Expand All @@ -91,11 +91,11 @@ func isAllowedErrorComparison(info *TypesInfoExt, binExpr *ast.BinaryExpr) bool
case *ast.SelectorExpr:
// A selector which we assume refers to a staticaly declared error
// in a package.
errName = selectorToString(t)
errName = selectorToString(pass, t)
case *ast.Ident:
// Identifier, most likely to be the `err` variable or whatever
// produces it.
callExprs = assigningCallExprs(info, t)
callExprs = assigningCallExprs(pass, t)
case *ast.CallExpr:
callExprs = append(callExprs, t)
}
Expand All @@ -115,11 +115,11 @@ func isAllowedErrorComparison(info *TypesInfoExt, binExpr *ast.BinaryExpr) bool
// allowed.
return false
}
if sel, ok := info.Selections[functionSelector]; ok {
if sel, ok := pass.TypesInfo.Selections[functionSelector]; ok {
functionNames[i] = fmt.Sprintf("(%s).%s", sel.Recv(), sel.Obj().Name())
} else {
// If there is no selection, assume it is a package.
functionNames[i] = selectorToString(callExpr.Fun.(*ast.SelectorExpr))
functionNames[i] = selectorToString(pass, callExpr.Fun.(*ast.SelectorExpr))
}
}

Expand All @@ -134,17 +134,17 @@ func isAllowedErrorComparison(info *TypesInfoExt, binExpr *ast.BinaryExpr) bool

// assigningCallExprs finds all *ast.CallExpr nodes that are part of an
// *ast.AssignStmt that assign to the subject identifier.
func assigningCallExprs(info *TypesInfoExt, subject *ast.Ident) []*ast.CallExpr {
func assigningCallExprs(pass *TypesInfoExt, subject *ast.Ident) []*ast.CallExpr {
if subject.Obj == nil {
return nil
}

// Find other identifiers that reference this same object. Make sure to
// exclude the subject identifier as it will cause an infinite recursion
// and is being used in a read operation anyway.
sobj := info.ObjectOf(subject)
sobj := pass.TypesInfo.ObjectOf(subject)
identifiers := []*ast.Ident{}
for _, ident := range info.IdentifiersForObject[sobj] {
for _, ident := range pass.IdentifiersForObject[sobj] {
if subject.Pos() != ident.Pos() {
identifiers = append(identifiers, ident)
}
Expand All @@ -153,7 +153,7 @@ func assigningCallExprs(info *TypesInfoExt, subject *ast.Ident) []*ast.CallExpr
// Find out whether the identifiers are part of an assignment statement.
var callExprs []*ast.CallExpr
for _, ident := range identifiers {
parent := info.NodeParent[ident]
parent := pass.NodeParent[ident]
switch declT := parent.(type) {
case *ast.AssignStmt:
// The identifier is LHS of an assignment.
Expand Down Expand Up @@ -181,7 +181,7 @@ func assigningCallExprs(info *TypesInfoExt, subject *ast.Ident) []*ast.CallExpr
continue
}
// The subject was the result of assigning from another identifier.
callExprs = append(callExprs, assigningCallExprs(info, assignT)...)
callExprs = append(callExprs, assigningCallExprs(pass, assignT)...)
default:
// TODO: inconclusive?
}
Expand All @@ -190,9 +190,7 @@ func assigningCallExprs(info *TypesInfoExt, subject *ast.Ident) []*ast.CallExpr
return callExprs
}

func selectorToString(selExpr *ast.SelectorExpr) string {
if ident, ok := selExpr.X.(*ast.Ident); ok {
return ident.Name + "." + selExpr.Sel.Name
}
return ""
func selectorToString(pass *TypesInfoExt, selExpr *ast.SelectorExpr) string {
o := pass.TypesInfo.Uses[selExpr.Sel]
return fmt.Sprintf("%s.%s", o.Pkg().Path(), o.Name())
}
16 changes: 8 additions & 8 deletions errorlint/analysis.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ func init() {

func run(pass *analysis.Pass) (interface{}, error) {
lints := []analysis.Diagnostic{}
extInfo := newTypesInfoExt(pass.TypesInfo)
extInfo := newTypesInfoExt(pass)
if checkComparison {
l := LintErrorComparisons(pass.Fset, extInfo)
l := LintErrorComparisons(extInfo)
lints = append(lints, l...)
}
if checkAsserts {
Expand All @@ -57,7 +57,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
}

type TypesInfoExt struct {
types.Info
*analysis.Pass

// Maps AST nodes back to the node they are contained within.
NodeParent map[ast.Node]ast.Node
Expand All @@ -66,9 +66,9 @@ type TypesInfoExt struct {
IdentifiersForObject map[types.Object][]*ast.Ident
}

func newTypesInfoExt(info *types.Info) *TypesInfoExt {
func newTypesInfoExt(pass *analysis.Pass) *TypesInfoExt {
nodeParent := map[ast.Node]ast.Node{}
for node := range info.Scopes {
for node := range pass.TypesInfo.Scopes {
file, ok := node.(*ast.File)
if !ok {
continue
Expand All @@ -86,15 +86,15 @@ func newTypesInfoExt(info *types.Info) *TypesInfoExt {
}

identifiersForObject := map[types.Object][]*ast.Ident{}
for node, obj := range info.Defs {
for node, obj := range pass.TypesInfo.Defs {
identifiersForObject[obj] = append(identifiersForObject[obj], node)
}
for node, obj := range info.Uses {
for node, obj := range pass.TypesInfo.Uses {
identifiersForObject[obj] = append(identifiersForObject[obj], node)
}

return &TypesInfoExt{
Info: *info,
Pass: pass,
NodeParent: nodeParent,
IdentifiersForObject: identifiersForObject,
}
Expand Down
16 changes: 8 additions & 8 deletions errorlint/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,10 @@ func isFmtErrorfCallExpr(info types.Info, expr ast.Expr) (*ast.CallExpr, bool) {
return nil, false
}

func LintErrorComparisons(fset *token.FileSet, info *TypesInfoExt) []analysis.Diagnostic {
func LintErrorComparisons(info *TypesInfoExt) []analysis.Diagnostic {
lints := []analysis.Diagnostic{}

for expr := range info.Types {
for expr := range info.TypesInfo.Types {
// Find == and != operations.
binExpr, ok := expr.(*ast.BinaryExpr)
if !ok {
Expand All @@ -175,7 +175,7 @@ func LintErrorComparisons(fset *token.FileSet, info *TypesInfoExt) []analysis.Di
continue
}
// Find comparisons of which one side is a of type error.
if !isErrorComparison(info.Info, binExpr) {
if !isErrorComparison(info.TypesInfo, binExpr) {
continue
}
// Some errors that are returned from some functions are exempt.
Expand All @@ -193,7 +193,7 @@ func LintErrorComparisons(fset *token.FileSet, info *TypesInfoExt) []analysis.Di
})
}

for scope := range info.Scopes {
for scope := range info.TypesInfo.Scopes {
// Find value switch blocks.
switchStmt, ok := scope.(*ast.SwitchStmt)
if !ok {
Expand All @@ -203,7 +203,7 @@ func LintErrorComparisons(fset *token.FileSet, info *TypesInfoExt) []analysis.Di
if switchStmt.Tag == nil {
continue
}
tagType := info.Types[switchStmt.Tag]
tagType := info.TypesInfo.Types[switchStmt.Tag]
if tagType.Type.String() != "error" {
continue
}
Expand Down Expand Up @@ -233,7 +233,7 @@ func isNilComparison(binExpr *ast.BinaryExpr) bool {
return false
}

func isErrorComparison(info types.Info, binExpr *ast.BinaryExpr) bool {
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"
Expand All @@ -252,11 +252,11 @@ func isNodeInErrorIsFunc(info *TypesInfoExt, node ast.Node) bool {
return false
}
// There should be 1 argument of type error.
if ii := funcDecl.Type.Params.List; len(ii) != 1 || info.Types[ii[0].Type].Type.String() != "error" {
if ii := funcDecl.Type.Params.List; len(ii) != 1 || info.TypesInfo.Types[ii[0].Type].Type.String() != "error" {
return false
}
// The return type should be bool.
if ii := funcDecl.Type.Results.List; len(ii) != 1 || info.Types[ii[0].Type].Type.String() != "bool" {
if ii := funcDecl.Type.Results.List; len(ii) != 1 || info.TypesInfo.Types[ii[0].Type].Type.String() != "bool" {
return false
}

Expand Down

0 comments on commit cd16050

Please sign in to comment.