Skip to content

feat: Add automatic error comparison and type assertion fixes #96

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 22, 2025

Conversation

kakkoyun
Copy link
Contributor

@kakkoyun kakkoyun commented Mar 7, 2025

This is an attempt to add auto-fixes for error comparisons. By all means, I'm no expert on the topic. Please let me know what I don't know. I would love to have this functionality to fix large codebases. I'm all open to iterate on this an improve the implementation. I'm opening it as soon as the tests pass so it could be in a rather rough shape.

Changes

Enhance errorlint analyzer with automatic fixes for:

  • Error comparisons using == and !=
  • Type assertions on errors
  • Error switches

Adds suggested fixes to:

  • Replace direct error comparisons with errors.Is()
  • Convert type assertions to use errors.As()
  • Transform error switches to use errors.Is() and errors.As()

Includes new test cases and golden files to validate the new automatic fixing capabilities.

Enhance errorlint analyzer with automatic fixes for:
- Error comparisons using `==` and `!=`
- Type assertions on errors
- Error switches

Adds suggested fixes to:
- Replace direct error comparisons with `errors.Is()`
- Convert type assertions to use `errors.As()`
- Transform error switches to use `errors.Is()` and `errors.As()`

Includes new test cases and golden files to validate the new automatic fixing capabilities.
@kakkoyun
Copy link
Contributor Author

kakkoyun commented Mar 7, 2025

cc @polyfloyd

Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider my review

@kakkoyun
Copy link
Contributor Author

@ccoVeille Thanks for the review. It's greatly appreciated. I will check them in-depth as soon as possible.

Copy link
Owner

@polyfloyd polyfloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I'm sure this will be useful to a lot of people :)

Major remark, but hopefully a minor fix:

err.(*MyError) is rewritten to errors.As(err, recv) where recv = new(MyError). This will not work, as the pointer bit is significant for the error type. new(MyError) does return a *MyError, but in order for errors.As to work it needs another pointer, so **MyError. Otherwise it will panic:

panic: errors: *target must be interface or implement error

Changing the rewrite rule to copy the type verbatim should be enough to fix this.

You can play with this example to get an idea for how this works:

package main

import (
	"errors"
	"fmt"
)

type MyError struct{}

func (*MyError) Error() string { return "*MyError" }

func main() {

	var err error = &MyError{}

	if me, ok := err.(*MyError); ok {
		fmt.Println(me)
	} else {
		fmt.Println("-")
	}

	me := new(*MyError)
	if errors.As(err, me) {
		fmt.Println(me)
	} else {
		fmt.Println("-")
	}
}

I have only looked at the test files now, I hopefully have some time to look over the code this weekend...

Signed-off-by: Kemal Akkoyun <[email protected]>
@kakkoyun kakkoyun requested review from ccoVeille and polyfloyd March 21, 2025 15:44
Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm amazed by the amount of work you did, and tremendous use cases you thought about, handled, and hopefully covered by unit tests.

I made a small suggestion, feel free to ignore.

- Add function for meaningful error variable names
- Preserve original "ok" variable names in type assertions
- Use errors.As() with generated variable names
- Add tests for custom and wrapped errors

Signed-off-by: Kemal Akkoyun <[email protected]>
@kakkoyun kakkoyun force-pushed the error_comparison_fix branch from 1df0923 to df889d9 Compare March 21, 2025 16:28
Signed-off-by: Kemal Akkoyun <[email protected]>
@kakkoyun
Copy link
Contributor Author

@ccoVeille for the review and kind words. You made my day. I hope this will be useable and we (as Go community) benefit from it.

@ccoVeille
Copy link

You are welcome, please consider tagging one of your commit with Co-authored-by tag if you felt my contribution was valuable enough to be mentioned.

Adding something like this to your commit messages

Co-authored-by: ccoVeille <[email protected]>

For example

Commit message followed by a line feed then the Co-authored-by tag

Co-authored-by: ccoVeille <[email protected]>

https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors#creating-co-authored-commits-on-the-command-line

@polyfloyd polyfloyd merged commit 040cb97 into polyfloyd:main Mar 22, 2025
1 of 2 checks passed
@kakkoyun kakkoyun deleted the error_comparison_fix branch March 23, 2025 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants