4 ms·

> Go interfaces generally belong in the package that consumes values of the interface type, not a package that implements the interface type. The implementing p

> Go interfaces generally belong in the package that consumes values of the interface type, not a package that implements the interface type. The implementing package should return concrete (usually pointer or struct) types. I like this rule. Most companies violate it everywhere. There are good times to ignore it but I always push for func NewThing To return something other than the interface type. The last Go interview I was in I brought this up and the argument was that you cant test things if you are not returning the interface and I disagree entirely. I love the power of duck typing with interfaces but give me concrete things that quack.

robmccoll 4y ago
Yep - return concrete types, but also make sure you have an assertion along the lines of: var _ IfaceType = &ConcreteType{} Somewhere, else you risk not knowing that a change to ConcreteType broke is implementation of IfaceType in a way that you might not know about until a rare runtime code path is executed.
- masklinn 4y ago
  > Somewhere, else you risk not knowing that a change to ConcreteType broke is implementation of IfaceType in a way that you might not know about until a rare runtime code path is executed. Why would runtime be involved? Surely if ConcreteType doesn't satisfy the interface anymore then the compiler will catch that at any site where a ConcreteType is being used as / cast to an IfaceType? Or are you talking about iface->iface side-cast?
  - jrockway 4y ago
    The compiler will catch it at the point of use, but it's clearer if the error is at the point of declaration. Consider: type FooReader struct{} var _ io.Reader = FooReader{} func (r FooReader) Raed(p []byte) (int, error) { return 0, nil } Before you even use FooReader as an io.Reader, you can find that you typo'd "Read". This is useful in practice because you probably wrote FooReader to have a bunch of other methods that aren't implementing io.Reader, and it's possible that your tests for this object don't actually ever use it as an io.Reader. So you won't notice this mistake until someone tries it elsewhere in the codebase. It also acts as documentation that you intend for this thing to be an io.Reader. But I also recommend that you document Read as "// Read implements io.Reader." to be extra explicit.
    - masklinn 4y ago
      Hah, how’s that for an endorsement of structurally typed interfaces.
