Golang Code Style among TiDB Projects

#1

Recently a performance issue is located as unimplemented interface causing incorrect boolean result. The interface was introduced 5 years ago and the break change was introduced 1 year later.

Such subtle issue can hardly be found without defensive programming practice, which includes code style. Thus I’d like to start a discussion about the Golang Code Style among TiDB Projects; of course, for its Go projects such as tidb, parser, br, dm, ticdc, and so on.

Here is Uber Go Style Guide that could be a reference. But I think we should collect best practices and pain points from our developers, and the join force constructs our own guide.

Looking forward to your experience and suggestion.

4 Likes
#3

For the original issue, we can possibly write a guard to ensure a struct implements an interface.

var _ SomeInterface = &Instance{}

The remaining question is where to place this guard.

1 Like
#4

I think it does not matter much. Maybe where Instance is defined, maybe where SomeInterface is defined. I believe this kind of problems will appear again and again, more and more. We need more than “code style”, like a simple&clean session/runtime implementation…

#5

@xhebox what you figure out is another question about the modularity of projects. If you have a draft thoughts about which or what simple and clear session / runtime implementation should look like, you can create another topic and share the thoughts.

I agree it is not matter much but helps on review that we can spot and figure out potential bugs on problematic code. I would suggest the guard placed just above where the interface is implemented.

For example, you define a struct S as somewhere, and before implement T, write out

var _ T = &S{};
func (t *T) SFn0(...) ... {
  // ...
}

func (t *T) SFn1(...) ... {
  // ...
}

// ...
#6

IMO, it depends. For the interfaces that used internally by the current repo/project, it is fine to define them around the interface. But for those general enough to be used as a library(like Stringer), it is more suitable to define them in the implementation.