From ba8a7bc58989fc7d06bdb3ab2a19072cbed27bc3 Mon Sep 17 00:00:00 2001 From: Kemal Akkoyun Date: Wed, 6 Aug 2025 20:18:38 +0200 Subject: [PATCH] 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