fix: resolve conflicting edits for type assertion and error comparison (fixes #100) #104
Conversation
This file contains hidden or 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
fixes polyfloyd#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 <[email protected]>
|
Testing this PR on the same code as in the error description, the comment lines were dropped. - } else if e, ok := err.(*os.PathError); ok && e.Err == syscall.ESRCH {
- // If the process exits while reading its /proc/$PID/maps, the kernel will
- // return ESRCH. Handle it as if the process did not exist.
- pm.mappingStats.errProcESRCH.Add(1)
+ } else {
+ e := &os.PathError{}
+ if errors.As(err, &e) && errors.Is(e.Err, syscall.ESRCH) {
+ pm.mappingStats.errProcESRCH.Add(1)
+ }
}Also (maybe a different issue), with the following change, the - if n == 0 || (err != nil && err != io.EOF) {
+ if n == 0 || (err != nil && !errors.Is(err, io.EOF)) {Let me know if this is unrelated and I'll open another issue. |
ccoVeille
reviewed
Aug 9, 2025
Comment on lines
+93
to
+95
| if strings.HasPrefix(targetTypeStr, "*") { | ||
| baseType, _ := strings.CutPrefix(targetTypeStr, "*") | ||
| return fmt.Sprintf("%s := &%s{}", assertion.varName, baseType) |
There was a problem hiding this comment.
What about this?
Suggested change
| if strings.HasPrefix(targetTypeStr, "*") { | |
| baseType, _ := strings.CutPrefix(targetTypeStr, "*") | |
| return fmt.Sprintf("%s := &%s{}", assertion.varName, baseType) | |
| if baseType, found := strings.CutPrefix(targetTypeStr, "*") ; found { | |
| return fmt.Sprintf("%s := &%s{}", assertion.varName, baseType) |
There was a problem hiding this comment.
I should have remembered this one from your comments from the first PR 🙈 Muscle memory!
Comment on lines
+1064
to
+1084
| // 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 | ||
| } |
There was a problem hiding this comment.
Maybe this to be consistent
Suggested change
| // 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 | |
| } | |
| // 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() | |
| } | |
| // statementContainsIf finds the if statement that contains the given node | |
| // by walking up the AST parent chain. | |
| func statementContainsIf(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 | |
| } |
Comment on lines
+1181
to
+1185
| // context holds if statement context information | ||
| type context struct { | ||
| isElseIf bool | ||
| bodyStmts []ast.Stmt | ||
| } |
There was a problem hiding this comment.
It's strange to call it context, especially because it shadows context package
ifInfo maybe?
Comment on lines
+24
to
+39
| 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 | ||
| } |
There was a problem hiding this comment.
I'm surprised the detection is made on the message.
Should we use this constant when raising the diagnostic? Maybe I missed something 🤔
Good catch. I have to make sure we preserve the comments (hard with stdlib). I will think ways to simplify the implementation. It is hard to rewrite control flow statements. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Kemal Akkoyun [email protected]