Skip to content
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

feat(analyzer): recognize custom error values in return #102

Conversation

ruslanSorokin
Copy link
Contributor

@ruslanSorokin ruslanSorokin commented May 6, 2024

This commit adds ability to recognize return of custom error values on the position of [error] interface type, e.g.,

package example

type S struct {
  i int
  s string
}

type CustomError struct { msg string }

func (e CustomError) Error() string { return e.msg }

func returnCustomError_1() (S, error) {
  return S{}, &CustomError{msg: "error message"} // Ok
}

func returnCustomError_2() (S, error) {
  return S{}, &CustomError{} // Warning: example.CustomError is missing field msg
}

func returnCustomError_3() (S, error) {
  return S{}, nil // Warning: example.S is missing fields i, s
}

This commit adds ability to recognize return of custom error literals under
[error] interface type, e.g.,
```golang
package example

type S struct {
  i int
  s string
}

type CustomError struct { msg string }

func (e CustomError) Error() string { return e.msg }

func returnCustomError_1() (S, error) {
  return S{}, &CustomError{msg: "error message"} // Ok
}

func returnCustomError_2() (S, error) {
  return S{}, &CustomError{} // Warning: example.CustomError is missing field msg
}

func returnCustomError_3() (S, error) {
  return S{}, nil // Warning: example.S is missing fields i, s
}
```
@ruslanSorokin ruslanSorokin marked this pull request as draft May 6, 2024 11:02
containsNonNilValOfErrType => containsErrorIfaceValue

containsNonNilValUnderErrType => containsValUnderErrorIface
@ruslanSorokin ruslanSorokin changed the title feat(analyzer): recognize custom error literals in return feat(analyzer): recognize custom error values in return May 8, 2024
@xobotyi
Copy link
Collaborator

xobotyi commented May 20, 2024

Thank you for pull request, first of all.

It is definely a step towards #89, but slightly in wrong direction, as it still implies that function should return an error interface, which covers most of the cases as it is the way recommended by go-team, but still not covers all of the cases since tehere are lots of places where concrete type being returned instead of error.

I will commit to your pullrequest, but change the code, reusing testcases.

@xobotyi xobotyi self-requested a review May 20, 2024 15:27
xobotyi added 2 commits May 21, 2024 01:03
Drastically simplify error fields detection, slightly changing the way
these cases are handled. Now, in case return statement contains types
that satisfies `error` interface - only those types will be checked (in
case they're structures too).
@xobotyi xobotyi marked this pull request as ready for review May 20, 2024 23:11
@xobotyi
Copy link
Collaborator

xobotyi commented May 20, 2024

@ruslanSorokin @navijation - pushed the version. All details can be found in tests and readme.

This solution is way simpler that one proposed by Ruslan. Also i trimmed tests a bit, leaving only ones that cover cornercases of errors handling.

@xobotyi
Copy link
Collaborator

xobotyi commented May 20, 2024

As for diff - it handles more false positives on kubernetes repo
diff.txt

@xobotyi xobotyi merged commit 5f38877 into GaijinEntertainment:master May 23, 2024
8 checks passed
@ruslanSorokin
Copy link
Contributor Author

ruslanSorokin commented May 23, 2024

Thank you for pull request, first of all.

It is definely a step towards #89, but slightly in wrong direction, as it still implies that function should return an error interface, which covers most of the cases as it is the way recommended by go-team, but still not covers all of the cases since tehere are lots of places where concrete type being returned instead of error.

I will commit to your pullrequest, but change the code, reusing testcases.

Yeah, I definitely took the wrong approach which also gives less functionality. Thanks for taking the time to revise it.

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.

2 participants