Don't warn about funcs that want to use a specific type
Closed this issue · 18 comments
For example:
func WriteToSomeStruct(s SomeStruct) {
b := make([]byte, 10)
s.Write(b)
}We shouldn't be recommending io.Writer here.
Just to clarify, that is if the type name appears in the function name - doesn't have to be at the end. Case insensitive.
Even if the function name doesn't include the struct name, but takes the argument by value, using an interface should not be recommended as that will cause an allocation.
So you're saying this is correct?
func Foo(s SomeStruct) {
s.Close()
}(ignoring the fact that this is closing a copy of the struct)
Yes.
Just because of the overhead of encapsulating the struct into an interface? I don't see why passing by value or by reference makes any difference in that aspect.
If you were to say that encapsulating as an interface is like passing by reference, then I do see why passing by value is very different and shouldn't trigger a warning. In which case I'll change the example from SomeStruct to *SomeStruct.
And open a separate issue to avoid those warnings, of course.
Yes, just because of the overhead. Encapsulating in an interface doesn't imply passing by reference (it depends if the value or the pointer implements the interface), but it results in an allocation either way.
I don't have a good source for this, but that's how it is.
Sure, it results in an allocation because that's how interfaces work in Go. But if we're to drop all of those, then this tool becomes rather useless because it would only suggest changes from interfaces to narrower interfaces. A huge part of the improvements this tool suggests (and which are valid) rely on struct types which sholud be interfaces.
I still don't see why you don't care about the overhead if the structs were passed by reference before.
Because if they're passed by reference then they're already on the heap,
and wrapping them in an interface won't allocate.
On Mar 8, 2016 10:38, "Daniel Martí" notifications@github.com wrote:
Sure, it results in an allocation because that's how interfaces work in
Go. But if we're to drop all of those, then this tool becomes rather
useless because it would only suggest changes from interfaces to narrower
interfaces. A huge part of the improvements this tool suggests (and which
are valid) rely on struct types which sholud be interfaces.I still don't see why you don't care about the overhead if the structs
were passed by reference before.—
Reply to this email directly or view it on GitHub
#17 (comment).
Ah, I get what you mean now. I thought if you passed a local struct variable as an interface type it would just use the pointer to that local variable. If what you say is true, then it probably makes sense to do this change.
@tamird I just opened #19 for the passing by value thing. If you have any suggestions, they'll be welcome :)
I also can't seem to find any reference to this copying-to-heap overhead when passing by reference. Would be useful to find it. Although if you're sure about it, I'll trust you since I myself had no idea.
Perhaps I don't fully understand, but even passing by reference causes this warning, e.g.:
// Produces: foo.go:77:22: fh can be io.Writer
func createCSVWriter(fh *os.File) *csv.Writer {
outputWriter := csv.NewWriter(fh)
outputWriter.Write([]string{
"...",
})
outputWriter.Flush()
return outputWriter
}So why is this not recommended.. ?
@Dynom I don't understand what you mean. That warning is expected and AFAIK is correct. This issue is about the warning not being shown e.g. if the func name was createCSVWriterFromFile.
Ah apologies, I figured the warning is incorrect.
Can you explain why my example is incorrect?
NewWriter() takes an io.Writer. All you're doing with fh is passing it to NewWriter(). Hence it can be an io.Writer.
So if I understand it correctly, the only reason Interfacer complains is because the function doesn't actually do anything specifically with os.File. I suppose that makes sense, although I don't think I necessarily agree with it.
I understand the simplification. However I expect the function to work with file handles, not writers in general, even though it might not reflect the current behaviour per se.
This is why I'm opening these issues, to detect when the developer really means *os.File and doesn't want io.Writer. What this issue suggests is for you to mention File in the function name somewhere.
What else would you do? I need blacklisting rules like these. Otherwise, interfacer would do nothing at all.
Makes total sense. I'll just play along for now see how I feel about it.
Thanks for your time and efforts! It's greatly appreciated and I did help me out a few times already 👍