go-errorlint icon indicating copy to clipboard operation
go-errorlint copied to clipboard

fix: resolve conflicting edits for type assertion and error comparison (fixes #100)

Open kakkoyun opened this issue 4 months ago • 5 comments

  • 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]

kakkoyun avatar Aug 06 '25 18:08 kakkoyun

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 errors import is missing

-       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.

rockdaboot avatar Aug 07 '25 20:08 rockdaboot

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 errors import is missing

-       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.

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.

kakkoyun avatar Aug 11 '25 11:08 kakkoyun

Hey @kakkoyun, is this still considered a draft? Or is this getting to a state where it can be reviewed?

polyfloyd avatar Sep 07 '25 07:09 polyfloyd

Hey @kakkoyun, is this still considered a draft? Or is this getting to a state where it can be reviewed?

It is technically in a reviewable state. I can mark it ready. However, I want to do some clean up and refactoring.

What would you prefer? Merge this and iterate or have the refactoring in this PR?

kakkoyun avatar Sep 09 '25 08:09 kakkoyun

Yes, please include the refactoring in this PR :pray:

polyfloyd avatar Sep 09 '25 09:09 polyfloyd