From c10555a867cf1af48db5716c18e97b5ac6079797 Mon Sep 17 00:00:00 2001 From: "Matt T. Proud" Date: Tue, 30 Jan 2024 17:19:28 +0100 Subject: [PATCH] go: Export the latest version of internal guide. The updates chiefly include copy editing, deduplication of material, and additional considerations around documentation standards for APIs. --- go/best-practices.md | 195 ++++++++++++++++++++++++++++++++----------- go/decisions.md | 185 ++++++++++++++++------------------------ go/guide.md | 4 +- 3 files changed, 220 insertions(+), 164 deletions(-) diff --git a/go/best-practices.md b/go/best-practices.md index 46c6148..2e8457b 100644 --- a/go/best-practices.md +++ b/go/best-practices.md @@ -597,7 +597,11 @@ to have them in the same package. Code within a package can access unexported identifiers in the package. If you have a few related types whose *implementation* is tightly coupled, placing them in the same package lets you achieve this coupling without polluting the public -API with these details. +API with these details. A good test for this coupling is to imagine a +hypothetical user of two packages, where the packages cover closely related +topics: if the user must import both packages in order to use either in any +meaningful way, combining them together is usually the right thing to do. The +standard library generally demonstrates this kind of scoping and layering well. All of that being said, putting your entire project in a single package would likely make that package too large. When something is conceptually distinct, @@ -776,7 +780,7 @@ var ( ErrMarsupial = errors.New("marsupials are not supported") ) -func pet(animal Animal) error { +func process(animal Animal) error { switch { case seen[animal]: return ErrDuplicate @@ -849,6 +853,8 @@ to know if using status codes is the right choice. [`os.PathError`]: https://pkg.go.dev/os#PathError [`errors.Is`]: https://pkg.go.dev/errors#Is +[`errors.As`]: https://pkg.go.dev/errors#As +[`package cmp`]: https://pkg.go.dev/github.com/google/go-cmp/cmp [status]: https://pkg.go.dev/google.golang.org/grpc/status [canonical codes]: https://pkg.go.dev/google.golang.org/grpc/codes @@ -978,6 +984,10 @@ func (*FortuneTeller) SuggestFortune(context.Context, *pb.SuggestionRequest) (*p } ``` +See also: + +* [Error Documentation Conventions](#documentation-conventions-errors) + ### Placement of %w in errors @@ -1255,7 +1265,7 @@ information to the reader: // string. // // format is the format, and data is the interpolation data. -func Sprintf(format string, data ...interface{}) string +func Sprintf(format string, data ...any) string ``` However, this snippet demonstrates a code scenario similar to the previous where @@ -1272,7 +1282,7 @@ reader: // the format specification, the function will inline warnings about formatting // errors into the output string as described by the Format errors section // above. -func Sprintf(format string, data ...interface{}) string +func Sprintf(format string, data ...any) string ``` Consider your likely audience in choosing what to document and at what depth. @@ -1317,9 +1327,9 @@ func (Worker) Run(ctx context.Context) error ``` Where context behavior is different or non-obvious, it should be expressly -documented: +documented if any of the following are true. -* If the function returns an error other than `ctx.Err()` when the context is +* The function returns an error other than `ctx.Err()` when the context is cancelled: ```go @@ -1330,8 +1340,7 @@ documented: func (Worker) Run(ctx context.Context) error ``` -* If the function has other mechanisms that may interrupt it or affect - lifetime: +* The function has other mechanisms that may interrupt it or affect lifetime: ```go // Good: @@ -1347,7 +1356,7 @@ documented: func (Worker) Stop() ``` -* If the function has special expectations about context lifetime, lineage, or +* The function has special expectations about context lifetime, lineage, or attached values: ```go @@ -1394,9 +1403,9 @@ Similarly, the extra remark about concurrency can safely be removed here: func (*Buffer) Grow(n int) ``` -Documentation is strongly encouraged if: +Documentation is strongly encouraged if any of the following are true. -* it is unclear whether the operation is read-only or a mutating +* It is unclear whether the operation is read-only or mutating: ```go // Good: @@ -1411,7 +1420,7 @@ Documentation is strongly encouraged if: Why? A cache hit when looking up the key mutate a LRU cache internally. How this is implemented may not be obvious to all readers. -* synchronization is provided by API +* Synchronization is provided by the API: ```go // Good: @@ -1427,7 +1436,7 @@ Documentation is strongly encouraged if: **Note:** If the API is a type and the API provides synchronization in entirety, conventionally only the type definition documents the semantics. -* the API consumes user-implemented types of interfaces, and the interface's +* The API consumes user-implemented types of interfaces, and the interface's consumer has particular concurrency requirements: ```go @@ -1489,6 +1498,84 @@ If it is potentially unclear how to clean up the resources, explain how: func (c *Client) Get(url string) (resp *Response, err error) ``` +See also: + +* [GoTip #110: Don’t Mix Exit With Defer] + +[GoTip #110: Don’t Mix Exit With Defer]: https://google.github.io/styleguide/go/index.html#gotip + + + +#### Errors + +Document significant error sentinel values or error types that your functions +return to callers so that callers can anticipate what types of conditions they +can handle in their code. + +```go +// Good: +package os + +// Read reads up to len(b) bytes from the File and stores them in b. It returns +// the number of bytes read and any error encountered. +// +// At end of file, Read returns 0, io.EOF. +func (*File) Read(b []byte) (n int, err error) { +``` + +When a function returns a specific error type, correctly note whether the error +is a pointer receiver or not: + +```go +// Good: +package os + +type PathError struct { + Op string + Path string + Err error +} + +// Chdir changes the current working directory to the named directory. +// +// If there is an error, it will be of type *PathError. +func Chdir(dir string) error { +``` + +Documenting whether the values returned are pointer receivers enables callers to +correctly compare the errors using [`errors.Is`], [`errors.As`], and +[`package cmp`]. This is because a non-pointer value is not equivalent to a +pointer value. + +**Note:** In the `Chdir` example, the return type is written as `error` rather +than `*PathError` due to +[how nil interface values work](https://go.dev/doc/faq#nil_error). + +Document overall error conventions in the +[package's documentation](decisions#package-comments) when the behavior is +applicable to most errors found in the package: + +```go +// Good: +// Package os provides a platform-independent interface to operating system +// functionality. +// +// Often, more information is available within the error. For example, if a +// call that takes a file name fails, such as Open or Stat, the error will +// include the failing file name when printed and will be of type *PathError, +// which may be unpacked for more information. +package os +``` + +Thoughtful application of these approaches can add +[extra information to errors](#error-extra-info) without much effort and help +callers avoid adding redundant annotations. + +See also: + +* [Go Tip #106: Error Naming Conventions](https://google.github.io/styleguide/go/index.html#gotip) +* [Go Tip #89: When to Use Canonical Status Codes as Errors](https://google.github.io/styleguide/go/index.html#gotip) + ### Preview @@ -1944,7 +2031,7 @@ func foo(ctx context.Context) { } ``` -**Note**: [Contexts are never included in option structs](decisions#contexts). +**Note:** [Contexts are never included in option structs](decisions#contexts). This option is often preferred when some of the following apply: @@ -2411,7 +2498,7 @@ func ExerciseGame(t *testing.T, cfg *Config, p chess.Player) error { if cfg.Simulation == Modem { conn, err := modempool.Allocate() if err != nil { - t.Fatalf("no modem for the opponent could be provisioned: %v", err) + t.Fatalf("No modem for the opponent could be provisioned: %v", err) } t.Cleanup(func() { modempool.Return(conn) }) } @@ -2437,7 +2524,7 @@ func TestAcceptance(t *testing.T) { player := deepblue.New() err := chesstest.ExerciseGame(t, chesstest.SimpleGame, player) if err != nil { - t.Errorf("deepblue player failed acceptance test: %v", err) + t.Errorf("Deep Blue player failed acceptance test: %v", err) } } ``` @@ -2578,14 +2665,14 @@ func paint(color string) error { func badSetup(t *testing.T) { // This should call t.Helper, but doesn't. if err := paint("taupe"); err != nil { - t.Fatalf("could not paint the house under test: %v", err) // line 15 + t.Fatalf("Could not paint the house under test: %v", err) // line 15 } } func mustGoodSetup(t *testing.T) { t.Helper() if err := paint("lilac"); err != nil { - t.Fatalf("could not paint the house under test: %v", err) + t.Fatalf("Could not paint the house under test: %v", err) } } @@ -2605,10 +2692,10 @@ differs: ```text === RUN TestBad - paint_test.go:15: could not paint the house under test: no "taupe" paint today + paint_test.go:15: Could not paint the house under test: no "taupe" paint today --- FAIL: TestBad (0.00s) === RUN TestGood - paint_test.go:32: could not paint the house under test: no "lilac" paint today + paint_test.go:32: Could not paint the house under test: no "lilac" paint today --- FAIL: TestGood (0.00s) FAIL ``` @@ -2616,7 +2703,7 @@ FAIL The error with `paint_test.go:15` refers to the line of the setup function that failed in `badSetup`: -`t.Fatalf("could not paint the house under test: %v", err)` +`t.Fatalf("Could not paint the house under test: %v", err)` Whereas `paint_test.go:32` refers to the line of the test that failed in `TestGood`: @@ -2695,33 +2782,41 @@ and those should not depend on the system under test. Therefore, if a test helper [registers a fatal test failure](#test-helper-error-handling), it can and should do so from the test's goroutine. + + +### Use field names in struct literals + -### Use field labels for struct literals - -In table-driven tests, prefer to specify the key for each test case specified. -This is helpful when the test cases cover a large amount of vertical space (e.g. -more than 20-30 lines), when there are adjacent fields with the same type, and -also when you wish to omit fields which have the zero value. For example: +In table-driven tests, prefer to specify field names when initializing test case +struct literals. This is helpful when the test cases cover a large amount of +vertical space (e.g. more than 20-30 lines), when there are adjacent fields with +the same type, and also when you wish to omit fields which have the zero value. +For example: ```go // Good: -tests := []struct { - foo *pb.Foo - bar *pb.Bar - want string -}{ - { - foo: pb.Foo_builder{ - Name: "foo", - // ... - }.Build(), - bar: pb.Bar_builder{ - Name: "bar", - // ... - }.Build(), - want: "result", - }, +func TestStrJoin(t *testing.T) { + tests := []struct { + slice []string + separator string + skipEmpty bool + want string + }{ + { + slice: []string{"a", "b", ""}, + separator: ",", + want: "a,b,", + }, + { + slice: []string{"a", "b", ""}, + separator: ",", + skipEmpty: true, + want: "a,b", + }, + // ... + } + // ... } ``` @@ -2742,7 +2837,7 @@ func mustLoadDataset(t *testing.T) []byte { data, err := os.ReadFile("path/to/your/project/testdata/dataset") if err != nil { - t.Fatalf("could not load dataset: %v", err) + t.Fatalf("Could not load dataset: %v", err) } return data } @@ -2756,7 +2851,7 @@ func TestParseData(t *testing.T) { data := mustLoadDataset(t) parsed, err := ParseData(data) if err != nil { - t.Fatalf("unexpected error parsing data: %v", err) + t.Fatalf("Unexpected error parsing data: %v", err) } want := &DataTable{ /* ... */ } if got := parsed; !cmp.Equal(got, want) { @@ -2768,7 +2863,7 @@ func TestListContents(t *testing.T) { data := mustLoadDataset(t) contents, err := ListContents(data) if err != nil { - t.Fatalf("unexpected error listing contents: %v", err) + t.Fatalf("Unexpected error listing contents: %v", err) } want := []string{ /* ... */ } if got := contents; !cmp.Equal(got, want) { @@ -2916,7 +3011,7 @@ func mustLoadDataset(t *testing.T) []byte { dataset.err = err }) if err := dataset.err; err != nil { - t.Fatalf("could not load dataset: %v", err) + t.Fatalf("Could not load dataset: %v", err) } return dataset.data } @@ -2972,8 +3067,8 @@ guidance outlines when each method is preferred. ### Prefer "+" for simple cases -Prefer using "+" when concatenating few strings. This method is the -syntactically the simplest and requires no import. +Prefer using "+" when concatenating few strings. This method is syntactically +the simplest and requires no import. ```go // Good: @@ -3024,7 +3119,7 @@ for i, d := range digitsOfPi { str := b.String() ``` -**NOTE**: For more discussion, see +**Note:** For more discussion, see [GoTip #29: Building Strings Efficiently](https://google.github.io/styleguide/go/index.html#gotip). diff --git a/go/decisions.md b/go/decisions.md index 8c842e9..1968b56 100644 --- a/go/decisions.md +++ b/go/decisions.md @@ -897,9 +897,14 @@ import ( ) ``` -It is acceptable to split the project packages into multiple groups, for example -if you want a separate group for renamed, imported-only-for-side-effects or -another special group of imports. +It is acceptable to split the project packages into multiple groups if you want +a separate group, as long as the groups have some meaning. Common reasons to do +this: + +* renamed imports +* packages imported for their side-effects + +Example: ```go // Good: @@ -1273,14 +1278,19 @@ maintainable. #### Field names -Struct literals should usually specify **field names** for types defined outside -the current package. +Struct literals must specify **field names** for types defined outside the +current package. * Include field names for types from other packages. ```go // Good: - good := otherpkg.Type{A: 42} + // https://pkg.go.dev/encoding/csv#Reader + r := csv.Reader{ + Comma: ',', + Comment: '#', + FieldsPerRecord: 4, + } ``` The position of fields in a struct and the full set of fields (both of which @@ -1290,19 +1300,9 @@ the current package. ```go // Bad: - // https://pkg.go.dev/encoding/csv#Reader r := csv.Reader{',', '#', 4, false, false, false, false} ``` - Field names may be omitted within small, simple structs whose composition - and order are documented as being stable. - - ```go - // Good: - okay := image.Point{42, 54} - also := image.Point{X: 42, Y: 54} - ``` - * For package-local types, field names are optional. ```go @@ -1721,33 +1721,6 @@ func (r *SomeType) SomeLongFunctionName(foo1, foo2, foo3 string, See [best practices](best-practices#funcargs) for a few options for shortening the call sites of functions that would otherwise have many arguments. -```go -// Good: -good := foo.Call(long, CallOptions{ - Names: list, - Of: of, - The: parameters, - Func: all, - Args: on, - Now: separate, - Visible: lines, -}) -``` - -```go -// Bad: -bad := foo.Call( - long, - list, - of, - parameters, - all, - on, - separate, - lines, -) -``` - Lines can often be shortened by factoring out local variables. ```go @@ -1770,9 +1743,9 @@ bad := foo.Call(long, list, of, parameters, with, arbitrary, line, breaks) ``` -Do not add comments to specific function parameters. Instead, use an -[option struct](best-practices#option-structure) or add more detail to the -function documentation. +Avoid adding inline comments to specific function arguments where possible. +Instead, use an [option struct](best-practices#option-structure) or add more +detail to the function documentation. ```go // Good: @@ -1787,21 +1760,6 @@ bad := server.New( ) ``` -If call-sites are uncomfortably long, consider refactoring: - -```go -// Good: -// Sometimes variadic arguments can be factored out -replacements := []string{ - "from", "to", // related values can be formatted adjacent to one another - "source", "dest", - "original", "new", -} - -// Use the replacement struct as inputs to NewReplacer. -replacer := strings.NewReplacer(replacements...) -``` - If the API cannot be changed or if the local call is unusual (whether or not the call is too long), it is always permissible to add line breaks if it aids in understanding the call. @@ -2110,7 +2068,7 @@ exclusively at func MustParse(version string) *Version { v, err := Parse(version) if err != nil { - log.Fatalf("MustParse(%q) = _, %v", version, err) + panic(fmt.Sprintf("MustParse(%q) = _, %v", version, err)) } return v } @@ -2120,8 +2078,6 @@ func MustParse(version string) *Version { var DefaultVersion = MustParse("1.2.3") ``` -**Note:** `log.Fatalf` is not the standard library log. See [#logging]. - The same convention may be used in test helpers that only stop the current test (using `t.Fatal`). Such helpers are often convenient in creating test values, for example in struct fields of [table driven tests](#table-driven-tests), as @@ -2133,7 +2089,7 @@ func mustMarshalAny(t *testing.T, m proto.Message) *anypb.Any { t.Helper() any, err := anypb.New(m) if err != nil { - t.Fatalf("MustMarshalAny(t, m) = %v; want %v", err, nil) + t.Fatalf("mustMarshalAny(t, m) = %v; want %v", err, nil) } return any } @@ -2189,8 +2145,8 @@ func Version(o *servicepb.Object) (*version.Version, error) { When you spawn goroutines, make it clear when or whether they exit. Goroutines can leak by blocking on channel sends or receives. The garbage -collector will not terminate a goroutine even if the channels it is blocked on -are unreachable. +collector will not terminate a goroutine blocked on a channel even if no other +goroutine has a reference to the channel. Even when goroutines do not leak, leaving them in-flight when they are no longer needed can cause other subtle and hard-to-diagnose problems. Sending on a @@ -2764,11 +2720,11 @@ See also: ### Logging -Go programs in the Google codebase use a variant of the -[standard `log` package]. It has a similar but more powerful interface and -interoperates well with internal Google systems. An open source version of this -library is available as [package `glog`], and open source Google projects may -use that, but this guide refers to it as `log` throughout. +Go programs in the Google codebase use a variant of the standard [`log`] +package. It has a similar but more powerful interface and interoperates well +with internal Google systems. An open source version of this library is +available as [package `glog`], and open source Google projects may use that, but +this guide refers to it as `log` throughout. **Note:** For abnormal program exits, this library uses `log.Fatal` to abort with a stacktrace, and `log.Exit` to stop without one. There is no `log.Panic` @@ -2785,7 +2741,8 @@ See also: * When and how to use the log package to [stop the program](best-practices#checks-and-panics) -[standard `log` package]: https://pkg.go.dev/log +[`log`]: https://pkg.go.dev/log +[`log/slog`]: https://pkg.go.dev/log/slog [package `glog`]: https://pkg.go.dev/github.com/golang/glog [`log.Exit`]: https://pkg.go.dev/github.com/golang/glog#Exit [`log.Fatal`]: https://pkg.go.dev/github.com/golang/glog#Fatal @@ -2978,15 +2935,15 @@ right: // Bad: package assert -func IsNotNil(t *testing.T, name string, val interface{}) { +func IsNotNil(t *testing.T, name string, val any) { if val == nil { - t.Fatalf("data %s = nil, want not nil", name) + t.Fatalf("Data %s = nil, want not nil", name) } } func StringEq(t *testing.T, name, got, want string) { if got != want { - t.Fatalf("data %s = %q, want %q", name, got, want) + t.Fatalf("Data %s = %q, want %q", name, got, want) } } ``` @@ -3013,7 +2970,7 @@ want := BlogPost{ } if !cmp.Equal(got, want) { - t.Errorf("blog post = %v, want = %v", got, want) + t.Errorf("Blog post = %v, want = %v", got, want) } ``` @@ -3029,7 +2986,7 @@ func TestBlogPost_VeritableRant(t *testing.T) { post := BlogPost{Body: "I am Gunnery Sergeant Hartman, your senior drill instructor."} if got, want := postLength(post), 60; got != want { - t.Errorf("length of post = %v, want %v", got, want) + t.Errorf("Length of post = %v, want %v", got, want) } } ``` @@ -3361,7 +3318,8 @@ than relying on parsing the error message. Within unit tests, it is common to only care whether an error occurred or not. If so, then it is sufficient to only test whether the error was non-nil when you expected an error. If you would like to test that the error semantically matches -some other error, then consider using `cmp` with [`cmpopts.EquateErrors`]. +some other error, then consider using [`errors.Is`] or `cmp` with +[`cmpopts.EquateErrors`]. > **Note:** If a test uses [`cmpopts.EquateErrors`] but all of its `wantErr` > values are either `nil` or `cmpopts.AnyError`, then using `cmp` is @@ -3370,9 +3328,10 @@ some other error, then consider using `cmp` with [`cmpopts.EquateErrors`]. > > ```go > // Good: -> gotErr := f(test.input) != nil +> err := f(test.input) +> gotErr := err != nil > if gotErr != test.wantErr { -> t.Errorf("f(%q) returned err = %v, want error presence = %v", test.input, gotErr, test.wantErr) +> t.Errorf("f(%q) = %v, want error presence = %v", test.input, err, test.wantErr) > } > ``` @@ -3381,6 +3340,7 @@ See also [tott-350]: https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html [`cmpopts.EquateErrors`]: https://pkg.go.dev/github.com/google/go-cmp/cmp/cmpopts#EquateErrors +[`errors.Is`]: https://pkg.go.dev/errors#Is @@ -3471,6 +3431,9 @@ t.Run("check that there is no mention of scratched records or hovercrafts", ...) t.Run("AM/PM confusion", ...) ``` +See also +[Go Tip #117: Subtest Names](https://google.github.io/styleguide/go/index.html#gotip). + [Go test runner]: https://golang.org/cmd/go/#hdr-Testing_flags [identify the inputs]: #identify-the-input [special meaning for test filters]: https://blog.golang.org/subtests#:~:text=Perhaps%20a%20bit,match%20any%20tests @@ -3491,39 +3454,37 @@ similar testing logic. [tests of `fmt.Sprintf`]: https://cs.opensource.google/go/go/+/master:src/fmt/fmt_test.go [tests for `net.Dial`]: https://cs.opensource.google/go/go/+/master:src/net/dial_test.go;l=318;drc=5b606a9d2b7649532fe25794fa6b99bd24e7697c -Here is the minimal structure of a table-driven test, copied from the standard -`strings` library. If needed, you may use different names, move the test slice -into the test function, or add extra facilities such as subtests or setup and -cleanup functions. Always keep [useful test failures](#useful-test-failures) in -mind. +Here is the minimal structure of a table-driven test. If needed, you may use +different names or add extra facilities such as subtests or setup and cleanup +functions. Always keep [useful test failures](#useful-test-failures) in mind. ```go // Good: -var compareTests = []struct { - a, b string - i int -}{ - {"", "", 0}, - {"a", "", 1}, - {"", "a", -1}, - {"abc", "abc", 0}, - {"ab", "abc", -1}, - {"abc", "ab", 1}, - {"x", "ab", 1}, - {"ab", "x", -1}, - {"x", "a", 1}, - {"b", "x", -1}, - // test runtime·memeq's chunked implementation - {"abcdefgh", "abcdefgh", 0}, - {"abcdefghi", "abcdefghi", 0}, - {"abcdefghi", "abcdefghj", -1}, -} - func TestCompare(t *testing.T) { - for _, tt := range compareTests { - cmp := Compare(tt.a, tt.b) - if cmp != tt.i { - t.Errorf(`Compare(%q, %q) = %v`, tt.a, tt.b, cmp) + compareTests := []struct { + a, b string + want int + }{ + {"", "", 0}, + {"a", "", 1}, + {"", "a", -1}, + {"abc", "abc", 0}, + {"ab", "abc", -1}, + {"abc", "ab", 1}, + {"x", "ab", 1}, + {"ab", "x", -1}, + {"x", "a", 1}, + {"b", "x", -1}, + // test runtime·memeq's chunked implementation + {"abcdefgh", "abcdefgh", 0}, + {"abcdefghi", "abcdefghi", 0}, + {"abcdefghi", "abcdefghj", -1}, + } + + for _, test := range compareTests { + got := Compare(test.a, test.b) + if got != test.want { + t.Errorf("Compare(%q, %q) = %v, want %v", test.a, test.b, got, test.want) } } } @@ -3639,7 +3600,7 @@ func TestDecode(t *testing.T) { case prod: codex = setupCodex(t) default: - t.Fatalf("unknown codex type: %v", codex) + t.Fatalf("Unknown codex type: %v", codex) } output, err := Decode(test.input, codex) if got, want := output, test.output; got != want { @@ -3673,7 +3634,7 @@ tests := []struct { } for i, d := range tests { if strings.ToUpper(d.input) != d.want { - t.Errorf("failed on case #%d", i) + t.Errorf("Failed on case #%d", i) } } ``` diff --git a/go/guide.md b/go/guide.md index e051b09..961469f 100644 --- a/go/guide.md +++ b/go/guide.md @@ -419,8 +419,8 @@ initial capitalization. ### Line length -There is no fixed line length for Go source code. If a line feels too long, it -should be refactored instead of broken. If it is already as short as it is +There is no fixed line length for Go source code. If a line feels too long, +prefer refactoring instead of splitting it. If it is already as short as it is practical for it to be, the line should be allowed to remain long. Do not split a line: