warn when passing a pointer explicitely and method receiver is not a pointer receiver

[hbt]

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

type fnCallsVisitor struct {
	fnNames []string 
}

// Notice method receiver is not a pointer receiver "f *fnCallsVisitor".
// Therefore object is passed as a value and not by reference
func (f fnCallsVisitor) Visit(node dst.Node) (w dst.Visitor) {
    // f.Names = append(f.Names, node.StringProperty)
    return f
}


func getHelperNames(fn *dst.FuncDecl) []string {
	fncalls := fnCallsVisitor{}
    // Notice: explicit call to pass pointer
	dst.Walk(&fncalls, fn)
}

This is an issue when an API changes underneath you. Making a call to an API, passing the pointer with "&" then later API method changes from pointer receiver to value and there are no warnings.

Or in my case, using the IDE to implement the interface "dst.Visitor" but IDE generated code didn't use a pointer receiver and not noticing until debugging

Describe the solution you'd like

Passing a pointer explicitely (using "&") to a method receiver that is not a pointer receiver should issue a warning because at the very least, the "&" is not necessary or it is indicative of an error.

This will be a problem and could create false positives. e.g method receiver is "interface{}" and using reflect to manipulate the pointer.
If reflection is involved, warning confidence should be reduced.

Describe alternatives you've considered

Other than modifying the ast and decorating it with some asserts to ensure it's the same pointer. A lint warning would be better though.

Additional context

Thank you for considering this as a rule proposal and feel free to highlight any false positives that may occur that I haven't considered.

[chavacava]

Hi @hbt, thanks for the rule proposal.
Some questions/comments:

1. Why passing a pointer to a non pointer receiver is an error? (GO intentionally handle this case)
2. When you say "explicitly" you mean cases like the following are not in the scope of the rule?

p := &s{}
...
p.MethodWithANoPointerReciver

but

p := s{}
...
&p.MethodWithANoPointerReciver

it is in the scope of the rule?

3. The file-scoped nature of analysis in revive might make this rule not very effective, i.e. it will detect only few local cases.

[hbt]

2. Yes. Explicitly passing by reference with "&" when it is 100% unnecessary should be a code smell warning or potential bug (i.e did you mean to do X?)

3. Understood. I can see how the goal is to have high value linters to minimize compute time so people keep using revive in pre-commit hooks (majority of my usage).
   But there is also a case for using revive and other linters as a debugging tool. For example, I use errcheck to find unhandled or unwrapped errors when debugging or channel/loop issues or shadowed variables.
   In those cases, the ineffectiveness of the linter and compute time are less relevant since we are debugging anyway.
   Finally, a case for code simplification similarly to https://staticcheck.io/docs/checks#SA4001

Regardless, thank you for considering and thank you for the thoughtful reply.
Feel free to close it.