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

Allow to ignoring "unhandled errors" by their type and method #671

Open
zimmski opened this issue Apr 7, 2022 · 6 comments
Open

Allow to ignoring "unhandled errors" by their type and method #671

zimmski opened this issue Apr 7, 2022 · 6 comments

Comments

@zimmski
Copy link
Contributor

zimmski commented Apr 7, 2022

Is your feature request related to a problem? Please describe.

When one is using the method "WriteString" of the type "strings::Builder" and does not handle the error it is flagged because all errors must be handled. However, This method never returns an error. Hence, it makes sense to ignore it (even by default).

The problem with revive is, that it does not allow to ignore methods with their type, since one can only ignore them with their variable name. So instead of "strings.Builder.WriteString" (or another annoation) to ignore it, i need to write "b.WriteString" (in our code this is one instance) which however, would include implementations that do return an error.

Describe the solution you'd like

Allow to define "strings.Builder.WriteString" (or "strings::Builder.WriteString") to allow to ignore the method "WriteString" of the type "Builder" in the package "strings".

@zimmski
Copy link
Contributor Author

zimmski commented Apr 7, 2022

I know that this might be a breaking change, because "fmt.Print" could also mean variable "fmt" and function "Print" instead of import identifier "fmt".

@git-hulk
Copy link
Contributor

Currently, revive supported the ignore list but it used AST subtree to match
the ignore function name. For example, we need to add b1.WriteString
to the ignore list:

var b1 strings.Builder
b1.WriteString

This will be trivial when the variable names were different, so I'm wondering
whether below rules is good to guys or not:

  1. WriteString will ignore all function calls named with WriteString
  2. xxx.WriteString will ignore all function calls named with WriteString in xxx package
  3. xxx.yyy.WriteString will ignore all function calls named with String and receiver is yyy in package xxx

@git-hulk
Copy link
Contributor

@chavacava How do you think about the above solution?

@chavacava
Copy link
Collaborator

chavacava commented Jul 13, 2022

Trying to handle all cases by matching strings will necessarily be an incomplete and uncomfortable solution. For example, using string matching to ignore the following call

var b1 strings.Builder
b1.WriteString

will require to add b1.WriteString to the ignore list. So far so good. If we now add a new variable to our code:

var b1 strings.Builder
var b2 strings.Builder
b1.WriteString
b2.WriteString

we need to add b2.WriteString to the ignore list... and that is not comfortable and it does not scale well.

The root of the problem is lack of type information. We need to know the actual full canonical method name of b2.WriteString and b1.WriteString: strings.Builder.WriteString
Ignore list should contain full canonical method names. That way we abstract away the details of variable names.

@git-hulk
Copy link
Contributor

Trying to handle all cases by matching strings will necessarily be an incomplete and uncomfortable solution. For example, using string matching to ignore the following call

var b1 strings.Builder
b1.WriteString

will require to add b1.WriteString to the ignore list. So far so good. If we now add a new variable to our code:

var b1 strings.Builder
var b2 strings.Builder
b1.WriteString
b2.WriteString

we need to add b2.WriteString to the ignore list... and that is not comfortable and it does not scale well.

The root of the problem is lack of type information. We need to know the actual full canonical method name of b2.WriteString and b1.WriteString: strings.Builder.WriteString Ignore list should contain full canonical method names. That way we abstract away the details of variable names.

Yes, I didn't explain it clearly, we are on the same page.

@winterjung
Copy link

I think this issue seems to be resolved by #757.
In my case, the use of arguments = ["bytes.Buffer.WriteString"] ignores buf := &bytes.Buffer{}; buf.WriteString("...") well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants