From a9c6c3ea3619e2fd3f614da2b3af0614fd258e21 Mon Sep 17 00:00:00 2001 From: upstream-mirror Date: Tue, 15 Sep 2026 21:54:05 +0500 Subject: [PATCH] snapshot of https://github.com/uber-go/guide.git (master, 2026-09-15; blobs > 131072B omitted) --- .github/dependabot.yml | 11 + .github/workflows/ci.yml | 40 + .gitignore | 1 + .golangci.yml | 19 + CHANGELOG.md | 80 + CODE_OF_CONDUCT.md | 76 + CONTRIBUTING.md | 86 + LICENSE | 176 ++ Makefile | 27 + README.md | 30 + src/README.md | 2 + src/SUMMARY.md | 66 + src/atomic.md | 57 + src/builtin-name.md | 93 + src/channel-size.md | 29 + src/consistency.md | 19 + src/container-capacity.md | 119 + src/container-copy.md | 106 + src/decl-group.md | 178 ++ src/defer-clean.md | 49 + src/else-unnecessary.md | 30 + src/embed-public.md | 132 ++ src/enum-start.md | 56 + src/error-name.md | 52 + src/error-once.md | 111 + src/error-type.md | 137 ++ src/error-wrap.md | 79 + src/exit-main.md | 76 + src/exit-once.md | 117 + src/function-name.md | 8 + src/function-order.md | 55 + src/functional-option.md | 157 ++ src/global-decl.md | 42 + src/global-mut.md | 76 + src/global-name.md | 50 + src/goroutine-exit.md | 32 + src/goroutine-forget.md | 79 + src/goroutine-init.md | 78 + src/import-alias.md | 46 + src/import-group.md | 37 + src/init.md | 116 + src/interface-compliance.md | 72 + src/interface-pointer.md | 14 + src/interface-receiver.md | 78 + src/intro.md | 37 + src/line-length.md | 9 + src/lint.md | 39 + src/map-init.md | 80 + src/mutex-zero-value.md | 91 + src/nest-less.md | 45 + src/package-name.md | 15 + src/panic.md | 87 + src/param-naked.md | 49 + src/performance.md | 3 + src/preface.txt | 7 + src/printf-const.md | 26 + src/printf-name.md | 22 + src/slice-nil.md | 95 + src/strconv.md | 39 + src/string-byte-slice.md | 40 + src/string-escape.md | 23 + src/struct-embed.md | 156 ++ src/struct-field-key.md | 41 + src/struct-field-zero.md | 48 + src/struct-pointer.md | 28 + src/struct-tag.md | 47 + src/struct-zero.md | 28 + src/test-table.md | 267 +++ src/time.md | 168 ++ src/type-assert.md | 30 + src/var-decl.md | 59 + src/var-scope.md | 102 + style.md | 4107 +++++++++++++++++++++++++++++++++++ 73 files changed, 8782 insertions(+) create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/ci.yml create mode 100644 .gitignore create mode 100644 .golangci.yml create mode 100644 CHANGELOG.md create mode 100644 CODE_OF_CONDUCT.md create mode 100644 CONTRIBUTING.md create mode 100644 LICENSE create mode 100644 Makefile create mode 100644 README.md create mode 100644 src/README.md create mode 100644 src/SUMMARY.md create mode 100644 src/atomic.md create mode 100644 src/builtin-name.md create mode 100644 src/channel-size.md create mode 100644 src/consistency.md create mode 100644 src/container-capacity.md create mode 100644 src/container-copy.md create mode 100644 src/decl-group.md create mode 100644 src/defer-clean.md create mode 100644 src/else-unnecessary.md create mode 100644 src/embed-public.md create mode 100644 src/enum-start.md create mode 100644 src/error-name.md create mode 100644 src/error-once.md create mode 100644 src/error-type.md create mode 100644 src/error-wrap.md create mode 100644 src/exit-main.md create mode 100644 src/exit-once.md create mode 100644 src/function-name.md create mode 100644 src/function-order.md create mode 100644 src/functional-option.md create mode 100644 src/global-decl.md create mode 100644 src/global-mut.md create mode 100644 src/global-name.md create mode 100644 src/goroutine-exit.md create mode 100644 src/goroutine-forget.md create mode 100644 src/goroutine-init.md create mode 100644 src/import-alias.md create mode 100644 src/import-group.md create mode 100644 src/init.md create mode 100644 src/interface-compliance.md create mode 100644 src/interface-pointer.md create mode 100644 src/interface-receiver.md create mode 100644 src/intro.md create mode 100644 src/line-length.md create mode 100644 src/lint.md create mode 100644 src/map-init.md create mode 100644 src/mutex-zero-value.md create mode 100644 src/nest-less.md create mode 100644 src/package-name.md create mode 100644 src/panic.md create mode 100644 src/param-naked.md create mode 100644 src/performance.md create mode 100644 src/preface.txt create mode 100644 src/printf-const.md create mode 100644 src/printf-name.md create mode 100644 src/slice-nil.md create mode 100644 src/strconv.md create mode 100644 src/string-byte-slice.md create mode 100644 src/string-escape.md create mode 100644 src/struct-embed.md create mode 100644 src/struct-field-key.md create mode 100644 src/struct-field-zero.md create mode 100644 src/struct-pointer.md create mode 100644 src/struct-tag.md create mode 100644 src/struct-zero.md create mode 100644 src/test-table.md create mode 100644 src/time.md create mode 100644 src/type-assert.md create mode 100644 src/var-decl.md create mode 100644 src/var-scope.md create mode 100644 style.md diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..547aaca --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,11 @@ +version: 2 +updates: + - package-ecosystem: "gomod" + directory: "/tools" + schedule: + interval: "daily" + + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..11e726a --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,40 @@ +name: CI + +on: + push: + branches: [ main ] + pull_request: + branches: [ '*' ] + +jobs: + stitchmd: + name: Check or update style.md + runs-on: ubuntu-latest + + # Needed to give the job permission + # to push to branches. + permissions: + contents: write + + steps: + - name: Check out repository + uses: actions/checkout@v4 + + - name: Check or update style.md + uses: abhinav/stitchmd-action@v1 + with: + # For pull requests, run in 'write' mode so that edits made + # directly in the GitHub UI get propagated to style.md + # without a local checkout. + # + # Otherwise, run in 'check' mode to fail CI if style.md is out-of-date. + mode: ${{ github.event_name == 'pull_request' && 'write' || 'check' }} + summary: src/SUMMARY.md + preface: src/preface.txt + output: style.md + + - uses: stefanzweifel/git-auto-commit-action@v5 + if: ${{ github.event_name == 'pull_request' }} + with: + file_pattern: style.md + commit_message: 'Auto-update style.md' diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..5e56e04 --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +/bin diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..823ab97 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,19 @@ +# Refer to golangci-lint's example config file for more options and information: +# https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml + +run: + timeout: 5m + modules-download-mode: readonly + +linters: + enable: + - errcheck + - goimports + - golint + - govet + - staticcheck + +issues: + exclude-use-default: false + max-issues-per-linter: 0 + max-same-issues: 0 diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..88442c1 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,80 @@ +# 2023-05-09 + +- Test tables: Discourage tables with complex, branching test bodies. + +# 2023-04-13 + +- Errors: Add guidance on handling errors only once. + +# 2023-03-03 + +- Receivers and Interfaces: Clarify subtlety with pointer receivers and values. + +# 2022-10-18 + +- Add guidance on managing goroutine lifecycles. + +# 2022-03-30 + +- Add guidance on using field tags in marshaled structs. + +# 2021-11-16 + +- Add guidance on use of `%w` vs `%v` with `fmt.Errorf`, and where to use + `errors.New` or custom error types. + +# 2021-11-12 + +- Add soft line length limit of 99 characters. + +# 2021-04-19 + +- Programs should exit only in `main()`, preferably at most once. + +# 2021-03-15 + +- Add guidance on omitting zero-value fields during struct initialization. +- Add guidance on `Foo{}` versus `var` form for initialization of empty + structs. +- Add new section for Initializing Structs, moving relevant guidances into + subsections of it. + +# 2020-06-10 + +- Add guidance on avoiding `init()`. +- Add guidance to avoid using built-in names. +- Add reminder that nil slices are not always the same as empty slices. + +# 2020-02-24 + +- Add guidance on verifying interface compliance with compile-time checks. + +# 2020-01-30 + +- Recommend using the `time` package when dealing with time. + +# 2020-01-25 + +- Add guidance against embedding types in public structs. + +# 2019-12-17 + +- Functional Options: Recommend struct implementations of `Option` interface + instead of capturing values with a closure. + +# 2019-11-26 + +- Add guidance against mutating global variables. + +# 2019-10-21 + +- Add section on remaining consistent with existing practices. +- Add guidance on map initialization and size hints. + +# 2019-10-11 + +- Suggest succinct context for error messages. + +# 2019-10-10 + +- Initial release. diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md new file mode 100644 index 0000000..7b84771 --- /dev/null +++ b/CODE_OF_CONDUCT.md @@ -0,0 +1,76 @@ +# Contributor Covenant Code of Conduct + +## Our Pledge + +In the interest of fostering an open and welcoming environment, we as +contributors and maintainers pledge to making participation in our project and +our community a harassment-free experience for everyone, regardless of age, body +size, disability, ethnicity, sex characteristics, gender identity and expression, +level of experience, education, socio-economic status, nationality, personal +appearance, race, religion, or sexual identity and orientation. + +## Our Standards + +Examples of behavior that contributes to creating a positive environment +include: + +* Using welcoming and inclusive language +* Being respectful of differing viewpoints and experiences +* Gracefully accepting constructive criticism +* Focusing on what is best for the community +* Showing empathy towards other community members + +Examples of unacceptable behavior by participants include: + +* The use of sexualized language or imagery and unwelcome sexual attention or + advances +* Trolling, insulting/derogatory comments, and personal or political attacks +* Public or private harassment +* Publishing others' private information, such as a physical or electronic + address, without explicit permission +* Other conduct which could reasonably be considered inappropriate in a + professional setting + +## Our Responsibilities + +Project maintainers are responsible for clarifying the standards of acceptable +behavior and are expected to take appropriate and fair corrective action in +response to any instances of unacceptable behavior. + +Project maintainers have the right and responsibility to remove, edit, or +reject comments, commits, code, wiki edits, issues, and other contributions +that are not aligned to this Code of Conduct, or to ban temporarily or +permanently any contributor for other behaviors that they deem inappropriate, +threatening, offensive, or harmful. + +## Scope + +This Code of Conduct applies both within project spaces and in public spaces +when an individual is representing the project or its community. Examples of +representing a project or community include using an official project e-mail +address, posting via an official social media account, or acting as an appointed +representative at an online or offline event. Representation of a project may be +further defined and clarified by project maintainers. + +## Enforcement + +Instances of abusive, harassing, or otherwise unacceptable behavior may be +reported by contacting the project team at oss-conduct@uber.com. All +complaints will be reviewed and investigated and will result in a response that +is deemed necessary and appropriate to the circumstances. The project team is +obligated to maintain confidentiality with regard to the reporter of an incident. +Further details of specific enforcement policies may be posted separately. + +Project maintainers who do not follow or enforce the Code of Conduct in good +faith may face temporary or permanent repercussions as determined by other +members of the project's leadership. + +## Attribution + +This Code of Conduct is adapted from the [Contributor Covenant][homepage], version 1.4, +available at https://www.contributor-covenant.org/version/1/4/code-of-conduct.html + +[homepage]: https://www.contributor-covenant.org + +For answers to common questions about this code of conduct, see +https://www.contributor-covenant.org/faq diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..9dfd17c --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,86 @@ +# Contributing + +Before making any changes, +please discuss your plans on GitHub +and get agreement on the general direction of the change. + +## Making changes + +- style.md is generated from the contents of the src/ folder. + All changes must be made to files in the src/ folder. +- For new entries, create a new file with a short name + (see [File names](#file-names)) and add it to [SUMMARY.md](src/SUMMARY.md). + The file must have a single level 1 heading and any number of subsections. +- Use tables for side-by-side code samples. +- Link to other sections with their file names (`[..](foo.md)`). + +## Updating style.md + +`style.md` is generated from the contents of `src/` using [stitchmd](https://github.com/abhinav/stitchmd). +If CI fails because `style.md` is out of date, regenerate it by running: + +```bash +make +``` + +This will install `stitchmd` if needed and update `style.md`. +Commit the result and push it to your branch. + +## Writing style + +### Line breaks + +Use [semantic line breaks](https://sembr.org/) in your writing. +This keeps the Markdown files easily reviewable and editable. + +### File names + +Files in src/ follow a rough naming convention of: + + {subject}-{desc}.md + +Where `{subject}` is the **singular form** of subject that the entry is about +(e.g `string`, `struct`, `time`, `var`, `error`) +and `{desc}` is a short one or two word description of the topic. +For subjects where their name is enough, the `-{desc}` may be omitted. + +### Code samples + +Use two spaces to indent code samples. +Horizontal space is limited in side-by-side samples. + +### Side-by-side samples + +Create side-by-side code samples with the following template: + +~~~ + + + + +
BadGood
+ +```go +BAD CODE GOES HERE +``` + + + +```go +GOOD CODE GOES HERE +``` + +
+~~~ + +The empty lines between the HTML tags and code samples are necessary. + +If you need to add labels or descriptions below the code samples, +add another row before the `` line. + +~~~ + +DESCRIBE BAD CODE +DESCRIBE GOOD CODE + +~~~ diff --git a/LICENSE b/LICENSE new file mode 100644 index 0000000..d9a10c0 --- /dev/null +++ b/LICENSE @@ -0,0 +1,176 @@ + Apache License + Version 2.0, January 2004 + http://www.apache.org/licenses/ + + TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION + + 1. Definitions. + + "License" shall mean the terms and conditions for use, reproduction, + and distribution as defined by Sections 1 through 9 of this document. + + "Licensor" shall mean the copyright owner or entity authorized by + the copyright owner that is granting the License. + + "Legal Entity" shall mean the union of the acting entity and all + other entities that control, are controlled by, or are under common + control with that entity. For the purposes of this definition, + "control" means (i) the power, direct or indirect, to cause the + direction or management of such entity, whether by contract or + otherwise, or (ii) ownership of fifty percent (50%) or more of the + outstanding shares, or (iii) beneficial ownership of such entity. + + "You" (or "Your") shall mean an individual or Legal Entity + exercising permissions granted by this License. + + "Source" form shall mean the preferred form for making modifications, + including but not limited to software source code, documentation + source, and configuration files. + + "Object" form shall mean any form resulting from mechanical + transformation or translation of a Source form, including but + not limited to compiled object code, generated documentation, + and conversions to other media types. + + "Work" shall mean the work of authorship, whether in Source or + Object form, made available under the License, as indicated by a + copyright notice that is included in or attached to the work + (an example is provided in the Appendix below). + + "Derivative Works" shall mean any work, whether in Source or Object + form, that is based on (or derived from) the Work and for which the + editorial revisions, annotations, elaborations, or other modifications + represent, as a whole, an original work of authorship. For the purposes + of this License, Derivative Works shall not include works that remain + separable from, or merely link (or bind by name) to the interfaces of, + the Work and Derivative Works thereof. + + "Contribution" shall mean any work of authorship, including + the original version of the Work and any modifications or additions + to that Work or Derivative Works thereof, that is intentionally + submitted to Licensor for inclusion in the Work by the copyright owner + or by an individual or Legal Entity authorized to submit on behalf of + the copyright owner. For the purposes of this definition, "submitted" + means any form of electronic, verbal, or written communication sent + to the Licensor or its representatives, including but not limited to + communication on electronic mailing lists, source code control systems, + and issue tracking systems that are managed by, or on behalf of, the + Licensor for the purpose of discussing and improving the Work, but + excluding communication that is conspicuously marked or otherwise + designated in writing by the copyright owner as "Not a Contribution." + + "Contributor" shall mean Licensor and any individual or Legal Entity + on behalf of whom a Contribution has been received by Licensor and + subsequently incorporated within the Work. + + 2. Grant of Copyright License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + copyright license to reproduce, prepare Derivative Works of, + publicly display, publicly perform, sublicense, and distribute the + Work and such Derivative Works in Source or Object form. + + 3. Grant of Patent License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + (except as stated in this section) patent license to make, have made, + use, offer to sell, sell, import, and otherwise transfer the Work, + where such license applies only to those patent claims licensable + by such Contributor that are necessarily infringed by their + Contribution(s) alone or by combination of their Contribution(s) + with the Work to which such Contribution(s) was submitted. If You + institute patent litigation against any entity (including a + cross-claim or counterclaim in a lawsuit) alleging that the Work + or a Contribution incorporated within the Work constitutes direct + or contributory patent infringement, then any patent licenses + granted to You under this License for that Work shall terminate + as of the date such litigation is filed. + + 4. Redistribution. You may reproduce and distribute copies of the + Work or Derivative Works thereof in any medium, with or without + modifications, and in Source or Object form, provided that You + meet the following conditions: + + (a) You must give any other recipients of the Work or + Derivative Works a copy of this License; and + + (b) You must cause any modified files to carry prominent notices + stating that You changed the files; and + + (c) You must retain, in the Source form of any Derivative Works + that You distribute, all copyright, patent, trademark, and + attribution notices from the Source form of the Work, + excluding those notices that do not pertain to any part of + the Derivative Works; and + + (d) If the Work includes a "NOTICE" text file as part of its + distribution, then any Derivative Works that You distribute must + include a readable copy of the attribution notices contained + within such NOTICE file, excluding those notices that do not + pertain to any part of the Derivative Works, in at least one + of the following places: within a NOTICE text file distributed + as part of the Derivative Works; within the Source form or + documentation, if provided along with the Derivative Works; or, + within a display generated by the Derivative Works, if and + wherever such third-party notices normally appear. The contents + of the NOTICE file are for informational purposes only and + do not modify the License. You may add Your own attribution + notices within Derivative Works that You distribute, alongside + or as an addendum to the NOTICE text from the Work, provided + that such additional attribution notices cannot be construed + as modifying the License. + + You may add Your own copyright statement to Your modifications and + may provide additional or different license terms and conditions + for use, reproduction, or distribution of Your modifications, or + for any such Derivative Works as a whole, provided Your use, + reproduction, and distribution of the Work otherwise complies with + the conditions stated in this License. + + 5. Submission of Contributions. Unless You explicitly state otherwise, + any Contribution intentionally submitted for inclusion in the Work + by You to the Licensor shall be under the terms and conditions of + this License, without any additional terms or conditions. + Notwithstanding the above, nothing herein shall supersede or modify + the terms of any separate license agreement you may have executed + with Licensor regarding such Contributions. + + 6. Trademarks. This License does not grant permission to use the trade + names, trademarks, service marks, or product names of the Licensor, + except as required for reasonable and customary use in describing the + origin of the Work and reproducing the content of the NOTICE file. + + 7. Disclaimer of Warranty. Unless required by applicable law or + agreed to in writing, Licensor provides the Work (and each + Contributor provides its Contributions) on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + implied, including, without limitation, any warranties or conditions + of TITLE, NON-INFRINGEMENT, MERCHANTABILITY, or FITNESS FOR A + PARTICULAR PURPOSE. You are solely responsible for determining the + appropriateness of using or redistributing the Work and assume any + risks associated with Your exercise of permissions under this License. + + 8. Limitation of Liability. In no event and under no legal theory, + whether in tort (including negligence), contract, or otherwise, + unless required by applicable law (such as deliberate and grossly + negligent acts) or agreed to in writing, shall any Contributor be + liable to You for damages, including any direct, indirect, special, + incidental, or consequential damages of any character arising as a + result of this License or out of the use or inability to use the + Work (including but not limited to damages for loss of goodwill, + work stoppage, computer failure or malfunction, or any and all + other commercial damages or losses), even if such Contributor + has been advised of the possibility of such damages. + + 9. Accepting Warranty or Additional Liability. While redistributing + the Work or Derivative Works thereof, You may choose to offer, + and charge a fee for, acceptance of support, warranty, indemnity, + or other liability obligations and/or rights consistent with this + License. However, in accepting such obligations, You may act only + on Your own behalf and on Your sole responsibility, not on behalf + of any other Contributor, and only if You agree to indemnify, + defend, and hold each Contributor harmless for any liability + incurred by, or claims asserted against, such Contributor by reason + of your accepting any such warranty or additional liability. + + END OF TERMS AND CONDITIONS diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..71263df --- /dev/null +++ b/Makefile @@ -0,0 +1,27 @@ +SHELL = /bin/bash + +# Setting GOBIN makes 'go install' put the binary in the bin/ directory. +export GOBIN ?= $(dir $(abspath $(lastword $(MAKEFILE_LIST))))/bin + +STITCHMD = $(GOBIN)/stitchmd + +# Keep these options in-sync with .github/workflows/ci.yml. +STITCHMD_ARGS = -o style.md -preface src/preface.txt src/SUMMARY.md + +.PHONY: all +all: style.md + +.PHONY: lint +lint: $(STITCHMD) + @DIFF=$$($(STITCHMD) -d $(STITCHMD_ARGS)); \ + if [[ -n "$$DIFF" ]]; then \ + echo "style.md is out of date:"; \ + echo "$$DIFF"; \ + false; \ + fi + +style.md: $(STITCHMD) $(wildcard src/*) + $(STITCHMD) $(STITCHMD_ARGS) + +$(STITCHMD): + go install go.abhg.dev/stitchmd@latest diff --git a/README.md b/README.md new file mode 100644 index 0000000..1d1ccd2 --- /dev/null +++ b/README.md @@ -0,0 +1,30 @@ +This repository holds the [Uber Go Style Guide](style.md), which documents +patterns and conventions used in Go code at Uber. + +## Style Guide + +See [Uber Go Style Guide](style.md) for the style guide. + +## Translations + +We are aware of the following translations of this guide by the Go community. + +- **中文翻译** (Chinese): [xxjwxc/uber_go_guide_cn](https://github.com/xxjwxc/uber_go_guide_cn) +- **繁體中文** （Traditional Chinese）：[ianchen0119/uber_go_guide_tw](https://github.com/ianchen0119/uber_go_guide_tw) +- **한국어 번역** (Korean): [TangoEnSkai/uber-go-style-guide-kr](https://github.com/TangoEnSkai/uber-go-style-guide-kr) +- **日本語訳** (Japanese): [knsh14/uber-style-guide-ja](https://github.com/knsh14/uber-style-guide-ja) +- **Traducción al Español** (Spanish): [friendsofgo/uber-go-guide-es](https://github.com/friendsofgo/uber-go-guide-es) +- **แปลภาษาไทย** (Thai): [pallat/uber-go-style-guide-th](https://github.com/pallat/uber-go-style-guide-th) +- **Tradução em português** (Portuguese): [lucassscaravelli/uber-go-guide-pt](https://github.com/lucassscaravelli/uber-go-guide-pt) +- **Tradução em português** (Portuguese BR): [alcir-junior-caju/uber-go-style-guide-pt-br](https://github.com/alcir-junior-caju/uber-go-style-guide-pt-br) +- **Tłumaczenie polskie** (Polish): [DamianSkrzypczak/uber-go-guide-pl](https://github.com/DamianSkrzypczak/uber-go-guide-pl) +- **Русский перевод** (Russian): [alekarah/uber-go-guide-ru](https://github.com/alekarah/uber-go-guide-ru) +- **Français** (French): [rm3l/uber-go-style-guide-fr](https://github.com/rm3l/uber-go-style-guide-fr) +- **Türkçe** (Turkish): [ksckaan1/uber-go-style-guide-tr](https://github.com/ksckaan1/uber-go-style-guide-tr) +- **Український переклад** (Ukrainian): [vorobeyme/uber-go-style-guide-uk](https://github.com/vorobeyme/uber-go-style-guide-uk) +- **ترجمه فارسی** (Persian): [jamalkaksouri/uber-go-guide-ir](https://github.com/jamalkaksouri/uber-go-guide-ir) +- **Tiếng việt** (Vietnamese): [nc-minh/uber-go-guide-vi](https://github.com/nc-minh/uber-go-guide-vi) +- **العربية** (Arabic): [anqorithm/uber-go-guide-ar](https://github.com/anqorithm/uber-go-guide-ar) +- **Bahasa Indonesia** (Indonesian): [stanleydv12/uber-go-guide-id](https://github.com/stanleydv12/uber-go-guide-id) + +If you have a translation, feel free to submit a PR adding it to the list. diff --git a/src/README.md b/src/README.md new file mode 100644 index 0000000..c867bbd --- /dev/null +++ b/src/README.md @@ -0,0 +1,2 @@ +The contents of this directory are used to generate the top-level style.md. +The layout is controlled by SUMMARY.md. diff --git a/src/SUMMARY.md b/src/SUMMARY.md new file mode 100644 index 0000000..6ea2044 --- /dev/null +++ b/src/SUMMARY.md @@ -0,0 +1,66 @@ +# Uber Go Style Guide + +- [Introduction](intro.md) +- Guidelines + - [Pointers to Interfaces](interface-pointer.md) + - [Verify Interface Compliance](interface-compliance.md) + - [Receivers and Interfaces](interface-receiver.md) + - [Zero-value Mutexes are Valid](mutex-zero-value.md) + - [Copy Slices and Maps at Boundaries](container-copy.md) + - [Defer to Clean Up](defer-clean.md) + - [Channel Size is One or None](channel-size.md) + - [Start Enums at One](enum-start.md) + - [Use `"time"` to handle time](time.md) + - Errors + - [Error Types](error-type.md) + - [Error Wrapping](error-wrap.md) + - [Error Naming](error-name.md) + - [Handle Errors Once](error-once.md) + - [Handle Type Assertion Failures](type-assert.md) + - [Don't Panic](panic.md) + - [Use go.uber.org/atomic](atomic.md) + - [Avoid Mutable Globals](global-mut.md) + - [Avoid Embedding Types in Public Structs](embed-public.md) + - [Avoid Using Built-In Names](builtin-name.md) + - [Avoid `init()`](init.md) + - [Exit in Main](exit-main.md) + - [Exit Once](exit-once.md) + - [Use field tags in marshaled structs](struct-tag.md) + - [Don't fire-and-forget goroutines](goroutine-forget.md) + - [Wait for goroutines to exit](goroutine-exit.md) + - [No goroutines in `init()`](goroutine-init.md) +- [Performance](performance.md) + - [Prefer strconv over fmt](strconv.md) + - [Avoid repeated string-to-byte conversions](string-byte-slice.md) + - [Prefer Specifying Container Capacity](container-capacity.md) +- Style + - [Avoid overly long lines](line-length.md) + - [Be Consistent](consistency.md) + - [Group Similar Declarations](decl-group.md) + - [Import Group Ordering](import-group.md) + - [Package Names](package-name.md) + - [Function Names](function-name.md) + - [Import Aliasing](import-alias.md) + - [Function Grouping and Ordering](function-order.md) + - [Reduce Nesting](nest-less.md) + - [Unnecessary Else](else-unnecessary.md) + - [Top-level Variable Declarations](global-decl.md) + - [Prefix Unexported Globals with _](global-name.md) + - [Embedding in Structs](struct-embed.md) + - [Local Variable Declarations](var-decl.md) + - [nil is a valid slice](slice-nil.md) + - [Reduce Scope of Variables](var-scope.md) + - [Avoid Naked Parameters](param-naked.md) + - [Use Raw String Literals to Avoid Escaping](string-escape.md) + - Initializing Structs + - [Use Field Names to Initialize Structs](struct-field-key.md) + - [Omit Zero Value Fields in Structs](struct-field-zero.md) + - [Use `var` for Zero Value Structs](struct-zero.md) + - [Initializing Struct References](struct-pointer.md) + - [Initializing Maps](map-init.md) + - [Format Strings outside Printf](printf-const.md) + - [Naming Printf-style Functions](printf-name.md) +- Patterns + - [Test Tables](test-table.md) + - [Functional Options](functional-option.md) +- [Linting](lint.md) diff --git a/src/atomic.md b/src/atomic.md new file mode 100644 index 0000000..9a72a8b --- /dev/null +++ b/src/atomic.md @@ -0,0 +1,57 @@ +# Use go.uber.org/atomic + +Atomic operations with the [sync/atomic] package operate on the raw types +(`int32`, `int64`, etc.) so it is easy to forget to use the atomic operation to +read or modify the variables. + +[go.uber.org/atomic] adds type safety to these operations by hiding the +underlying type. Additionally, it includes a convenient `atomic.Bool` type. + + [go.uber.org/atomic]: https://pkg.go.dev/go.uber.org/atomic + [sync/atomic]: https://pkg.go.dev/sync/atomic + + + + + +
BadGood
+ +```go +type foo struct { + running int32 // atomic +} + +func (f *foo) start() { + if atomic.SwapInt32(&f.running, 1) == 1 { + // already running… + return + } + // start the Foo +} + +func (f *foo) isRunning() bool { + return f.running == 1 // race! +} +``` + + + +```go +type foo struct { + running atomic.Bool +} + +func (f *foo) start() { + if f.running.Swap(true) { + // already running… + return + } + // start the Foo +} + +func (f *foo) isRunning() bool { + return f.running.Load() +} +``` + +
diff --git a/src/builtin-name.md b/src/builtin-name.md new file mode 100644 index 0000000..8ff4dee --- /dev/null +++ b/src/builtin-name.md @@ -0,0 +1,93 @@ +# Avoid Using Built-In Names + +The Go [language specification] outlines several built-in, +[predeclared identifiers] that should not be used as names within Go programs. + +Depending on context, reusing these identifiers as names will either shadow +the original within the current lexical scope (and any nested scopes) or make +affected code confusing. In the best case, the compiler will complain; in the +worst case, such code may introduce latent, hard-to-grep bugs. + + [language specification]: https://go.dev/ref/spec + [predeclared identifiers]: https://go.dev/ref/spec#Predeclared_identifiers + + + + + + +
BadGood
+ +```go +var error string +// `error` shadows the builtin + +// or + +func handleErrorMessage(error string) { + // `error` shadows the builtin +} +``` + + + +```go +var errorMessage string +// `error` refers to the builtin + +// or + +func handleErrorMessage(msg string) { + // `error` refers to the builtin +} +``` + +
+ +```go +type Foo struct { + // While these fields technically don't + // constitute shadowing, grepping for + // `error` or `string` strings is now + // ambiguous. + error error + string string +} + +func (f Foo) Error() error { + // `error` and `f.error` are + // visually similar + return f.error +} + +func (f Foo) String() string { + // `string` and `f.string` are + // visually similar + return f.string +} +``` + + + +```go +type Foo struct { + // `error` and `string` strings are + // now unambiguous. + err error + str string +} + +func (f Foo) Error() error { + return f.err +} + +func (f Foo) String() string { + return f.str +} +``` + +
+ +Note that the compiler will not generate errors when using predeclared +identifiers, but tools such as `go vet` should correctly point out these and +other cases of shadowing. diff --git a/src/channel-size.md b/src/channel-size.md new file mode 100644 index 0000000..02f0050 --- /dev/null +++ b/src/channel-size.md @@ -0,0 +1,29 @@ +# Channel Size is One or None + +Channels should usually have a size of one or be unbuffered. By default, +channels are unbuffered and have a size of zero. Any other size +must be subject to a high level of scrutiny. Consider how the size is +determined, what prevents the channel from filling up under load and blocking +writers, and what happens when this occurs. + + + + + +
BadGood
+ +```go +// Ought to be enough for anybody! +c := make(chan int, 64) +``` + + + +```go +// Size of one +c := make(chan int, 1) // or +// Unbuffered channel, size of zero +c := make(chan int) +``` + +
diff --git a/src/consistency.md b/src/consistency.md new file mode 100644 index 0000000..f6c5978 --- /dev/null +++ b/src/consistency.md @@ -0,0 +1,19 @@ +# Be Consistent + +Some of the guidelines outlined in this document can be evaluated objectively; +others are situational, contextual, or subjective. + +Above all else, **be consistent**. + +Consistent code is easier to maintain, is easier to rationalize, requires less +cognitive overhead, and is easier to migrate or update as new conventions emerge +or classes of bugs are fixed. + +Conversely, having multiple disparate or conflicting styles within a single +codebase causes maintenance overhead, uncertainty, and cognitive dissonance, +all of which can directly contribute to lower velocity, painful code reviews, +and bugs. + +When applying these guidelines to a codebase, it is recommended that changes +are made at a package (or larger) level: application at a sub-package level +violates the above concern by introducing multiple styles into the same code. diff --git a/src/container-capacity.md b/src/container-capacity.md new file mode 100644 index 0000000..b141d97 --- /dev/null +++ b/src/container-capacity.md @@ -0,0 +1,119 @@ +# Prefer Specifying Container Capacity + +Specify container capacity where possible in order to allocate memory for the +container up front. This minimizes subsequent allocations (by copying and +resizing of the container) as elements are added. + +## Specifying Map Capacity Hints + +Where possible, provide capacity hints when initializing +maps with `make()`. + +```go +make(map[T1]T2, hint) +``` + +Providing a capacity hint to `make()` tries to right-size the +map at initialization time, which reduces the need for growing +the map and allocations as elements are added to the map. + +Note that, unlike slices, map capacity hints do not guarantee complete, +preemptive allocation, but are used to approximate the number of hashmap buckets +required. Consequently, allocations may still occur when adding elements to the +map, even up to the specified capacity. + + + + + + +
BadGood
+ +```go +files, _ := os.ReadDir("./files") + +m := make(map[string]os.DirEntry) +for _, f := range files { + m[f.Name()] = f +} +``` + + + +```go + +files, _ := os.ReadDir("./files") + +m := make(map[string]os.DirEntry, len(files)) +for _, f := range files { + m[f.Name()] = f +} +``` + +
+ +`m` is created without a size hint; the map will resize +dynamically, causing multiple allocations as it grows. + + + +`m` is created with a size hint; there may be fewer +allocations at assignment time. + +
+ +## Specifying Slice Capacity + +Where possible, provide capacity hints when initializing slices with `make()`, +particularly when appending. + +```go +make([]T, length, capacity) +``` + +Unlike maps, slice capacity is not a hint: the compiler will allocate enough +memory for the capacity of the slice as provided to `make()`, which means that +subsequent `append()` operations will incur zero allocations (until the length +of the slice matches the capacity, after which any appends will require a resize +to hold additional elements). + + + + + + +
BadGood
+ +```go +for n := 0; n < b.N; n++ { + data := make([]int, 0) + for k := 0; k < size; k++{ + data = append(data, k) + } +} +``` + + + +```go +for n := 0; n < b.N; n++ { + data := make([]int, 0, size) + for k := 0; k < size; k++{ + data = append(data, k) + } +} +``` + +
+ +```plain +BenchmarkBad-4 100000000 2.48s +``` + + + +```plain +BenchmarkGood-4 100000000 0.21s +``` + +
diff --git a/src/container-copy.md b/src/container-copy.md new file mode 100644 index 0000000..ada0489 --- /dev/null +++ b/src/container-copy.md @@ -0,0 +1,106 @@ +# Copy Slices and Maps at Boundaries + +Slices and maps contain pointers to the underlying data so be wary of scenarios +when they need to be copied. + +## Receiving Slices and Maps + +Keep in mind that users can modify a map or slice you received as an argument +if you store a reference to it. + + + + + + + + + + +
Bad Good
+ +```go +func (d *Driver) SetTrips(trips []Trip) { + d.trips = trips +} + +trips := ... +d1.SetTrips(trips) + +// Did you mean to modify d1.trips? +trips[0] = ... +``` + + + +```go +func (d *Driver) SetTrips(trips []Trip) { + d.trips = make([]Trip, len(trips)) + copy(d.trips, trips) +} + +trips := ... +d1.SetTrips(trips) + +// We can now modify trips[0] without affecting d1.trips. +trips[0] = ... +``` + +
+ +## Returning Slices and Maps + +Similarly, be wary of user modifications to maps or slices exposing internal +state. + + + + + +
BadGood
+ +```go +type Stats struct { + mu sync.Mutex + counters map[string]int +} + +// Snapshot returns the current stats. +func (s *Stats) Snapshot() map[string]int { + s.mu.Lock() + defer s.mu.Unlock() + + return s.counters +} + +// snapshot is no longer protected by the mutex, so any +// access to the snapshot is subject to data races. +snapshot := stats.Snapshot() +``` + + + +```go +type Stats struct { + mu sync.Mutex + counters map[string]int +} + +func (s *Stats) Snapshot() map[string]int { + s.mu.Lock() + defer s.mu.Unlock() + + result := make(map[string]int, len(s.counters)) + for k, v := range s.counters { + result[k] = v + } + return result +} + +// Snapshot is now a copy. +snapshot := stats.Snapshot() +``` + +
+ + diff --git a/src/decl-group.md b/src/decl-group.md new file mode 100644 index 0000000..08dcf95 --- /dev/null +++ b/src/decl-group.md @@ -0,0 +1,178 @@ +# Group Similar Declarations + +Go supports grouping similar declarations. + + + + + +
BadGood
+ +```go +import "a" +import "b" +``` + + + +```go +import ( + "a" + "b" +) +``` + +
+ +This also applies to constants, variables, and type declarations. + + + + + +
BadGood
+ +```go + +const a = 1 +const b = 2 + + + +var a = 1 +var b = 2 + + + +type Area float64 +type Volume float64 +``` + + + +```go +const ( + a = 1 + b = 2 +) + +var ( + a = 1 + b = 2 +) + +type ( + Area float64 + Volume float64 +) +``` + +
+ +Only group related declarations. Do not group declarations that are unrelated. + + + + + +
BadGood
+ +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply + EnvVar = "MY_ENV" +) +``` + + + +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply +) + +const EnvVar = "MY_ENV" +``` + +
+ +Groups are not limited in where they can be used. For example, you can use them +inside of functions. + + + + + +
BadGood
+ +```go +func f() string { + red := color.New(0xff0000) + green := color.New(0x00ff00) + blue := color.New(0x0000ff) + + // ... +} +``` + + + +```go +func f() string { + var ( + red = color.New(0xff0000) + green = color.New(0x00ff00) + blue = color.New(0x0000ff) + ) + + // ... +} +``` + +
+ +Exception: Variable declarations, particularly inside functions, should be +grouped together if declared adjacent to other variables. Do this for variables +declared together even if they are unrelated. + + + + + +
BadGood
+ +```go +func (c *client) request() { + caller := c.name + format := "json" + timeout := 5*time.Second + var err error + + // ... +} +``` + + + +```go +func (c *client) request() { + var ( + caller = c.name + format = "json" + timeout = 5*time.Second + err error + ) + + // ... +} +``` + +
diff --git a/src/defer-clean.md b/src/defer-clean.md new file mode 100644 index 0000000..79e43c9 --- /dev/null +++ b/src/defer-clean.md @@ -0,0 +1,49 @@ +# Defer to Clean Up + +Use defer to clean up resources such as files and locks. + + + + + +
BadGood
+ +```go +p.Lock() +if p.count < 10 { + p.Unlock() + return p.count +} + +p.count++ +newCount := p.count +p.Unlock() + +return newCount + +// easy to miss unlocks due to multiple returns +``` + + + +```go +p.Lock() +defer p.Unlock() + +if p.count < 10 { + return p.count +} + +p.count++ +return p.count + +// more readable +``` + +
+ +Defer has an extremely small overhead and should be avoided only if you can +prove that your function execution time is in the order of nanoseconds. The +readability win of using defers is worth the miniscule cost of using them. This +is especially true for larger methods that have more than simple memory +accesses, where the other computations are more significant than the `defer`. diff --git a/src/else-unnecessary.md b/src/else-unnecessary.md new file mode 100644 index 0000000..49cc0ce --- /dev/null +++ b/src/else-unnecessary.md @@ -0,0 +1,30 @@ +# Unnecessary Else + +If a variable is set in both branches of an if, it can be replaced with a +single if. + + + + + +
BadGood
+ +```go +var a int +if b { + a = 100 +} else { + a = 10 +} +``` + + + +```go +a := 10 +if b { + a = 100 +} +``` + +
diff --git a/src/embed-public.md b/src/embed-public.md new file mode 100644 index 0000000..dea5e30 --- /dev/null +++ b/src/embed-public.md @@ -0,0 +1,132 @@ +# Avoid Embedding Types in Public Structs + +These embedded types leak implementation details, inhibit type evolution, and +obscure documentation. + +Assuming you have implemented a variety of list types using a shared +`AbstractList`, avoid embedding the `AbstractList` in your concrete list +implementations. +Instead, hand-write only the methods to your concrete list that will delegate +to the abstract list. + +```go +type AbstractList struct {} + +// Add adds an entity to the list. +func (l *AbstractList) Add(e Entity) { + // ... +} + +// Remove removes an entity from the list. +func (l *AbstractList) Remove(e Entity) { + // ... +} +``` + + + + + +
BadGood
+ +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + *AbstractList +} +``` + + + +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + list *AbstractList +} + +// Add adds an entity to the list. +func (l *ConcreteList) Add(e Entity) { + l.list.Add(e) +} + +// Remove removes an entity from the list. +func (l *ConcreteList) Remove(e Entity) { + l.list.Remove(e) +} +``` + +
+ +Go allows [type embedding] as a compromise between inheritance and composition. +The outer type gets implicit copies of the embedded type's methods. +These methods, by default, delegate to the same method of the embedded +instance. + + [type embedding]: https://go.dev/doc/effective_go#embedding + +The struct also gains a field by the same name as the type. +So, if the embedded type is public, the field is public. +To maintain backward compatibility, every future version of the outer type must +keep the embedded type. + +An embedded type is rarely necessary. +It is a convenience that helps you avoid writing tedious delegate methods. + +Even embedding a compatible AbstractList *interface*, instead of the struct, +would offer the developer more flexibility to change in the future, but still +leak the detail that the concrete lists use an abstract implementation. + + + + + +
BadGood
+ +```go +// AbstractList is a generalized implementation +// for various kinds of lists of entities. +type AbstractList interface { + Add(Entity) + Remove(Entity) +} + +// ConcreteList is a list of entities. +type ConcreteList struct { + AbstractList +} +``` + + + +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + list AbstractList +} + +// Add adds an entity to the list. +func (l *ConcreteList) Add(e Entity) { + l.list.Add(e) +} + +// Remove removes an entity from the list. +func (l *ConcreteList) Remove(e Entity) { + l.list.Remove(e) +} +``` + +
+ +Either with an embedded struct or an embedded interface, the embedded type +places limits on the evolution of the type. + +- Adding methods to an embedded interface is a breaking change. +- Removing methods from an embedded struct is a breaking change. +- Removing the embedded type is a breaking change. +- Replacing the embedded type, even with an alternative that satisfies the same + interface, is a breaking change. + +Although writing these delegate methods is tedious, the additional effort hides +an implementation detail, leaves more opportunities for change, and also +eliminates indirection for discovering the full List interface in +documentation. diff --git a/src/enum-start.md b/src/enum-start.md new file mode 100644 index 0000000..ec46602 --- /dev/null +++ b/src/enum-start.md @@ -0,0 +1,56 @@ +# Start Enums at One + +The standard way of introducing enumerations in Go is to declare a custom type +and a `const` group with `iota`. Since variables have a 0 default value, you +should usually start your enums on a non-zero value. + + + + + +
BadGood
+ +```go +type Operation int + +const ( + Add Operation = iota + Subtract + Multiply +) + +// Add=0, Subtract=1, Multiply=2 +``` + + + +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply +) + +// Add=1, Subtract=2, Multiply=3 +``` + +
+ +There are cases where using the zero value makes sense, for example when the +zero value case is the desirable default behavior. + +```go +type LogOutput int + +const ( + LogToStdout LogOutput = iota + LogToFile + LogToRemote +) + +// LogToStdout=0, LogToFile=1, LogToRemote=2 +``` + + diff --git a/src/error-name.md b/src/error-name.md new file mode 100644 index 0000000..9f23335 --- /dev/null +++ b/src/error-name.md @@ -0,0 +1,52 @@ +# Error Naming + +For error values stored as global variables, +use the prefix `Err` or `err` depending on whether they're exported. +This guidance supersedes the [Prefix Unexported Globals with _](global-name.md). + +```go +var ( + // The following two errors are exported + // so that users of this package can match them + // with errors.Is. + + ErrBrokenLink = errors.New("link is broken") + ErrCouldNotOpen = errors.New("could not open") + + // This error is not exported because + // we don't want to make it part of our public API. + // We may still use it inside the package + // with errors.Is. + + errNotFound = errors.New("not found") +) +``` + +For custom error types, use the suffix `Error` instead. + +```go +// Similarly, this error is exported +// so that users of this package can match it +// with errors.As. + +type NotFoundError struct { + File string +} + +func (e *NotFoundError) Error() string { + return fmt.Sprintf("file %q not found", e.File) +} + +// And this error is not exported because +// we don't want to make it part of the public API. +// We can still use it inside the package +// with errors.As. + +type resolveError struct { + Path string +} + +func (e *resolveError) Error() string { + return fmt.Sprintf("resolve %q", e.Path) +} +``` diff --git a/src/error-once.md b/src/error-once.md new file mode 100644 index 0000000..8a1cda5 --- /dev/null +++ b/src/error-once.md @@ -0,0 +1,111 @@ +# Handle Errors Once + +When a caller receives an error from a callee, +it can handle it in a variety of different ways +depending on what it knows about the error. + +These include, but not are limited to: + +- if the callee contract defines specific errors, + matching the error with `errors.Is` or `errors.As` + and handling the branches differently +- if the error is recoverable, + logging the error and degrading gracefully +- if the error represents a domain-specific failure condition, + returning a well-defined error +- returning the error, either [wrapped](error-wrap.md) or verbatim + +Regardless of how the caller handles the error, +it should typically handle each error only once. +The caller should not, for example, log the error and then return it, +because *its* callers may handle the error as well. + +For example, consider the following cases: + + + + + + + + +
DescriptionCode
+ +**Bad**: Log the error and return it + +Callers further up the stack will likely take a similar action with the error. +Doing so makes a lot of noise in the application logs for little value. + + + +```go +u, err := getUser(id) +if err != nil { + // BAD: See description + log.Printf("Could not get user %q: %v", id, err) + return err +} +``` + +
+ +**Good**: Wrap the error and return it + +Callers further up the stack will handle the error. +Use of `%w` ensures they can match the error with `errors.Is` or `errors.As` +if relevant. + + + +```go +u, err := getUser(id) +if err != nil { + return fmt.Errorf("get user %q: %w", id, err) +} +``` + +
+ +**Good**: Log the error and degrade gracefully + +If the operation isn't strictly necessary, +we can provide a degraded but unbroken experience +by recovering from it. + + + +```go +if err := emitMetrics(); err != nil { + // Failure to write metrics should not + // break the application. + log.Printf("Could not emit metrics: %v", err) +} + +``` + +
+ +**Good**: Match the error and degrade gracefully + +If the callee defines a specific error in its contract, +and the failure is recoverable, +match on that error case and degrade gracefully. +For all other cases, wrap the error and return it. + +Callers further up the stack will handle other errors. + + + +```go +tz, err := getUserTimeZone(id) +if err != nil { + if errors.Is(err, ErrUserNotFound) { + // User doesn't exist. Use UTC. + tz = time.UTC + } else { + return fmt.Errorf("get user %q: %w", id, err) + } +} +``` + +
diff --git a/src/error-type.md b/src/error-type.md new file mode 100644 index 0000000..4142269 --- /dev/null +++ b/src/error-type.md @@ -0,0 +1,137 @@ +# Error Types + +There are few options for declaring errors. +Consider the following before picking the option best suited for your use case. + +- Does the caller need to match the error so that they can handle it? + If yes, we must support the [`errors.Is`] or [`errors.As`] functions + by declaring a top-level error variable or a custom type. +- Is the error message a static string, + or is it a dynamic string that requires contextual information? + For the former, we can use [`errors.New`], but for the latter we must + use [`fmt.Errorf`] or a custom error type. +- Are we propagating a new error returned by a downstream function? + If so, see the [section on error wrapping](error-wrap.md). + +[`errors.Is`]: https://pkg.go.dev/errors#Is +[`errors.As`]: https://pkg.go.dev/errors#As + +| Error matching? | Error Message | Guidance | +|-----------------|---------------|-------------------------------------| +| No | static | [`errors.New`] | +| No | dynamic | [`fmt.Errorf`] | +| Yes | static | top-level `var` with [`errors.New`] | +| Yes | dynamic | custom `error` type | + +[`errors.New`]: https://pkg.go.dev/errors#New +[`fmt.Errorf`]: https://pkg.go.dev/fmt#Errorf + +For example, +use [`errors.New`] for an error with a static string. +Export this error as a variable to support matching it with `errors.Is` +if the caller needs to match and handle this error. + + + + + +
No error matchingError matching
+ +```go +// package foo + +func Open() error { + return errors.New("could not open") +} + +// package bar + +if err := foo.Open(); err != nil { + // Can't handle the error. + panic("unknown error") +} +``` + + + +```go +// package foo + +var ErrCouldNotOpen = errors.New("could not open") + +func Open() error { + return ErrCouldNotOpen +} + +// package bar + +if err := foo.Open(); err != nil { + if errors.Is(err, foo.ErrCouldNotOpen) { + // handle the error + } else { + panic("unknown error") + } +} +``` + +
+ +For an error with a dynamic string, +use [`fmt.Errorf`] if the caller does not need to match it, +and a custom `error` if the caller does need to match it. + + + + + +
No error matchingError matching
+ +```go +// package foo + +func Open(file string) error { + return fmt.Errorf("file %q not found", file) +} + +// package bar + +if err := foo.Open("testfile.txt"); err != nil { + // Can't handle the error. + panic("unknown error") +} +``` + + + +```go +// package foo + +type NotFoundError struct { + File string +} + +func (e *NotFoundError) Error() string { + return fmt.Sprintf("file %q not found", e.File) +} + +func Open(file string) error { + return &NotFoundError{File: file} +} + + +// package bar + +if err := foo.Open("testfile.txt"); err != nil { + var notFound *NotFoundError + if errors.As(err, ¬Found) { + // handle the error + } else { + panic("unknown error") + } +} +``` + +
+ +Note that if you export error variables or types from a package, +they will become part of the public API of the package. diff --git a/src/error-wrap.md b/src/error-wrap.md new file mode 100644 index 0000000..fba3110 --- /dev/null +++ b/src/error-wrap.md @@ -0,0 +1,79 @@ +# Error Wrapping + +There are three main options for propagating errors if a call fails: + +- return the original error as-is +- add context with `fmt.Errorf` and the `%w` verb +- add context with `fmt.Errorf` and the `%v` verb + +Return the original error as-is if there is no additional context to add. +This maintains the original error type and message. +This is well suited for cases when the underlying error message +has sufficient information to track down where it came from. + +Otherwise, add context to the error message where possible +so that instead of a vague error such as "connection refused", +you get more useful errors such as "call service foo: connection refused". + +Use `fmt.Errorf` to add context to your errors, +picking between the `%w` or `%v` verbs +based on whether the caller should be able to +match and extract the underlying cause. + +- Use `%w` if the caller should have access to the underlying error. + This is a good default for most wrapped errors, + but be aware that callers may begin to rely on this behavior. + So for cases where the wrapped error is a known `var` or type, + document and test it as part of your function's contract. +- Use `%v` to obfuscate the underlying error. + Callers will be unable to match it, + but you can switch to `%w` in the future if needed. + +When adding context to returned errors, keep the context succinct by avoiding +phrases like "failed to", which state the obvious and pile up as the error +percolates up through the stack: + + + + + +
BadGood
+ +```go +s, err := store.New() +if err != nil { + return fmt.Errorf( + "failed to create new store: %w", err) +} +``` + + + +```go +s, err := store.New() +if err != nil { + return fmt.Errorf( + "new store: %w", err) +} +``` + +
+ +```plain +failed to x: failed to y: failed to create new store: the error +``` + + + +```plain +x: y: new store: the error +``` + +
+ +However once the error is sent to another system, it should be clear the +message is an error (e.g. an `err` tag or "Failed" prefix in logs). + +See also [Don't just check errors, handle them gracefully]. + + [Don't just check errors, handle them gracefully]: https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully diff --git a/src/exit-main.md b/src/exit-main.md new file mode 100644 index 0000000..0af3b1d --- /dev/null +++ b/src/exit-main.md @@ -0,0 +1,76 @@ +# Exit in Main + +Go programs use [`os.Exit`] or [`log.Fatal*`] to exit immediately. (Panicking +is not a good way to exit programs, please [don't panic](panic.md).) + + [`os.Exit`]: https://pkg.go.dev/os#Exit + [`log.Fatal*`]: https://pkg.go.dev/log#Fatal + +Call one of `os.Exit` or `log.Fatal*` **only in `main()`**. All other +functions should return errors to signal failure. + + + + + +
BadGood
+ +```go +func main() { + body := readFile(path) + fmt.Println(body) +} + +func readFile(path string) string { + f, err := os.Open(path) + if err != nil { + log.Fatal(err) + } + + b, err := io.ReadAll(f) + if err != nil { + log.Fatal(err) + } + + return string(b) +} +``` + + + +```go +func main() { + body, err := readFile(path) + if err != nil { + log.Fatal(err) + } + fmt.Println(body) +} + +func readFile(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", err + } + + b, err := io.ReadAll(f) + if err != nil { + return "", err + } + + return string(b), nil +} +``` + +
+ +Rationale: Programs with multiple functions that exit present a few issues: + +- Non-obvious control flow: Any function can exit the program so it becomes + difficult to reason about the control flow. +- Difficult to test: A function that exits the program will also exit the test + calling it. This makes the function difficult to test and introduces risk of + skipping other tests that have not yet been run by `go test`. +- Skipped cleanup: When a function exits the program, it skips function calls + enqueued with `defer` statements. This adds risk of skipping important + cleanup tasks. diff --git a/src/exit-once.md b/src/exit-once.md new file mode 100644 index 0000000..812fa14 --- /dev/null +++ b/src/exit-once.md @@ -0,0 +1,117 @@ +# Exit Once + +If possible, prefer to call `os.Exit` or `log.Fatal` **at most once** in your +`main()`. If there are multiple error scenarios that halt program execution, +put that logic under a separate function and return errors from it. + +This has the effect of shortening your `main()` function and putting all key +business logic into a separate, testable function. + + + + + +
BadGood
+ +```go +package main + +func main() { + args := os.Args[1:] + if len(args) != 1 { + log.Fatal("missing file") + } + name := args[0] + + f, err := os.Open(name) + if err != nil { + log.Fatal(err) + } + defer f.Close() + + // If we call log.Fatal after this line, + // f.Close will not be called. + + b, err := io.ReadAll(f) + if err != nil { + log.Fatal(err) + } + + // ... +} +``` + + + +```go +package main + +func main() { + if err := run(); err != nil { + log.Fatal(err) + } +} + +func run() error { + args := os.Args[1:] + if len(args) != 1 { + return errors.New("missing file") + } + name := args[0] + + f, err := os.Open(name) + if err != nil { + return err + } + defer f.Close() + + b, err := io.ReadAll(f) + if err != nil { + return err + } + + // ... +} +``` + +
+ +The example above uses `log.Fatal`, but the guidance also applies to +`os.Exit` or any library code that calls `os.Exit`. + +```go +func main() { + if err := run(); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} +``` + +You may alter the signature of `run()` to fit your needs. +For example, if your program must exit with specific exit codes for failures, +`run()` may return the exit code instead of an error. +This allows unit tests to verify this behavior directly as well. + +```go +func main() { + os.Exit(run(args)) +} + +func run() (exitCode int) { + // ... +} +``` + +More generally, note that the `run()` function used in these examples +is not intended to be prescriptive. +There's flexibility in the name, signature, and setup of the `run()` function. +Among other things, you may: + +- accept unparsed command line arguments (e.g., `run(os.Args[1:])`) +- parse command line arguments in `main()` and pass them onto `run` +- use a custom error type to carry the exit code back to `main()` +- put business logic in a different layer of abstraction from `package main` + +This guidance only requires that there's a single place in your `main()` +responsible for actually exiting the process. diff --git a/src/function-name.md b/src/function-name.md new file mode 100644 index 0000000..685a44a --- /dev/null +++ b/src/function-name.md @@ -0,0 +1,8 @@ +# Function Names + +We follow the Go community's convention of using [MixedCaps for function +names]. An exception is made for test functions, which may contain underscores +for the purpose of grouping related test cases, e.g., +`TestMyFunction_WhatIsBeingTested`. + + [MixedCaps for function names]: https://go.dev/doc/effective_go#mixed-caps diff --git a/src/function-order.md b/src/function-order.md new file mode 100644 index 0000000..117a2bd --- /dev/null +++ b/src/function-order.md @@ -0,0 +1,55 @@ +# Function Grouping and Ordering + +- Functions should be sorted in rough call order. +- Functions in a file should be grouped by receiver. + +Therefore, exported functions should appear first in a file, after +`struct`, `const`, `var` definitions. + +A `newXYZ()`/`NewXYZ()` may appear after the type is defined, but before the +rest of the methods on the receiver. + +Since functions are grouped by receiver, plain utility functions should appear +towards the end of the file. + + + + + +
BadGood
+ +```go +func (s *something) Cost() { + return calcCost(s.weights) +} + +type something struct{ ... } + +func calcCost(n []int) int {...} + +func (s *something) Stop() {...} + +func newSomething() *something { + return &something{} +} +``` + + + +```go +type something struct{ ... } + +func newSomething() *something { + return &something{} +} + +func (s *something) Cost() { + return calcCost(s.weights) +} + +func (s *something) Stop() {...} + +func calcCost(n []int) int {...} +``` + +
diff --git a/src/functional-option.md b/src/functional-option.md new file mode 100644 index 0000000..da7194f --- /dev/null +++ b/src/functional-option.md @@ -0,0 +1,157 @@ +# Functional Options + +Functional options is a pattern in which you declare an opaque `Option` type +that records information in some internal struct. You accept a variadic number +of these options and act upon the full information recorded by the options on +the internal struct. + +Use this pattern for optional arguments in constructors and other public APIs +that you foresee needing to expand, especially if you already have three or +more arguments on those functions. + + + + + + +
BadGood
+ +```go +// package db + +func Open( + addr string, + cache bool, + logger *zap.Logger +) (*Connection, error) { + // ... +} +``` + + + +```go +// package db + +type Option interface { + // ... +} + +func WithCache(c bool) Option { + // ... +} + +func WithLogger(log *zap.Logger) Option { + // ... +} + +// Open creates a connection. +func Open( + addr string, + opts ...Option, +) (*Connection, error) { + // ... +} +``` + +
+ +The cache and logger parameters must always be provided, even if the user +wants to use the default. + +```go +db.Open(addr, db.DefaultCache, zap.NewNop()) +db.Open(addr, db.DefaultCache, log) +db.Open(addr, false /* cache */, zap.NewNop()) +db.Open(addr, false /* cache */, log) +``` + + + +Options are provided only if needed. + +```go +db.Open(addr) +db.Open(addr, db.WithLogger(log)) +db.Open(addr, db.WithCache(false)) +db.Open( + addr, + db.WithCache(false), + db.WithLogger(log), +) +``` + +
+ +Our suggested way of implementing this pattern is with an `Option` interface +that holds an unexported method, recording options on an unexported `options` +struct. + +```go +type options struct { + cache bool + logger *zap.Logger +} + +type Option interface { + apply(*options) +} + +type cacheOption bool + +func (c cacheOption) apply(opts *options) { + opts.cache = bool(c) +} + +func WithCache(c bool) Option { + return cacheOption(c) +} + +type loggerOption struct { + Log *zap.Logger +} + +func (l loggerOption) apply(opts *options) { + opts.logger = l.Log +} + +func WithLogger(log *zap.Logger) Option { + return loggerOption{Log: log} +} + +// Open creates a connection. +func Open( + addr string, + opts ...Option, +) (*Connection, error) { + options := options{ + cache: defaultCache, + logger: zap.NewNop(), + } + + for _, o := range opts { + o.apply(&options) + } + + // ... +} +``` + +Note that there's a method of implementing this pattern with closures but we +believe that the pattern above provides more flexibility for authors and is +easier to debug and test for users. In particular, it allows options to be +compared against each other in tests and mocks, versus closures where this is +impossible. Further, it lets options implement other interfaces, including +`fmt.Stringer` which allows for user-readable string representations of the +options. + +See also, + +- [Self-referential functions and the design of options] +- [Functional options for friendly APIs] + + [Self-referential functions and the design of options]: https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html + [Functional options for friendly APIs]: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis + + diff --git a/src/global-decl.md b/src/global-decl.md new file mode 100644 index 0000000..6473189 --- /dev/null +++ b/src/global-decl.md @@ -0,0 +1,42 @@ +# Top-level Variable Declarations + +At the top level, use the standard `var` keyword. Do not specify the type, +unless it is not the same type as the expression. + + + + + +
BadGood
+ +```go +var _s string = F() + +func F() string { return "A" } +``` + + + +```go +var _s = F() +// Since F already states that it returns a string, we don't need to specify +// the type again. + +func F() string { return "A" } +``` + +
+ +Specify the type if the type of the expression does not match the desired type +exactly. + +```go +type myError struct{} + +func (myError) Error() string { return "error" } + +func F() myError { return myError{} } + +var _e error = F() +// F returns an object of type myError but we want error. +``` diff --git a/src/global-mut.md b/src/global-mut.md new file mode 100644 index 0000000..884c88c --- /dev/null +++ b/src/global-mut.md @@ -0,0 +1,76 @@ +# Avoid Mutable Globals + +Avoid mutating global variables, instead opting for dependency injection. +This applies to function pointers as well as other kinds of values. + + + + + + +
BadGood
+ +```go +// sign.go + +var _timeNow = time.Now + +func sign(msg string) string { + now := _timeNow() + return signWithTime(msg, now) +} +``` + + + +```go +// sign.go + +type signer struct { + now func() time.Time +} + +func newSigner() *signer { + return &signer{ + now: time.Now, + } +} + +func (s *signer) Sign(msg string) string { + now := s.now() + return signWithTime(msg, now) +} +``` + +
+ +```go +// sign_test.go + +func TestSign(t *testing.T) { + oldTimeNow := _timeNow + _timeNow = func() time.Time { + return someFixedTime + } + defer func() { _timeNow = oldTimeNow }() + + assert.Equal(t, want, sign(give)) +} +``` + + + +```go +// sign_test.go + +func TestSigner(t *testing.T) { + s := newSigner() + s.now = func() time.Time { + return someFixedTime + } + + assert.Equal(t, want, s.Sign(give)) +} +``` + +
diff --git a/src/global-name.md b/src/global-name.md new file mode 100644 index 0000000..4811330 --- /dev/null +++ b/src/global-name.md @@ -0,0 +1,50 @@ +# Prefix Unexported Globals with _ + +Prefix unexported top-level `var`s and `const`s with `_` to make it clear when +they are used that they are global symbols. + +Rationale: Top-level variables and constants have a package scope. Using a +generic name makes it easy to accidentally use the wrong value in a different +file. + + + + + +
BadGood
+ +```go +// foo.go + +const ( + defaultPort = 8080 + defaultUser = "user" +) + +// bar.go + +func Bar() { + defaultPort := 9090 + ... + fmt.Println("Default port", defaultPort) + + // We will not see a compile error if the first line of + // Bar() is deleted. +} +``` + + + +```go +// foo.go + +const ( + _defaultPort = 8080 + _defaultUser = "user" +) +``` + +
+ +**Exception**: Unexported error values may use the prefix `err` without the underscore. +See [Error Naming](error-name.md). diff --git a/src/goroutine-exit.md b/src/goroutine-exit.md new file mode 100644 index 0000000..9cffb91 --- /dev/null +++ b/src/goroutine-exit.md @@ -0,0 +1,32 @@ +# Wait for goroutines to exit + +Given a goroutine spawned by the system, +there must be a way to wait for the goroutine to exit. +There are two popular ways to do this: + +- Use a `sync.WaitGroup` to wait for multiple goroutines to complete. + Do this if there are multiple goroutines that you want to wait for. + + ```go + var wg sync.WaitGroup + for i := 0; i < N; i++ { + wg.Go(...) + } + + // To wait for all to finish: + wg.Wait() + ``` + +- Add another `chan struct{}` that the goroutine closes when it's done. + Do this if there's only one goroutine. + + ```go + done := make(chan struct{}) + go func() { + defer close(done) + // ... + }() + + // To wait for the goroutine to finish: + <-done + ``` diff --git a/src/goroutine-forget.md b/src/goroutine-forget.md new file mode 100644 index 0000000..f17e820 --- /dev/null +++ b/src/goroutine-forget.md @@ -0,0 +1,79 @@ +# Don't fire-and-forget goroutines + +Goroutines are lightweight, but they're not free: +at minimum, they cost memory for their stack and CPU to be scheduled. +While these costs are small for typical uses of goroutines, +they can cause significant performance issues +when spawned in large numbers without controlled lifetimes. +Goroutines with unmanaged lifetimes can also cause other issues +like preventing unused objects from being garbage collected +and holding onto resources that are otherwise no longer used. + +Therefore, do not leak goroutines in production code. +Use [go.uber.org/goleak](https://pkg.go.dev/go.uber.org/goleak) +to test for goroutine leaks inside packages that may spawn goroutines. + +In general, every goroutine: + +- must have a predictable time at which it will stop running; or +- there must be a way to signal to the goroutine that it should stop + +In both cases, there must be a way for code to block and wait for the goroutine to +finish. + +For example: + + + + + + +
BadGood
+ +```go +go func() { + for { + flush() + time.Sleep(delay) + } +}() +``` + + + +```go +var ( + stop = make(chan struct{}) // tells the goroutine to stop + done = make(chan struct{}) // tells us that the goroutine exited +) +go func() { + defer close(done) + + ticker := time.NewTicker(delay) + defer ticker.Stop() + for { + select { + case <-ticker.C: + flush() + case <-stop: + return + } + } +}() + +// Elsewhere... +close(stop) // signal the goroutine to stop +<-done // and wait for it to exit +``` + +
+ +There's no way to stop this goroutine. +This will run until the application exits. + + + +This goroutine can be stopped with `close(stop)`, +and we can wait for it to exit with `<-done`. + +
diff --git a/src/goroutine-init.md b/src/goroutine-init.md new file mode 100644 index 0000000..03ef713 --- /dev/null +++ b/src/goroutine-init.md @@ -0,0 +1,78 @@ +# No goroutines in `init()` + +`init()` functions should not spawn goroutines. +See also [Avoid init()](init.md). + +If a package has need of a background goroutine, +it must expose an object that is responsible for managing a goroutine's +lifetime. +The object must provide a method (`Close`, `Stop`, `Shutdown`, etc) +that signals the background goroutine to stop, and waits for it to exit. + + + + + + +
BadGood
+ +```go +func init() { + go doWork() +} + +func doWork() { + for { + // ... + } +} +``` + + + +```go +type Worker struct{ /* ... */ } + +func NewWorker(...) *Worker { + w := &Worker{ + stop: make(chan struct{}), + done: make(chan struct{}), + // ... + } + go w.doWork() + return w +} + +func (w *Worker) doWork() { + defer close(w.done) + for { + // ... + case <-w.stop: + return + } +} + +// Shutdown tells the worker to stop +// and waits until it has finished. +func (w *Worker) Shutdown() { + close(w.stop) + <-w.done +} +``` + +
+ +Spawns a background goroutine unconditionally when the user exports this package. +The user has no control over the goroutine or a means of stopping it. + + + +Spawns the worker only if the user requests it. +Provides a means of shutting down the worker so that the user can free up +resources used by the worker. + +Note that you should use `WaitGroup`s if the worker manages multiple +goroutines. +See [Wait for goroutines to exit](goroutine-exit.md). + +
diff --git a/src/import-alias.md b/src/import-alias.md new file mode 100644 index 0000000..6903222 --- /dev/null +++ b/src/import-alias.md @@ -0,0 +1,46 @@ +# Import Aliasing + +Import aliasing must be used if the package name does not match the last +element of the import path. + +```go +import ( + "net/http" + + client "example.com/client-go" + trace "example.com/trace/v2" +) +``` + +In all other scenarios, import aliases should be avoided unless there is a +direct conflict between imports. + + + + + +
BadGood
+ +```go +import ( + "fmt" + "os" + runtimetrace "runtime/trace" + + nettrace "golang.net/x/trace" +) +``` + + + +```go +import ( + "fmt" + "os" + "runtime/trace" + + nettrace "golang.net/x/trace" +) +``` + +
diff --git a/src/import-group.md b/src/import-group.md new file mode 100644 index 0000000..384c8ac --- /dev/null +++ b/src/import-group.md @@ -0,0 +1,37 @@ +# Import Group Ordering + +There should be two import groups: + +- Standard library +- Everything else + +This is the grouping applied by goimports by default. + + + + + +
BadGood
+ +```go +import ( + "fmt" + "os" + "go.uber.org/atomic" + "golang.org/x/sync/errgroup" +) +``` + + + +```go +import ( + "fmt" + "os" + + "go.uber.org/atomic" + "golang.org/x/sync/errgroup" +) +``` + +
diff --git a/src/init.md b/src/init.md new file mode 100644 index 0000000..13cece3 --- /dev/null +++ b/src/init.md @@ -0,0 +1,116 @@ +# Avoid `init()` + +Avoid `init()` where possible. When `init()` is unavoidable or desirable, code +should attempt to: + +1. Be completely deterministic, regardless of program environment or invocation. +2. Avoid depending on the ordering or side-effects of other `init()` functions. + While `init()` ordering is well-known, code can change, and thus + relationships between `init()` functions can make code brittle and + error-prone. +3. Avoid accessing or manipulating global or environment state, such as machine + information, environment variables, working directory, program + arguments/inputs, etc. +4. Avoid I/O, including both filesystem, network, and system calls. + +Code that cannot satisfy these requirements likely belongs as a helper to be +called as part of `main()` (or elsewhere in a program's lifecycle), or be +written as part of `main()` itself. In particular, libraries that are intended +to be used by other programs should take special care to be completely +deterministic and not perform "init magic". + + + + + + +
BadGood
+ +```go +type Foo struct { + // ... +} + +var _defaultFoo Foo + +func init() { + _defaultFoo = Foo{ + // ... + } +} +``` + + + +```go +var _defaultFoo = Foo{ + // ... +} + +// or, better, for testability: + +var _defaultFoo = defaultFoo() + +func defaultFoo() Foo { + return Foo{ + // ... + } +} +``` + +
+ +```go +type Config struct { + // ... +} + +var _config Config + +func init() { + // Bad: based on current directory + cwd, _ := os.Getwd() + + // Bad: I/O + raw, _ := os.ReadFile( + path.Join(cwd, "config", "config.yaml"), + ) + + yaml.Unmarshal(raw, &_config) +} +``` + + + +```go +type Config struct { + // ... +} + +func loadConfig() Config { + cwd, err := os.Getwd() + // handle err + + raw, err := os.ReadFile( + path.Join(cwd, "config", "config.yaml"), + ) + // handle err + + var config Config + yaml.Unmarshal(raw, &config) + + return config +} +``` + +
+ +Considering the above, some situations in which `init()` may be preferable or +necessary might include: + +- Complex expressions that cannot be represented as single assignments. +- Pluggable hooks, such as `database/sql` dialects, encoding type registries, etc. +- Optimizations to [Google Cloud Functions] and other forms of deterministic + precomputation. + + [Google Cloud Functions]: https://cloud.google.com/functions/docs/bestpractices/tips#use_global_variables_to_reuse_objects_in_future_invocations diff --git a/src/interface-compliance.md b/src/interface-compliance.md new file mode 100644 index 0000000..e6d6d74 --- /dev/null +++ b/src/interface-compliance.md @@ -0,0 +1,72 @@ +# Verify Interface Compliance + +Verify interface compliance at compile time where appropriate. This includes: + +- Exported types that are required to implement specific interfaces as part of + their API contract +- Exported or unexported types that are part of a collection of types + implementing the same interface +- Other cases where violating an interface would break users + + + + + +
BadGood
+ +```go +type Handler struct { + // ... +} + + + +func (h *Handler) ServeHTTP( + w http.ResponseWriter, + r *http.Request, +) { + ... +} +``` + + + +```go +type Handler struct { + // ... +} + +var _ http.Handler = (*Handler)(nil) + +func (h *Handler) ServeHTTP( + w http.ResponseWriter, + r *http.Request, +) { + // ... +} +``` + +
+ +The statement `var _ http.Handler = (*Handler)(nil)` will fail to compile if +`*Handler` ever stops matching the `http.Handler` interface. + +The right hand side of the assignment should be the zero value of the asserted +type. This is `nil` for pointer types (like `*Handler`), slices, and maps, and +an empty struct for struct types. + +```go +type LogHandler struct { + h http.Handler + log *zap.Logger +} + +var _ http.Handler = LogHandler{} + +func (h LogHandler) ServeHTTP( + w http.ResponseWriter, + r *http.Request, +) { + // ... +} +``` diff --git a/src/interface-pointer.md b/src/interface-pointer.md new file mode 100644 index 0000000..91338d8 --- /dev/null +++ b/src/interface-pointer.md @@ -0,0 +1,14 @@ +# Pointers to Interfaces + +You almost never need a pointer to an interface. You should be passing +interfaces as values—the underlying data can still be a pointer. + +An interface is two fields: + +1. A pointer to some type-specific information. You can think of this as + "type." +2. Data pointer. If the data stored is a pointer, it’s stored directly. If + the data stored is a value, then a pointer to the value is stored. + +If you want interface methods to modify the underlying data, you must use a +pointer. diff --git a/src/interface-receiver.md b/src/interface-receiver.md new file mode 100644 index 0000000..e0102d6 --- /dev/null +++ b/src/interface-receiver.md @@ -0,0 +1,78 @@ +# Receivers and Interfaces + +Methods with value receivers can be called on pointers as well as values. +Methods with pointer receivers can only be called on pointers or [addressable values]. + + [addressable values]: https://go.dev/ref/spec#Method_values + +For example, + +```go +type S struct { + data string +} + +func (s S) Read() string { + return s.data +} + +func (s *S) Write(str string) { + s.data = str +} + +// We cannot get pointers to values stored in maps, because they are not +// addressable values. +sVals := map[int]S{1: {"A"}} + +// We can call Read on values stored in the map because Read +// has a value receiver, which does not require the value to +// be addressable. +sVals[1].Read() + +// We cannot call Write on values stored in the map because Write +// has a pointer receiver, and it's not possible to get a pointer +// to a value stored in a map. +// +// sVals[1].Write("test") + +sPtrs := map[int]*S{1: {"A"}} + +// You can call both Read and Write if the map stores pointers, +// because pointers are intrinsically addressable. +sPtrs[1].Read() +sPtrs[1].Write("test") +``` + +Similarly, an interface can be satisfied by a pointer, even if the method has a +value receiver. + +```go +type F interface { + f() +} + +type S1 struct{} + +func (s S1) f() {} + +type S2 struct{} + +func (s *S2) f() {} + +s1Val := S1{} +s1Ptr := &S1{} +s2Val := S2{} +s2Ptr := &S2{} + +var i F +i = s1Val +i = s1Ptr +i = s2Ptr + +// The following doesn't compile, since s2Val is a value, and there is no value receiver for f. +// i = s2Val +``` + +Effective Go has a good write up on [Pointers vs. Values]. + + [Pointers vs. Values]: https://go.dev/doc/effective_go#pointers_vs_values diff --git a/src/intro.md b/src/intro.md new file mode 100644 index 0000000..e50dacf --- /dev/null +++ b/src/intro.md @@ -0,0 +1,37 @@ +# Introduction + +Styles are the conventions that govern our code. The term style is a bit of a +misnomer, since these conventions cover far more than just source file +formatting—gofmt handles that for us. + +The goal of this guide is to manage this complexity by describing in detail the +Dos and Don'ts of writing Go code at Uber. These rules exist to keep the code +base manageable while still allowing engineers to use Go language features +productively. + +This guide was originally created by [Prashant Varanasi] and [Simon Newton] as +a way to bring some colleagues up to speed with using Go. Over the years it has +been amended based on feedback from others. + + [Prashant Varanasi]: https://github.com/prashantv + [Simon Newton]: https://github.com/nomis52 + +This documents idiomatic conventions in Go code that we follow at Uber. A lot +of these are general guidelines for Go, while others extend upon external +resources: + +1. [Effective Go](https://go.dev/doc/effective_go) +2. [Go Common Mistakes](https://go.dev/wiki/CommonMistakes) +3. [Go Code Review Comments](https://go.dev/wiki/CodeReviewComments) + +We aim for the code samples to be accurate for the two most recent minor versions +of Go [releases](https://go.dev/doc/devel/release). + +All code should be error-free when run through `golint` and `go vet`. We +recommend setting up your editor to: + +- Run `goimports` on save +- Run `golint` and `go vet` to check for errors + +You can find information in editor support for Go tools here: + diff --git a/src/line-length.md b/src/line-length.md new file mode 100644 index 0000000..163b71c --- /dev/null +++ b/src/line-length.md @@ -0,0 +1,9 @@ +# Avoid overly long lines + +Avoid lines of code that require readers to scroll horizontally +or turn their heads too much. + +We recommend a soft line length limit of **99 characters**. +Authors should aim to wrap lines before hitting this limit, +but it is not a hard limit. +Code is allowed to exceed this limit. diff --git a/src/lint.md b/src/lint.md new file mode 100644 index 0000000..41fd519 --- /dev/null +++ b/src/lint.md @@ -0,0 +1,39 @@ +# Linting + +More importantly than any "blessed" set of linters, lint consistently across a +codebase. + +We recommend using the following linters at a minimum, because we feel that they +help to catch the most common issues and also establish a high bar for code +quality without being unnecessarily prescriptive: + +- [errcheck] to ensure that errors are handled +- [goimports] to format code and manage imports +- [revive] to point out common style mistakes +- [govet] to analyze code for common mistakes +- [staticcheck] to do various static analysis checks + + [errcheck]: https://github.com/kisielk/errcheck + [goimports]: https://pkg.go.dev/golang.org/x/tools/cmd/goimports + [revive]: https://github.com/mgechev/revive + [govet]: https://pkg.go.dev/cmd/vet + [staticcheck]: https://staticcheck.dev + + > **Note**: [revive] is the modern, faster successor to the now-deprecated [golint]. + +## Lint Runners + +We recommend [golangci-lint] as the go-to lint runner for Go code, largely due +to its performance in larger codebases and ability to configure and use many +canonical linters at once. This repo has an example [.golangci.yml] config file +with recommended linters and settings. + +golangci-lint has [various linters] available for use. The above linters are +recommended as a base set, and we encourage teams to add any additional linters +that make sense for their projects. + + [golangci-lint]: https://github.com/golangci/golangci-lint + [.golangci.yml]: https://github.com/uber-go/guide/blob/master/.golangci.yml + [various linters]: https://golangci-lint.run/usage/linters/ + [golint]: https://github.com/golang/lint + diff --git a/src/map-init.md b/src/map-init.md new file mode 100644 index 0000000..5fecf0e --- /dev/null +++ b/src/map-init.md @@ -0,0 +1,80 @@ +# Initializing Maps + +Prefer `make(..)` for empty maps, and maps populated +programmatically. This makes map initialization visually +distinct from declaration, and it makes it easy to add size +hints later if available. + + + + + + +
BadGood
+ +```go +var ( + // m1 is safe to read and write; + // m2 will panic on writes. + m1 = map[T1]T2{} + m2 map[T1]T2 +) +``` + + + +```go +var ( + // m1 is safe to read and write; + // m2 will panic on writes. + m1 = make(map[T1]T2) + m2 map[T1]T2 +) +``` + +
+ +Declaration and initialization are visually similar. + + + +Declaration and initialization are visually distinct. + +
+ +Where possible, provide capacity hints when initializing +maps with `make()`. See +[Specifying Map Capacity Hints](container-capacity.md#specifying-map-capacity-hints) +for more information. + +On the other hand, if the map holds a fixed list of elements, +use map literals to initialize the map. + + + + + +
BadGood
+ +```go +m := make(map[T1]T2, 3) +m[k1] = v1 +m[k2] = v2 +m[k3] = v3 +``` + + + +```go +m := map[T1]T2{ + k1: v1, + k2: v2, + k3: v3, +} +``` + +
+ +The basic rule of thumb is to use map literals when adding a fixed set of +elements at initialization time, otherwise use `make` (and specify a size hint +if available). diff --git a/src/mutex-zero-value.md b/src/mutex-zero-value.md new file mode 100644 index 0000000..86c7b68 --- /dev/null +++ b/src/mutex-zero-value.md @@ -0,0 +1,91 @@ +# Zero-value Mutexes are Valid + +The zero-value of `sync.Mutex` and `sync.RWMutex` is valid, so you almost +never need a pointer to a mutex. + + + + + +
BadGood
+ +```go +mu := new(sync.Mutex) +mu.Lock() +``` + + + +```go +var mu sync.Mutex +mu.Lock() +``` + +
+ +If you use a struct by pointer, then the mutex should be a non-pointer field on +it. Do not embed the mutex on the struct, even if the struct is not exported. + + + + + + + +
BadGood
+ +```go +type SMap struct { + sync.Mutex + + data map[string]string +} + +func NewSMap() *SMap { + return &SMap{ + data: make(map[string]string), + } +} + +func (m *SMap) Get(k string) string { + m.Lock() + defer m.Unlock() + + return m.data[k] +} +``` + + + +```go +type SMap struct { + mu sync.Mutex + + data map[string]string +} + +func NewSMap() *SMap { + return &SMap{ + data: make(map[string]string), + } +} + +func (m *SMap) Get(k string) string { + m.mu.Lock() + defer m.mu.Unlock() + + return m.data[k] +} +``` + +
+ +The `Mutex` field, and the `Lock` and `Unlock` methods are unintentionally part +of the exported API of `SMap`. + + + +The mutex and its methods are implementation details of `SMap` hidden from its +callers. + +
diff --git a/src/nest-less.md b/src/nest-less.md new file mode 100644 index 0000000..93d23c5 --- /dev/null +++ b/src/nest-less.md @@ -0,0 +1,45 @@ +# Reduce Nesting + +Code should reduce nesting where possible by handling error cases/special +conditions first and returning early or continuing the loop. Reduce the amount +of code that is nested multiple levels. + + + + + +
BadGood
+ +```go +for _, v := range data { + if v.F1 == 1 { + v = process(v) + if err := v.Call(); err == nil { + v.Send() + } else { + return err + } + } else { + log.Printf("Invalid v: %v", v) + } +} +``` + + + +```go +for _, v := range data { + if v.F1 != 1 { + log.Printf("Invalid v: %v", v) + continue + } + + v = process(v) + if err := v.Call(); err != nil { + return err + } + v.Send() +} +``` + +
diff --git a/src/package-name.md b/src/package-name.md new file mode 100644 index 0000000..edc1220 --- /dev/null +++ b/src/package-name.md @@ -0,0 +1,15 @@ +# Package Names + +When naming packages, choose a name that is: + +- All lower-case. No capitals or underscores. +- Does not need to be renamed using named imports at most call sites. +- Short and succinct. Remember that the name is identified in full at every call + site. +- Not plural. For example, `net/url`, not `net/urls`. +- Not "common", "util", "shared", or "lib". These are bad, uninformative names. + +See also [Package Names] and [Style guideline for Go packages]. + + [Package Names]: https://go.dev/blog/package-names + [Style guideline for Go packages]: https://rakyll.org/style-packages/ diff --git a/src/panic.md b/src/panic.md new file mode 100644 index 0000000..62c5808 --- /dev/null +++ b/src/panic.md @@ -0,0 +1,87 @@ +# Don't Panic + +Code running in production must avoid panics. Panics are a major source of +[cascading failures]. If an error occurs, the function must return an error and +allow the caller to decide how to handle it. + + [cascading failures]: https://en.wikipedia.org/wiki/Cascading_failure + + + + + +
BadGood
+ +```go +func run(args []string) { + if len(args) == 0 { + panic("an argument is required") + } + // ... +} + +func main() { + run(os.Args[1:]) +} +``` + + + +```go +func run(args []string) error { + if len(args) == 0 { + return errors.New("an argument is required") + } + // ... + return nil +} + +func main() { + if err := run(os.Args[1:]); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} +``` + +
+ +Panic/recover is not an error handling strategy. A program must panic only when +something irrecoverable happens such as a nil dereference. An exception to this is +program initialization: bad things at program startup that should abort the +program may cause panic. + +```go +var _statusTemplate = template.Must(template.New("name").Parse("_statusHTML")) +``` + +Even in tests, prefer `t.Fatal` or `t.FailNow` over panics to ensure that the +test is marked as failed. + + + + + +
BadGood
+ +```go +// func TestFoo(t *testing.T) + +f, err := os.CreateTemp("", "test") +if err != nil { + panic("failed to set up test") +} +``` + + + +```go +// func TestFoo(t *testing.T) + +f, err := os.CreateTemp("", "test") +if err != nil { + t.Fatal("failed to set up test") +} +``` + +
diff --git a/src/param-naked.md b/src/param-naked.md new file mode 100644 index 0000000..c1b969e --- /dev/null +++ b/src/param-naked.md @@ -0,0 +1,49 @@ +# Avoid Naked Parameters + +Naked parameters in function calls can hurt readability. Add C-style comments +(`/* ... */`) for parameter names when their meaning is not obvious. + + + + + +
BadGood
+ +```go +// func printInfo(name string, isLocal, done bool) + +printInfo("foo", true, true) +``` + + + +```go +// func printInfo(name string, isLocal, done bool) + +printInfo("foo", true /* isLocal */, true /* done */) +``` + +
+ +Better yet, replace naked `bool` types with custom types for more readable and +type-safe code. This allows more than just two states (true/false) for that +parameter in the future. + +```go +type Region int + +const ( + UnknownRegion Region = iota + Local +) + +type Status int + +const ( + StatusReady Status = iota + 1 + StatusDone + // Maybe we will have a StatusInProgress in the future. +) + +func printInfo(name string, region Region, status Status) +``` diff --git a/src/performance.md b/src/performance.md new file mode 100644 index 0000000..88edb7e --- /dev/null +++ b/src/performance.md @@ -0,0 +1,3 @@ +# Performance + +Performance-specific guidelines apply only to the hot path. diff --git a/src/preface.txt b/src/preface.txt new file mode 100644 index 0000000..357b1b2 --- /dev/null +++ b/src/preface.txt @@ -0,0 +1,7 @@ + + + + diff --git a/src/printf-const.md b/src/printf-const.md new file mode 100644 index 0000000..4ad19ac --- /dev/null +++ b/src/printf-const.md @@ -0,0 +1,26 @@ +# Format Strings outside Printf + +If you declare format strings for `Printf`-style functions outside a string +literal, make them `const` values. + +This helps `go vet` perform static analysis of the format string. + + + + + +
BadGood
+ +```go +msg := "unexpected values %v, %v\n" +fmt.Printf(msg, 1, 2) +``` + + + +```go +const msg = "unexpected values %v, %v\n" +fmt.Printf(msg, 1, 2) +``` + +
diff --git a/src/printf-name.md b/src/printf-name.md new file mode 100644 index 0000000..4fa0311 --- /dev/null +++ b/src/printf-name.md @@ -0,0 +1,22 @@ +# Naming Printf-style Functions + +When you declare a `Printf`-style function, make sure that `go vet` can detect +it and check the format string. + +This means that you should use predefined `Printf`-style function +names if possible. `go vet` will check these by default. See [Printf family] +for more information. + + [Printf family]: https://pkg.go.dev/cmd/vet#hdr-Printf_family + +If using the predefined names is not an option, end the name you choose with +f: `Wrapf`, not `Wrap`. `go vet` can be asked to check specific `Printf`-style +names but they must end with f. + +```shell +go vet -printfuncs=wrapf,statusf +``` + +See also [go vet: Printf family check]. + + [go vet: Printf family check]: https://kuzminva.wordpress.com/2017/11/07/go-vet-printf-family-check/ diff --git a/src/slice-nil.md b/src/slice-nil.md new file mode 100644 index 0000000..3cd9e30 --- /dev/null +++ b/src/slice-nil.md @@ -0,0 +1,95 @@ +# nil is a valid slice + +`nil` is a valid slice of length 0. This means that, + +- You should not return a slice of length zero explicitly. Return `nil` + instead. + + + + + +
BadGood
+ + ```go + if x == "" { + return []int{} + } + ``` + + + + ```go + if x == "" { + return nil + } + ``` + +
+ +- To check if a slice is empty, always use `len(s) == 0`. Do not check for + `nil`. + + + + + +
BadGood
+ + ```go + func isEmpty(s []string) bool { + return s == nil + } + ``` + + + + ```go + func isEmpty(s []string) bool { + return len(s) == 0 + } + ``` + +
+ +- The zero value (a slice declared with `var`) is usable immediately without + `make()`. + + + + + +
BadGood
+ + ```go + nums := []int{} + // or, nums := make([]int) + + if add1 { + nums = append(nums, 1) + } + + if add2 { + nums = append(nums, 2) + } + ``` + + + + ```go + var nums []int + + if add1 { + nums = append(nums, 1) + } + + if add2 { + nums = append(nums, 2) + } + ``` + +
+ +Remember that, while it is a valid slice, a nil slice is not equivalent to an +allocated slice of length 0 - one is nil and the other is not - and the two may +be treated differently in different situations (such as serialization). diff --git a/src/strconv.md b/src/strconv.md new file mode 100644 index 0000000..b73d0ca --- /dev/null +++ b/src/strconv.md @@ -0,0 +1,39 @@ +# Prefer strconv over fmt + +When converting primitives to/from strings, `strconv` is faster than +`fmt`. + + + + + + +
BadGood
+ +```go +for i := 0; i < b.N; i++ { + s := fmt.Sprint(rand.Int()) +} +``` + + + +```go +for i := 0; i < b.N; i++ { + s := strconv.Itoa(rand.Int()) +} +``` + +
+ +```plain +BenchmarkFmtSprint-4 143 ns/op 2 allocs/op +``` + + + +```plain +BenchmarkStrconv-4 64.2 ns/op 1 allocs/op +``` + +
diff --git a/src/string-byte-slice.md b/src/string-byte-slice.md new file mode 100644 index 0000000..078e714 --- /dev/null +++ b/src/string-byte-slice.md @@ -0,0 +1,40 @@ +# Avoid repeated string-to-byte conversions + +Do not create byte slices from a fixed string repeatedly. Instead, perform the +conversion once and capture the result. + + + + + + +
BadGood
+ +```go +for i := 0; i < b.N; i++ { + w.Write([]byte("Hello world")) +} +``` + + + +```go +data := []byte("Hello world") +for i := 0; i < b.N; i++ { + w.Write(data) +} +``` + +
+ +```plain +BenchmarkBad-4 50000000 22.2 ns/op +``` + + + +```plain +BenchmarkGood-4 500000000 3.25 ns/op +``` + +
diff --git a/src/string-escape.md b/src/string-escape.md new file mode 100644 index 0000000..1c07835 --- /dev/null +++ b/src/string-escape.md @@ -0,0 +1,23 @@ +# Use Raw String Literals to Avoid Escaping + +Go supports [raw string literals](https://go.dev/ref/spec#raw_string_lit), +which can span multiple lines and include quotes. Use these to avoid +hand-escaped strings which are much harder to read. + + + + + +
BadGood
+ +```go +wantError := "unknown name:\"test\"" +``` + + + +```go +wantError := `unknown error:"test"` +``` + +
diff --git a/src/struct-embed.md b/src/struct-embed.md new file mode 100644 index 0000000..88b1a81 --- /dev/null +++ b/src/struct-embed.md @@ -0,0 +1,156 @@ +# Embedding in Structs + +Embedded types should be at the top of the field list of a +struct, and there must be an empty line separating embedded fields from regular +fields. + + + + + +
BadGood
+ +```go +type Client struct { + version int + http.Client +} +``` + + + +```go +type Client struct { + http.Client + + version int +} +``` + +
+ +Embedding should provide tangible benefit, like adding or augmenting +functionality in a semantically-appropriate way. It should do this with zero +adverse user-facing effects (see also: [Avoid Embedding Types in Public Structs](embed-public.md)). + +Exception: Mutexes should not be embedded, even on unexported types. See also: [Zero-value Mutexes are Valid](mutex-zero-value.md). + +Embedding **should not**: + +- Be purely cosmetic or convenience-oriented. +- Make outer types more difficult to construct or use. +- Affect outer types' zero values. If the outer type has a useful zero value, it + should still have a useful zero value after embedding the inner type. +- Expose unrelated functions or fields from the outer type as a side-effect of + embedding the inner type. +- Expose unexported types. +- Affect outer types' copy semantics. +- Change the outer type's API or type semantics. +- Embed a non-canonical form of the inner type. +- Expose implementation details of the outer type. +- Allow users to observe or control type internals. +- Change the general behavior of inner functions through wrapping in a way that + would reasonably surprise users. + +Simply put, embed consciously and intentionally. A good litmus test is, "would +all of these exported inner methods/fields be added directly to the outer type"; +if the answer is "some" or "no", don't embed the inner type - use a field +instead. + + + + + + + +
BadGood
+ +```go +type A struct { + // Bad: A.Lock() and A.Unlock() are + // now available, provide no + // functional benefit, and allow + // users to control details about + // the internals of A. + sync.Mutex +} +``` + + + +```go +type countingWriteCloser struct { + // Good: Write() is provided at this + // outer layer for a specific + // purpose, and delegates work + // to the inner type's Write(). + io.WriteCloser + + count int +} + +func (w *countingWriteCloser) Write(bs []byte) (int, error) { + w.count += len(bs) + return w.WriteCloser.Write(bs) +} +``` + +
+ +```go +type Book struct { + // Bad: pointer changes zero value usefulness + io.ReadWriter + + // other fields +} + +// later + +var b Book +b.Read(...) // panic: nil pointer +b.String() // panic: nil pointer +b.Write(...) // panic: nil pointer +``` + + + +```go +type Book struct { + // Good: has useful zero value + bytes.Buffer + + // other fields +} + +// later + +var b Book +b.Read(...) // ok +b.String() // ok +b.Write(...) // ok +``` + +
+ +```go +type Client struct { + sync.Mutex + sync.WaitGroup + bytes.Buffer + url.URL +} +``` + + + +```go +type Client struct { + mtx sync.Mutex + wg sync.WaitGroup + buf bytes.Buffer + url url.URL +} +``` + +
diff --git a/src/struct-field-key.md b/src/struct-field-key.md new file mode 100644 index 0000000..f08b077 --- /dev/null +++ b/src/struct-field-key.md @@ -0,0 +1,41 @@ +# Use Field Names to Initialize Structs + +You should almost always specify field names when initializing structs. This is +now enforced by [`go vet`]. + + [`go vet`]: https://pkg.go.dev/cmd/vet + + + + + +
BadGood
+ +```go +k := User{"John", "Doe", true} +``` + + + +```go +k := User{ + FirstName: "John", + LastName: "Doe", + Admin: true, +} +``` + +
+ +Exception: Field names *may* be omitted in test tables when there are 3 or +fewer fields. + +```go +tests := []struct{ + op Operation + want string +}{ + {Add, "add"}, + {Subtract, "subtract"}, +} +``` diff --git a/src/struct-field-zero.md b/src/struct-field-zero.md new file mode 100644 index 0000000..736c17a --- /dev/null +++ b/src/struct-field-zero.md @@ -0,0 +1,48 @@ +# Omit Zero Value Fields in Structs + +When initializing structs with field names, omit fields that have zero values +unless they provide meaningful context. Otherwise, let Go set these to zero +values automatically. + + + + + +
BadGood
+ +```go +user := User{ + FirstName: "John", + LastName: "Doe", + MiddleName: "", + Admin: false, +} +``` + + + +```go +user := User{ + FirstName: "John", + LastName: "Doe", +} +``` + +
+ +This helps reduce noise for readers by omitting values that are default in +that context. Only meaningful values are specified. + +Include zero values where field names provide meaningful context. For example, +test cases in [Test Tables](test-table.md) can benefit from names of fields +even when they are zero-valued. + +```go +tests := []struct{ + give string + want int +}{ + {give: "0", want: 0}, + // ... +} +``` diff --git a/src/struct-pointer.md b/src/struct-pointer.md new file mode 100644 index 0000000..19230fe --- /dev/null +++ b/src/struct-pointer.md @@ -0,0 +1,28 @@ +# Initializing Struct References + +Use `&T{}` instead of `new(T)` when initializing struct references so that it +is consistent with the struct initialization. + + + + + +
BadGood
+ +```go +sval := T{Name: "foo"} + +// inconsistent +sptr := new(T) +sptr.Name = "bar" +``` + + + +```go +sval := T{Name: "foo"} + +sptr := &T{Name: "bar"} +``` + +
diff --git a/src/struct-tag.md b/src/struct-tag.md new file mode 100644 index 0000000..6cfe2df --- /dev/null +++ b/src/struct-tag.md @@ -0,0 +1,47 @@ +# Use field tags in marshaled structs + +Any struct field that is marshaled into JSON, YAML, +or other formats that support tag-based field naming +should be annotated with the relevant tag. + + + + + +
BadGood
+ +```go +type Stock struct { + Price int + Name string +} + +bytes, err := json.Marshal(Stock{ + Price: 137, + Name: "UBER", +}) +``` + + + +```go +type Stock struct { + Price int `json:"price"` + Name string `json:"name"` + // Safe to rename Name to Symbol. +} + +bytes, err := json.Marshal(Stock{ + Price: 137, + Name: "UBER", +}) +``` + +
+ +Rationale: +The serialized form of the structure is a contract between different systems. +Changes to the structure of the serialized form--including field names--break +this contract. Specifying field names inside tags makes the contract explicit, +and it guards against accidentally breaking the contract by refactoring or +renaming fields. diff --git a/src/struct-zero.md b/src/struct-zero.md new file mode 100644 index 0000000..461adae --- /dev/null +++ b/src/struct-zero.md @@ -0,0 +1,28 @@ +# Use `var` for Zero Value Structs + +When all the fields of a struct are omitted in a declaration, use the `var` +form to declare the struct. + + + + + +
BadGood
+ +```go +user := User{} +``` + + + +```go +var user User +``` + +
+ +This differentiates zero valued structs from those with non-zero fields +similar to the distinction created for [map initialization](map-init.md), and matches how +we prefer to [declare empty slices]. + + [declare empty slices]: https://go.dev/wiki/CodeReviewComments#declaring-empty-slices diff --git a/src/test-table.md b/src/test-table.md new file mode 100644 index 0000000..6847225 --- /dev/null +++ b/src/test-table.md @@ -0,0 +1,267 @@ +# Test Tables + +Table-driven tests with [subtests] can be a helpful pattern for writing tests +to avoid duplicating code when the core test logic is repetitive. + +If a system under test needs to be tested against _multiple conditions_ where +certain parts of the inputs and outputs change, a table-driven test should +be used to reduce redundancy and improve readability. + + [subtests]: https://go.dev/blog/subtests + + + + + +
BadGood
+ +```go +// func TestSplitHostPort(t *testing.T) + +host, port, err := net.SplitHostPort("192.0.2.0:8000") +require.NoError(t, err) +assert.Equal(t, "192.0.2.0", host) +assert.Equal(t, "8000", port) + +host, port, err = net.SplitHostPort("192.0.2.0:http") +require.NoError(t, err) +assert.Equal(t, "192.0.2.0", host) +assert.Equal(t, "http", port) + +host, port, err = net.SplitHostPort(":8000") +require.NoError(t, err) +assert.Equal(t, "", host) +assert.Equal(t, "8000", port) + +host, port, err = net.SplitHostPort("1:8") +require.NoError(t, err) +assert.Equal(t, "1", host) +assert.Equal(t, "8", port) +``` + + + +```go +// func TestSplitHostPort(t *testing.T) + +tests := []struct{ + give string + wantHost string + wantPort string +}{ + { + give: "192.0.2.0:8000", + wantHost: "192.0.2.0", + wantPort: "8000", + }, + { + give: "192.0.2.0:http", + wantHost: "192.0.2.0", + wantPort: "http", + }, + { + give: ":8000", + wantHost: "", + wantPort: "8000", + }, + { + give: "1:8", + wantHost: "1", + wantPort: "8", + }, +} + +for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + host, port, err := net.SplitHostPort(tt.give) + require.NoError(t, err) + assert.Equal(t, tt.wantHost, host) + assert.Equal(t, tt.wantPort, port) + }) +} +``` + +
+ +Test tables make it easier to add context to error messages, reduce duplicate +logic, and add new test cases. + +We follow the convention that the slice of structs is referred to as `tests` +and each test case `tt`. Further, we encourage explicating the input and output +values for each test case with `give` and `want` prefixes. + +```go +tests := []struct{ + give string + wantHost string + wantPort string +}{ + // ... +} + +for _, tt := range tests { + // ... +} +``` + +## Avoid Unnecessary Complexity in Table Tests + +Table tests can be difficult to read and maintain if the subtests contain conditional +assertions or other branching logic. Table tests should **NOT** be used whenever +there needs to be complex or conditional logic inside subtests (i.e. complex logic inside the `for` loop). + +Large, complex table tests harm readability and maintainability because test readers may +have difficulty debugging test failures that occur. + +Table tests like this should be split into either multiple test tables or multiple +individual `Test...` functions. + +Some ideals to aim for are: + +* Focus on the narrowest unit of behavior +* Minimize "test depth", and avoid conditional assertions (see below) +* Ensure that all table fields are used in all tests +* Ensure that all test logic runs for all table cases + +In this context, "test depth" means "within a given test, the number of +successive assertions that require previous assertions to hold" (similar +to cyclomatic complexity). +Having "shallower" tests means that there are fewer relationships between +assertions and, more importantly, that those assertions are less likely +to be conditional by default. + +Concretely, table tests can become confusing and difficult to read if they use multiple branching +pathways (e.g. `shouldError`, `expectCall`, etc.), use many `if` statements for +specific mock expectations (e.g. `shouldCallFoo`), or place functions inside the +table (e.g. `setupMocks func(*FooMock)`). + +However, when testing behavior that only +changes based on changed input, it may be preferable to group similar cases +together in a table test to better illustrate how behavior changes across all inputs, +rather than splitting otherwise comparable units into separate tests +and making them harder to compare and contrast. + +If the test body is short and straightforward, +it's acceptable to have a single branching pathway for success versus failure cases +with a table field like `shouldErr` to specify error expectations. + + + + + +
BadGood
+ +```go +func TestComplicatedTable(t *testing.T) { + tests := []struct { + give string + want string + wantErr error + shouldCallX bool + shouldCallY bool + giveXResponse string + giveXErr error + giveYResponse string + giveYErr error + }{ + // ... + } + + for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + if tt.shouldCallX { + xMock.EXPECT().Call().Return( + tt.giveXResponse, tt.giveXErr, + ) + } + yMock := ymock.NewMockY(ctrl) + if tt.shouldCallY { + yMock.EXPECT().Call().Return( + tt.giveYResponse, tt.giveYErr, + ) + } + + got, err := DoComplexThing(tt.give, xMock, yMock) + + // verify results + if tt.wantErr != nil { + require.EqualError(t, err, tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, want, got) + }) + } +} +``` + + + +```go +func TestShouldCallX(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + xMock.EXPECT().Call().Return("XResponse", nil) + + yMock := ymock.NewMockY(ctrl) + + got, err := DoComplexThing("inputX", xMock, yMock) + + require.NoError(t, err) + assert.Equal(t, "want", got) +} + +func TestShouldCallYAndFail(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + + yMock := ymock.NewMockY(ctrl) + yMock.EXPECT().Call().Return("YResponse", nil) + + _, err := DoComplexThing("inputY", xMock, yMock) + assert.EqualError(t, err, "Y failed") +} +``` +
+ +This complexity makes it more difficult to change, understand, and prove the +correctness of the test. + +While there are no strict guidelines, readability and maintainability should +always be top-of-mind when deciding between Table Tests versus separate tests +for multiple inputs/outputs to a system. + +## Parallel Tests + +Parallel tests, like some specialized loops (for example, those that spawn +goroutines or capture references as part of the loop body), +must take care to explicitly assign loop variables within the loop's scope to +ensure that they hold the expected values. + +```go +tests := []struct{ + give string + // ... +}{ + // ... +} + +for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + t.Parallel() + // ... + }) +} +``` + +In the example above, we must declare a `tt` variable scoped to the loop +iteration because of the use of `t.Parallel()` below. +If we do not do that, most or all tests will receive an unexpected value for +`tt`, or a value that changes as they're running. + + diff --git a/src/time.md b/src/time.md new file mode 100644 index 0000000..e7225e8 --- /dev/null +++ b/src/time.md @@ -0,0 +1,168 @@ +# Use `"time"` to handle time + +Time is complicated. Incorrect assumptions often made about time include the +following. + +1. A day has 24 hours +2. An hour has 60 minutes +3. A week has 7 days +4. A year has 365 days +5. [And a lot more](https://infiniteundo.com/post/25326999628/falsehoods-programmers-believe-about-time) + +For example, *1* means that adding 24 hours to a time instant will not always +yield a new calendar day. + +Therefore, always use the [`"time"`] package when dealing with time because it +helps deal with these incorrect assumptions in a safer, more accurate manner. + + [`"time"`]: https://pkg.go.dev/time + +## Use `time.Time` for instants of time + +Use [`time.Time`] when dealing with instants of time, and the methods on +`time.Time` when comparing, adding, or subtracting time. + + [`time.Time`]: https://pkg.go.dev/time#Time + + + + + +
BadGood
+ +```go +func isActive(now, start, stop int) bool { + return start <= now && now < stop +} +``` + + + +```go +func isActive(now, start, stop time.Time) bool { + return (start.Before(now) || start.Equal(now)) && now.Before(stop) +} +``` + +
+ +## Use `time.Duration` for periods of time + +Use [`time.Duration`] when dealing with periods of time. + + [`time.Duration`]: https://pkg.go.dev/time#Duration + + + + + +
BadGood
+ +```go +func poll(delay int) { + for { + // ... + time.Sleep(time.Duration(delay) * time.Millisecond) + } +} + +poll(10) // was it seconds or milliseconds? +``` + + + +```go +func poll(delay time.Duration) { + for { + // ... + time.Sleep(delay) + } +} + +poll(10*time.Second) +``` + +
+ +Going back to the example of adding 24 hours to a time instant, the method we +use to add time depends on intent. If we want the same time of the day, but on +the next calendar day, we should use [`Time.AddDate`]. However, if we want an +instant of time guaranteed to be 24 hours after the previous time, we should +use [`Time.Add`]. + + [`Time.AddDate`]: https://pkg.go.dev/time#Time.AddDate + [`Time.Add`]: https://pkg.go.dev/time#Time.Add + +```go +newDay := t.AddDate(0 /* years */, 0 /* months */, 1 /* days */) +maybeNewDay := t.Add(24 * time.Hour) +``` + +## Use `time.Time` and `time.Duration` with external systems + +Use `time.Duration` and `time.Time` in interactions with external systems when +possible. For example: + +- Command-line flags: [`flag`] supports `time.Duration` via + [`time.ParseDuration`] +- JSON: [`encoding/json`] supports encoding `time.Time` as an [RFC 3339] + string via its [`UnmarshalJSON` method] +- SQL: [`database/sql`] supports converting `DATETIME` or `TIMESTAMP` columns + into `time.Time` and back if the underlying driver supports it +- YAML: [`gopkg.in/yaml.v2`] supports `time.Time` as an [RFC 3339] string, and + `time.Duration` via [`time.ParseDuration`]. + + [`flag`]: https://pkg.go.dev/flag + [`time.ParseDuration`]: https://pkg.go.dev/time#ParseDuration + [`encoding/json`]: https://pkg.go.dev/encoding/json + [RFC 3339]: https://tools.ietf.org/html/rfc3339 + [`UnmarshalJSON` method]: https://pkg.go.dev/time#Time.UnmarshalJSON + [`database/sql`]: https://pkg.go.dev/database/sql + [`gopkg.in/yaml.v2`]: https://pkg.go.dev/gopkg.in/yaml.v2 + +When it is not possible to use `time.Duration` in these interactions, use +`int` or `float64` and include the unit in the name of the field. + +For example, since `encoding/json` does not support `time.Duration`, the unit +is included in the name of the field. + + + + + +
BadGood
+ +```go +// {"interval": 2} +type Config struct { + Interval int `json:"interval"` +} +``` + + + +```go +// {"intervalMillis": 2000} +type Config struct { + IntervalMillis int `json:"intervalMillis"` +} +``` + +
+ +When it is not possible to use `time.Time` in these interactions, unless an +alternative is agreed upon, use `string` and format timestamps as defined in +[RFC 3339]. This format is used by default by [`Time.UnmarshalText`] and is +available for use in `Time.Format` and `time.Parse` via [`time.RFC3339`]. + + [`Time.UnmarshalText`]: https://pkg.go.dev/time#Time.UnmarshalText + [`time.RFC3339`]: https://pkg.go.dev/time#RFC3339 + +Although this tends to not be a problem in practice, keep in mind that the +`"time"` package does not support parsing timestamps with leap seconds +([8728]), nor does it account for leap seconds in calculations ([15190]). If +you compare two instants of time, the difference will not include the leap +seconds that may have occurred between those two instants. + + [8728]: https://github.com/golang/go/issues/8728 + [15190]: https://github.com/golang/go/issues/15190 diff --git a/src/type-assert.md b/src/type-assert.md new file mode 100644 index 0000000..87ae89c --- /dev/null +++ b/src/type-assert.md @@ -0,0 +1,30 @@ +# Handle Type Assertion Failures + +The single return value form of a [type assertion] will panic on an incorrect +type. Therefore, always use the "comma ok" idiom. + + [type assertion]: https://go.dev/ref/spec#Type_assertions + + + + + +
BadGood
+ +```go +t := i.(string) +``` + + + +```go +t, ok := i.(string) +if !ok { + // handle the error gracefully +} +``` + +
+ + diff --git a/src/var-decl.md b/src/var-decl.md new file mode 100644 index 0000000..780a018 --- /dev/null +++ b/src/var-decl.md @@ -0,0 +1,59 @@ +# Local Variable Declarations + +Short variable declarations (`:=`) should be used if a variable is being set to +some value explicitly. + + + + + +
BadGood
+ +```go +var s = "foo" +``` + + + +```go +s := "foo" +``` + +
+ +However, there are cases where the default value is clearer when the `var` +keyword is used. [Declaring Empty Slices], for example. + + [Declaring Empty Slices]: https://go.dev/wiki/CodeReviewComments#declaring-empty-slices + + + + + +
BadGood
+ +```go +func f(list []int) { + filtered := []int{} + for _, v := range list { + if v > 10 { + filtered = append(filtered, v) + } + } +} +``` + + + +```go +func f(list []int) { + var filtered []int + for _, v := range list { + if v > 10 { + filtered = append(filtered, v) + } + } +} +``` + +
diff --git a/src/var-scope.md b/src/var-scope.md new file mode 100644 index 0000000..f3ae6d5 --- /dev/null +++ b/src/var-scope.md @@ -0,0 +1,102 @@ +# Reduce Scope of Variables + +Where possible, reduce scope of variables and constants. Do not reduce the scope if it +conflicts with [Reduce Nesting](nest-less.md). + + + + + +
BadGood
+ +```go +err := os.WriteFile(name, data, 0644) +if err != nil { + return err +} +``` + + + +```go +if err := os.WriteFile(name, data, 0644); err != nil { + return err +} +``` + +
+ +If you need a result of a function call outside of the if, then you should not +try to reduce the scope. + + + + + +
BadGood
+ +```go +if data, err := os.ReadFile(name); err == nil { + err = cfg.Decode(data) + if err != nil { + return err + } + + fmt.Println(cfg) + return nil +} else { + return err +} +``` + + + +```go +data, err := os.ReadFile(name) +if err != nil { + return err +} + +if err := cfg.Decode(data); err != nil { + return err +} + +fmt.Println(cfg) +return nil +``` + +
+ +Constants do not need to be global unless they are used in multiple functions or files +or are part of an external contract of the package. + + + + + +
BadGood
+ +```go +const ( + _defaultPort = 8080 + _defaultUser = "user" +) + +func Bar() { + fmt.Println("Default port", _defaultPort) +} +``` + + + +```go +func Bar() { + const ( + defaultPort = 8080 + defaultUser = "user" + ) + fmt.Println("Default port", defaultPort) +} +``` + +
diff --git a/style.md b/style.md new file mode 100644 index 0000000..818eab6 --- /dev/null +++ b/style.md @@ -0,0 +1,4107 @@ + + + + +# Uber Go Style Guide + +- [Introduction](#introduction) +- [Guidelines](#guidelines) + - [Pointers to Interfaces](#pointers-to-interfaces) + - [Verify Interface Compliance](#verify-interface-compliance) + - [Receivers and Interfaces](#receivers-and-interfaces) + - [Zero-value Mutexes are Valid](#zero-value-mutexes-are-valid) + - [Copy Slices and Maps at Boundaries](#copy-slices-and-maps-at-boundaries) + - [Defer to Clean Up](#defer-to-clean-up) + - [Channel Size is One or None](#channel-size-is-one-or-none) + - [Start Enums at One](#start-enums-at-one) + - [Use `"time"` to handle time](#use-time-to-handle-time) + - [Errors](#errors) + - [Error Types](#error-types) + - [Error Wrapping](#error-wrapping) + - [Error Naming](#error-naming) + - [Handle Errors Once](#handle-errors-once) + - [Handle Type Assertion Failures](#handle-type-assertion-failures) + - [Don't Panic](#dont-panic) + - [Use go.uber.org/atomic](#use-gouberorgatomic) + - [Avoid Mutable Globals](#avoid-mutable-globals) + - [Avoid Embedding Types in Public Structs](#avoid-embedding-types-in-public-structs) + - [Avoid Using Built-In Names](#avoid-using-built-in-names) + - [Avoid `init()`](#avoid-init) + - [Exit in Main](#exit-in-main) + - [Exit Once](#exit-once) + - [Use field tags in marshaled structs](#use-field-tags-in-marshaled-structs) + - [Don't fire-and-forget goroutines](#dont-fire-and-forget-goroutines) + - [Wait for goroutines to exit](#wait-for-goroutines-to-exit) + - [No goroutines in `init()`](#no-goroutines-in-init) +- [Performance](#performance) + - [Prefer strconv over fmt](#prefer-strconv-over-fmt) + - [Avoid repeated string-to-byte conversions](#avoid-repeated-string-to-byte-conversions) + - [Prefer Specifying Container Capacity](#prefer-specifying-container-capacity) +- [Style](#style) + - [Avoid overly long lines](#avoid-overly-long-lines) + - [Be Consistent](#be-consistent) + - [Group Similar Declarations](#group-similar-declarations) + - [Import Group Ordering](#import-group-ordering) + - [Package Names](#package-names) + - [Function Names](#function-names) + - [Import Aliasing](#import-aliasing) + - [Function Grouping and Ordering](#function-grouping-and-ordering) + - [Reduce Nesting](#reduce-nesting) + - [Unnecessary Else](#unnecessary-else) + - [Top-level Variable Declarations](#top-level-variable-declarations) + - [Prefix Unexported Globals with _](#prefix-unexported-globals-with-_) + - [Embedding in Structs](#embedding-in-structs) + - [Local Variable Declarations](#local-variable-declarations) + - [nil is a valid slice](#nil-is-a-valid-slice) + - [Reduce Scope of Variables](#reduce-scope-of-variables) + - [Avoid Naked Parameters](#avoid-naked-parameters) + - [Use Raw String Literals to Avoid Escaping](#use-raw-string-literals-to-avoid-escaping) + - [Initializing Structs](#initializing-structs) + - [Use Field Names to Initialize Structs](#use-field-names-to-initialize-structs) + - [Omit Zero Value Fields in Structs](#omit-zero-value-fields-in-structs) + - [Use `var` for Zero Value Structs](#use-var-for-zero-value-structs) + - [Initializing Struct References](#initializing-struct-references) + - [Initializing Maps](#initializing-maps) + - [Format Strings outside Printf](#format-strings-outside-printf) + - [Naming Printf-style Functions](#naming-printf-style-functions) +- [Patterns](#patterns) + - [Test Tables](#test-tables) + - [Functional Options](#functional-options) +- [Linting](#linting) + +## Introduction + +Styles are the conventions that govern our code. The term style is a bit of a +misnomer, since these conventions cover far more than just source file +formatting—gofmt handles that for us. + +The goal of this guide is to manage this complexity by describing in detail the +Dos and Don'ts of writing Go code at Uber. These rules exist to keep the code +base manageable while still allowing engineers to use Go language features +productively. + +This guide was originally created by [Prashant Varanasi](https://github.com/prashantv) and [Simon Newton](https://github.com/nomis52) as +a way to bring some colleagues up to speed with using Go. Over the years it has +been amended based on feedback from others. + +This documents idiomatic conventions in Go code that we follow at Uber. A lot +of these are general guidelines for Go, while others extend upon external +resources: + +1. [Effective Go](https://go.dev/doc/effective_go) +2. [Go Common Mistakes](https://go.dev/wiki/CommonMistakes) +3. [Go Code Review Comments](https://go.dev/wiki/CodeReviewComments) + +We aim for the code samples to be accurate for the two most recent minor versions +of Go [releases](https://go.dev/doc/devel/release). + +All code should be error-free when run through `golint` and `go vet`. We +recommend setting up your editor to: + +- Run `goimports` on save +- Run `golint` and `go vet` to check for errors + +You can find information in editor support for Go tools here: +https://go.dev/wiki/IDEsAndTextEditorPlugins + +## Guidelines + +### Pointers to Interfaces + +You almost never need a pointer to an interface. You should be passing +interfaces as values—the underlying data can still be a pointer. + +An interface is two fields: + +1. A pointer to some type-specific information. You can think of this as + "type." +2. Data pointer. If the data stored is a pointer, it’s stored directly. If + the data stored is a value, then a pointer to the value is stored. + +If you want interface methods to modify the underlying data, you must use a +pointer. + +### Verify Interface Compliance + +Verify interface compliance at compile time where appropriate. This includes: + +- Exported types that are required to implement specific interfaces as part of + their API contract +- Exported or unexported types that are part of a collection of types + implementing the same interface +- Other cases where violating an interface would break users + + + + + +
BadGood
+ +```go +type Handler struct { + // ... +} + + + +func (h *Handler) ServeHTTP( + w http.ResponseWriter, + r *http.Request, +) { + ... +} +``` + + + +```go +type Handler struct { + // ... +} + +var _ http.Handler = (*Handler)(nil) + +func (h *Handler) ServeHTTP( + w http.ResponseWriter, + r *http.Request, +) { + // ... +} +``` + +
+ +The statement `var _ http.Handler = (*Handler)(nil)` will fail to compile if +`*Handler` ever stops matching the `http.Handler` interface. + +The right hand side of the assignment should be the zero value of the asserted +type. This is `nil` for pointer types (like `*Handler`), slices, and maps, and +an empty struct for struct types. + +```go +type LogHandler struct { + h http.Handler + log *zap.Logger +} + +var _ http.Handler = LogHandler{} + +func (h LogHandler) ServeHTTP( + w http.ResponseWriter, + r *http.Request, +) { + // ... +} +``` + +### Receivers and Interfaces + +Methods with value receivers can be called on pointers as well as values. +Methods with pointer receivers can only be called on pointers or [addressable values](https://go.dev/ref/spec#Method_values). + +For example, + +```go +type S struct { + data string +} + +func (s S) Read() string { + return s.data +} + +func (s *S) Write(str string) { + s.data = str +} + +// We cannot get pointers to values stored in maps, because they are not +// addressable values. +sVals := map[int]S{1: {"A"}} + +// We can call Read on values stored in the map because Read +// has a value receiver, which does not require the value to +// be addressable. +sVals[1].Read() + +// We cannot call Write on values stored in the map because Write +// has a pointer receiver, and it's not possible to get a pointer +// to a value stored in a map. +// +// sVals[1].Write("test") + +sPtrs := map[int]*S{1: {"A"}} + +// You can call both Read and Write if the map stores pointers, +// because pointers are intrinsically addressable. +sPtrs[1].Read() +sPtrs[1].Write("test") +``` + +Similarly, an interface can be satisfied by a pointer, even if the method has a +value receiver. + +```go +type F interface { + f() +} + +type S1 struct{} + +func (s S1) f() {} + +type S2 struct{} + +func (s *S2) f() {} + +s1Val := S1{} +s1Ptr := &S1{} +s2Val := S2{} +s2Ptr := &S2{} + +var i F +i = s1Val +i = s1Ptr +i = s2Ptr + +// The following doesn't compile, since s2Val is a value, and there is no value receiver for f. +// i = s2Val +``` + +Effective Go has a good write up on [Pointers vs. Values](https://go.dev/doc/effective_go#pointers_vs_values). + +### Zero-value Mutexes are Valid + +The zero-value of `sync.Mutex` and `sync.RWMutex` is valid, so you almost +never need a pointer to a mutex. + + + + + +
BadGood
+ +```go +mu := new(sync.Mutex) +mu.Lock() +``` + + + +```go +var mu sync.Mutex +mu.Lock() +``` + +
+ +If you use a struct by pointer, then the mutex should be a non-pointer field on +it. Do not embed the mutex on the struct, even if the struct is not exported. + + + + + + + +
BadGood
+ +```go +type SMap struct { + sync.Mutex + + data map[string]string +} + +func NewSMap() *SMap { + return &SMap{ + data: make(map[string]string), + } +} + +func (m *SMap) Get(k string) string { + m.Lock() + defer m.Unlock() + + return m.data[k] +} +``` + + + +```go +type SMap struct { + mu sync.Mutex + + data map[string]string +} + +func NewSMap() *SMap { + return &SMap{ + data: make(map[string]string), + } +} + +func (m *SMap) Get(k string) string { + m.mu.Lock() + defer m.mu.Unlock() + + return m.data[k] +} +``` + +
+ +The `Mutex` field, and the `Lock` and `Unlock` methods are unintentionally part +of the exported API of `SMap`. + + + +The mutex and its methods are implementation details of `SMap` hidden from its +callers. + +
+ +### Copy Slices and Maps at Boundaries + +Slices and maps contain pointers to the underlying data so be wary of scenarios +when they need to be copied. + +#### Receiving Slices and Maps + +Keep in mind that users can modify a map or slice you received as an argument +if you store a reference to it. + + + + + + + + + + +
Bad Good
+ +```go +func (d *Driver) SetTrips(trips []Trip) { + d.trips = trips +} + +trips := ... +d1.SetTrips(trips) + +// Did you mean to modify d1.trips? +trips[0] = ... +``` + + + +```go +func (d *Driver) SetTrips(trips []Trip) { + d.trips = make([]Trip, len(trips)) + copy(d.trips, trips) +} + +trips := ... +d1.SetTrips(trips) + +// We can now modify trips[0] without affecting d1.trips. +trips[0] = ... +``` + +
+ +#### Returning Slices and Maps + +Similarly, be wary of user modifications to maps or slices exposing internal +state. + + + + + +
BadGood
+ +```go +type Stats struct { + mu sync.Mutex + counters map[string]int +} + +// Snapshot returns the current stats. +func (s *Stats) Snapshot() map[string]int { + s.mu.Lock() + defer s.mu.Unlock() + + return s.counters +} + +// snapshot is no longer protected by the mutex, so any +// access to the snapshot is subject to data races. +snapshot := stats.Snapshot() +``` + + + +```go +type Stats struct { + mu sync.Mutex + counters map[string]int +} + +func (s *Stats) Snapshot() map[string]int { + s.mu.Lock() + defer s.mu.Unlock() + + result := make(map[string]int, len(s.counters)) + for k, v := range s.counters { + result[k] = v + } + return result +} + +// Snapshot is now a copy. +snapshot := stats.Snapshot() +``` + +
+ +### Defer to Clean Up + +Use defer to clean up resources such as files and locks. + + + + + +
BadGood
+ +```go +p.Lock() +if p.count < 10 { + p.Unlock() + return p.count +} + +p.count++ +newCount := p.count +p.Unlock() + +return newCount + +// easy to miss unlocks due to multiple returns +``` + + + +```go +p.Lock() +defer p.Unlock() + +if p.count < 10 { + return p.count +} + +p.count++ +return p.count + +// more readable +``` + +
+ +Defer has an extremely small overhead and should be avoided only if you can +prove that your function execution time is in the order of nanoseconds. The +readability win of using defers is worth the miniscule cost of using them. This +is especially true for larger methods that have more than simple memory +accesses, where the other computations are more significant than the `defer`. + +### Channel Size is One or None + +Channels should usually have a size of one or be unbuffered. By default, +channels are unbuffered and have a size of zero. Any other size +must be subject to a high level of scrutiny. Consider how the size is +determined, what prevents the channel from filling up under load and blocking +writers, and what happens when this occurs. + + + + + +
BadGood
+ +```go +// Ought to be enough for anybody! +c := make(chan int, 64) +``` + + + +```go +// Size of one +c := make(chan int, 1) // or +// Unbuffered channel, size of zero +c := make(chan int) +``` + +
+ +### Start Enums at One + +The standard way of introducing enumerations in Go is to declare a custom type +and a `const` group with `iota`. Since variables have a 0 default value, you +should usually start your enums on a non-zero value. + + + + + +
BadGood
+ +```go +type Operation int + +const ( + Add Operation = iota + Subtract + Multiply +) + +// Add=0, Subtract=1, Multiply=2 +``` + + + +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply +) + +// Add=1, Subtract=2, Multiply=3 +``` + +
+ +There are cases where using the zero value makes sense, for example when the +zero value case is the desirable default behavior. + +```go +type LogOutput int + +const ( + LogToStdout LogOutput = iota + LogToFile + LogToRemote +) + +// LogToStdout=0, LogToFile=1, LogToRemote=2 +``` + + + +### Use `"time"` to handle time + +Time is complicated. Incorrect assumptions often made about time include the +following. + +1. A day has 24 hours +2. An hour has 60 minutes +3. A week has 7 days +4. A year has 365 days +5. [And a lot more](https://infiniteundo.com/post/25326999628/falsehoods-programmers-believe-about-time) + +For example, *1* means that adding 24 hours to a time instant will not always +yield a new calendar day. + +Therefore, always use the [`"time"`](https://pkg.go.dev/time) package when dealing with time because it +helps deal with these incorrect assumptions in a safer, more accurate manner. + +#### Use `time.Time` for instants of time + +Use [`time.Time`](https://pkg.go.dev/time#Time) when dealing with instants of time, and the methods on +`time.Time` when comparing, adding, or subtracting time. + + + + + +
BadGood
+ +```go +func isActive(now, start, stop int) bool { + return start <= now && now < stop +} +``` + + + +```go +func isActive(now, start, stop time.Time) bool { + return (start.Before(now) || start.Equal(now)) && now.Before(stop) +} +``` + +
+ +#### Use `time.Duration` for periods of time + +Use [`time.Duration`](https://pkg.go.dev/time#Duration) when dealing with periods of time. + + + + + +
BadGood
+ +```go +func poll(delay int) { + for { + // ... + time.Sleep(time.Duration(delay) * time.Millisecond) + } +} + +poll(10) // was it seconds or milliseconds? +``` + + + +```go +func poll(delay time.Duration) { + for { + // ... + time.Sleep(delay) + } +} + +poll(10*time.Second) +``` + +
+ +Going back to the example of adding 24 hours to a time instant, the method we +use to add time depends on intent. If we want the same time of the day, but on +the next calendar day, we should use [`Time.AddDate`](https://pkg.go.dev/time#Time.AddDate). However, if we want an +instant of time guaranteed to be 24 hours after the previous time, we should +use [`Time.Add`](https://pkg.go.dev/time#Time.Add). + +```go +newDay := t.AddDate(0 /* years */, 0 /* months */, 1 /* days */) +maybeNewDay := t.Add(24 * time.Hour) +``` + +#### Use `time.Time` and `time.Duration` with external systems + +Use `time.Duration` and `time.Time` in interactions with external systems when +possible. For example: + +- Command-line flags: [`flag`](https://pkg.go.dev/flag) supports `time.Duration` via + [`time.ParseDuration`](https://pkg.go.dev/time#ParseDuration) +- JSON: [`encoding/json`](https://pkg.go.dev/encoding/json) supports encoding `time.Time` as an [RFC 3339](https://tools.ietf.org/html/rfc3339) + string via its [`UnmarshalJSON` method](https://pkg.go.dev/time#Time.UnmarshalJSON) +- SQL: [`database/sql`](https://pkg.go.dev/database/sql) supports converting `DATETIME` or `TIMESTAMP` columns + into `time.Time` and back if the underlying driver supports it +- YAML: [`gopkg.in/yaml.v2`](https://pkg.go.dev/gopkg.in/yaml.v2) supports `time.Time` as an [RFC 3339](https://tools.ietf.org/html/rfc3339) string, and + `time.Duration` via [`time.ParseDuration`](https://pkg.go.dev/time#ParseDuration). + +When it is not possible to use `time.Duration` in these interactions, use +`int` or `float64` and include the unit in the name of the field. + +For example, since `encoding/json` does not support `time.Duration`, the unit +is included in the name of the field. + + + + + +
BadGood
+ +```go +// {"interval": 2} +type Config struct { + Interval int `json:"interval"` +} +``` + + + +```go +// {"intervalMillis": 2000} +type Config struct { + IntervalMillis int `json:"intervalMillis"` +} +``` + +
+ +When it is not possible to use `time.Time` in these interactions, unless an +alternative is agreed upon, use `string` and format timestamps as defined in +[RFC 3339](https://tools.ietf.org/html/rfc3339). This format is used by default by [`Time.UnmarshalText`](https://pkg.go.dev/time#Time.UnmarshalText) and is +available for use in `Time.Format` and `time.Parse` via [`time.RFC3339`](https://pkg.go.dev/time#RFC3339). + +Although this tends to not be a problem in practice, keep in mind that the +`"time"` package does not support parsing timestamps with leap seconds +([8728](https://github.com/golang/go/issues/8728)), nor does it account for leap seconds in calculations ([15190](https://github.com/golang/go/issues/15190)). If +you compare two instants of time, the difference will not include the leap +seconds that may have occurred between those two instants. + +### Errors + +#### Error Types + +There are few options for declaring errors. +Consider the following before picking the option best suited for your use case. + +- Does the caller need to match the error so that they can handle it? + If yes, we must support the [`errors.Is`](https://pkg.go.dev/errors#Is) or [`errors.As`](https://pkg.go.dev/errors#As) functions + by declaring a top-level error variable or a custom type. +- Is the error message a static string, + or is it a dynamic string that requires contextual information? + For the former, we can use [`errors.New`](https://pkg.go.dev/errors#New), but for the latter we must + use [`fmt.Errorf`](https://pkg.go.dev/fmt#Errorf) or a custom error type. +- Are we propagating a new error returned by a downstream function? + If so, see the [section on error wrapping](#error-wrapping). + +| Error matching? | Error Message | Guidance | +|-----------------|---------------|--------------------------------------------------------------------| +| No | static | [`errors.New`](https://pkg.go.dev/errors#New) | +| No | dynamic | [`fmt.Errorf`](https://pkg.go.dev/fmt#Errorf) | +| Yes | static | top-level `var` with [`errors.New`](https://pkg.go.dev/errors#New) | +| Yes | dynamic | custom `error` type | + +For example, +use [`errors.New`](https://pkg.go.dev/errors#New) for an error with a static string. +Export this error as a variable to support matching it with `errors.Is` +if the caller needs to match and handle this error. + + + + + +
No error matchingError matching
+ +```go +// package foo + +func Open() error { + return errors.New("could not open") +} + +// package bar + +if err := foo.Open(); err != nil { + // Can't handle the error. + panic("unknown error") +} +``` + + + +```go +// package foo + +var ErrCouldNotOpen = errors.New("could not open") + +func Open() error { + return ErrCouldNotOpen +} + +// package bar + +if err := foo.Open(); err != nil { + if errors.Is(err, foo.ErrCouldNotOpen) { + // handle the error + } else { + panic("unknown error") + } +} +``` + +
+ +For an error with a dynamic string, +use [`fmt.Errorf`](https://pkg.go.dev/fmt#Errorf) if the caller does not need to match it, +and a custom `error` if the caller does need to match it. + + + + + +
No error matchingError matching
+ +```go +// package foo + +func Open(file string) error { + return fmt.Errorf("file %q not found", file) +} + +// package bar + +if err := foo.Open("testfile.txt"); err != nil { + // Can't handle the error. + panic("unknown error") +} +``` + + + +```go +// package foo + +type NotFoundError struct { + File string +} + +func (e *NotFoundError) Error() string { + return fmt.Sprintf("file %q not found", e.File) +} + +func Open(file string) error { + return &NotFoundError{File: file} +} + + +// package bar + +if err := foo.Open("testfile.txt"); err != nil { + var notFound *NotFoundError + if errors.As(err, ¬Found) { + // handle the error + } else { + panic("unknown error") + } +} +``` + +
+ +Note that if you export error variables or types from a package, +they will become part of the public API of the package. + +#### Error Wrapping + +There are three main options for propagating errors if a call fails: + +- return the original error as-is +- add context with `fmt.Errorf` and the `%w` verb +- add context with `fmt.Errorf` and the `%v` verb + +Return the original error as-is if there is no additional context to add. +This maintains the original error type and message. +This is well suited for cases when the underlying error message +has sufficient information to track down where it came from. + +Otherwise, add context to the error message where possible +so that instead of a vague error such as "connection refused", +you get more useful errors such as "call service foo: connection refused". + +Use `fmt.Errorf` to add context to your errors, +picking between the `%w` or `%v` verbs +based on whether the caller should be able to +match and extract the underlying cause. + +- Use `%w` if the caller should have access to the underlying error. + This is a good default for most wrapped errors, + but be aware that callers may begin to rely on this behavior. + So for cases where the wrapped error is a known `var` or type, + document and test it as part of your function's contract. +- Use `%v` to obfuscate the underlying error. + Callers will be unable to match it, + but you can switch to `%w` in the future if needed. + +When adding context to returned errors, keep the context succinct by avoiding +phrases like "failed to", which state the obvious and pile up as the error +percolates up through the stack: + + + + + +
BadGood
+ +```go +s, err := store.New() +if err != nil { + return fmt.Errorf( + "failed to create new store: %w", err) +} +``` + + + +```go +s, err := store.New() +if err != nil { + return fmt.Errorf( + "new store: %w", err) +} +``` + +
+ +```plain +failed to x: failed to y: failed to create new store: the error +``` + + + +```plain +x: y: new store: the error +``` + +
+ +However once the error is sent to another system, it should be clear the +message is an error (e.g. an `err` tag or "Failed" prefix in logs). + +See also [Don't just check errors, handle them gracefully](https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully). + +#### Error Naming + +For error values stored as global variables, +use the prefix `Err` or `err` depending on whether they're exported. +This guidance supersedes the [Prefix Unexported Globals with _](#prefix-unexported-globals-with-_). + +```go +var ( + // The following two errors are exported + // so that users of this package can match them + // with errors.Is. + + ErrBrokenLink = errors.New("link is broken") + ErrCouldNotOpen = errors.New("could not open") + + // This error is not exported because + // we don't want to make it part of our public API. + // We may still use it inside the package + // with errors.Is. + + errNotFound = errors.New("not found") +) +``` + +For custom error types, use the suffix `Error` instead. + +```go +// Similarly, this error is exported +// so that users of this package can match it +// with errors.As. + +type NotFoundError struct { + File string +} + +func (e *NotFoundError) Error() string { + return fmt.Sprintf("file %q not found", e.File) +} + +// And this error is not exported because +// we don't want to make it part of the public API. +// We can still use it inside the package +// with errors.As. + +type resolveError struct { + Path string +} + +func (e *resolveError) Error() string { + return fmt.Sprintf("resolve %q", e.Path) +} +``` + +#### Handle Errors Once + +When a caller receives an error from a callee, +it can handle it in a variety of different ways +depending on what it knows about the error. + +These include, but not are limited to: + +- if the callee contract defines specific errors, + matching the error with `errors.Is` or `errors.As` + and handling the branches differently +- if the error is recoverable, + logging the error and degrading gracefully +- if the error represents a domain-specific failure condition, + returning a well-defined error +- returning the error, either [wrapped](#error-wrapping) or verbatim + +Regardless of how the caller handles the error, +it should typically handle each error only once. +The caller should not, for example, log the error and then return it, +because *its* callers may handle the error as well. + +For example, consider the following cases: + + + + + + + + +
DescriptionCode
+ +**Bad**: Log the error and return it + +Callers further up the stack will likely take a similar action with the error. +Doing so makes a lot of noise in the application logs for little value. + + + +```go +u, err := getUser(id) +if err != nil { + // BAD: See description + log.Printf("Could not get user %q: %v", id, err) + return err +} +``` + +
+ +**Good**: Wrap the error and return it + +Callers further up the stack will handle the error. +Use of `%w` ensures they can match the error with `errors.Is` or `errors.As` +if relevant. + + + +```go +u, err := getUser(id) +if err != nil { + return fmt.Errorf("get user %q: %w", id, err) +} +``` + +
+ +**Good**: Log the error and degrade gracefully + +If the operation isn't strictly necessary, +we can provide a degraded but unbroken experience +by recovering from it. + + + +```go +if err := emitMetrics(); err != nil { + // Failure to write metrics should not + // break the application. + log.Printf("Could not emit metrics: %v", err) +} + +``` + +
+ +**Good**: Match the error and degrade gracefully + +If the callee defines a specific error in its contract, +and the failure is recoverable, +match on that error case and degrade gracefully. +For all other cases, wrap the error and return it. + +Callers further up the stack will handle other errors. + + + +```go +tz, err := getUserTimeZone(id) +if err != nil { + if errors.Is(err, ErrUserNotFound) { + // User doesn't exist. Use UTC. + tz = time.UTC + } else { + return fmt.Errorf("get user %q: %w", id, err) + } +} +``` + +
+ +### Handle Type Assertion Failures + +The single return value form of a [type assertion](https://go.dev/ref/spec#Type_assertions) will panic on an incorrect +type. Therefore, always use the "comma ok" idiom. + + + + + +
BadGood
+ +```go +t := i.(string) +``` + + + +```go +t, ok := i.(string) +if !ok { + // handle the error gracefully +} +``` + +
+ + + +### Don't Panic + +Code running in production must avoid panics. Panics are a major source of +[cascading failures](https://en.wikipedia.org/wiki/Cascading_failure). If an error occurs, the function must return an error and +allow the caller to decide how to handle it. + + + + + +
BadGood
+ +```go +func run(args []string) { + if len(args) == 0 { + panic("an argument is required") + } + // ... +} + +func main() { + run(os.Args[1:]) +} +``` + + + +```go +func run(args []string) error { + if len(args) == 0 { + return errors.New("an argument is required") + } + // ... + return nil +} + +func main() { + if err := run(os.Args[1:]); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} +``` + +
+ +Panic/recover is not an error handling strategy. A program must panic only when +something irrecoverable happens such as a nil dereference. An exception to this is +program initialization: bad things at program startup that should abort the +program may cause panic. + +```go +var _statusTemplate = template.Must(template.New("name").Parse("_statusHTML")) +``` + +Even in tests, prefer `t.Fatal` or `t.FailNow` over panics to ensure that the +test is marked as failed. + + + + + +
BadGood
+ +```go +// func TestFoo(t *testing.T) + +f, err := os.CreateTemp("", "test") +if err != nil { + panic("failed to set up test") +} +``` + + + +```go +// func TestFoo(t *testing.T) + +f, err := os.CreateTemp("", "test") +if err != nil { + t.Fatal("failed to set up test") +} +``` + +
+ +### Use go.uber.org/atomic + +Atomic operations with the [sync/atomic](https://pkg.go.dev/sync/atomic) package operate on the raw types +(`int32`, `int64`, etc.) so it is easy to forget to use the atomic operation to +read or modify the variables. + +[go.uber.org/atomic](https://pkg.go.dev/go.uber.org/atomic) adds type safety to these operations by hiding the +underlying type. Additionally, it includes a convenient `atomic.Bool` type. + + + + + +
BadGood
+ +```go +type foo struct { + running int32 // atomic +} + +func (f *foo) start() { + if atomic.SwapInt32(&f.running, 1) == 1 { + // already running… + return + } + // start the Foo +} + +func (f *foo) isRunning() bool { + return f.running == 1 // race! +} +``` + + + +```go +type foo struct { + running atomic.Bool +} + +func (f *foo) start() { + if f.running.Swap(true) { + // already running… + return + } + // start the Foo +} + +func (f *foo) isRunning() bool { + return f.running.Load() +} +``` + +
+ +### Avoid Mutable Globals + +Avoid mutating global variables, instead opting for dependency injection. +This applies to function pointers as well as other kinds of values. + + + + + + +
BadGood
+ +```go +// sign.go + +var _timeNow = time.Now + +func sign(msg string) string { + now := _timeNow() + return signWithTime(msg, now) +} +``` + + + +```go +// sign.go + +type signer struct { + now func() time.Time +} + +func newSigner() *signer { + return &signer{ + now: time.Now, + } +} + +func (s *signer) Sign(msg string) string { + now := s.now() + return signWithTime(msg, now) +} +``` + +
+ +```go +// sign_test.go + +func TestSign(t *testing.T) { + oldTimeNow := _timeNow + _timeNow = func() time.Time { + return someFixedTime + } + defer func() { _timeNow = oldTimeNow }() + + assert.Equal(t, want, sign(give)) +} +``` + + + +```go +// sign_test.go + +func TestSigner(t *testing.T) { + s := newSigner() + s.now = func() time.Time { + return someFixedTime + } + + assert.Equal(t, want, s.Sign(give)) +} +``` + +
+ +### Avoid Embedding Types in Public Structs + +These embedded types leak implementation details, inhibit type evolution, and +obscure documentation. + +Assuming you have implemented a variety of list types using a shared +`AbstractList`, avoid embedding the `AbstractList` in your concrete list +implementations. +Instead, hand-write only the methods to your concrete list that will delegate +to the abstract list. + +```go +type AbstractList struct {} + +// Add adds an entity to the list. +func (l *AbstractList) Add(e Entity) { + // ... +} + +// Remove removes an entity from the list. +func (l *AbstractList) Remove(e Entity) { + // ... +} +``` + + + + + +
BadGood
+ +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + *AbstractList +} +``` + + + +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + list *AbstractList +} + +// Add adds an entity to the list. +func (l *ConcreteList) Add(e Entity) { + l.list.Add(e) +} + +// Remove removes an entity from the list. +func (l *ConcreteList) Remove(e Entity) { + l.list.Remove(e) +} +``` + +
+ +Go allows [type embedding](https://go.dev/doc/effective_go#embedding) as a compromise between inheritance and composition. +The outer type gets implicit copies of the embedded type's methods. +These methods, by default, delegate to the same method of the embedded +instance. + +The struct also gains a field by the same name as the type. +So, if the embedded type is public, the field is public. +To maintain backward compatibility, every future version of the outer type must +keep the embedded type. + +An embedded type is rarely necessary. +It is a convenience that helps you avoid writing tedious delegate methods. + +Even embedding a compatible AbstractList *interface*, instead of the struct, +would offer the developer more flexibility to change in the future, but still +leak the detail that the concrete lists use an abstract implementation. + + + + + +
BadGood
+ +```go +// AbstractList is a generalized implementation +// for various kinds of lists of entities. +type AbstractList interface { + Add(Entity) + Remove(Entity) +} + +// ConcreteList is a list of entities. +type ConcreteList struct { + AbstractList +} +``` + + + +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + list AbstractList +} + +// Add adds an entity to the list. +func (l *ConcreteList) Add(e Entity) { + l.list.Add(e) +} + +// Remove removes an entity from the list. +func (l *ConcreteList) Remove(e Entity) { + l.list.Remove(e) +} +``` + +
+ +Either with an embedded struct or an embedded interface, the embedded type +places limits on the evolution of the type. + +- Adding methods to an embedded interface is a breaking change. +- Removing methods from an embedded struct is a breaking change. +- Removing the embedded type is a breaking change. +- Replacing the embedded type, even with an alternative that satisfies the same + interface, is a breaking change. + +Although writing these delegate methods is tedious, the additional effort hides +an implementation detail, leaves more opportunities for change, and also +eliminates indirection for discovering the full List interface in +documentation. + +### Avoid Using Built-In Names + +The Go [language specification](https://go.dev/ref/spec) outlines several built-in, +[predeclared identifiers](https://go.dev/ref/spec#Predeclared_identifiers) that should not be used as names within Go programs. + +Depending on context, reusing these identifiers as names will either shadow +the original within the current lexical scope (and any nested scopes) or make +affected code confusing. In the best case, the compiler will complain; in the +worst case, such code may introduce latent, hard-to-grep bugs. + + + + + + +
BadGood
+ +```go +var error string +// `error` shadows the builtin + +// or + +func handleErrorMessage(error string) { + // `error` shadows the builtin +} +``` + + + +```go +var errorMessage string +// `error` refers to the builtin + +// or + +func handleErrorMessage(msg string) { + // `error` refers to the builtin +} +``` + +
+ +```go +type Foo struct { + // While these fields technically don't + // constitute shadowing, grepping for + // `error` or `string` strings is now + // ambiguous. + error error + string string +} + +func (f Foo) Error() error { + // `error` and `f.error` are + // visually similar + return f.error +} + +func (f Foo) String() string { + // `string` and `f.string` are + // visually similar + return f.string +} +``` + + + +```go +type Foo struct { + // `error` and `string` strings are + // now unambiguous. + err error + str string +} + +func (f Foo) Error() error { + return f.err +} + +func (f Foo) String() string { + return f.str +} +``` + +
+ +Note that the compiler will not generate errors when using predeclared +identifiers, but tools such as `go vet` should correctly point out these and +other cases of shadowing. + +### Avoid `init()` + +Avoid `init()` where possible. When `init()` is unavoidable or desirable, code +should attempt to: + +1. Be completely deterministic, regardless of program environment or invocation. +2. Avoid depending on the ordering or side-effects of other `init()` functions. + While `init()` ordering is well-known, code can change, and thus + relationships between `init()` functions can make code brittle and + error-prone. +3. Avoid accessing or manipulating global or environment state, such as machine + information, environment variables, working directory, program + arguments/inputs, etc. +4. Avoid I/O, including both filesystem, network, and system calls. + +Code that cannot satisfy these requirements likely belongs as a helper to be +called as part of `main()` (or elsewhere in a program's lifecycle), or be +written as part of `main()` itself. In particular, libraries that are intended +to be used by other programs should take special care to be completely +deterministic and not perform "init magic". + + + + + + +
BadGood
+ +```go +type Foo struct { + // ... +} + +var _defaultFoo Foo + +func init() { + _defaultFoo = Foo{ + // ... + } +} +``` + + + +```go +var _defaultFoo = Foo{ + // ... +} + +// or, better, for testability: + +var _defaultFoo = defaultFoo() + +func defaultFoo() Foo { + return Foo{ + // ... + } +} +``` + +
+ +```go +type Config struct { + // ... +} + +var _config Config + +func init() { + // Bad: based on current directory + cwd, _ := os.Getwd() + + // Bad: I/O + raw, _ := os.ReadFile( + path.Join(cwd, "config", "config.yaml"), + ) + + yaml.Unmarshal(raw, &_config) +} +``` + + + +```go +type Config struct { + // ... +} + +func loadConfig() Config { + cwd, err := os.Getwd() + // handle err + + raw, err := os.ReadFile( + path.Join(cwd, "config", "config.yaml"), + ) + // handle err + + var config Config + yaml.Unmarshal(raw, &config) + + return config +} +``` + +
+ +Considering the above, some situations in which `init()` may be preferable or +necessary might include: + +- Complex expressions that cannot be represented as single assignments. +- Pluggable hooks, such as `database/sql` dialects, encoding type registries, etc. +- Optimizations to [Google Cloud Functions](https://cloud.google.com/functions/docs/bestpractices/tips#use_global_variables_to_reuse_objects_in_future_invocations) and other forms of deterministic + precomputation. + +### Exit in Main + +Go programs use [`os.Exit`](https://pkg.go.dev/os#Exit) or [`log.Fatal*`](https://pkg.go.dev/log#Fatal) to exit immediately. (Panicking +is not a good way to exit programs, please [don't panic](#dont-panic).) + +Call one of `os.Exit` or `log.Fatal*` **only in `main()`**. All other +functions should return errors to signal failure. + + + + + +
BadGood
+ +```go +func main() { + body := readFile(path) + fmt.Println(body) +} + +func readFile(path string) string { + f, err := os.Open(path) + if err != nil { + log.Fatal(err) + } + + b, err := io.ReadAll(f) + if err != nil { + log.Fatal(err) + } + + return string(b) +} +``` + + + +```go +func main() { + body, err := readFile(path) + if err != nil { + log.Fatal(err) + } + fmt.Println(body) +} + +func readFile(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", err + } + + b, err := io.ReadAll(f) + if err != nil { + return "", err + } + + return string(b), nil +} +``` + +
+ +Rationale: Programs with multiple functions that exit present a few issues: + +- Non-obvious control flow: Any function can exit the program so it becomes + difficult to reason about the control flow. +- Difficult to test: A function that exits the program will also exit the test + calling it. This makes the function difficult to test and introduces risk of + skipping other tests that have not yet been run by `go test`. +- Skipped cleanup: When a function exits the program, it skips function calls + enqueued with `defer` statements. This adds risk of skipping important + cleanup tasks. + +#### Exit Once + +If possible, prefer to call `os.Exit` or `log.Fatal` **at most once** in your +`main()`. If there are multiple error scenarios that halt program execution, +put that logic under a separate function and return errors from it. + +This has the effect of shortening your `main()` function and putting all key +business logic into a separate, testable function. + + + + + +
BadGood
+ +```go +package main + +func main() { + args := os.Args[1:] + if len(args) != 1 { + log.Fatal("missing file") + } + name := args[0] + + f, err := os.Open(name) + if err != nil { + log.Fatal(err) + } + defer f.Close() + + // If we call log.Fatal after this line, + // f.Close will not be called. + + b, err := io.ReadAll(f) + if err != nil { + log.Fatal(err) + } + + // ... +} +``` + + + +```go +package main + +func main() { + if err := run(); err != nil { + log.Fatal(err) + } +} + +func run() error { + args := os.Args[1:] + if len(args) != 1 { + return errors.New("missing file") + } + name := args[0] + + f, err := os.Open(name) + if err != nil { + return err + } + defer f.Close() + + b, err := io.ReadAll(f) + if err != nil { + return err + } + + // ... +} +``` + +
+ +The example above uses `log.Fatal`, but the guidance also applies to +`os.Exit` or any library code that calls `os.Exit`. + +```go +func main() { + if err := run(); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} +``` + +You may alter the signature of `run()` to fit your needs. +For example, if your program must exit with specific exit codes for failures, +`run()` may return the exit code instead of an error. +This allows unit tests to verify this behavior directly as well. + +```go +func main() { + os.Exit(run(args)) +} + +func run() (exitCode int) { + // ... +} +``` + +More generally, note that the `run()` function used in these examples +is not intended to be prescriptive. +There's flexibility in the name, signature, and setup of the `run()` function. +Among other things, you may: + +- accept unparsed command line arguments (e.g., `run(os.Args[1:])`) +- parse command line arguments in `main()` and pass them onto `run` +- use a custom error type to carry the exit code back to `main()` +- put business logic in a different layer of abstraction from `package main` + +This guidance only requires that there's a single place in your `main()` +responsible for actually exiting the process. + +### Use field tags in marshaled structs + +Any struct field that is marshaled into JSON, YAML, +or other formats that support tag-based field naming +should be annotated with the relevant tag. + + + + + +
BadGood
+ +```go +type Stock struct { + Price int + Name string +} + +bytes, err := json.Marshal(Stock{ + Price: 137, + Name: "UBER", +}) +``` + + + +```go +type Stock struct { + Price int `json:"price"` + Name string `json:"name"` + // Safe to rename Name to Symbol. +} + +bytes, err := json.Marshal(Stock{ + Price: 137, + Name: "UBER", +}) +``` + +
+ +Rationale: +The serialized form of the structure is a contract between different systems. +Changes to the structure of the serialized form--including field names--break +this contract. Specifying field names inside tags makes the contract explicit, +and it guards against accidentally breaking the contract by refactoring or +renaming fields. + +### Don't fire-and-forget goroutines + +Goroutines are lightweight, but they're not free: +at minimum, they cost memory for their stack and CPU to be scheduled. +While these costs are small for typical uses of goroutines, +they can cause significant performance issues +when spawned in large numbers without controlled lifetimes. +Goroutines with unmanaged lifetimes can also cause other issues +like preventing unused objects from being garbage collected +and holding onto resources that are otherwise no longer used. + +Therefore, do not leak goroutines in production code. +Use [go.uber.org/goleak](https://pkg.go.dev/go.uber.org/goleak) +to test for goroutine leaks inside packages that may spawn goroutines. + +In general, every goroutine: + +- must have a predictable time at which it will stop running; or +- there must be a way to signal to the goroutine that it should stop + +In both cases, there must be a way for code to block and wait for the goroutine to +finish. + +For example: + + + + + + +
BadGood
+ +```go +go func() { + for { + flush() + time.Sleep(delay) + } +}() +``` + + + +```go +var ( + stop = make(chan struct{}) // tells the goroutine to stop + done = make(chan struct{}) // tells us that the goroutine exited +) +go func() { + defer close(done) + + ticker := time.NewTicker(delay) + defer ticker.Stop() + for { + select { + case <-ticker.C: + flush() + case <-stop: + return + } + } +}() + +// Elsewhere... +close(stop) // signal the goroutine to stop +<-done // and wait for it to exit +``` + +
+ +There's no way to stop this goroutine. +This will run until the application exits. + + + +This goroutine can be stopped with `close(stop)`, +and we can wait for it to exit with `<-done`. + +
+ +#### Wait for goroutines to exit + +Given a goroutine spawned by the system, +there must be a way to wait for the goroutine to exit. +There are two popular ways to do this: + +- Use a `sync.WaitGroup` to wait for multiple goroutines to complete. + Do this if there are multiple goroutines that you want to wait for. + + ```go + var wg sync.WaitGroup + for i := 0; i < N; i++ { + wg.Go(...) + } + + // To wait for all to finish: + wg.Wait() + ``` + +- Add another `chan struct{}` that the goroutine closes when it's done. + Do this if there's only one goroutine. + + ```go + done := make(chan struct{}) + go func() { + defer close(done) + // ... + }() + + // To wait for the goroutine to finish: + <-done + ``` + +#### No goroutines in `init()` + +`init()` functions should not spawn goroutines. +See also [Avoid init()](#avoid-init). + +If a package has need of a background goroutine, +it must expose an object that is responsible for managing a goroutine's +lifetime. +The object must provide a method (`Close`, `Stop`, `Shutdown`, etc) +that signals the background goroutine to stop, and waits for it to exit. + + + + + + +
BadGood
+ +```go +func init() { + go doWork() +} + +func doWork() { + for { + // ... + } +} +``` + + + +```go +type Worker struct{ /* ... */ } + +func NewWorker(...) *Worker { + w := &Worker{ + stop: make(chan struct{}), + done: make(chan struct{}), + // ... + } + go w.doWork() + return w +} + +func (w *Worker) doWork() { + defer close(w.done) + for { + // ... + case <-w.stop: + return + } +} + +// Shutdown tells the worker to stop +// and waits until it has finished. +func (w *Worker) Shutdown() { + close(w.stop) + <-w.done +} +``` + +
+ +Spawns a background goroutine unconditionally when the user exports this package. +The user has no control over the goroutine or a means of stopping it. + + + +Spawns the worker only if the user requests it. +Provides a means of shutting down the worker so that the user can free up +resources used by the worker. + +Note that you should use `WaitGroup`s if the worker manages multiple +goroutines. +See [Wait for goroutines to exit](#wait-for-goroutines-to-exit). + +
+ +## Performance + +Performance-specific guidelines apply only to the hot path. + +### Prefer strconv over fmt + +When converting primitives to/from strings, `strconv` is faster than +`fmt`. + + + + + + +
BadGood
+ +```go +for i := 0; i < b.N; i++ { + s := fmt.Sprint(rand.Int()) +} +``` + + + +```go +for i := 0; i < b.N; i++ { + s := strconv.Itoa(rand.Int()) +} +``` + +
+ +```plain +BenchmarkFmtSprint-4 143 ns/op 2 allocs/op +``` + + + +```plain +BenchmarkStrconv-4 64.2 ns/op 1 allocs/op +``` + +
+ +### Avoid repeated string-to-byte conversions + +Do not create byte slices from a fixed string repeatedly. Instead, perform the +conversion once and capture the result. + + + + + + +
BadGood
+ +```go +for i := 0; i < b.N; i++ { + w.Write([]byte("Hello world")) +} +``` + + + +```go +data := []byte("Hello world") +for i := 0; i < b.N; i++ { + w.Write(data) +} +``` + +
+ +```plain +BenchmarkBad-4 50000000 22.2 ns/op +``` + + + +```plain +BenchmarkGood-4 500000000 3.25 ns/op +``` + +
+ +### Prefer Specifying Container Capacity + +Specify container capacity where possible in order to allocate memory for the +container up front. This minimizes subsequent allocations (by copying and +resizing of the container) as elements are added. + +#### Specifying Map Capacity Hints + +Where possible, provide capacity hints when initializing +maps with `make()`. + +```go +make(map[T1]T2, hint) +``` + +Providing a capacity hint to `make()` tries to right-size the +map at initialization time, which reduces the need for growing +the map and allocations as elements are added to the map. + +Note that, unlike slices, map capacity hints do not guarantee complete, +preemptive allocation, but are used to approximate the number of hashmap buckets +required. Consequently, allocations may still occur when adding elements to the +map, even up to the specified capacity. + + + + + + +
BadGood
+ +```go +files, _ := os.ReadDir("./files") + +m := make(map[string]os.DirEntry) +for _, f := range files { + m[f.Name()] = f +} +``` + + + +```go + +files, _ := os.ReadDir("./files") + +m := make(map[string]os.DirEntry, len(files)) +for _, f := range files { + m[f.Name()] = f +} +``` + +
+ +`m` is created without a size hint; the map will resize +dynamically, causing multiple allocations as it grows. + + + +`m` is created with a size hint; there may be fewer +allocations at assignment time. + +
+ +#### Specifying Slice Capacity + +Where possible, provide capacity hints when initializing slices with `make()`, +particularly when appending. + +```go +make([]T, length, capacity) +``` + +Unlike maps, slice capacity is not a hint: the compiler will allocate enough +memory for the capacity of the slice as provided to `make()`, which means that +subsequent `append()` operations will incur zero allocations (until the length +of the slice matches the capacity, after which any appends will require a resize +to hold additional elements). + + + + + + +
BadGood
+ +```go +for n := 0; n < b.N; n++ { + data := make([]int, 0) + for k := 0; k < size; k++{ + data = append(data, k) + } +} +``` + + + +```go +for n := 0; n < b.N; n++ { + data := make([]int, 0, size) + for k := 0; k < size; k++{ + data = append(data, k) + } +} +``` + +
+ +```plain +BenchmarkBad-4 100000000 2.48s +``` + + + +```plain +BenchmarkGood-4 100000000 0.21s +``` + +
+ +## Style + +### Avoid overly long lines + +Avoid lines of code that require readers to scroll horizontally +or turn their heads too much. + +We recommend a soft line length limit of **99 characters**. +Authors should aim to wrap lines before hitting this limit, +but it is not a hard limit. +Code is allowed to exceed this limit. + +### Be Consistent + +Some of the guidelines outlined in this document can be evaluated objectively; +others are situational, contextual, or subjective. + +Above all else, **be consistent**. + +Consistent code is easier to maintain, is easier to rationalize, requires less +cognitive overhead, and is easier to migrate or update as new conventions emerge +or classes of bugs are fixed. + +Conversely, having multiple disparate or conflicting styles within a single +codebase causes maintenance overhead, uncertainty, and cognitive dissonance, +all of which can directly contribute to lower velocity, painful code reviews, +and bugs. + +When applying these guidelines to a codebase, it is recommended that changes +are made at a package (or larger) level: application at a sub-package level +violates the above concern by introducing multiple styles into the same code. + +### Group Similar Declarations + +Go supports grouping similar declarations. + + + + + +
BadGood
+ +```go +import "a" +import "b" +``` + + + +```go +import ( + "a" + "b" +) +``` + +
+ +This also applies to constants, variables, and type declarations. + + + + + +
BadGood
+ +```go + +const a = 1 +const b = 2 + + + +var a = 1 +var b = 2 + + + +type Area float64 +type Volume float64 +``` + + + +```go +const ( + a = 1 + b = 2 +) + +var ( + a = 1 + b = 2 +) + +type ( + Area float64 + Volume float64 +) +``` + +
+ +Only group related declarations. Do not group declarations that are unrelated. + + + + + +
BadGood
+ +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply + EnvVar = "MY_ENV" +) +``` + + + +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply +) + +const EnvVar = "MY_ENV" +``` + +
+ +Groups are not limited in where they can be used. For example, you can use them +inside of functions. + + + + + +
BadGood
+ +```go +func f() string { + red := color.New(0xff0000) + green := color.New(0x00ff00) + blue := color.New(0x0000ff) + + // ... +} +``` + + + +```go +func f() string { + var ( + red = color.New(0xff0000) + green = color.New(0x00ff00) + blue = color.New(0x0000ff) + ) + + // ... +} +``` + +
+ +Exception: Variable declarations, particularly inside functions, should be +grouped together if declared adjacent to other variables. Do this for variables +declared together even if they are unrelated. + + + + + +
BadGood
+ +```go +func (c *client) request() { + caller := c.name + format := "json" + timeout := 5*time.Second + var err error + + // ... +} +``` + + + +```go +func (c *client) request() { + var ( + caller = c.name + format = "json" + timeout = 5*time.Second + err error + ) + + // ... +} +``` + +
+ +### Import Group Ordering + +There should be two import groups: + +- Standard library +- Everything else + +This is the grouping applied by goimports by default. + + + + + +
BadGood
+ +```go +import ( + "fmt" + "os" + "go.uber.org/atomic" + "golang.org/x/sync/errgroup" +) +``` + + + +```go +import ( + "fmt" + "os" + + "go.uber.org/atomic" + "golang.org/x/sync/errgroup" +) +``` + +
+ +### Package Names + +When naming packages, choose a name that is: + +- All lower-case. No capitals or underscores. +- Does not need to be renamed using named imports at most call sites. +- Short and succinct. Remember that the name is identified in full at every call + site. +- Not plural. For example, `net/url`, not `net/urls`. +- Not "common", "util", "shared", or "lib". These are bad, uninformative names. + +See also [Package Names](https://go.dev/blog/package-names) and [Style guideline for Go packages](https://rakyll.org/style-packages/). + +### Function Names + +We follow the Go community's convention of using [MixedCaps for function +names](https://go.dev/doc/effective_go#mixed-caps). An exception is made for test functions, which may contain underscores +for the purpose of grouping related test cases, e.g., +`TestMyFunction_WhatIsBeingTested`. + +### Import Aliasing + +Import aliasing must be used if the package name does not match the last +element of the import path. + +```go +import ( + "net/http" + + client "example.com/client-go" + trace "example.com/trace/v2" +) +``` + +In all other scenarios, import aliases should be avoided unless there is a +direct conflict between imports. + + + + + +
BadGood
+ +```go +import ( + "fmt" + "os" + runtimetrace "runtime/trace" + + nettrace "golang.net/x/trace" +) +``` + + + +```go +import ( + "fmt" + "os" + "runtime/trace" + + nettrace "golang.net/x/trace" +) +``` + +
+ +### Function Grouping and Ordering + +- Functions should be sorted in rough call order. +- Functions in a file should be grouped by receiver. + +Therefore, exported functions should appear first in a file, after +`struct`, `const`, `var` definitions. + +A `newXYZ()`/`NewXYZ()` may appear after the type is defined, but before the +rest of the methods on the receiver. + +Since functions are grouped by receiver, plain utility functions should appear +towards the end of the file. + + + + + +
BadGood
+ +```go +func (s *something) Cost() { + return calcCost(s.weights) +} + +type something struct{ ... } + +func calcCost(n []int) int {...} + +func (s *something) Stop() {...} + +func newSomething() *something { + return &something{} +} +``` + + + +```go +type something struct{ ... } + +func newSomething() *something { + return &something{} +} + +func (s *something) Cost() { + return calcCost(s.weights) +} + +func (s *something) Stop() {...} + +func calcCost(n []int) int {...} +``` + +
+ +### Reduce Nesting + +Code should reduce nesting where possible by handling error cases/special +conditions first and returning early or continuing the loop. Reduce the amount +of code that is nested multiple levels. + + + + + +
BadGood
+ +```go +for _, v := range data { + if v.F1 == 1 { + v = process(v) + if err := v.Call(); err == nil { + v.Send() + } else { + return err + } + } else { + log.Printf("Invalid v: %v", v) + } +} +``` + + + +```go +for _, v := range data { + if v.F1 != 1 { + log.Printf("Invalid v: %v", v) + continue + } + + v = process(v) + if err := v.Call(); err != nil { + return err + } + v.Send() +} +``` + +
+ +### Unnecessary Else + +If a variable is set in both branches of an if, it can be replaced with a +single if. + + + + + +
BadGood
+ +```go +var a int +if b { + a = 100 +} else { + a = 10 +} +``` + + + +```go +a := 10 +if b { + a = 100 +} +``` + +
+ +### Top-level Variable Declarations + +At the top level, use the standard `var` keyword. Do not specify the type, +unless it is not the same type as the expression. + + + + + +
BadGood
+ +```go +var _s string = F() + +func F() string { return "A" } +``` + + + +```go +var _s = F() +// Since F already states that it returns a string, we don't need to specify +// the type again. + +func F() string { return "A" } +``` + +
+ +Specify the type if the type of the expression does not match the desired type +exactly. + +```go +type myError struct{} + +func (myError) Error() string { return "error" } + +func F() myError { return myError{} } + +var _e error = F() +// F returns an object of type myError but we want error. +``` + +### Prefix Unexported Globals with _ + +Prefix unexported top-level `var`s and `const`s with `_` to make it clear when +they are used that they are global symbols. + +Rationale: Top-level variables and constants have a package scope. Using a +generic name makes it easy to accidentally use the wrong value in a different +file. + + + + + +
BadGood
+ +```go +// foo.go + +const ( + defaultPort = 8080 + defaultUser = "user" +) + +// bar.go + +func Bar() { + defaultPort := 9090 + ... + fmt.Println("Default port", defaultPort) + + // We will not see a compile error if the first line of + // Bar() is deleted. +} +``` + + + +```go +// foo.go + +const ( + _defaultPort = 8080 + _defaultUser = "user" +) +``` + +
+ +**Exception**: Unexported error values may use the prefix `err` without the underscore. +See [Error Naming](#error-naming). + +### Embedding in Structs + +Embedded types should be at the top of the field list of a +struct, and there must be an empty line separating embedded fields from regular +fields. + + + + + +
BadGood
+ +```go +type Client struct { + version int + http.Client +} +``` + + + +```go +type Client struct { + http.Client + + version int +} +``` + +
+ +Embedding should provide tangible benefit, like adding or augmenting +functionality in a semantically-appropriate way. It should do this with zero +adverse user-facing effects (see also: [Avoid Embedding Types in Public Structs](#avoid-embedding-types-in-public-structs)). + +Exception: Mutexes should not be embedded, even on unexported types. See also: [Zero-value Mutexes are Valid](#zero-value-mutexes-are-valid). + +Embedding **should not**: + +- Be purely cosmetic or convenience-oriented. +- Make outer types more difficult to construct or use. +- Affect outer types' zero values. If the outer type has a useful zero value, it + should still have a useful zero value after embedding the inner type. +- Expose unrelated functions or fields from the outer type as a side-effect of + embedding the inner type. +- Expose unexported types. +- Affect outer types' copy semantics. +- Change the outer type's API or type semantics. +- Embed a non-canonical form of the inner type. +- Expose implementation details of the outer type. +- Allow users to observe or control type internals. +- Change the general behavior of inner functions through wrapping in a way that + would reasonably surprise users. + +Simply put, embed consciously and intentionally. A good litmus test is, "would +all of these exported inner methods/fields be added directly to the outer type"; +if the answer is "some" or "no", don't embed the inner type - use a field +instead. + + + + + + + +
BadGood
+ +```go +type A struct { + // Bad: A.Lock() and A.Unlock() are + // now available, provide no + // functional benefit, and allow + // users to control details about + // the internals of A. + sync.Mutex +} +``` + + + +```go +type countingWriteCloser struct { + // Good: Write() is provided at this + // outer layer for a specific + // purpose, and delegates work + // to the inner type's Write(). + io.WriteCloser + + count int +} + +func (w *countingWriteCloser) Write(bs []byte) (int, error) { + w.count += len(bs) + return w.WriteCloser.Write(bs) +} +``` + +
+ +```go +type Book struct { + // Bad: pointer changes zero value usefulness + io.ReadWriter + + // other fields +} + +// later + +var b Book +b.Read(...) // panic: nil pointer +b.String() // panic: nil pointer +b.Write(...) // panic: nil pointer +``` + + + +```go +type Book struct { + // Good: has useful zero value + bytes.Buffer + + // other fields +} + +// later + +var b Book +b.Read(...) // ok +b.String() // ok +b.Write(...) // ok +``` + +
+ +```go +type Client struct { + sync.Mutex + sync.WaitGroup + bytes.Buffer + url.URL +} +``` + + + +```go +type Client struct { + mtx sync.Mutex + wg sync.WaitGroup + buf bytes.Buffer + url url.URL +} +``` + +
+ +### Local Variable Declarations + +Short variable declarations (`:=`) should be used if a variable is being set to +some value explicitly. + + + + + +
BadGood
+ +```go +var s = "foo" +``` + + + +```go +s := "foo" +``` + +
+ +However, there are cases where the default value is clearer when the `var` +keyword is used. [Declaring Empty Slices](https://go.dev/wiki/CodeReviewComments#declaring-empty-slices), for example. + + + + + +
BadGood
+ +```go +func f(list []int) { + filtered := []int{} + for _, v := range list { + if v > 10 { + filtered = append(filtered, v) + } + } +} +``` + + + +```go +func f(list []int) { + var filtered []int + for _, v := range list { + if v > 10 { + filtered = append(filtered, v) + } + } +} +``` + +
+ +### nil is a valid slice + +`nil` is a valid slice of length 0. This means that, + +- You should not return a slice of length zero explicitly. Return `nil` + instead. + + + + + +
BadGood
+ + ```go + if x == "" { + return []int{} + } + ``` + + + + ```go + if x == "" { + return nil + } + ``` + +
+ +- To check if a slice is empty, always use `len(s) == 0`. Do not check for + `nil`. + + + + + +
BadGood
+ + ```go + func isEmpty(s []string) bool { + return s == nil + } + ``` + + + + ```go + func isEmpty(s []string) bool { + return len(s) == 0 + } + ``` + +
+ +- The zero value (a slice declared with `var`) is usable immediately without + `make()`. + + + + + +
BadGood
+ + ```go + nums := []int{} + // or, nums := make([]int) + + if add1 { + nums = append(nums, 1) + } + + if add2 { + nums = append(nums, 2) + } + ``` + + + + ```go + var nums []int + + if add1 { + nums = append(nums, 1) + } + + if add2 { + nums = append(nums, 2) + } + ``` + +
+ +Remember that, while it is a valid slice, a nil slice is not equivalent to an +allocated slice of length 0 - one is nil and the other is not - and the two may +be treated differently in different situations (such as serialization). + +### Reduce Scope of Variables + +Where possible, reduce scope of variables and constants. Do not reduce the scope if it +conflicts with [Reduce Nesting](#reduce-nesting). + + + + + +
BadGood
+ +```go +err := os.WriteFile(name, data, 0644) +if err != nil { + return err +} +``` + + + +```go +if err := os.WriteFile(name, data, 0644); err != nil { + return err +} +``` + +
+ +If you need a result of a function call outside of the if, then you should not +try to reduce the scope. + + + + + +
BadGood
+ +```go +if data, err := os.ReadFile(name); err == nil { + err = cfg.Decode(data) + if err != nil { + return err + } + + fmt.Println(cfg) + return nil +} else { + return err +} +``` + + + +```go +data, err := os.ReadFile(name) +if err != nil { + return err +} + +if err := cfg.Decode(data); err != nil { + return err +} + +fmt.Println(cfg) +return nil +``` + +
+ +Constants do not need to be global unless they are used in multiple functions or files +or are part of an external contract of the package. + + + + + +
BadGood
+ +```go +const ( + _defaultPort = 8080 + _defaultUser = "user" +) + +func Bar() { + fmt.Println("Default port", _defaultPort) +} +``` + + + +```go +func Bar() { + const ( + defaultPort = 8080 + defaultUser = "user" + ) + fmt.Println("Default port", defaultPort) +} +``` + +
+ +### Avoid Naked Parameters + +Naked parameters in function calls can hurt readability. Add C-style comments +(`/* ... */`) for parameter names when their meaning is not obvious. + + + + + +
BadGood
+ +```go +// func printInfo(name string, isLocal, done bool) + +printInfo("foo", true, true) +``` + + + +```go +// func printInfo(name string, isLocal, done bool) + +printInfo("foo", true /* isLocal */, true /* done */) +``` + +
+ +Better yet, replace naked `bool` types with custom types for more readable and +type-safe code. This allows more than just two states (true/false) for that +parameter in the future. + +```go +type Region int + +const ( + UnknownRegion Region = iota + Local +) + +type Status int + +const ( + StatusReady Status = iota + 1 + StatusDone + // Maybe we will have a StatusInProgress in the future. +) + +func printInfo(name string, region Region, status Status) +``` + +### Use Raw String Literals to Avoid Escaping + +Go supports [raw string literals](https://go.dev/ref/spec#raw_string_lit), +which can span multiple lines and include quotes. Use these to avoid +hand-escaped strings which are much harder to read. + + + + + +
BadGood
+ +```go +wantError := "unknown name:\"test\"" +``` + + + +```go +wantError := `unknown error:"test"` +``` + +
+ +### Initializing Structs + +#### Use Field Names to Initialize Structs + +You should almost always specify field names when initializing structs. This is +now enforced by [`go vet`](https://pkg.go.dev/cmd/vet). + + + + + +
BadGood
+ +```go +k := User{"John", "Doe", true} +``` + + + +```go +k := User{ + FirstName: "John", + LastName: "Doe", + Admin: true, +} +``` + +
+ +Exception: Field names *may* be omitted in test tables when there are 3 or +fewer fields. + +```go +tests := []struct{ + op Operation + want string +}{ + {Add, "add"}, + {Subtract, "subtract"}, +} +``` + +#### Omit Zero Value Fields in Structs + +When initializing structs with field names, omit fields that have zero values +unless they provide meaningful context. Otherwise, let Go set these to zero +values automatically. + + + + + +
BadGood
+ +```go +user := User{ + FirstName: "John", + LastName: "Doe", + MiddleName: "", + Admin: false, +} +``` + + + +```go +user := User{ + FirstName: "John", + LastName: "Doe", +} +``` + +
+ +This helps reduce noise for readers by omitting values that are default in +that context. Only meaningful values are specified. + +Include zero values where field names provide meaningful context. For example, +test cases in [Test Tables](#test-tables) can benefit from names of fields +even when they are zero-valued. + +```go +tests := []struct{ + give string + want int +}{ + {give: "0", want: 0}, + // ... +} +``` + +#### Use `var` for Zero Value Structs + +When all the fields of a struct are omitted in a declaration, use the `var` +form to declare the struct. + + + + + +
BadGood
+ +```go +user := User{} +``` + + + +```go +var user User +``` + +
+ +This differentiates zero valued structs from those with non-zero fields +similar to the distinction created for [map initialization](#initializing-maps), and matches how +we prefer to [declare empty slices](https://go.dev/wiki/CodeReviewComments#declaring-empty-slices). + +#### Initializing Struct References + +Use `&T{}` instead of `new(T)` when initializing struct references so that it +is consistent with the struct initialization. + + + + + +
BadGood
+ +```go +sval := T{Name: "foo"} + +// inconsistent +sptr := new(T) +sptr.Name = "bar" +``` + + + +```go +sval := T{Name: "foo"} + +sptr := &T{Name: "bar"} +``` + +
+ +### Initializing Maps + +Prefer `make(..)` for empty maps, and maps populated +programmatically. This makes map initialization visually +distinct from declaration, and it makes it easy to add size +hints later if available. + + + + + + +
BadGood
+ +```go +var ( + // m1 is safe to read and write; + // m2 will panic on writes. + m1 = map[T1]T2{} + m2 map[T1]T2 +) +``` + + + +```go +var ( + // m1 is safe to read and write; + // m2 will panic on writes. + m1 = make(map[T1]T2) + m2 map[T1]T2 +) +``` + +
+ +Declaration and initialization are visually similar. + + + +Declaration and initialization are visually distinct. + +
+ +Where possible, provide capacity hints when initializing +maps with `make()`. See +[Specifying Map Capacity Hints](#specifying-map-capacity-hints) +for more information. + +On the other hand, if the map holds a fixed list of elements, +use map literals to initialize the map. + + + + + +
BadGood
+ +```go +m := make(map[T1]T2, 3) +m[k1] = v1 +m[k2] = v2 +m[k3] = v3 +``` + + + +```go +m := map[T1]T2{ + k1: v1, + k2: v2, + k3: v3, +} +``` + +
+ +The basic rule of thumb is to use map literals when adding a fixed set of +elements at initialization time, otherwise use `make` (and specify a size hint +if available). + +### Format Strings outside Printf + +If you declare format strings for `Printf`-style functions outside a string +literal, make them `const` values. + +This helps `go vet` perform static analysis of the format string. + + + + + +
BadGood
+ +```go +msg := "unexpected values %v, %v\n" +fmt.Printf(msg, 1, 2) +``` + + + +```go +const msg = "unexpected values %v, %v\n" +fmt.Printf(msg, 1, 2) +``` + +
+ +### Naming Printf-style Functions + +When you declare a `Printf`-style function, make sure that `go vet` can detect +it and check the format string. + +This means that you should use predefined `Printf`-style function +names if possible. `go vet` will check these by default. See [Printf family](https://pkg.go.dev/cmd/vet#hdr-Printf_family) +for more information. + +If using the predefined names is not an option, end the name you choose with +f: `Wrapf`, not `Wrap`. `go vet` can be asked to check specific `Printf`-style +names but they must end with f. + +```shell +go vet -printfuncs=wrapf,statusf +``` + +See also [go vet: Printf family check](https://kuzminva.wordpress.com/2017/11/07/go-vet-printf-family-check/). + +## Patterns + +### Test Tables + +Table-driven tests with [subtests](https://go.dev/blog/subtests) can be a helpful pattern for writing tests +to avoid duplicating code when the core test logic is repetitive. + +If a system under test needs to be tested against *multiple conditions* where +certain parts of the inputs and outputs change, a table-driven test should +be used to reduce redundancy and improve readability. + + + + + +
BadGood
+ +```go +// func TestSplitHostPort(t *testing.T) + +host, port, err := net.SplitHostPort("192.0.2.0:8000") +require.NoError(t, err) +assert.Equal(t, "192.0.2.0", host) +assert.Equal(t, "8000", port) + +host, port, err = net.SplitHostPort("192.0.2.0:http") +require.NoError(t, err) +assert.Equal(t, "192.0.2.0", host) +assert.Equal(t, "http", port) + +host, port, err = net.SplitHostPort(":8000") +require.NoError(t, err) +assert.Equal(t, "", host) +assert.Equal(t, "8000", port) + +host, port, err = net.SplitHostPort("1:8") +require.NoError(t, err) +assert.Equal(t, "1", host) +assert.Equal(t, "8", port) +``` + + + +```go +// func TestSplitHostPort(t *testing.T) + +tests := []struct{ + give string + wantHost string + wantPort string +}{ + { + give: "192.0.2.0:8000", + wantHost: "192.0.2.0", + wantPort: "8000", + }, + { + give: "192.0.2.0:http", + wantHost: "192.0.2.0", + wantPort: "http", + }, + { + give: ":8000", + wantHost: "", + wantPort: "8000", + }, + { + give: "1:8", + wantHost: "1", + wantPort: "8", + }, +} + +for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + host, port, err := net.SplitHostPort(tt.give) + require.NoError(t, err) + assert.Equal(t, tt.wantHost, host) + assert.Equal(t, tt.wantPort, port) + }) +} +``` + +
+ +Test tables make it easier to add context to error messages, reduce duplicate +logic, and add new test cases. + +We follow the convention that the slice of structs is referred to as `tests` +and each test case `tt`. Further, we encourage explicating the input and output +values for each test case with `give` and `want` prefixes. + +```go +tests := []struct{ + give string + wantHost string + wantPort string +}{ + // ... +} + +for _, tt := range tests { + // ... +} +``` + +#### Avoid Unnecessary Complexity in Table Tests + +Table tests can be difficult to read and maintain if the subtests contain conditional +assertions or other branching logic. Table tests should **NOT** be used whenever +there needs to be complex or conditional logic inside subtests (i.e. complex logic inside the `for` loop). + +Large, complex table tests harm readability and maintainability because test readers may +have difficulty debugging test failures that occur. + +Table tests like this should be split into either multiple test tables or multiple +individual `Test...` functions. + +Some ideals to aim for are: + +* Focus on the narrowest unit of behavior +* Minimize "test depth", and avoid conditional assertions (see below) +* Ensure that all table fields are used in all tests +* Ensure that all test logic runs for all table cases + +In this context, "test depth" means "within a given test, the number of +successive assertions that require previous assertions to hold" (similar +to cyclomatic complexity). +Having "shallower" tests means that there are fewer relationships between +assertions and, more importantly, that those assertions are less likely +to be conditional by default. + +Concretely, table tests can become confusing and difficult to read if they use multiple branching +pathways (e.g. `shouldError`, `expectCall`, etc.), use many `if` statements for +specific mock expectations (e.g. `shouldCallFoo`), or place functions inside the +table (e.g. `setupMocks func(*FooMock)`). + +However, when testing behavior that only +changes based on changed input, it may be preferable to group similar cases +together in a table test to better illustrate how behavior changes across all inputs, +rather than splitting otherwise comparable units into separate tests +and making them harder to compare and contrast. + +If the test body is short and straightforward, +it's acceptable to have a single branching pathway for success versus failure cases +with a table field like `shouldErr` to specify error expectations. + + + + + +
BadGood
+ +```go +func TestComplicatedTable(t *testing.T) { + tests := []struct { + give string + want string + wantErr error + shouldCallX bool + shouldCallY bool + giveXResponse string + giveXErr error + giveYResponse string + giveYErr error + }{ + // ... + } + + for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + if tt.shouldCallX { + xMock.EXPECT().Call().Return( + tt.giveXResponse, tt.giveXErr, + ) + } + yMock := ymock.NewMockY(ctrl) + if tt.shouldCallY { + yMock.EXPECT().Call().Return( + tt.giveYResponse, tt.giveYErr, + ) + } + + got, err := DoComplexThing(tt.give, xMock, yMock) + + // verify results + if tt.wantErr != nil { + require.EqualError(t, err, tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, want, got) + }) + } +} +``` + + + +```go +func TestShouldCallX(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + xMock.EXPECT().Call().Return("XResponse", nil) + + yMock := ymock.NewMockY(ctrl) + + got, err := DoComplexThing("inputX", xMock, yMock) + + require.NoError(t, err) + assert.Equal(t, "want", got) +} + +func TestShouldCallYAndFail(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + + yMock := ymock.NewMockY(ctrl) + yMock.EXPECT().Call().Return("YResponse", nil) + + _, err := DoComplexThing("inputY", xMock, yMock) + assert.EqualError(t, err, "Y failed") +} +``` +
+ +This complexity makes it more difficult to change, understand, and prove the +correctness of the test. + +While there are no strict guidelines, readability and maintainability should +always be top-of-mind when deciding between Table Tests versus separate tests +for multiple inputs/outputs to a system. + +#### Parallel Tests + +Parallel tests, like some specialized loops (for example, those that spawn +goroutines or capture references as part of the loop body), +must take care to explicitly assign loop variables within the loop's scope to +ensure that they hold the expected values. + +```go +tests := []struct{ + give string + // ... +}{ + // ... +} + +for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + t.Parallel() + // ... + }) +} +``` + +In the example above, we must declare a `tt` variable scoped to the loop +iteration because of the use of `t.Parallel()` below. +If we do not do that, most or all tests will receive an unexpected value for +`tt`, or a value that changes as they're running. + + + +### Functional Options + +Functional options is a pattern in which you declare an opaque `Option` type +that records information in some internal struct. You accept a variadic number +of these options and act upon the full information recorded by the options on +the internal struct. + +Use this pattern for optional arguments in constructors and other public APIs +that you foresee needing to expand, especially if you already have three or +more arguments on those functions. + + + + + + +
BadGood
+ +```go +// package db + +func Open( + addr string, + cache bool, + logger *zap.Logger +) (*Connection, error) { + // ... +} +``` + + + +```go +// package db + +type Option interface { + // ... +} + +func WithCache(c bool) Option { + // ... +} + +func WithLogger(log *zap.Logger) Option { + // ... +} + +// Open creates a connection. +func Open( + addr string, + opts ...Option, +) (*Connection, error) { + // ... +} +``` + +
+ +The cache and logger parameters must always be provided, even if the user +wants to use the default. + +```go +db.Open(addr, db.DefaultCache, zap.NewNop()) +db.Open(addr, db.DefaultCache, log) +db.Open(addr, false /* cache */, zap.NewNop()) +db.Open(addr, false /* cache */, log) +``` + + + +Options are provided only if needed. + +```go +db.Open(addr) +db.Open(addr, db.WithLogger(log)) +db.Open(addr, db.WithCache(false)) +db.Open( + addr, + db.WithCache(false), + db.WithLogger(log), +) +``` + +
+ +Our suggested way of implementing this pattern is with an `Option` interface +that holds an unexported method, recording options on an unexported `options` +struct. + +```go +type options struct { + cache bool + logger *zap.Logger +} + +type Option interface { + apply(*options) +} + +type cacheOption bool + +func (c cacheOption) apply(opts *options) { + opts.cache = bool(c) +} + +func WithCache(c bool) Option { + return cacheOption(c) +} + +type loggerOption struct { + Log *zap.Logger +} + +func (l loggerOption) apply(opts *options) { + opts.logger = l.Log +} + +func WithLogger(log *zap.Logger) Option { + return loggerOption{Log: log} +} + +// Open creates a connection. +func Open( + addr string, + opts ...Option, +) (*Connection, error) { + options := options{ + cache: defaultCache, + logger: zap.NewNop(), + } + + for _, o := range opts { + o.apply(&options) + } + + // ... +} +``` + +Note that there's a method of implementing this pattern with closures but we +believe that the pattern above provides more flexibility for authors and is +easier to debug and test for users. In particular, it allows options to be +compared against each other in tests and mocks, versus closures where this is +impossible. Further, it lets options implement other interfaces, including +`fmt.Stringer` which allows for user-readable string representations of the +options. + +See also, + +- [Self-referential functions and the design of options](https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html) +- [Functional options for friendly APIs](https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis) + + + +## Linting + +More importantly than any "blessed" set of linters, lint consistently across a +codebase. + +We recommend using the following linters at a minimum, because we feel that they +help to catch the most common issues and also establish a high bar for code +quality without being unnecessarily prescriptive: + +- [errcheck](https://github.com/kisielk/errcheck) to ensure that errors are handled +- [goimports](https://pkg.go.dev/golang.org/x/tools/cmd/goimports) to format code and manage imports +- [revive](https://github.com/mgechev/revive) to point out common style mistakes +- [govet](https://pkg.go.dev/cmd/vet) to analyze code for common mistakes +- [staticcheck](https://staticcheck.dev) to do various static analysis checks + + > **Note**: [revive](https://github.com/mgechev/revive) is the modern, faster successor to the now-deprecated [golint](https://github.com/golang/lint). + +### Lint Runners + +We recommend [golangci-lint](https://github.com/golangci/golangci-lint) as the go-to lint runner for Go code, largely due +to its performance in larger codebases and ability to configure and use many +canonical linters at once. This repo has an example [.golangci.yml](https://github.com/uber-go/guide/blob/master/.golangci.yml) config file +with recommended linters and settings. + +golangci-lint has [various linters](https://golangci-lint.run/usage/linters/) available for use. The above linters are +recommended as a base set, and we encourage teams to add any additional linters +that make sense for their projects.