masklinn 4y ago
That rule is not without its issues when pointers get involved, due to the typed nil problem.
- gregwebs 4y ago
  Yeah, this is a setup for hard to understand bugs unless you have a corresponding rule (imposed by a linter) that a concrete type cannot be cast into an interface and then checked against nil (mostly this means when returning a concrete type from a function it should be assigned to a freshly declared variable rather than re-using a variable that could already have an interface type.
  - Ferret7446 4y ago
    In practice this never happens unless you're violating other critical best practices like explicitly ignoring errors from constructors and trying to use the (nil) returned value anyway.
    - gregwebs 4y ago
      I think it doesn’t happen for errors because the error interface is almost always what is returned. It doesn’t seem that hard across if this style guide were followed: https://news.ycombinator.com/item?id=33566422 https://news.ycombinator.com/item?id=33566422
- Ferret7446 4y ago
  Typed nil isn't a problem, it indicates a crucial lack of understanding about interfaces, which should be resolved with education, not restricting good API design.
jrockway 4y ago
Interfaces are the most misused feature of Go. I think people like to prototype their API by typing in all the functions they intend to implement, so the interface acts as a template for their program that they can fill out. This, unfortunately, is not what they're for. I have a longer rant about this here: https://jrock.us/posts/go-interfaces/ https://jrock.us/posts/go-interfaces/
- mprime1 4y ago
  Thank you for sharing this. I was struggling with this just today: Realizing I keep putting off writing tests for some modules because mocking a certain mega interface is a pain. Your post gives me some great hints for things to try. Looking forward to experimenting with it.
  - jrockway 4y ago
    It took me a long time to figure out how to make small interfaces. I had an "aha!" moment not too long before writing that post. It seems bad, then it suddenly makes sense, and then the other way seems bad. Adding private members to my structs for testing was something the Go team forced me kicking and screaming to do when I worked at Google. I remember writing this adaptor for some internal network filesystem, and had an interface and two implementations: one that used the network for real, one that did everything in-memory for tests. I sent it off for code review and they were like "nope! don't do that in go!" and suggested an if statement in every method to handle the in-memory implementation. I was unhappy about it for months because it went against everything I knew about programming at the time, but like 10 years later, I appreciate that they were right. (I will say that everyone I explain this to has approximately the reaction that I had at the time. It is an easier pill to swallow when the person telling you not to do it wrote Go, but they don't have that benefit ;)
    - vrnvu 4y ago
      Really interesting approach. Thanks for sharing. Do you remember by any case where to find any real example? I'm digging into Golang's repositories trying to find one...
gouggoug 4y ago
Slightly unrelated to Parent: I struggle very hard creating my own interfaces for my own programs. I haven't found any literature online teaching the _right way_ of coming up with your interface. For example, I very often struggle with function parameters and return types: should my interface functions only take basic type and return basic types? Can my interface function take more concrete types? Should my interface function take interface types? I'd love to be directed to a guide on how to create a good interface. As an example, here's a recent interface declaration I wrote: type Checker interface { Check(context.Context, *object.Commit, bool) (bool, []string, error) Name() string Describe() string } My program runs through a list of commits and runs "Checks" on them. These checks "pass (true)" or "fail (false)". I want the user of my package to be in charge of the implementation details of a "Check", so I created the interface above. Is it OK for the Check() function to take a *object.Commit? Should I instead pass a `SHA string` and let the implementer figure out how to get a *object.Commit from that string?
- dpifke 4y ago
  Looking up *object.Commit from SHA string doesn't seem like something you want to make every Checker implementation do (even if it's mostly boilerplate), so passing a higher-level object makes sense here. What you might want to do is also define an interface for what Git calls "commit-ish"[0] objects, and use that in place of *object.Commit. Then you could pass e.g. *object.Tag in place of *object.Commit. Think of the interface as a contract between implementations and users of the data structure. Typically, you use an interface when multiple implementations are capable of fulfilling the same contract. If there's only one implementation, you likely don't need an interface—for instance, you can ignore my suggestion about abstracting commit-ish if you're pretty sure your checker would never be called for a tag. Interfaces are often useful when testing, because a mock implementation can fulfill that same contract. For instance, you might implement a *object.MockCommit which just uses static values instead of actually operating on a Git repository, and then pass that for your commit-ish in unit tests for various Checkers. [0] https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefcommit-ishacommit-ishalsocommittish https://git-scm.com/docs/gitglossary#Documentation/gitglossa...
dpifke 4y ago
I prefer to make the implementation package-private and hide it behind an interface if there's no way to make the implementation's zero value useful. Doing so prevents someone from trying to use it without calling the (mandatory) constructor.

robmccoll 4y ago

Yep - return concrete types, but also make sure you have an assertion along the lines of: var _ IfaceType = &ConcreteType{} Somewhere, else you risk not knowing that a change to ConcreteType broke is implementation of IfaceType in a way that you might not know about until a rare runtime code path is executed.

masklinn 4y ago
> Somewhere, else you risk not knowing that a change to ConcreteType broke is implementation of IfaceType in a way that you might not know about until a rare runtime code path is executed. Why would runtime be involved? Surely if ConcreteType doesn't satisfy the interface anymore then the compiler will catch that at any site where a ConcreteType is being used as / cast to an IfaceType? Or are you talking about iface->iface side-cast?
- jrockway 4y ago
  The compiler will catch it at the point of use, but it's clearer if the error is at the point of declaration. Consider: type FooReader struct{} var _ io.Reader = FooReader{} func (r FooReader) Raed(p []byte) (int, error) { return 0, nil } Before you even use FooReader as an io.Reader, you can find that you typo'd "Read". This is useful in practice because you probably wrote FooReader to have a bunch of other methods that aren't implementing io.Reader, and it's possible that your tests for this object don't actually ever use it as an io.Reader. So you won't notice this mistake until someone tries it elsewhere in the codebase. It also acts as documentation that you intend for this thing to be an io.Reader. But I also recommend that you document Read as "// Read implements io.Reader." to be extra explicit.
  - masklinn 4y ago
    Hah, how’s that for an endorsement of structurally typed interfaces.

masklinn 4y ago

That rule is not without its issues when pointers get involved, due to the typed nil problem.

gregwebs 4y ago
Yeah, this is a setup for hard to understand bugs unless you have a corresponding rule (imposed by a linter) that a concrete type cannot be cast into an interface and then checked against nil (mostly this means when returning a concrete type from a function it should be assigned to a freshly declared variable rather than re-using a variable that could already have an interface type.
- Ferret7446 4y ago
  In practice this never happens unless you're violating other critical best practices like explicitly ignoring errors from constructors and trying to use the (nil) returned value anyway.
  - gregwebs 4y ago
    I think it doesn’t happen for errors because the error interface is almost always what is returned. It doesn’t seem that hard across if this style guide were followed: https://news.ycombinator.com/item?id=33566422 https://news.ycombinator.com/item?id=33566422
Ferret7446 4y ago
Typed nil isn't a problem, it indicates a crucial lack of understanding about interfaces, which should be resolved with education, not restricting good API design.

jrockway 4y ago

Interfaces are the most misused feature of Go. I think people like to prototype their API by typing in all the functions they intend to implement, so the interface acts as a template for their program that they can fill out. This, unfortunately, is not what they're for. I have a longer rant about this here: https://jrock.us/posts/go-interfaces/ https://jrock.us/posts/go-interfaces/

mprime1 4y ago
Thank you for sharing this. I was struggling with this just today: Realizing I keep putting off writing tests for some modules because mocking a certain mega interface is a pain. Your post gives me some great hints for things to try. Looking forward to experimenting with it.
- jrockway 4y ago
  It took me a long time to figure out how to make small interfaces. I had an "aha!" moment not too long before writing that post. It seems bad, then it suddenly makes sense, and then the other way seems bad. Adding private members to my structs for testing was something the Go team forced me kicking and screaming to do when I worked at Google. I remember writing this adaptor for some internal network filesystem, and had an interface and two implementations: one that used the network for real, one that did everything in-memory for tests. I sent it off for code review and they were like "nope! don't do that in go!" and suggested an if statement in every method to handle the in-memory implementation. I was unhappy about it for months because it went against everything I knew about programming at the time, but like 10 years later, I appreciate that they were right. (I will say that everyone I explain this to has approximately the reaction that I had at the time. It is an easier pill to swallow when the person telling you not to do it wrote Go, but they don't have that benefit ;)
  - vrnvu 4y ago
    Really interesting approach. Thanks for sharing. Do you remember by any case where to find any real example? I'm digging into Golang's repositories trying to find one...

gouggoug 4y ago

Slightly unrelated to Parent: I struggle very hard creating my own interfaces for my own programs. I haven't found any literature online teaching the _right way_ of coming up with your interface. For example, I very often struggle with function parameters and return types: should my interface functions only take basic type and return basic types? Can my interface function take more concrete types? Should my interface function take interface types? I'd love to be directed to a guide on how to create a good interface. As an example, here's a recent interface declaration I wrote: type Checker interface { Check(context.Context, *object.Commit, bool) (bool, []string, error) Name() string Describe() string } My program runs through a list of commits and runs "Checks" on them. These checks "pass (true)" or "fail (false)". I want the user of my package to be in charge of the implementation details of a "Check", so I created the interface above. Is it OK for the Check() function to take a *object.Commit? Should I instead pass a `SHA string` and let the implementer figure out how to get a *object.Commit from that string?

dpifke 4y ago
Looking up *object.Commit from SHA string doesn't seem like something you want to make every Checker implementation do (even if it's mostly boilerplate), so passing a higher-level object makes sense here. What you might want to do is also define an interface for what Git calls "commit-ish"[0] objects, and use that in place of *object.Commit. Then you could pass e.g. *object.Tag in place of *object.Commit. Think of the interface as a contract between implementations and users of the data structure. Typically, you use an interface when multiple implementations are capable of fulfilling the same contract. If there's only one implementation, you likely don't need an interface—for instance, you can ignore my suggestion about abstracting commit-ish if you're pretty sure your checker would never be called for a tag. Interfaces are often useful when testing, because a mock implementation can fulfill that same contract. For instance, you might implement a *object.MockCommit which just uses static values instead of actually operating on a Git repository, and then pass that for your commit-ish in unit tests for various Checkers. [0] https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefcommit-ishacommit-ishalsocommittish https://git-scm.com/docs/gitglossary#Documentation/gitglossa...

dpifke 4y ago

I prefer to make the implementation package-private and hide it behind an interface if there's no way to make the implementation's zero value useful. Doing so prevents someone from trying to use it without calling the (mandatory) constructor.