From ba8a7bc58989fc7d06bdb3ab2a19072cbed27bc3 Mon Sep 17 00:00:00 2001 From: Kemal Akkoyun Date: Wed, 6 Aug 2025 20:18:38 +0200 Subject: [PATCH 1/4] fix: resolve conflicting edits for type assertion and error comparison (fixes #100) - Implement conflict resolution when both type assertion and error comparison linting rules apply to the same line - Add support for else-if statement transformations using errors.As and errors.Is - Generate combined diagnostics with unified suggested fixes Signed-off-by: Kemal Akkoyun --- errorlint/analysis.go | 16 +- errorlint/analysis_test.go | 7 +- errorlint/lint.go | 374 +++++++++++++++++- .../testdata/src/issues/fix/github-100.go | 36 ++ .../src/issues/fix/github-100.go.golden | 39 ++ .../src/issues/{ => lint}/github-11.go | 0 .../src/issues/{ => lint}/github-19.go | 0 .../src/issues/{ => lint}/github-21.go | 0 .../src/issues/{ => lint}/github-26.go | 0 .../src/issues/{ => lint}/github-29.go | 0 .../src/issues/{ => lint}/github-42.go | 0 .../src/issues/{ => lint}/github-50.go | 0 .../src/issues/{ => lint}/github-54.go | 0 .../src/issues/{ => lint}/github-57.go | 0 .../src/issues/{ => lint}/github-61.go | 0 15 files changed, 465 insertions(+), 7 deletions(-) create mode 100644 errorlint/testdata/src/issues/fix/github-100.go create mode 100644 errorlint/testdata/src/issues/fix/github-100.go.golden rename errorlint/testdata/src/issues/{ => lint}/github-11.go (100%) rename errorlint/testdata/src/issues/{ => lint}/github-19.go (100%) rename errorlint/testdata/src/issues/{ => lint}/github-21.go (100%) rename errorlint/testdata/src/issues/{ => lint}/github-26.go (100%) rename errorlint/testdata/src/issues/{ => lint}/github-29.go (100%) rename errorlint/testdata/src/issues/{ => lint}/github-42.go (100%) rename errorlint/testdata/src/issues/{ => lint}/github-50.go (100%) rename errorlint/testdata/src/issues/{ => lint}/github-54.go (100%) rename errorlint/testdata/src/issues/{ => lint}/github-57.go (100%) rename errorlint/testdata/src/issues/{ => lint}/github-61.go (100%) diff --git a/errorlint/analysis.go b/errorlint/analysis.go index a26067b..ca2e08f 100644 --- a/errorlint/analysis.go +++ b/errorlint/analysis.go @@ -51,6 +51,10 @@ func run(pass *analysis.Pass) (interface{}, error) { l := LintFmtErrorfCalls(pass.Fset, *pass.TypesInfo, checkErrorfMulti) lints = append(lints, l...) } + + // Resolve conflicts between type assertion and error comparison diagnostics + lints = resolveConflicts(lints, extInfo) + sort.Sort(ByPosition(lints)) for _, l := range lints { @@ -78,11 +82,15 @@ func newTypesInfoExt(pass *analysis.Pass) *TypesInfoExt { } stack := []ast.Node{file} ast.Inspect(file, func(n ast.Node) bool { - nodeParent[n] = stack[len(stack)-1] - if n == nil { - stack = stack[:len(stack)-1] - } else { + if n != nil && len(stack) > 0 { + // Only set parent if the node is not the same as the current stack top + // This prevents self-references that cause infinite loops + if n != stack[len(stack)-1] { + nodeParent[n] = stack[len(stack)-1] + } stack = append(stack, n) + } else if n == nil { + stack = stack[:len(stack)-1] } return true }) diff --git a/errorlint/analysis_test.go b/errorlint/analysis_test.go index a7243cd..517f243 100644 --- a/errorlint/analysis_test.go +++ b/errorlint/analysis_test.go @@ -41,7 +41,7 @@ func TestAllowedComparisons(t *testing.T) { func TestIssueRegressions(t *testing.T) { analyzer := NewAnalyzer() - analysistest.Run(t, analysistest.TestData(), analyzer, "issues") + analysistest.Run(t, analysistest.TestData(), analyzer, "issues/lint") } func TestErrorComparisonFixes(t *testing.T) { @@ -53,3 +53,8 @@ func TestErrorTypeAssertionFixes(t *testing.T) { analyzer := NewAnalyzer() analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), analyzer, "errorassert") } + +func TestIssueFixRegressions(t *testing.T) { + analyzer := NewAnalyzer() + analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), analyzer, "issues/fix") +} diff --git a/errorlint/lint.go b/errorlint/lint.go index 9ef6d58..f312f43 100644 --- a/errorlint/lint.go +++ b/errorlint/lint.go @@ -13,6 +13,139 @@ import ( "golang.org/x/tools/go/analysis" ) +type diagnosticType int + +const ( + typeAssertionDiag diagnosticType = iota + errorComparisonDiag + otherDiag +) + +const ( + typeAssertionPattern = "type assertion on error" + errorComparisonPattern = "comparing with" +) + +// classifyDiagnostic determines the type of diagnostic based on its message +func classifyDiagnostic(diagnostic analysis.Diagnostic) diagnosticType { + msg := diagnostic.Message + if strings.Contains(msg, typeAssertionPattern) { + return typeAssertionDiag + } + if strings.Contains(msg, errorComparisonPattern) { + return errorComparisonDiag + } + return otherDiag +} + +func hasConflictingDiagnostics(lints []analysis.Diagnostic) bool { + var hasTypeAssertion, hasErrorComparison bool + + for _, lint := range lints { + switch classifyDiagnostic(lint) { + case typeAssertionDiag: + hasTypeAssertion = true + case errorComparisonDiag: + hasErrorComparison = true + } + + if hasTypeAssertion && hasErrorComparison { + return true + } + } + + return false +} + +func extractTypeAssignment(init ast.Stmt) (*ast.AssignStmt, *ast.TypeAssertExpr) { + assignStmt, ok := init.(*ast.AssignStmt) + if !ok || len(assignStmt.Lhs) != 2 || len(assignStmt.Rhs) != 1 { + return nil, nil + } + + typeAssert, ok := assignStmt.Rhs[0].(*ast.TypeAssertExpr) + if !ok { + return nil, nil + } + + return assignStmt, typeAssert +} + +func extractComparison(cond ast.Expr) *ast.BinaryExpr { + binExpr, ok := cond.(*ast.BinaryExpr) + if !ok || binExpr.Op != token.LAND { + return nil + } + + if _, ok := binExpr.X.(*ast.Ident); !ok { + return nil + } + rightBinExpr, ok := binExpr.Y.(*ast.BinaryExpr) + if !ok || (rightBinExpr.Op != token.EQL && rightBinExpr.Op != token.NEQ) { + return nil + } + + return rightBinExpr +} + +func buildVarDeclaration(assertion typeAssertion) string { + targetTypeStr := exprToString(assertion.targetType) + if strings.HasPrefix(targetTypeStr, "*") { + baseType, _ := strings.CutPrefix(targetTypeStr, "*") + return fmt.Sprintf("%s := &%s{}", assertion.varName, baseType) + } + return fmt.Sprintf("var %s %s", assertion.varName, targetTypeStr) +} + +func buildErrorsIsCall(comp comparison) string { + comparisonTarget := exprToString(comp.target) + comparisonExpr := exprToString(comp.expr) + + if comp.negated { + return fmt.Sprintf("!errors.Is(%s, %s)", comparisonExpr, comparisonTarget) + } + return fmt.Sprintf("errors.Is(%s, %s)", comparisonExpr, comparisonTarget) +} + +func formatBodyStmts(bodyStmts []ast.Stmt) string { + if len(bodyStmts) == 0 { + return "" + } + + var bodyBuf bytes.Buffer + for _, stmt := range bodyStmts { + if err := printer.Fprint(&bodyBuf, token.NewFileSet(), stmt); err != nil { + // TODO: How to handle this? Panic? + continue + } + bodyBuf.WriteString("\n\t\t") + } + return strings.TrimSpace(bodyBuf.String()) +} + +func groupDiagnosticsByIfStmt(lints []analysis.Diagnostic, extInfo *TypesInfoExt) (map[*ast.IfStmt][]analysis.Diagnostic, []analysis.Diagnostic) { + ifGroups := make(map[*ast.IfStmt][]analysis.Diagnostic) + var otherLints []analysis.Diagnostic + + for _, lint := range lints { + node := findNodeAtPosition(extInfo, lint.Pos) + if node == nil { + otherLints = append(otherLints, lint) + continue + } + + ifStmt := containingIf(extInfo, node) + if ifStmt == nil { + otherLints = append(otherLints, lint) + continue + } + + ifGroups[ifStmt] = append(ifGroups[ifStmt], lint) + } + + return ifGroups, otherLints +} + type ByPosition []analysis.Diagnostic func (l ByPosition) Len() int { return len(l) } @@ -335,7 +468,10 @@ func LintErrorComparisons(info *TypesInfoExt) []analysis.Diagnostic { // Print the modified AST to get the fix text. var buf bytes.Buffer - printer.Fprint(&buf, token.NewFileSet(), newSwitchStmt) + if err := printer.Fprint(&buf, token.NewFileSet(), newSwitchStmt); err != nil { + // TODO: How to handle this? Panic? + continue + } fixText := buf.String() diagnostic.SuggestedFixes = []analysis.SuggestedFix{{ @@ -804,7 +940,9 @@ func LintErrorTypeAssertions(fset *token.FileSet, info *TypesInfoExt) []analysis // Print the resulting block to get the fix text. var buf bytes.Buffer - printer.Fprint(&buf, token.NewFileSet(), blockStmt) + if err := printer.Fprint(&buf, token.NewFileSet(), blockStmt); err != nil { + continue + } fixText := buf.String() diagnostic.SuggestedFixes = []analysis.SuggestedFix{{ @@ -880,3 +1018,235 @@ func generateErrorVarName(originalName, typeName string) string { // If we couldn't determine a good name, use default. return "anErr" } + +func resolveConflicts(lints []analysis.Diagnostic, extInfo *TypesInfoExt) []analysis.Diagnostic { + ifGroups, otherLints := groupDiagnosticsByIfStmt(lints, extInfo) + + var result []analysis.Diagnostic + + for ifStmt, groupLints := range ifGroups { + if len(groupLints) <= 1 { + result = append(result, groupLints...) + continue + } + + if hasConflictingDiagnostics(groupLints) { + if combined := createCombinedDiagnostic(ifStmt, groupLints, extInfo); combined != nil { + result = append(result, *combined) + continue + } + } + + result = append(result, groupLints...) + } + + return append(result, otherLints...) +} + +func findNodeAtPosition(extInfo *TypesInfoExt, pos token.Pos) ast.Node { + // First check type-checked expressions (most common case) + for node := range extInfo.TypesInfo.Types { + if nodeContainsPos(node, pos) { + return node + } + } + + // Fallback: check scopes + for scope := range extInfo.TypesInfo.Scopes { + if nodeContainsPos(scope, pos) { + return scope + } + } + + return nil +} + +// nodeContainsPos checks if a node contains the given position +func nodeContainsPos(node ast.Node, pos token.Pos) bool { + return node.Pos() <= pos && pos < node.End() +} + +// containingIf finds the if statement that contains the given node +// by walking up the AST parent chain. +func containingIf(extInfo *TypesInfoExt, node ast.Node) *ast.IfStmt { + current := node + for current != nil { + if ifStmt, ok := current.(*ast.IfStmt); ok { + return ifStmt + } + parent := extInfo.NodeParent[current] + if parent == nil { + break + } + current = parent + } + return nil +} + +// createCombinedDiagnostic creates a single diagnostic that handles both +// type assertion and error comparison issues in the same if statement. +// It generates a combined suggested fix that uses both errors.As and errors.Is. +func createCombinedDiagnostic(ifStmt *ast.IfStmt, lints []analysis.Diagnostic, extInfo *TypesInfoExt) *analysis.Diagnostic { + // Find the earliest position for the combined diagnostic + earliestPos := token.NoPos + for _, lint := range lints { + if earliestPos == token.NoPos || lint.Pos < earliestPos { + earliestPos = lint.Pos + } + } + + // Create the combined diagnostic + combined := &analysis.Diagnostic{ + Pos: earliestPos, + Message: "type assertion and error comparison will fail on wrapped errors. Use errors.As and errors.Is to check for specific errors", + } + + // Try to create a combined fix for the if statement + suggestedFix := combinedFix(ifStmt, extInfo) + if suggestedFix != nil { + combined.SuggestedFixes = []analysis.SuggestedFix{*suggestedFix} + } + + return combined +} + +// combinedFix creates a suggested fix that handles both type assertion +// and error comparison in the same if statement. +// Transforms: if e, ok := err.(*Type); ok && e.Field == value { +// Into: e := &Type{}; if errors.As(err, &e) && errors.Is(e.Field, value) { +func combinedFix(ifStmt *ast.IfStmt, extInfo *TypesInfoExt) *analysis.SuggestedFix { + // Parse the if statement structure to extract components + components := parseIfComponents(ifStmt) + if components == nil { + return nil + } + + // Check if this is an else-if statement + components.context.isElseIf = isElseIfStatement(ifStmt, extInfo) + + // Build the replacement text using the extracted components + replacement := buildReplacement(components) + if replacement == "" { + return nil + } + + // Determine the replacement range based on whether it's an else-if + endPos := ifStmt.Body.Pos() + if components.context.isElseIf { + // For else-if cases, we need to replace from the "if" to the end of the block + // to properly handle the transformation + endPos = ifStmt.Body.End() + } + + return &analysis.SuggestedFix{ + Message: "Use errors.As and errors.Is for error handling", + TextEdits: []analysis.TextEdit{{ + Pos: ifStmt.Pos(), + End: endPos, + NewText: []byte(replacement), + }}, + } +} + +// isElseIfStatement checks if the given if statement is part of an else-if construct +// by checking if it's in the Else field of a parent if statement. +func isElseIfStatement(ifStmt *ast.IfStmt, extInfo *TypesInfoExt) bool { + parent := extInfo.NodeParent[ifStmt] + if parent == nil { + return false + } + + // Check if the parent is an if statement's Else field + if parentIf, ok := parent.(*ast.IfStmt); ok { + return parentIf.Else == ifStmt + } + + return false +} + +// typeAssertion holds type assertion specific data +type typeAssertion struct { + varName string + errorExpr ast.Expr + targetType ast.Expr +} + +// comparison holds error comparison specific data +type comparison struct { + expr ast.Expr + target ast.Expr + negated bool +} + +// context holds if statement context information +type context struct { + isElseIf bool + bodyStmts []ast.Stmt +} + +// ifComponents holds the parsed components of an if statement +// that can be converted to use errors.As and errors.Is. +type ifComponents struct { + assertion typeAssertion + comparison comparison + context context +} + +// parseIfComponents extracts the components of the if statement pattern +// we want to fix: if e, ok := err.(*Type); ok && e.Field == value { +func parseIfComponents(ifStmt *ast.IfStmt) *ifComponents { + if ifStmt.Init == nil || ifStmt.Cond == nil { + return nil + } + + assignStmt, typeAssert := extractTypeAssignment(ifStmt.Init) + if assignStmt == nil || typeAssert == nil { + return nil + } + + rightBinExpr := extractComparison(ifStmt.Cond) + if rightBinExpr == nil { + return nil + } + + varIdent, ok := assignStmt.Lhs[0].(*ast.Ident) + if !ok { + return nil + } + + return &ifComponents{ + assertion: typeAssertion{ + varName: varIdent.Name, + errorExpr: typeAssert.X, + targetType: typeAssert.Type, + }, + comparison: comparison{ + expr: rightBinExpr.X, + target: rightBinExpr.Y, + negated: rightBinExpr.Op == token.NEQ, + }, + context: context{ + isElseIf: false, // Will be set by the calling function if needed + bodyStmts: ifStmt.Body.List, // Capture body statements + }, + } +} + +// buildReplacement creates the replacement text using proper formatting. +// It generates code like: e := &Type{}; if errors.As(err, &e) && errors.Is(e.Field, value) { +func buildReplacement(components *ifComponents) string { + var ( + errExpr = exprToString(components.assertion.errorExpr) + varDecl = buildVarDeclaration(components.assertion) + errorsIsCall = buildErrorsIsCall(components.comparison) + ) + + if components.context.isElseIf { + bodyText := formatBodyStmts(components.context.bodyStmts) + return fmt.Sprintf("{\n\t\t%s\n\t\tif errors.As(%s, &%s) && %s {\n\t\t\t%s\n\t\t}\n\t}", + varDecl, errExpr, components.assertion.varName, errorsIsCall, bodyText) + } + + return fmt.Sprintf("%s\n\tif errors.As(%s, &%s) && %s ", + varDecl, errExpr, components.assertion.varName, errorsIsCall) +} diff --git a/errorlint/testdata/src/issues/fix/github-100.go b/errorlint/testdata/src/issues/fix/github-100.go new file mode 100644 index 0000000..c9e1294 --- /dev/null +++ b/errorlint/testdata/src/issues/fix/github-100.go @@ -0,0 +1,36 @@ +package issues + +import ( + "os" + "syscall" +) + +// Test case for https://github.com/polyfloyd/go-errorlint/issues/100 +// This reproduces the conflicting edits issue where both type assertion +// and error comparison rules apply to the same line of code. +func ConflictingEdits() { + var err error = &os.PathError{Err: syscall.ESRCH} + + // This line should trigger both: + // 1. Type assertion linting (err.(*os.PathError)) + // 2. Error comparison linting (e.Err == syscall.ESRCH) + // Leading to a combined fix instead of conflicting edits + if e, ok := err.(*os.PathError); ok && e.Err == syscall.ESRCH { // want "type assertion and error comparison will fail on wrapped errors. Use errors.As and errors.Is to check for specific errors" + println("Found PathError with ESRCH") + } + + // Additional test cases with similar patterns + if pathErr, ok := err.(*os.PathError); ok && pathErr.Err == syscall.ENOENT { // want "type assertion and error comparison will fail on wrapped errors. Use errors.As and errors.Is to check for specific errors" + println("Found PathError with ENOENT") + } + + // Test the exact pattern from the original issue #100 + // This is the actual problematic line that caused conflicting edits + if err != nil { + println("error occurred") + } else if e, ok := err.(*os.PathError); ok && e.Err == syscall.ESRCH { // want "type assertion and error comparison will fail on wrapped errors. Use errors.As and errors.Is to check for specific errors" + // If the process exits while reading its /proc/$PID/maps, the kernel will + // return ESRCH. Handle it as if the process did not exist. + println("Found PathError with ESRCH in else if - from original issue") + } +} \ No newline at end of file diff --git a/errorlint/testdata/src/issues/fix/github-100.go.golden b/errorlint/testdata/src/issues/fix/github-100.go.golden new file mode 100644 index 0000000..8e5f73a --- /dev/null +++ b/errorlint/testdata/src/issues/fix/github-100.go.golden @@ -0,0 +1,39 @@ +package issues + +import ( + "os" + "syscall" +) + +// Test case for https://github.com/polyfloyd/go-errorlint/issues/100 +// This reproduces the conflicting edits issue where both type assertion +// and error comparison rules apply to the same line of code. +func ConflictingEdits() { + var err error = &os.PathError{Err: syscall.ESRCH} + + // This line should trigger both: + // 1. Type assertion linting (err.(*os.PathError)) + // 2. Error comparison linting (e.Err == syscall.ESRCH) + // Leading to a combined fix instead of conflicting edits + e := &os.PathError{} + if errors.As(err, &e) && errors.Is(e.Err, syscall.ESRCH) { // want "type assertion and error comparison will fail on wrapped errors. Use errors.As and errors.Is to check for specific errors" + println("Found PathError with ESRCH") + } + + // Additional test cases with similar patterns + pathErr := &os.PathError{} + if errors.As(err, &pathErr) && errors.Is(pathErr.Err, syscall.ENOENT) { // want "type assertion and error comparison will fail on wrapped errors. Use errors.As and errors.Is to check for specific errors" + println("Found PathError with ENOENT") + } + + // Test the exact pattern from the original issue #100 + // This is the actual problematic line that caused conflicting edits + if err != nil { + println("error occurred") + } else { + e := &os.PathError{} + if errors.As(err, &e) && errors.Is(e.Err, syscall.ESRCH) { + println("Found PathError with ESRCH in else if - from original issue") + } + } +} \ No newline at end of file diff --git a/errorlint/testdata/src/issues/github-11.go b/errorlint/testdata/src/issues/lint/github-11.go similarity index 100% rename from errorlint/testdata/src/issues/github-11.go rename to errorlint/testdata/src/issues/lint/github-11.go diff --git a/errorlint/testdata/src/issues/github-19.go b/errorlint/testdata/src/issues/lint/github-19.go similarity index 100% rename from errorlint/testdata/src/issues/github-19.go rename to errorlint/testdata/src/issues/lint/github-19.go diff --git a/errorlint/testdata/src/issues/github-21.go b/errorlint/testdata/src/issues/lint/github-21.go similarity index 100% rename from errorlint/testdata/src/issues/github-21.go rename to errorlint/testdata/src/issues/lint/github-21.go diff --git a/errorlint/testdata/src/issues/github-26.go b/errorlint/testdata/src/issues/lint/github-26.go similarity index 100% rename from errorlint/testdata/src/issues/github-26.go rename to errorlint/testdata/src/issues/lint/github-26.go diff --git a/errorlint/testdata/src/issues/github-29.go b/errorlint/testdata/src/issues/lint/github-29.go similarity index 100% rename from errorlint/testdata/src/issues/github-29.go rename to errorlint/testdata/src/issues/lint/github-29.go diff --git a/errorlint/testdata/src/issues/github-42.go b/errorlint/testdata/src/issues/lint/github-42.go similarity index 100% rename from errorlint/testdata/src/issues/github-42.go rename to errorlint/testdata/src/issues/lint/github-42.go diff --git a/errorlint/testdata/src/issues/github-50.go b/errorlint/testdata/src/issues/lint/github-50.go similarity index 100% rename from errorlint/testdata/src/issues/github-50.go rename to errorlint/testdata/src/issues/lint/github-50.go diff --git a/errorlint/testdata/src/issues/github-54.go b/errorlint/testdata/src/issues/lint/github-54.go similarity index 100% rename from errorlint/testdata/src/issues/github-54.go rename to errorlint/testdata/src/issues/lint/github-54.go diff --git a/errorlint/testdata/src/issues/github-57.go b/errorlint/testdata/src/issues/lint/github-57.go similarity index 100% rename from errorlint/testdata/src/issues/github-57.go rename to errorlint/testdata/src/issues/lint/github-57.go diff --git a/errorlint/testdata/src/issues/github-61.go b/errorlint/testdata/src/issues/lint/github-61.go similarity index 100% rename from errorlint/testdata/src/issues/github-61.go rename to errorlint/testdata/src/issues/lint/github-61.go From 0b8cbf63ea6959ac9854b7f411e522c0c741b35d Mon Sep 17 00:00:00 2001 From: Kemal Akkoyun Date: Mon, 1 Sep 2025 18:12:58 +0300 Subject: [PATCH 2/4] refactor: rename containingIf to statementContainsIf --- errorlint/lint.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/errorlint/lint.go b/errorlint/lint.go index f312f43..93d97e3 100644 --- a/errorlint/lint.go +++ b/errorlint/lint.go @@ -1066,9 +1066,9 @@ func nodeContainsPos(node ast.Node, pos token.Pos) bool { return node.Pos() <= pos && pos < node.End() } -// containingIf finds the if statement that contains the given node +// statementContainsIf finds the if statement that contains the given node // by walking up the AST parent chain. -func containingIf(extInfo *TypesInfoExt, node ast.Node) *ast.IfStmt { +func statementContainsIf(extInfo *TypesInfoExt, node ast.Node) *ast.IfStmt { current := node for current != nil { if ifStmt, ok := current.(*ast.IfStmt); ok { From 5f5f6a0a8a3b055f1ea23dc4927a5e614a0c37df Mon Sep 17 00:00:00 2001 From: Kemal Akkoyun Date: Mon, 1 Sep 2025 18:13:05 +0300 Subject: [PATCH 3/4] refactor: use strings.CutPrefix for cleaner prefix checking Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com> --- errorlint/lint.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/errorlint/lint.go b/errorlint/lint.go index 93d97e3..7a36d8c 100644 --- a/errorlint/lint.go +++ b/errorlint/lint.go @@ -90,8 +90,7 @@ func extractComparison(cond ast.Expr) *ast.BinaryExpr { func buildVarDeclaration(assertion typeAssertion) string { targetTypeStr := exprToString(assertion.targetType) - if strings.HasPrefix(targetTypeStr, "*") { - baseType, _ := strings.CutPrefix(targetTypeStr, "*") + if baseType, found := strings.CutPrefix(targetTypeStr, "*") ; found { return fmt.Sprintf("%s := &%s{}", assertion.varName, baseType) } return fmt.Sprintf("var %s %s", assertion.varName, targetTypeStr) From 81371b51204483084f870b370bd95062363836d8 Mon Sep 17 00:00:00 2001 From: Kemal Akkoyun Date: Mon, 1 Sep 2025 21:54:49 +0300 Subject: [PATCH 4/4] chore: Address review comments Signed-off-by: Kemal Akkoyun --- errorlint/analysis.go | 2 +- errorlint/analysis_test.go | 9 +- errorlint/lint.go | 174 ++++++++++++------ .../testdata/src/astmanipulation/comments.go | 39 ++++ .../src/astmanipulation/comments.go.golden | 42 +++++ .../testdata/src/astmanipulation/imports.go | 25 +++ .../src/astmanipulation/imports.go.golden | 27 +++ .../src/issues/fix/github-100.go.golden | 1 + 8 files changed, 259 insertions(+), 60 deletions(-) create mode 100644 errorlint/testdata/src/astmanipulation/comments.go create mode 100644 errorlint/testdata/src/astmanipulation/comments.go.golden create mode 100644 errorlint/testdata/src/astmanipulation/imports.go create mode 100644 errorlint/testdata/src/astmanipulation/imports.go.golden diff --git a/errorlint/analysis.go b/errorlint/analysis.go index ca2e08f..c0760fb 100644 --- a/errorlint/analysis.go +++ b/errorlint/analysis.go @@ -40,7 +40,7 @@ func run(pass *analysis.Pass) (interface{}, error) { var lints []analysis.Diagnostic extInfo := newTypesInfoExt(pass) if checkComparison { - l := LintErrorComparisons(extInfo) + l := LintErrorComparisons(pass.Fset, extInfo) lints = append(lints, l...) } if checkAsserts { diff --git a/errorlint/analysis_test.go b/errorlint/analysis_test.go index 517f243..56ac809 100644 --- a/errorlint/analysis_test.go +++ b/errorlint/analysis_test.go @@ -44,6 +44,11 @@ func TestIssueRegressions(t *testing.T) { analysistest.Run(t, analysistest.TestData(), analyzer, "issues/lint") } +func TestIssueFixRegressions(t *testing.T) { + analyzer := NewAnalyzer() + analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), analyzer, "issues/fix") +} + func TestErrorComparisonFixes(t *testing.T) { analyzer := NewAnalyzer() analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), analyzer, "errorcompare") @@ -54,7 +59,7 @@ func TestErrorTypeAssertionFixes(t *testing.T) { analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), analyzer, "errorassert") } -func TestIssueFixRegressions(t *testing.T) { +func TestASTManipulationFixes(t *testing.T) { analyzer := NewAnalyzer() - analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), analyzer, "issues/fix") + analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), analyzer, "astmanipulation") } diff --git a/errorlint/lint.go b/errorlint/lint.go index 7a36d8c..c1e50c8 100644 --- a/errorlint/lint.go +++ b/errorlint/lint.go @@ -13,40 +13,32 @@ import ( "golang.org/x/tools/go/analysis" ) -type diagnosticType int +type diagnosticCategory string const ( - typeAssertionDiag diagnosticType = iota - errorComparisonDiag - otherDiag + categoryTypeAssertion diagnosticCategory = "type-assertion" + categoryErrorComparison diagnosticCategory = "error-comparison" + categoryCombinedError diagnosticCategory = "combined-error" + categoryFormatVerb diagnosticCategory = "format-verb" + unknownDiagnostic diagnosticCategory = "" ) const ( - typeAssertionPattern = "type assertion on error" - errorComparisonPattern = "comparing with" + msgTypeAssertion = "type assertion on error" + msgErrorComparison = "comparing with" ) -// classifyDiagnostic determines the type of diagnostic based on its message -func classifyDiagnostic(diagnostic analysis.Diagnostic) diagnosticType { - msg := diagnostic.Message - if strings.Contains(msg, typeAssertionPattern) { - return typeAssertionDiag - } - if strings.Contains(msg, errorComparisonPattern) { - return errorComparisonDiag - } - return otherDiag -} - func hasConflictingDiagnostics(lints []analysis.Diagnostic) bool { var hasTypeAssertion, hasErrorComparison bool for _, lint := range lints { - switch classifyDiagnostic(lint) { - case typeAssertionDiag: + switch diagnosticCategory(lint.Category) { + case categoryTypeAssertion: hasTypeAssertion = true - case errorComparisonDiag: + case categoryErrorComparison: hasErrorComparison = true + case categoryCombinedError: + return true } if hasTypeAssertion && hasErrorComparison { @@ -90,7 +82,7 @@ func extractComparison(cond ast.Expr) *ast.BinaryExpr { func buildVarDeclaration(assertion typeAssertion) string { targetTypeStr := exprToString(assertion.targetType) - if baseType, found := strings.CutPrefix(targetTypeStr, "*") ; found { + if baseType, found := strings.CutPrefix(targetTypeStr, "*"); found { return fmt.Sprintf("%s := &%s{}", assertion.varName, baseType) } return fmt.Sprintf("var %s %s", assertion.varName, targetTypeStr) @@ -133,7 +125,7 @@ func groupDiagnosticsByIfStmt(lints []analysis.Diagnostic, extInfo *TypesInfoExt continue } - ifStmt := containingIf(extInfo, node) + ifStmt := statementContainsIf(extInfo, node) if ifStmt == nil { otherLints = append(otherLints, lint) continue @@ -190,8 +182,9 @@ func LintFmtErrorfCalls(fset *token.FileSet, info types.Info, multipleWraps bool wrapCount++ if wrapCount > 1 { lints = append(lints, analysis.Diagnostic{ - Message: "only one %w verb is permitted per format string", - Pos: arg.Pos(), + Category: string(categoryFormatVerb), + Message: "only one %w verb is permitted per format string", + Pos: arg.Pos(), }) break } @@ -199,8 +192,9 @@ func LintFmtErrorfCalls(fset *token.FileSet, info types.Info, multipleWraps bool if wrapCount == 0 { lints = append(lints, analysis.Diagnostic{ - Message: "non-wrapping format verb for fmt.Errorf. Use `%w` to format errors", - Pos: args[i].Pos(), + Category: string(categoryFormatVerb), + Message: "non-wrapping format verb for fmt.Errorf. Use `%w` to format errors", + Pos: args[i].Pos(), }) break } @@ -230,8 +224,9 @@ func LintFmtErrorfCalls(fset *token.FileSet, info types.Info, multipleWraps bool strStart := call.Args[0].Pos() if lint == nil { lint = &analysis.Diagnostic{ - Message: "non-wrapping format verb for fmt.Errorf. Use `%w` to format errors", - Pos: arg.Pos(), + Category: string(categoryFormatVerb), + Message: "non-wrapping format verb for fmt.Errorf. Use `%w` to format errors", + Pos: arg.Pos(), } } fixMessage := "Use `%w` to format errors" @@ -297,7 +292,7 @@ func isFmtErrorfCallExpr(info types.Info, expr ast.Expr) (*ast.CallExpr, bool) { return nil, false } -func LintErrorComparisons(info *TypesInfoExt) []analysis.Diagnostic { +func LintErrorComparisons(fset *token.FileSet, info *TypesInfoExt) []analysis.Diagnostic { var lints []analysis.Diagnostic // Check for error comparisons. @@ -328,8 +323,9 @@ func LintErrorComparisons(info *TypesInfoExt) []analysis.Diagnostic { } diagnostic := analysis.Diagnostic{ - Message: fmt.Sprintf("comparing with %s will fail on wrapped errors. Use errors.Is to check for a specific error", binExpr.Op), - Pos: binExpr.Pos(), + Category: string(categoryErrorComparison), + Message: fmt.Sprintf("comparing with %s will fail on wrapped errors. Use errors.Is to check for a specific error", binExpr.Op), + Pos: binExpr.Pos(), } // Add suggested fix. @@ -353,13 +349,20 @@ func LintErrorComparisons(info *TypesInfoExt) []analysis.Diagnostic { replacement = "!" + replacement } + textEdits := []analysis.TextEdit{{ + Pos: binExpr.Pos(), + End: binExpr.End(), + NewText: []byte(replacement), + }} + + if file := findContainingFile(info, binExpr); file != nil && needsErrorsImport(file) { + importFix := createImportFix(file) + textEdits = append(textEdits, importFix.TextEdits...) + } + diagnostic.SuggestedFixes = []analysis.SuggestedFix{{ - Message: "Use errors.Is() to compare errors", - TextEdits: []analysis.TextEdit{{ - Pos: binExpr.Pos(), - End: binExpr.End(), - NewText: []byte(replacement), - }}, + Message: "Use errors.Is() to compare errors", + TextEdits: textEdits, }} lints = append(lints, diagnostic) @@ -403,8 +406,9 @@ func LintErrorComparisons(info *TypesInfoExt) []analysis.Diagnostic { if switchComparesNonNil(switchStmt) { diagnostic := analysis.Diagnostic{ - Message: "switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors", - Pos: problematicCaseClause.Pos(), + Category: string(categoryErrorComparison), + Message: "switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors", + Pos: problematicCaseClause.Pos(), } // Create a simpler version of the fix for switch statements @@ -489,7 +493,49 @@ func LintErrorComparisons(info *TypesInfoExt) []analysis.Diagnostic { return lints } -// exprToString converts an expression to its string representation. +func findContainingFile(info *TypesInfoExt, node ast.Node) *ast.File { + current := node + for current != nil { + if file, ok := current.(*ast.File); ok { + return file + } + current = info.NodeParent[current] + } + return nil +} + +func needsErrorsImport(file *ast.File) bool { + for _, imp := range file.Imports { + if imp.Path.Value == `"errors"` { + return false + } + } + return true +} + +func createImportFix(file *ast.File) analysis.SuggestedFix { + var importPos token.Pos + var newText string + + if len(file.Imports) > 0 { + lastImport := file.Imports[len(file.Imports)-1] + importPos = lastImport.End() + newText = "\n\t\"errors\"" + } else { + importPos = file.Name.End() + newText = "\n\nimport \"errors\"" + } + + return analysis.SuggestedFix{ + Message: "Add missing errors import", + TextEdits: []analysis.TextEdit{{ + Pos: importPos, + End: importPos, + NewText: []byte(newText), + }}, + } +} + func exprToString(expr ast.Expr) string { switch e := expr.(type) { case *ast.Ident: @@ -606,8 +652,9 @@ func LintErrorTypeAssertions(fset *token.FileSet, info *TypesInfoExt) []analysis } diagnostic := analysis.Diagnostic{ - Message: "type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors", - Pos: typeAssert.Pos(), + Category: string(categoryTypeAssertion), + Message: "type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors", + Pos: typeAssert.Pos(), } // Create suggested fix for type assertion @@ -770,8 +817,9 @@ func LintErrorTypeAssertions(fset *token.FileSet, info *TypesInfoExt) []analysis } diagnostic := analysis.Diagnostic{ - Message: "type switch on error will fail on wrapped errors. Use errors.As to check for specific errors", - Pos: typeAssert.Pos(), + Category: string(categoryTypeAssertion), + Message: "type switch on error will fail on wrapped errors. Use errors.As to check for specific errors", + Pos: typeAssert.Pos(), } // Transform type switch into a switch statement with errors.As in each case @@ -1096,8 +1144,9 @@ func createCombinedDiagnostic(ifStmt *ast.IfStmt, lints []analysis.Diagnostic, e // Create the combined diagnostic combined := &analysis.Diagnostic{ - Pos: earliestPos, - Message: "type assertion and error comparison will fail on wrapped errors. Use errors.As and errors.Is to check for specific errors", + Category: string(categoryCombinedError), + Pos: earliestPos, + Message: "type assertion and error comparison will fail on wrapped errors. Use errors.As and errors.Is to check for specific errors", } // Try to create a combined fix for the if statement @@ -1123,6 +1172,9 @@ func combinedFix(ifStmt *ast.IfStmt, extInfo *TypesInfoExt) *analysis.SuggestedF // Check if this is an else-if statement components.context.isElseIf = isElseIfStatement(ifStmt, extInfo) + // Handle else-if transformations carefully + // Note: else-if transformations change structure but preserve functionality + // Build the replacement text using the extracted components replacement := buildReplacement(components) if replacement == "" { @@ -1137,13 +1189,21 @@ func combinedFix(ifStmt *ast.IfStmt, extInfo *TypesInfoExt) *analysis.SuggestedF endPos = ifStmt.Body.End() } + textEdits := []analysis.TextEdit{{ + Pos: ifStmt.Pos(), + End: endPos, + NewText: []byte(replacement), + }} + + // Add errors import if needed + if file := findContainingFile(extInfo, ifStmt); file != nil && needsErrorsImport(file) { + importFix := createImportFix(file) + textEdits = append(textEdits, importFix.TextEdits...) + } + return &analysis.SuggestedFix{ - Message: "Use errors.As and errors.Is for error handling", - TextEdits: []analysis.TextEdit{{ - Pos: ifStmt.Pos(), - End: endPos, - NewText: []byte(replacement), - }}, + Message: "Use errors.As and errors.Is for error handling", + TextEdits: textEdits, } } @@ -1177,8 +1237,8 @@ type comparison struct { negated bool } -// context holds if statement context information -type context struct { +// controlFlowContext holds if statement controlFlowContext information +type controlFlowContext struct { isElseIf bool bodyStmts []ast.Stmt } @@ -1188,7 +1248,7 @@ type context struct { type ifComponents struct { assertion typeAssertion comparison comparison - context context + context controlFlowContext } // parseIfComponents extracts the components of the if statement pattern @@ -1224,7 +1284,7 @@ func parseIfComponents(ifStmt *ast.IfStmt) *ifComponents { target: rightBinExpr.Y, negated: rightBinExpr.Op == token.NEQ, }, - context: context{ + context: controlFlowContext{ isElseIf: false, // Will be set by the calling function if needed bodyStmts: ifStmt.Body.List, // Capture body statements }, diff --git a/errorlint/testdata/src/astmanipulation/comments.go b/errorlint/testdata/src/astmanipulation/comments.go new file mode 100644 index 0000000..3ff358c --- /dev/null +++ b/errorlint/testdata/src/astmanipulation/comments.go @@ -0,0 +1,39 @@ +package astmanipulation + +import ( + "os" + "syscall" +) + +// Test inline comment preservation +func InlineComments() { + var err error + + if err == syscall.ENOENT { // want "comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error" + // This block comment should be preserved + println("File not found") + } +} + +// Test else-if comment preservation (main PR issue) +func ElseIfComments() { + var err error + + if err != nil { + println("error occurred") + } else if e, ok := err.(*os.PathError); ok && e.Err == syscall.ESRCH { // want "type assertion and error comparison will fail on wrapped errors. Use errors.As and errors.Is to check for specific errors" + // If the process exits while reading its /proc/$PID/maps, the kernel will + // return ESRCH. Handle it as if the process did not exist. + println("Process not found") + } +} + +// Test block comments +func BlockComments() { + var err error + + /* Pre-condition block comment */ + if e, ok := err.(*os.PathError); ok { // want "type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors" + /* Inline block comment */ println("Path error:", e.Path) + } +} diff --git a/errorlint/testdata/src/astmanipulation/comments.go.golden b/errorlint/testdata/src/astmanipulation/comments.go.golden new file mode 100644 index 0000000..449bb57 --- /dev/null +++ b/errorlint/testdata/src/astmanipulation/comments.go.golden @@ -0,0 +1,42 @@ +package astmanipulation + +import ( + "errors" + "os" + "syscall" +) + +// Test inline comment preservation +func InlineComments() { + var err error + + if errors.Is(err, syscall.ENOENT) { // want "comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error" + // This block comment should be preserved + println("File not found") + } +} + +// Test else-if comment preservation (main PR issue) +func ElseIfComments() { + var err error + + if err != nil { + println("error occurred") + } else { + e := &os.PathError{} + if errors.As(err, &e) && errors.Is(e.Err, syscall.ESRCH) { + println("Process not found") + } + } +} + +// Test block comments +func BlockComments() { + var err error + + /* Pre-condition block comment */ + e := &os.PathError{} + if errors.As(err, &e) { // want "type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors" + /* Inline block comment */ println("Path error:", e.Path) + } +} \ No newline at end of file diff --git a/errorlint/testdata/src/astmanipulation/imports.go b/errorlint/testdata/src/astmanipulation/imports.go new file mode 100644 index 0000000..b32e82e --- /dev/null +++ b/errorlint/testdata/src/astmanipulation/imports.go @@ -0,0 +1,25 @@ +package astmanipulation + +import ( + "io" + "os" +) + +// Test missing errors import with simple comparison +func MissingImportSimple() { + var err error + var n int + + if n == 0 || (err != nil && err != io.EOF) { // want "comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error" + return + } +} + +// Test missing errors import with type assertion +func MissingImportTypeAssertion() { + var err error + + if e, ok := err.(*os.PathError); ok { // want "type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors" + println("Path error:", e.Path) + } +} \ No newline at end of file diff --git a/errorlint/testdata/src/astmanipulation/imports.go.golden b/errorlint/testdata/src/astmanipulation/imports.go.golden new file mode 100644 index 0000000..74c4d54 --- /dev/null +++ b/errorlint/testdata/src/astmanipulation/imports.go.golden @@ -0,0 +1,27 @@ +package astmanipulation + +import ( + "errors" + "io" + "os" +) + +// Test missing errors import with simple comparison +func MissingImportSimple() { + var err error + var n int + + if n == 0 || (err != nil && !errors.Is(err, io.EOF)) { // want "comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error" + return + } +} + +// Test missing errors import with type assertion +func MissingImportTypeAssertion() { + var err error + + e := &os.PathError{} + if errors.As(err, &e) { // want "type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors" + println("Path error:", e.Path) + } +} \ No newline at end of file diff --git a/errorlint/testdata/src/issues/fix/github-100.go.golden b/errorlint/testdata/src/issues/fix/github-100.go.golden index 8e5f73a..d4d23f9 100644 --- a/errorlint/testdata/src/issues/fix/github-100.go.golden +++ b/errorlint/testdata/src/issues/fix/github-100.go.golden @@ -1,6 +1,7 @@ package issues import ( + "errors" "os" "syscall" )