From c493168e5ae254764d23a051cb56c9fd9e21161d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miro=20Spo=CC=88nemann?= Date: Fri, 3 Jul 2026 17:17:01 +0200 Subject: [PATCH 1/2] Improved and evaluated Go skills --- skills/go-documentation/SKILL.md | 262 ++++++------- skills/go-documentation/evals/evals.json | 67 ++++ .../evals/files/jsdoc-comments/go.mod | 3 + .../evals/files/jsdoc-comments/tokenbucket.go | 97 +++++ .../evals/files/package-overview/README.md | 59 +++ .../evals/files/package-overview/go.mod | 3 + .../evals/files/package-overview/store.go | 57 +++ .../evals/files/testable-examples/go.mod | 3 + .../evals/files/testable-examples/slug.go | 61 +++ .../references/comment-syntax.md | 24 ++ skills/idiomatic-go/SKILL.md | 92 +++-- skills/idiomatic-go/evals/evals.json | 105 ++++++ .../evals/files/ast-navigation/ast.go | 59 +++ .../evals/files/ast-navigation/go.mod | 3 + .../evals/files/concurrency-poller/go.mod | 3 + .../evals/files/concurrency-poller/poller.go | 108 ++++++ .../evals/files/interfaces-errors/go.mod | 3 + .../files/interfaces-errors/orders/orders.go | 28 ++ .../interfaces-errors/userstore/userstore.go | 55 +++ .../evals/files/reference-resolution/go.mod | 3 + .../evals/files/reference-resolution/types.go | 72 ++++ .../evals/files/typescript-port/accounts.go | 162 ++++++++ .../evals/files/typescript-port/go.mod | 3 + .../references/concurrency-patterns.md | 155 +++++++- .../references/interfaces-and-errors.md | 18 + .../references/language-engineering.md | 352 ++++++++++++++++++ 26 files changed, 1682 insertions(+), 175 deletions(-) create mode 100644 skills/go-documentation/evals/evals.json create mode 100644 skills/go-documentation/evals/files/jsdoc-comments/go.mod create mode 100644 skills/go-documentation/evals/files/jsdoc-comments/tokenbucket.go create mode 100644 skills/go-documentation/evals/files/package-overview/README.md create mode 100644 skills/go-documentation/evals/files/package-overview/go.mod create mode 100644 skills/go-documentation/evals/files/package-overview/store.go create mode 100644 skills/go-documentation/evals/files/testable-examples/go.mod create mode 100644 skills/go-documentation/evals/files/testable-examples/slug.go create mode 100644 skills/idiomatic-go/evals/evals.json create mode 100644 skills/idiomatic-go/evals/files/ast-navigation/ast.go create mode 100644 skills/idiomatic-go/evals/files/ast-navigation/go.mod create mode 100644 skills/idiomatic-go/evals/files/concurrency-poller/go.mod create mode 100644 skills/idiomatic-go/evals/files/concurrency-poller/poller.go create mode 100644 skills/idiomatic-go/evals/files/interfaces-errors/go.mod create mode 100644 skills/idiomatic-go/evals/files/interfaces-errors/orders/orders.go create mode 100644 skills/idiomatic-go/evals/files/interfaces-errors/userstore/userstore.go create mode 100644 skills/idiomatic-go/evals/files/reference-resolution/go.mod create mode 100644 skills/idiomatic-go/evals/files/reference-resolution/types.go create mode 100644 skills/idiomatic-go/evals/files/typescript-port/accounts.go create mode 100644 skills/idiomatic-go/evals/files/typescript-port/go.mod create mode 100644 skills/idiomatic-go/references/language-engineering.md diff --git a/skills/go-documentation/SKILL.md b/skills/go-documentation/SKILL.md index 7be7bb2..4b35add 100644 --- a/skills/go-documentation/SKILL.md +++ b/skills/go-documentation/SKILL.md @@ -1,79 +1,105 @@ --- name: go-documentation -description: Write idiomatic Go doc comments that render correctly on pkg.go.dev. Use whenever the user is writing or reviewing Go documentation — package comments, `doc.go` files, exported symbol comments, testable `Example` functions, deprecation notices — or wants to publish a module to pkg.go.dev, preview docs locally with `pkgsite`, or debug a broken pkg.go.dev page. Do not use for general Go programming questions or non-Go languages. +description: Write idiomatic Go doc comments that render correctly on pkg.go.dev — package comments, `doc.go`, exported-symbol comments, testable `Example` functions, deprecation notices — and publish or debug modules on pkg.go.dev (`pkgsite`, the module proxy). Use when writing or reviewing Go documentation, or when a developer used to JSDoc/TSDoc/Markdown is unsure how Go doc comments differ. Not for general Go programming or non-Go languages. --- # Go Documentation -Go treats documentation as part of the language toolchain, not an afterthought. `go doc` reads comments locally, [pkg.go.dev](https://pkg.go.dev) publishes them on the public index, and `gofmt` normalizes their formatting. The conventions in this skill come from the official spec at https://go.dev/doc/comment — they are not stylistic preferences. Tools depend on them. +Go treats documentation as part of the toolchain. `go doc` reads comments locally, [pkg.go.dev](https://pkg.go.dev) publishes them, and `gofmt` normalizes their markup. The conventions here come from the official spec at https://go.dev/doc/comment — tools depend on them, so they are not stylistic preferences. -## Reference files +## Coming from TypeScript -Load these as needed — don't read them upfront. The main SKILL.md has the conventions and decision points; the references hold operational detail. +A Go doc comment is a plain `//` comment above a declaration — there are no tags, and almost none of the Markdown that JSDoc/TSDoc allow. These are the habits that silently produce wrong or unrendered docs; reach for the Go column instead. -- `references/comment-syntax.md` — the full markup grammar of doc comments: paragraphs, headings, links (reference-style and doc links), lists, code blocks, notes, deprecations, directives, plus the gofmt reformatting rules and the half-dozen common pitfalls that produce unintended code blocks or broken lists. Read this whenever a comment includes anything richer than plain prose, or when `gofmt` reformats a comment in a surprising way. -- `references/examples.md` — testable `Example` functions: naming rules (including the `_suffix` casing constraint), `// Output:` and `// Unordered output:` assertion comments, whole-file examples, and the rule that examples without an Output comment are compiled but not run. Read this when adding examples, or when an example test fails unexpectedly. -- `references/publishing.md` — how pkg.go.dev discovers and indexes modules via `proxy.golang.org`, the `go.mod` and redistributable-license requirements, semantic-versioning expectations, README rendering on pkg.go.dev (filenames, links, Markdown limits), source-code linking, retraction, and removal. Read this before publishing a new module, when authoring a module README for pkg.go.dev, or when an existing module isn't showing up or is showing the wrong content. -- `references/pkgsite-preview.md` — installing and running `pkgsite` to preview the current module's docs in a browser before publication. Read this when iterating on doc comments locally. +| TypeScript / JSDoc habit | What Go actually does | +| ---------------------------------------- | --------------------------------------------------------------------------------------------------------- | +| `@param`, `@returns`, `@throws` tags | No tags exist. Name params and results in prose ("It returns the number of bytes written"). Types are in the signature — don't restate them. | +| `` `inline code` `` backticks | Backticks (and `'single quotes'`) render as curly ‘quotes’. There is no inline-code markup — use a doc link `[Name]` for a symbol, plain text otherwise. | +| Markdown link `[text](url)` | Not a link. Use a bare URL, or reference style: `[text]` in prose plus `[text]: https://…` defined at the end of the comment. | +| `{@link Foo}` cross-reference | Not recognized, renders as literal text. Use a doc link: `[Foo]`. | +| Fenced ```` ``` ```` blocks, `**bold**` | No fences, no emphasis. A code block is any line indented by one tab; `**bold**` renders literally. | +| `/** ... */` Javadoc-style block comment | Valid Go syntax, and it *does* attach as the doc comment — but `gofmt` recognizes the leading-`*` pattern and deliberately leaves it untouched, so the un-stripped `*` on every line makes `go doc`/pkg.go.dev render it as a mangled code block. Use a plain `//` on every line instead. | +| `@example` with a snippet | Examples are real compiled functions (`func ExampleFoo()`) in `_test.go`, run by `go test`. See `references/examples.md`. | +| `@deprecated` | A paragraph beginning with the exact prefix `Deprecated: `. | +| Summary in any phrasing | The first sentence must start with the symbol name: `Foo returns…`, `Package foo…` (see the table below). | +| README is the docs (npmjs.com) | pkg.go.dev and `go doc` show the **package comment**, not the README, above the API. Empty package docs = no docs. | +| `npm publish` to a registry | No push step. Tag `vX.Y.Z`; the module proxy indexes it. A published version is immutable. | +| Indent comment text freely | Any indented line becomes a code block. Keep prose flush left. | -## Core convention: every exported name gets a doc comment +These habits usually show up together. Porting one JSDoc comment typically means fixing several rows in the table at once. Before — reads fine in an editor, renders broken or missing on pkg.go.dev: -A doc comment is a comment placed immediately before a top-level `package`, `const`, `func`, `type`, or `var` declaration, with **no blank line in between**. The blank line is what separates a doc comment from an unrelated comment above it — `go/doc` uses that rule to associate the comment with the declaration. Get that detail wrong and the documentation disappears from `go doc` and pkg.go.dev even though the comment is still in the source. +```go +// `Cache` is a simple LRU cache. +// +// @param size max number of entries +// @returns a new Cache +func NewCache(size int) *Cache { ... } -Every exported (capitalized) name should have one. Unexported names benefit from comments too, but the conventions below specifically target what shows up in published documentation. +// Get looks up `key` and returns true if found. +// See the [tuning guide](https://example.com/cache-tuning) for eviction details. +func (c *Cache) Get(key string) (string, bool) { ... } +``` -## Naming the symbol in the first sentence +After: -Every doc comment starts with a full sentence whose first word names the symbol being documented. This isn't decorative: `go doc`, IDE hover cards, and pkg.go.dev's search results all surface the first sentence as a one-line summary, often without the symbol name shown alongside. A reader scanning the function list of a package sees only that first sentence — if it doesn't name the function, they can't tell what they're looking at. +```go +// Cache is a simple LRU cache. +type Cache struct{ ... } -| Symbol kind | First-sentence pattern | Example | -| ---------------------- | ------------------------------------- | ----------------------------------------------------------------------------- | -| Package | `Package ...` | `Package path implements utility routines for manipulating slash-separated paths.` | -| Command (`main`) | ` ...` (capitalized) | `Gofmt formats Go programs.` | -| Type | `A ...` / `An ...` | `A Reader serves content from a ZIP archive.` | -| Function (returns) | ` returns ...` | `Quote returns a double-quoted Go string literal representing s.` | -| Function (side effect) | ` ...` | `Exit causes the current program to exit with the given status code.` | -| Function (bool result) | ` reports whether ...` | `HasPrefix reports whether the string s begins with prefix.` | -| Constant / variable | ` is ...` / ` ...` | `Version is the Unicode edition from which the tables are derived.` | +// NewCache creates a Cache that holds at most size entries. +func NewCache(size int) *Cache { ... } -The `reports whether` phrasing for booleans is idiomatic Go — prefer it over `returns true if`. It reads more naturally in the doc index and matches the standard library convention. +// Get reports whether key is present in the cache, returning its +// value if so. See https://example.com/cache-tuning for eviction details. +func (c *Cache) Get(key string) (string, bool) { ... } +``` -## Package documentation +## Reference files -Package documentation is the single most important comment in a module — it is what pkg.go.dev's search indexes, what shows on the package's landing page, and what answers "what is this library for?" for everyone who arrives without context. The detail that follows applies broadly; for a focused checklist on multi-file packages and commands, the rules below are sufficient on their own. +Load on demand; don't read upfront. -### Where to put it +- `references/comment-syntax.md` — full markup grammar (paragraphs, headings, links, doc links, lists, code blocks, notes, deprecations, directives), the gofmt reformatting rules, and the pitfalls that produce unintended code blocks or broken lists. Read it for any comment richer than plain prose, or when gofmt reformats a comment unexpectedly. +- `references/examples.md` — testable `Example` functions: naming rules (including the `_suffix` lower-case constraint), `// Output:` / `// Unordered output:` assertions, whole-file examples. Read it when adding examples or when one misbehaves. +- `references/publishing.md` — how pkg.go.dev discovers and indexes modules via `proxy.golang.org`: `go.mod` and license requirements, semantic versioning, README rendering, source links, retraction, removal. Read it before publishing, or when a module isn't showing up. +- `references/pkgsite-preview.md` — running `pkgsite` to preview a module's docs locally before publishing. Read it when iterating on comments. -The package comment sits directly above the `package` declaration. In a multi-file package, by convention place it in a dedicated `doc.go` file: +## The attachment rule -```go -// Package json implements encoding and decoding of JSON as defined in -// RFC 7159. The mapping between JSON and Go values is described -// in the documentation for the [Marshal] and [Unmarshal] functions. -// -// See "JSON and Go" at https://golang.org/doc/articles/json_and_go.html -// for an introduction to this package. -package json -``` +A doc comment is a comment placed **immediately** before a top-level `package`, `const`, `func`, `type`, or `var` declaration, with **no blank line between them**. The blank line is what `go/doc` uses to tell a doc comment from an unrelated comment above it. Get it wrong and the docs vanish from `go doc` and pkg.go.dev even though the comment is still in the source. Every exported (capitalized) name should have one. -The `doc.go` file usually contains only the package comment and the `package` declaration — no other code. Two reasons to prefer this layout over attaching the comment to a regular source file: +## First sentence names the symbol -1. **Stability.** Source files get split, renamed, or deleted during refactors. A package comment attached to `client.go` quietly disappears the day someone renames `client.go` to `transport.go` and forgets to move the comment. `doc.go` is a stable home that future contributors recognize as "the file that holds package documentation." -2. **Discoverability.** Anyone opening the directory immediately sees that the package has dedicated documentation and where to edit it. +`go doc`, IDE hovers, and pkg.go.dev search all surface the first sentence alone as the one-line summary — often with no symbol name beside it. So the sentence must name the symbol itself. -In a multi-file package, only one file should hold the package comment; if multiple files do, `go/doc` concatenates them in an unspecified order, which is almost never what you want. +| Symbol kind | First-sentence pattern | Example | +| ---------------------- | ------------------------------------- | ------------------------------------------------------------------------------- | +| Package | `Package ...` | `Package path implements utility routines for manipulating slash-separated paths.` | +| Command (`main`) | ` ...` (capitalized) | `Gofmt formats Go programs.` | +| Type | `A ...` / `An ...` | `A Reader serves content from a ZIP archive.` | +| Function (returns) | ` returns ...` | `Quote returns a double-quoted Go string literal representing s.` | +| Function (side effect) | ` ...` | `Exit causes the current program to exit with the given status code.` | +| Function (bool result) | ` reports whether ...` | `HasPrefix reports whether the string s begins with prefix.` | +| Constant / variable | ` is ...` / ` ...` | `Version is the Unicode edition from which the tables are derived.` | + +Use `reports whether` for booleans, not `returns true if` — it is the standard-library convention and reads better in the index. -### First sentence +## Package documentation -The first sentence MUST begin with `Package ` (exact capitalization of the package name). pkg.go.dev indexes this sentence and shows it in search results. Write it as a self-contained one-liner — assume the reader sees nothing else. +The package comment is the most important comment in a module: it is what pkg.go.dev indexes and shows on the landing page, and what answers "what is this for?". Write the first sentence as a self-contained one-liner starting with `Package ` — assume the reader sees nothing else. Good: `Package errgroup provides synchronization, error propagation, and Context cancellation for groups of goroutines working on subtasks of a common task.` +Weak: `This package is a small library for working with goroutines.` — no `Package errgroup`, says nothing specific. -Weak: `This package is a small library for working with goroutines.` — doesn't start with `Package errgroup`, doesn't say what it does specifically. +Put the comment in a dedicated `doc.go` (only the comment plus the `package` clause). It is a stable home that survives the file renames that silently orphan a comment attached to a regular source file, and it signals where package docs live. Only one file may hold the package comment; multiple are concatenated in an unspecified order. -### Overview for larger packages +```go +// Package json implements encoding and decoding of JSON as defined in +// RFC 7159. The mapping between JSON and Go values is described +// in the documentation for the [Marshal] and [Unmarshal] functions. +package json +``` -For packages with more than a handful of exported symbols, include a short overview after the first paragraph. Point readers at the most important types and functions using doc links (see `references/comment-syntax.md` § Doc links): +For packages beyond a handful of symbols, add a short overview after the first paragraph pointing at the main entry points with doc links, so the page doesn't render as a wall of symbols: ```go // Package http provides HTTP client and server implementations. @@ -81,57 +107,24 @@ For packages with more than a handful of exported symbols, include a short overv // [Get], [Head], [Post], and [PostForm] make HTTP (or HTTPS) requests: // // resp, err := http.Get("http://example.com/") -// ... // // The client must close the response body when finished with it: // -// resp, err := http.Get("http://example.com/") -// if err != nil { -// // handle error -// } // defer resp.Body.Close() // -// For control over HTTP client headers, redirect policy, and other -// settings, create a [Client] ... +// For control over headers and redirect policy, create a [Client] ... package http ``` -The overview shows up at the top of the pkg.go.dev page, before the symbol index. Long packages with no overview render as a wall of symbols and force readers to guess where to start. - -### README.md vs package documentation (`doc.go`) - -Go modules typically need **both** a top-level `README.md` and package comments (often in `doc.go`). They serve different readers at different moments — copy-pasting the same text into both is a common mistake. - -**Two audiences:** - -- **Visitor (not yet using the module)** — finds the repo via search, a blog post, or GitHub. They read `README.md` on the host. **Sell and onboard**: what the project is, why it exists, install (`go get`), prerequisites, badges, contributing, links to broader docs. A short, informal quick-start snippet is fine here. -- **Developer (module on the import path)** — runs `go doc`, hovers in an IDE, or browses pkg.go.dev. They read the package comment in `doc.go` (or above `package`). **Technical overview**: what the package provides, design constraints, where to start in the API, doc links to key symbols — the same role as standard-library package docs. Assume the package is installed; do not make them open the README for a package-level overview. - -After `go get`, the README effectively disappears from day-to-day work. Package documentation is what `go doc path/to/pkg` and pkg.go.dev show above the symbol index. If that overview is empty or only says "see README", callers lose the toolchain integration that makes Go documentation distinctive. - -**What goes where:** +### README vs `doc.go` -Put in **README.md**: +Modules typically need both, for two readers — don't paste the same text into both. The `README.md` is read on the host (GitHub/pkg.go.dev landing area) by someone **not yet using** the module: pitch, `go get`, prerequisites, badges, a quick-start snippet. The package comment is read via `go doc`, IDE hover, and the pkg.go.dev API view by someone who **already imported** it: technical overview, main types and call flow, invariants, error and concurrency semantics, doc links to key symbols. After `go get` the README disappears from daily work — a package overview that only says "see README" loses the toolchain integration that makes Go docs distinctive. -- Project pitch, status, roadmap, community -- Install, env setup, credentials, deployment -- Badges, CI, license summary, changelog links -- "Try it in 30 seconds" sample for casual visitors -- Migration guides between major versions at the **module** level - -Put in **package comment** / **`doc.go`**: - -- `Package ...` first sentence (search-indexed on pkg.go.dev) -- API-oriented overview: main types, typical call flow -- Invariants, error semantics, concurrency, performance notes -- Doc links (`[Client]`, `[New]`), patterns that belong in `go doc` -- Per-package design notes; subpackage overviews in **that** package's `doc.go` - -**Multi-package modules:** one `README.md` at the module root for the whole project. Each package (including subpackages) gets its own package comment — usually in a `doc.go` in that directory — not a separate README per package unless you have an unusual layout. +For a multi-package module: one `README.md` at the root, and a package comment (usually a `doc.go`) in every package and subpackage — not a README per package. ### Commands (`package main`) -For binary commands, the package comment describes the program rather than its Go API. The first sentence begins with the program name capitalized as a normal sentence start: +For a binary, the package comment describes the program, not a Go API. Use semantic linefeeds (one sentence per line); gofmt preserves them, giving cleaner diffs. ```go // Gofmt formats Go programs. @@ -143,22 +136,16 @@ For binary commands, the package comment describes the program rather than its G // // The flags are: // -// -d -// Do not print reformatted sources to standard output. -// Instead, print diffs. // -l // List files whose formatting differs from gofmt's. -// ... package main ``` -Use semantic linefeeds (one sentence per line) in command documentation. Gofmt preserves line breaks within paragraphs, so this produces cleaner diffs when text is edited and reads naturally when rendered. - ## Function and method documentation -Pick the first-sentence pattern from the table above. Beyond that, include the details a caller would reasonably need: +Pick the first-sentence pattern from the table, then add what a caller needs. Refer to parameters and results by name in prose, without backticks; avoid names like `a` or `s` that read as ordinary words. -- **Special cases and edge cases.** Document edge cases as an indented preformatted block, as in the standard library's `math.Sqrt`: +- **Special cases** as an indented block: ```go // Sqrt returns the square root of x. @@ -166,27 +153,24 @@ Pick the first-sentence pattern from the table above. Beyond that, include the d // Special cases are: // // Sqrt(+Inf) = +Inf - // Sqrt(±0) = ±0 // Sqrt(x < 0) = NaN // Sqrt(NaN) = NaN func Sqrt(x float64) float64 ``` -- **Asymptotic complexity** when it matters to callers (e.g., `sort.Sort` documents its O(n log n) call count to `Less` and `Swap`). -- **Concurrency safety.** Top-level functions are conventionally assumed to be safe for concurrent use; state otherwise if not. For methods, the default assumption is the opposite — only safe for a single goroutine at a time. State exceptions in the method comment, or in the type's doc comment if it applies to all methods. -- **Deprecation.** A paragraph beginning `Deprecated:` triggers tools to warn callers and causes pkg.go.dev to hide the symbol by default. See `references/comment-syntax.md` § Deprecations. - -Leave out internal implementation details. The doc comment describes behavior callers can rely on, not the algorithm currently used. Documenting "uses quicksort internally" locks the implementation behind the doc contract — a future maintainer who switches to timsort will silently break that contract. +- **Concurrency safety.** Top-level functions are conventionally assumed safe for concurrent use; methods are assumed *not* safe (single goroutine at a time). State any exception explicitly. +- **Complexity** when it matters to callers (e.g. `sort.Sort` documents its O(n log n) comparisons). +- **Deprecation** via a `Deprecated:` paragraph — always name the replacement. See `references/comment-syntax.md` § Deprecations. -Refer to named parameters and results directly by name in prose, without backticks: `Copy copies from src to dst until either EOF is reached on src or an error occurs. It returns the total number of bytes written and the first error encountered while copying, if any.` Avoid parameter names like `a` or `s` that could be confused with ordinary words. +Document behavior callers can rely on, not the algorithm. "Uses quicksort internally" locks the implementation behind the doc contract; a maintainer who switches to timsort silently breaks it. ## Type documentation -Start with `A ` or `An ` and describe what an instance represents. Then add whichever of these apply: +Start with `A ` / `An ` describing what an instance represents, then add whichever apply: -- **Zero value.** Go encourages designing types whose zero value is useful. When the zero value is meant to work — `var b Buffer` is a valid empty buffer — document it: `The zero value for Buffer is an empty buffer ready to use.` Readers can only count on the zero value working if you say so. -- **Concurrency safety.** The default assumption is "not safe for concurrent use". Anything stronger (`A Regexp is safe for concurrent use by multiple goroutines, except for configuration methods, such as Longest.`) must be explicit. -- **Field documentation.** For structs with exported fields, either the type's doc comment describes the fields collectively, or each exported field has its own per-field comment. Pick one approach per type — don't mix the two awkwardly. +- **Zero value.** If the zero value is meant to work, say so — readers can only rely on it if documented: `The zero value for Buffer is an empty buffer ready to use.` +- **Concurrency safety.** Default is "not safe"; anything stronger must be explicit. +- **Fields.** Either the type comment describes exported fields collectively, or each field carries its own comment — pick one per type. ```go // A LimitedReader reads from R but limits the amount of @@ -199,11 +183,11 @@ type LimitedReader struct { } ``` -For methods, use a consistent receiver name across the type so the method list reads uniformly on pkg.go.dev. A type with `func (c *Conn) Read` and `func (conn *Conn) Write` looks unfinished. +Use one consistent receiver name across a type's methods; mixing `c *Conn` and `conn *Conn` looks unfinished in the method list. ## Constant and variable documentation -**Grouped** (a `const (...)` or `var (...)` block of related items): one introductory doc comment for the whole block, with brief end-of-line comments on individual entries: +**Grouped** (`const (...)` / `var (...)`): one intro comment for the block, brief end-of-line comments per entry. ```go // Generic file system errors. @@ -213,74 +197,60 @@ var ( ErrInvalid = errInvalid() // "invalid argument" ErrPermission = errPermission() // "permission denied" ErrExist = errExist() // "file already exists" - ErrNotExist = errNotExist() // "file does not exist" ) ``` -For typed constants displayed next to their type on pkg.go.dev, the type's doc comment can cover them; the const block need not repeat the explanation. +**Ungrouped** (a single top-level `const`/`var`): a full comment starting with the name, like any other symbol. -**Ungrouped** (a single top-level `const` or `var`): a full doc comment starting with the name, same as for any other top-level symbol: +## Doc links -```go -// Version is the Unicode edition from which the tables are derived. -const Version = "13.0.0" -``` - -## Testable examples - -Go's `testing` package supports `Example` functions: documented uses of a package that are compiled (and optionally run) by `go test` and rendered on pkg.go.dev. They are the most reliable form of documentation because they cannot drift away from the API — a renamed function breaks the example at the next test run. - -Add an example when the API has any non-trivial usage pattern that prose alone won't communicate. Place examples in a `_test.go` file in the same package (or a `*_test` external package if they need to import the package the same way users would). For naming rules, output assertions, and the whole-file example pattern, see `references/examples.md`. - -## Cross-references with doc links - -Inside any doc comment, use `[Name]` to link to another exported identifier in the same package, or `[pkg.Name]` to link across packages. Doc links render as hyperlinks on pkg.go.dev and are validated by `go doc` and IDEs — broken links surface immediately. +Inside any comment, `[Name]` links to an exported identifier in the same package and `[pkg.Name]` links across packages. They render as hyperlinks and are validated by `go doc` and IDEs, so broken links surface immediately. Use them liberally in overviews and wherever prose names another symbol. ```go // ReadFrom reads data from r until EOF and appends it to the buffer, growing -// the buffer as needed. The return value n is the number of bytes read. Any -// error except [io.EOF] encountered during the read is also returned. If the -// buffer becomes too large, ReadFrom will panic with [ErrTooLarge]. +// the buffer as needed. Any error except [io.EOF] encountered during the read +// is also returned. If the buffer becomes too large, ReadFrom panics with +// [ErrTooLarge]. func (b *Buffer) ReadFrom(r io.Reader) (n int64, err error) { ... } ``` -Use them liberally in package overviews and in any prose that names another symbol. See `references/comment-syntax.md` § Doc links for the full syntax (including the `[*pkg.Type]` pointer form and the rule about adjacent punctuation). +A link must be surrounded by punctuation, spaces, or line boundaries: `map[ast.Expr]TypeAndValue` is *not* a link because the brackets touch other identifier characters. See `references/comment-syntax.md` § Doc links. -## Publishing and previewing +## Testable examples -Two distinct workflows: +`Example` functions are documented uses compiled (and optionally run) by `go test` and rendered on pkg.go.dev. They can't drift from the API — a rename breaks them at the next test run. Add one for any non-trivial usage pattern prose won't convey; place them in a `_test.go` file, preferably in a `*_test` external package so they import the package as a user would. For naming, output assertions, and whole-file examples, see `references/examples.md`. -- **Local preview before publishing**: run `pkgsite` in the module directory and open the rendered docs in a browser. Iterate on comments, refresh, repeat. See `references/pkgsite-preview.md`. +## Publishing and previewing + +- **Preview locally** with `pkgsite` in the module directory, then iterate. See `references/pkgsite-preview.md`. ```bash go install golang.org/x/pkgsite/cmd/pkgsite@latest - cd /path/to/module - pkgsite -open # opens http://localhost:8080 + cd /path/to/module && pkgsite -open # http://localhost:8080 ``` -- **Publishing to pkg.go.dev**: pkg.go.dev indexes any public module reachable via the Go module proxy. Tag a semantic version, push the tag, and either visit `https://pkg.go.dev/` and click *Request*, or run `go get @` from any machine — both trigger indexing. See `references/publishing.md` for license, version, and README requirements. +- **Publish**: tag a semantic version and push; pkg.go.dev indexes any public module reachable via the proxy. Force it by visiting `https://pkg.go.dev/` and clicking *Request*, or running `go get @`. See `references/publishing.md` for license, version, and README requirements. -While editing, `go doc` is the fastest way to verify a comment renders as intended. It uses the same parsing as pkg.go.dev, so what you see locally matches what users will see published: +While editing, `go doc` is the fastest check — same parser as pkg.go.dev: ```bash -go doc # current package overview -go doc . # same -go doc SomeType # a specific symbol -go doc SomeType.Method # a method -go doc -all # everything in the current package -go doc -src SomeType # also show source +go doc # current package overview +go doc SomeType # a specific symbol +go doc SomeType.Method # a method +go doc -all # everything in the package ``` -Reach for `pkgsite` once the comment includes formatting (headings, lists, links, code blocks) — that's where the HTML rendering can diverge from the plain-text `go doc` view, particularly around code block indentation and list formatting. +Switch to `pkgsite` once a comment includes formatting (headings, lists, links, code blocks) — that's where HTML rendering can diverge from `go doc`'s plain text. + +For mechanical enforcement in CI, `staticcheck`'s stylecheck rules `ST1000` (package comment format) and `ST1020`–`ST1022` (function/type/const comments must start with the declared name) catch malformed openers — they're opt-in (excluded from the default check set), so enable them explicitly if you want this checked automatically. ## Quick checklist before committing -- [ ] Every exported name has a doc comment with no blank line between the comment and the declaration. -- [ ] The first sentence names the symbol and follows the pattern from the table above. +- [ ] Every exported name has a doc comment with no blank line before its declaration. +- [ ] The first sentence names the symbol and follows the table's pattern. - [ ] The package has a `Package ...` comment (in `doc.go` for multi-file packages). - [ ] Package overview and API entry points live in doc comments, not only in `README.md`. -- [ ] `README.md` complements package doc (install, motivation, project meta) without duplicating the full technical overview. -- [ ] `gofmt` produces no changes to the comments (`gofmt -d ./...`). -- [ ] `go doc` shows the symbol with the comment you expected. -- [ ] For new APIs: at least one `Example` function demonstrates the common usage path. -- [ ] If any comment uses links, lists, headings, or code blocks: previewed in `pkgsite` and renders as intended. +- [ ] No JSDoc leftovers: no `@param`/`@returns` tags, no `/** */` block comments, no `{@link}` refs, no `` `backtick` `` code, no `[text](url)` links, no `**bold**`. +- [ ] `gofmt -d ./...` produces no changes to the comments. +- [ ] `go doc` shows each symbol with the comment you expected. +- [ ] New APIs have at least one `Example`; comments with links/lists/headings/code blocks preview correctly in `pkgsite`. diff --git a/skills/go-documentation/evals/evals.json b/skills/go-documentation/evals/evals.json new file mode 100644 index 0000000..9de9ba0 --- /dev/null +++ b/skills/go-documentation/evals/evals.json @@ -0,0 +1,67 @@ +{ + "skill_name": "go-documentation", + "assertions_intent": "Each eval targets a cluster of Go doc conventions the go-documentation skill teaches, chosen so a no-skill baseline is likely to produce plausible-but-wrong docs — usually by carrying a TypeScript/JSDoc/npm habit straight into Go, where tooling (go doc, gofmt, pkg.go.dev) silently renders it wrong or drops it. Two design rules keep the delta honest, mirroring the sibling idiomatic-go suite: (1) prompts state the user's symptom and goal in plain terms and never name the Go fix or the convention (no 'doc link', 'attachment rule', 'reports whether', 'doc.go', 'Example function', 'Output comment', 'semantic linefeed', 'Deprecated: prefix') — a passing run had to know the convention, not echo the prompt; (2) fixtures compile and vet cleanly but their doc comments deliberately deviate from Go conventions, written the way a TS/JS developer would — and, per the iteration 1 finding below, deviations are written as plausible real-world comments rather than textbook-bad ones a model could pattern-match without any Go-specific knowledge. The three clusters are complementary: eval 0 is comment MARKUP (JSDoc tags, backtick 'inline code', Markdown links, bold/fences, the doc-comment attachment rule, first-sentence patterns), eval 1 is PACKAGE-LEVEL docs (the npm 'README is the docs' mindset, a non-self-contained overview, doc.go, README-vs-package-comment split, entry-point doc links), and eval 2 is TESTABLE EXAMPLES (turning @example prose into real compiled Example functions in an external _test.go with Output assertions and correct naming). Fixtures under evals/files/ are self-contained Go modules; all fixture paths in this file are repo-relative and must be resolved against the skill root when launching a run.\n\nIteration 1 measured result (cursor-auto subagent, six runs, all self-reported CLEAN isolation): with_skill 23/23 (100%), without_skill 19/23 (84%), +16pp overall — but the delta was entirely carried by two of the three evals. Eval 0 delivered +33pp (6/9 -> 9/9): the baseline left the Markdown link `[the tuning guide](https://…)` untouched (and, per its own write-up, rationalized it as 'valid in modern godoc (Go 1.19+)' — a fabricated justification, not an oversight), kept 'Available returns true if…' instead of 'reports whether', and never wrote a `Package tokenbucket` opener or fixed 'Creates a new Bucket'/'A Bucket holds'. Eval 1 delivered +14pp (6/7 -> 7/7): the only failing baseline assertion was entry-point-doc-links — baseline did add `[Open]`/`[Store.Get]`/`[ErrNotFound]` links, but only on individual method comments in store.go, never inside the package-level overview a reader sees first. Eval 2 tied 7/7 both configs (0pp): the fixture's second `@example` tag was already spelled `withNumbers` in lower-case, so copying it verbatim into `ExampleSlugifier_Make_withNumbers` produced the *correct* Go suffix by accident — the assertion designed to catch a baseline getting the strict lower-case-initial suffix rule wrong never actually exercised the mistake. attachment-blank-line-removed also tied (both configs noticed and fixed the blank line orphaning the `Bucket` doc comment) despite being tagged [discriminating, untested] going in: a capable baseline reads 'no blank line before a declaration' as an obviously-suspicious visual gap without needing the skill's specific attachment-rule knowledge.\n\nIteration 2 changes (this revision — assertions below are tagged [sharpened, untested] where the fixture or assertion wording changed in a way that could flip the measured result, [discriminating, iteration-1] where iteration 1 measured a real with/without split and the fixture is otherwise unchanged, [regression, iteration-1] where iteration 1 measured a tie and the fixture is otherwise unchanged): (a) eval 0's Bucket doc comment no longer sits above a visibly blank line before `type Bucket struct` — it now sits directly above an intervening, unexported `var bucketSchemaVersion = 1` declaration with no blank line, so it silently misattaches to that invisible symbol instead of visibly floating; `type Bucket struct` itself ends up with zero doc comment. Spotting this requires actually knowing go/doc's 'a comment attaches to whatever top-level declaration follows it, blank-line-free — whatever that declaration is' rule, not just noticing whitespace. (b) eval 1's package comment no longer echoes SKILL.md's own textbook-bad 'This package is a simple embedded key-value store. / See the README …' wording almost verbatim — it now opens with a correctly-formed 'Package kvstore implements a lightweight, embeddable key-value store.' sentence (already satisfying the first-sentence check, which iteration 1 showed baselines get right regardless of fixture wording) and defers the substantive overview to the README in more natural, plausible prose ('Check out the project README for the full rundown — install instructions, a quick-start walkthrough, and a few benchmarks against the alternatives'). This concentrates the fixture's difficulty on the part iteration 1 actually showed discriminates — extracting a doc.go with an inline technical overview and doc links — instead of an opener a baseline fixes unprompted either way; entry-point-doc-links now explicitly requires the links to live in that doc.go overview, not merely somewhere in the module. (c) eval 2's second `@example` on `Slugifier.Make` now carries a capitalized JSDoc `WithNumbers` (real JSDoc/TypeDoc syntax) instead of the already-correctly-cased `withNumbers` tag iteration 1 used. Porting the caption text verbatim into an Example function name produces the exact mis-attaching `ExampleSlugifier_Make_WithNumbers` the suffix rule forbids (the parser reads `WithNumbers` as a nonexistent method on `Slugifier` and silently fails to attach the example to anything useful), so getting this right now requires applying the strict lower-case-initial rule rather than copying a tag that was already correctly cased.", + "evals": [ + { + "id": 0, + "name": "jsdoc-comments-markup", + "prompt": "I just ported this little rate-limiter package (evals/files/jsdoc-comments/tokenbucket.go) from a TypeScript library I wrote, and I documented everything the way I always do in TS. A colleague looked at it and said the docs 'look broken' on pkg.go.dev — some of the formatting shows up as literal junk, and a couple of the comments don't appear at all when he runs `go doc`. I honestly don't know Go's documentation conventions. Can you fix the comments so they render correctly and read the way Go developers expect, and walk me through what was actually wrong? Please don't change the behavior of the code.", + "expected_output": "A rewrite of the doc comments (not the logic) that removes the JSDoc/Markdown accent and follows Go conventions, plus an explanation naming each problem. Expected corrections: the @param/@returns/@throws/@deprecated tags are gone — parameters and results are described in prose, and @deprecated becomes a paragraph beginning with the exact prefix 'Deprecated: ' that names Take as the replacement; backtick 'inline code' (`ErrEmpty`, `Take`, `n`) and the '**not**' bold are gone — symbol references become doc links like [ErrEmpty]/[Take] or plain text, never backticks; the Markdown link [the tuning guide](https://...) becomes either a bare URL or Go reference-style ([the tuning guide] in prose with a [the tuning guide]: URL definition at the end); the fenced ```go example block in Take's comment is either removed or turned into a Go doc code block (a tab-indented span with a blank line around it), and ideally the runnable snippet moves to an Example function; the Bucket doc comment currently sits directly above an unrelated `var bucketSchemaVersion = 1` declaration with no blank line — so it silently attaches to that unexported var instead of Bucket — and is then separated from `type Bucket struct` by a blank line, leaving Bucket with no doc comment at all; the fix moves the comment to sit immediately above `type Bucket struct` with no blank line and no intervening declaration (bucketSchemaVersion may keep a short comment of its own, or none, or be dropped entirely as an accidental leftover, but must not keep the Bucket text); first sentences are fixed to name the symbol — New's 'Creates a new Bucket ...' becomes 'New ...', and Available's 'returns true if ...' becomes the boolean convention 'Available reports whether ...'; error strings and the /** */ JSDoc block on DefaultCapacity are cleaned up to normal Go form. The write-up ties each change to the tool behavior it fixes (curly-quote rendering, non-rendering Markdown, the misattached/vanished comment). The code still builds and gofmt -d reports no further changes to the comments.", + "files": [ + "evals/files/jsdoc-comments/tokenbucket.go", + "evals/files/jsdoc-comments/go.mod" + ], + "assertions": [ + "no-backtick-inline-code [regression, iteration-1]: no doc comment uses backtick pairs (or single-quote pairs) to mark inline code — the `ErrEmpty`, `Take`, and `n` backticks are replaced by doc links ([ErrEmpty], [Take]) or plain text, because Go renders backticks as curly typographic quotes rather than code. (Iteration 1: both configs stripped these unprompted — a capable baseline recognizes backtick-as-code as a JSDoc/Markdown reflex on sight, without needing the skill's specific rendering explanation. Kept as a regression guard for weaker models.)", + "markdown-link-converted [discriminating, iteration-1]: the Markdown link [the tuning guide](https://example.com/tokenbucket/tuning) is gone — replaced by a bare URL or by Go reference-style ([the tuning guide] in prose plus a `[the tuning guide]: https://...` definition), because Go doc has no [text](url) form and would render it literally. (Iteration 1: the baseline not only left the link, its own write-up rationalized it as 'valid in modern godoc (Go 1.19+)' — a confident, fabricated justification rather than a simple miss, which is exactly the kind of plausible-wrong-answer the skill exists to prevent.)", + "attachment-fixed-to-correct-symbol [sharpened, untested]: the Bucket doc comment ends up immediately above `type Bucket struct` with no blank line and no other declaration between them, so `go doc Bucket` shows the full comment; if `bucketSchemaVersion` survives, it does not carry the Bucket text (it may have its own short comment or none). The response also explains that a doc comment attaches to whatever top-level declaration follows it with no blank line in between — whatever that declaration is — not necessarily the one the author intended. (Iteration 1: as a plain orphaning blank line directly above `type Bucket struct`, both configs caught and fixed it unprompted — retagged from [discriminating, untested] to reflect that tie. Iteration 2: the fixture now disguises the same underlying attachment-rule bug as a misattachment to an intervening unexported var rather than a visible gap, so recognizing it requires knowing the rule, not just noticing whitespace.)", + "boolean-reports-whether [discriminating, iteration-1]: the boolean-returning method Available leads with the 'reports whether' convention ('Available reports whether ...') rather than 'returns true if ...'. (Iteration 1: baseline kept 'returns true if', which reads fine but is not the standard-library convention; this was one of the three assertions the baseline failed.)", + "no-jsdoc-tags [regression, iteration-1]: no @param, @returns, @throws, or @deprecated tag survives in any comment; parameters/results are described in prose and types are left to the signature. (Iteration 1: both configs removed all tags unprompted.)", + "deprecated-prefix [regression, iteration-1]: the deprecation on Refill is expressed as a paragraph beginning with the exact prefix 'Deprecated: ' and names Take as the replacement, rather than an @deprecated tag or free-form 'is deprecated' prose. (Iteration 1: both configs produced this correctly.)", + "no-bold-or-fences [regression, iteration-1]: no comment contains **bold** or a ```-fenced code block; any code snippet that remains is a tab-indented Go doc code block set off by blank lines (or is moved into an Example function). (Iteration 1: both configs stripped bold/fences unprompted.)", + "no-jsdoc-block-comment [regression, untested]: the `/** ... */` block comment on DefaultCapacity is converted to a plain `//` comment starting with the symbol name (DefaultCapacity is ...), because gofmt deliberately leaves the leading-`*` block-comment style untouched, so an un-stripped `/** */` renders as a mangled code block on pkg.go.dev even though it looks harmless in source and gofmt raises no complaint about it. (New assertion — iteration 1's expected_output named this fix in prose but had no dedicated check; both configs fixed it in that run, so it is filed as a regression guard rather than a discriminator, but it is worth covering explicitly since it is easy for a weaker model to miss precisely because nothing flags it as malformed.)", + "first-sentence-names-symbol [discriminating, iteration-1]: each exported symbol's first sentence begins with its own name following the convention table — the package comment starts with 'Package tokenbucket', 'New ...' (not 'Creates ...'), 'A Bucket ...', 'Take ...', 'SetRate ...', 'Refill ...'. (Iteration 1: baseline's package comment never gained a 'Package tokenbucket' opener, New kept 'Creates a new Bucket ...', and Bucket's comment dropped the article 'A' — all three sub-checks failed on the baseline in the same run.)", + "behavior-unchanged-builds [regression, iteration-1]: only comments (and trivially error strings) change; the package still builds (go build ./...) and vets cleanly, and gofmt -d reports no pending reformatting of the rewritten comments. (Iteration 1: both configs passed cleanly.)" + ] + }, + { + "id": 1, + "name": "package-overview-readme-mindset", + "prompt": "I'm publishing my first Go module — a small in-memory key-value store in evals/files/package-overview/ (package kvstore). Coming from npm, my instinct was to write a really good README and let the package comment just point people to it, which is what I've done here. But when I look at the module on pkg.go.dev the main page barely has anything beyond a one-line description, and when I hover over the package in my editor I don't get much more — meanwhile all the real explanation (the concurrency model, what the sentinel error means, how the pieces fit together) is sitting in the README instead. Can you set up the documentation the way an experienced Go developer would, so the package actually documents itself for someone who's already importing it, and tell me what you changed and why? Feel free to add or reorganize files.", + "expected_output": "A package comment that carries the technical overview into the toolchain, plus an explanation of the README-vs-package-comment split. Expected: the package comment's overview no longer hands the substantive technical content off to the README (the 'Check out the project README for the full rundown' hand-off is removed, or reduced to pointing only at install/non-technical content) — instead the comment itself gives the overview a person who already imported the package needs: what a Store is, the concurrency model, the ErrNotFound sentinel semantics, and doc links to the main entry points ([Open], [Store], [Store.Get]/[Store.Set], [ErrNotFound]) placed directly in that overview, not only scattered across individual method comments. The package comment is placed in a dedicated doc.go (only the comment plus `package kvstore`), since it is the stable home that survives file renames; store.go's package comment is reduced/removed accordingly so only one file carries it. The README is kept for the not-yet-a-user audience (pitch, go get, quick start) rather than deleted, and the write-up explains that go doc / IDE hover / the pkg.go.dev API view read the package comment, not the README, so leaving the technical overview only in the README loses the docs after go get even when the opening sentence already looks well-formed. Optionally the thin per-symbol comments (Open 'makes a Store', Get 'looks up a key') are enriched. The package still builds.", + "files": [ + "evals/files/package-overview/store.go", + "evals/files/package-overview/README.md", + "evals/files/package-overview/go.mod" + ], + "assertions": [ + "doc-go-created [regression, iteration-1]: the package comment lives in a dedicated doc.go file (containing only the comment and `package kvstore`), and it is the ONLY file carrying a package comment (store.go's package comment is moved out, not duplicated). (Iteration 1: both configs created doc.go and removed store.go's package comment unprompted.)", + "no-see-readme-punt [sharpened, untested]: the package comment's overview no longer hands the substantive technical content off to the README — whatever the exact wording ('See the README', 'Check out the project README for the full rundown', or similar) — the concurrency model, the ErrNotFound semantics, and the entry points are explained in the comment itself; the README may still be mentioned for install/non-technical content. (Iteration 1: measured with a fixture whose opener near-verbatim echoed SKILL.md's own 'This package is... / See the README...' anti-pattern text, which both configs fixed unprompted — non-discriminating on that wording. Iteration 2: the fixture's opener is now a correctly-formed 'Package kvstore implements...' sentence that still defers the substantive overview to the README in ordinary, plausible prose, so passing this now requires noticing the hand-off survives even though the first sentence already looks compliant.)", + "entry-point-doc-links [discriminating, iteration-1]: the package overview inside doc.go (not merely somewhere else in the module) uses doc links ([Open], [Store], [ErrNotFound], and/or [Store.Get]) to point at the main entry points, rather than naming them in backticks or plain prose with no links, and rather than only linking them from store.go's individual method comments. (Iteration 1: this was the only baseline failure in this eval — the baseline did add [Open]/[Store.Get]/[ErrNotFound] doc links, but only on the corresponding methods in store.go, never inside the package-level overview a reader sees first; with_skill put the links in doc.go's overview itself.)", + "readme-not-duplicated [regression, iteration-1]: the solution does not simply paste the README prose into the package comment — the README is preserved for the pitch/getting-started audience and the package comment is written for the already-importing audience; the write-up articulates that split. (Iteration 1: both configs wrote a distinct, non-duplicated overview and explained the split.)", + "package-first-sentence-self-contained [regression, iteration-1]: the package comment's first sentence begins with 'Package kvstore' and stands on its own as a one-line summary. (Iteration 1: both configs produced this regardless of how badly the input's opener was worded. Iteration 2: the fixture's input opener already reads 'Package kvstore implements...', so this assertion now mainly guards against the rest of the rewrite accidentally breaking the opener — the discriminating work in this eval is entry-point-doc-links and no-see-readme-punt, not this sentence.)", + "readme-preserved [regression, iteration-1]: README.md is not deleted; it still serves the not-yet-a-user audience (install, quick start, pitch). (Iteration 1: both configs kept it.)", + "builds-clean [regression, iteration-1]: after the reorganization the module still builds (go build ./...) and vets cleanly, with exactly one package clause per file and no orphaned/duplicate package comment. (Iteration 1: both configs passed.)" + ] + }, + { + "id": 2, + "name": "testable-examples-from-jsdoc", + "prompt": "The package in evals/files/testable-examples/ (package slug) turns arbitrary text into URL slugs. I documented how to use it by sprinkling @example snippets — a couple of them with a short caption — into the doc comments, the way I would in TypeScript. What I actually want is for those to become real, runnable examples that show up on pkg.go.dev the way the standard library's do — the kind where the expected output is shown and verified when tests run. Can you set that up properly, clean up however I documented the examples in the comments, and double-check the details so they don't silently fail to run or attach to the wrong thing? Show me the files.", + "expected_output": "Real testable Example functions replacing the @example prose, plus cleanup of the comment markup. Expected: one or more Example functions in a new _test.go file — ExampleMake for the package function Make, and ExampleSlugifier / ExampleSlugifier_Make for the type and its method — each ending with a `// Output:` block whose expected text matches the real output (Make(\"Hello, World!\") => hello-world, Make(\" Go & TypeScript \") => go-typescript, Make(\"foo_bar 123\") => foo-bar-123), so `go test` compiles AND runs them. Slugifier.Make's second @example carries a `WithNumbers` tag; the corresponding Example function must be named ExampleSlugifier_Make_withNumbers — lower-casing the caption's leading letter — never the verbatim-cased ExampleSlugifier_Make_WithNumbers, which the parser would read as a nonexistent method WithNumbers on Slugifier and silently fail to attach the example to anything. The test file uses an external test package (package slug_test) and imports the package normally (import \"example.com/go-documentation-evals/slug\") so the rendered example includes the import line a user needs. Examples use fmt.Println for output, not log.Fatal/os.Exit. In slug.go the @example tags — including the `...` markup, which would otherwise render as literal angle-bracket text rather than a label — and the ```go fenced block in the package comment are removed; usage is now carried by the Example functions, with at most a tab-indented Go doc code block if prose still shows a snippet. go test ./... passes.", + "files": [ + "evals/files/testable-examples/slug.go", + "evals/files/testable-examples/go.mod" + ], + "assertions": [ + "real-example-functions-with-output [regression, iteration-1]: the solution adds compiled Example functions (func Example... in a _test.go file) with a trailing `// Output:` block as the final statement, whose expected text matches actual output, so `go test ./...` both compiles and runs them and passes — rather than leaving usage as @example prose or an unverified snippet. (Iteration 1: both configs produced this correctly.)", + "external-test-package [regression, iteration-1]: the example file declares `package slug_test` (not `package slug`) and imports the package under test normally, so the rendered pkg.go.dev example carries the `import` line a real user needs. (Iteration 1: both configs used the external test package unprompted.)", + "suffix-lowercase-initial [sharpened, untested]: the example attached to Slugifier.Make for the `WithNumbers` variant is named ExampleSlugifier_Make_withNumbers — the caption's leading letter is lower-cased — never left as the caption's original upper-case form (ExampleSlugifier_Make_WithNumbers), which the parser would read as a nonexistent method WithNumbers on Slugifier and misattach without any compile error. (Iteration 1: the fixture's tag was already spelled `withNumbers` in lower-case, so both configs trivially matched it verbatim and the assertion never exercised the actual mistake — tied 'discriminating, untested' with no real signal. Iteration 2: the tag is now the naturally-capitalized `WithNumbers`, so producing the correct lower-case suffix now requires applying the rule rather than copying it.)", + "jsdoc-example-tags-removed [regression, iteration-1]: the @example tags — including any `...` markup — and the ```go fenced code block in slug.go's comments are removed; no literal `` text survives in any doc comment (it would render as literal angle-bracket text, not a label, in go doc/pkg.go.dev). Usage lives in the Example functions now, and any remaining in-comment snippet is a tab-indented Go doc code block, not a Markdown fence. (Iteration 1: both configs removed the plain @example tags and fence unprompted; iteration 2 adds explicit coverage for the new `` sub-tag introduced alongside the suffix trap.)", + "correct-example-naming [regression, iteration-1]: example function names use exact identifier capitalization and the Example/ExampleF/ExampleT/ExampleT_M scheme — ExampleMake attaches to Make, ExampleSlugifier to the type, ExampleSlugifier_Make to the method (not e.g. ExampleSlugifierMake or ExampleMakeSlug). (Iteration 1: both configs got the base naming scheme right.)", + "no-fatal-in-examples [regression, iteration-1]: example bodies use fmt.Println (or similar) for visible output and do not call log.Fatal or os.Exit, which would terminate the test process. (Iteration 1: both configs passed.)", + "builds-and-tests-pass [regression, iteration-1]: the package still builds and vets cleanly and `go test ./...` passes with the new examples (Output assertions match), confirming the examples run rather than merely compile. (Iteration 1: both configs passed.)" + ] + } + ] +} diff --git a/skills/go-documentation/evals/files/jsdoc-comments/go.mod b/skills/go-documentation/evals/files/jsdoc-comments/go.mod new file mode 100644 index 0000000..4e9ca1f --- /dev/null +++ b/skills/go-documentation/evals/files/jsdoc-comments/go.mod @@ -0,0 +1,3 @@ +module example.com/go-documentation-evals/tokenbucket + +go 1.23 diff --git a/skills/go-documentation/evals/files/jsdoc-comments/tokenbucket.go b/skills/go-documentation/evals/files/jsdoc-comments/tokenbucket.go new file mode 100644 index 0000000..f30be2d --- /dev/null +++ b/skills/go-documentation/evals/files/jsdoc-comments/tokenbucket.go @@ -0,0 +1,97 @@ +// A rate limiter based on the token bucket algorithm. +// See https://en.wikipedia.org/wiki/Token_bucket for the algorithm. +package tokenbucket + +import ( + "errors" + "time" +) + +/** + * The maximum number of tokens a bucket can hold by default. + */ +const DefaultCapacity = 100 + +// `ErrEmpty` is returned by `Take` when there are not enough tokens available. +var ErrEmpty = errors.New("Not enough tokens available.") + +// A Bucket holds tokens that refill over time. +// +// It is **not** safe for concurrent use. Wrap it in a mutex if you +// share it between goroutines. +var bucketSchemaVersion = 1 + +type Bucket struct { + capacity int + tokens int + rate time.Duration + last time.Time +} + +// Creates a new Bucket with the given capacity and refill rate. +// +// @param capacity The maximum number of tokens the bucket can hold. +// @param rate How often a single token is added to the bucket. +// @returns A pointer to the newly created Bucket. +func New(capacity int, rate time.Duration) *Bucket { + return &Bucket{ + capacity: capacity, + tokens: capacity, + rate: rate, + last: time.Now(), + } +} + +// Take removes `n` tokens from the bucket. +// +// @param n the number of tokens to remove +// @throws ErrEmpty if there are fewer than n tokens left +// +// Example usage: +// ```go +// if err := b.Take(1); err != nil { +// log.Fatal(err) +// } +// ``` +func (b *Bucket) Take(n int) error { + b.refill() + if b.tokens < n { + return ErrEmpty + } + b.tokens -= n + return nil +} + +// Available returns true if at least one token is currently available. +func (b *Bucket) Available() bool { + b.refill() + return b.tokens > 0 +} + +// SetRate changes how often tokens are added. +// For more details see [the tuning guide](https://example.com/tokenbucket/tuning). +func (b *Bucket) SetRate(rate time.Duration) { + b.rate = rate +} + +// Refill tops the bucket up based on elapsed time. +// +// @deprecated Use Take instead, which refills the bucket automatically. +func (b *Bucket) Refill() { + b.refill() +} + +func (b *Bucket) refill() { + now := time.Now() + if b.rate <= 0 { + return + } + added := int(now.Sub(b.last) / b.rate) + if added > 0 { + b.tokens += added + if b.tokens > b.capacity { + b.tokens = b.capacity + } + b.last = now + } +} diff --git a/skills/go-documentation/evals/files/package-overview/README.md b/skills/go-documentation/evals/files/package-overview/README.md new file mode 100644 index 0000000..47be659 --- /dev/null +++ b/skills/go-documentation/evals/files/package-overview/README.md @@ -0,0 +1,59 @@ +# kvstore + +[![Go Reference](https://pkg.go.dev/badge/example.com/go-documentation-evals/kvstore.svg)](https://pkg.go.dev/example.com/go-documentation-evals/kvstore) + +**kvstore** is a tiny, dependency-free, concurrency-safe in-memory key-value +store for Go. It's the store I reach for in tests, small CLIs, and prototypes +where reaching for Redis or SQLite would be overkill. + +## Why kvstore? + +- **Zero dependencies** — just the standard library. +- **Concurrency-safe** — every operation is guarded by a `sync.RWMutex`, so you + can share one `Store` across goroutines without your own locking. +- **Tiny API** — five methods, nothing to learn. + +## Install + +```bash +go get example.com/go-documentation-evals/kvstore +``` + +## Quick start + +```go +package main + +import ( + "fmt" + + "example.com/go-documentation-evals/kvstore" +) + +func main() { + s := kvstore.Open() + s.Set("greeting", []byte("hello")) + + v, err := s.Get("greeting") + if err != nil { + panic(err) + } + fmt.Println(string(v)) // hello +} +``` + +## How it works + +A `Store` wraps a `map[string][]byte` behind a read-write mutex. Reads +(`Get`, `Len`) take the read lock so they can run concurrently; writes +(`Set`, `Delete`) take the write lock. A missing key from `Get` returns the +sentinel `ErrNotFound` rather than an empty slice, so callers can tell "no such +key" apart from "the value is empty". + +The store keeps everything in memory — there is no persistence. When the +process exits, the data is gone. That's intentional: kvstore is for ephemeral +state, not durable storage. + +## License + +MIT diff --git a/skills/go-documentation/evals/files/package-overview/go.mod b/skills/go-documentation/evals/files/package-overview/go.mod new file mode 100644 index 0000000..34699b1 --- /dev/null +++ b/skills/go-documentation/evals/files/package-overview/go.mod @@ -0,0 +1,3 @@ +module example.com/go-documentation-evals/kvstore + +go 1.23 diff --git a/skills/go-documentation/evals/files/package-overview/store.go b/skills/go-documentation/evals/files/package-overview/store.go new file mode 100644 index 0000000..a9a081d --- /dev/null +++ b/skills/go-documentation/evals/files/package-overview/store.go @@ -0,0 +1,57 @@ +// Package kvstore implements a lightweight, embeddable key-value store. +// +// Check out the project README for the full rundown — install +// instructions, a quick-start walkthrough, and a few benchmarks against +// the alternatives. +package kvstore + +import ( + "errors" + "sync" +) + +// ErrNotFound is returned when a key is missing. +var ErrNotFound = errors.New("key not found") + +// A Store is a concurrency-safe in-memory key-value store. +type Store struct { + mu sync.RWMutex + data map[string][]byte +} + +// Open makes a Store. +func Open() *Store { + return &Store{data: make(map[string][]byte)} +} + +// Get looks up a key. +func (s *Store) Get(key string) ([]byte, error) { + s.mu.RLock() + defer s.mu.RUnlock() + v, ok := s.data[key] + if !ok { + return nil, ErrNotFound + } + return v, nil +} + +// Set stores a value. +func (s *Store) Set(key string, value []byte) { + s.mu.Lock() + defer s.mu.Unlock() + s.data[key] = value +} + +// Delete removes a key. +func (s *Store) Delete(key string) { + s.mu.Lock() + defer s.mu.Unlock() + delete(s.data, key) +} + +// Len returns the number of keys. +func (s *Store) Len() int { + s.mu.RLock() + defer s.mu.RUnlock() + return len(s.data) +} diff --git a/skills/go-documentation/evals/files/testable-examples/go.mod b/skills/go-documentation/evals/files/testable-examples/go.mod new file mode 100644 index 0000000..82e7d8c --- /dev/null +++ b/skills/go-documentation/evals/files/testable-examples/go.mod @@ -0,0 +1,3 @@ +module example.com/go-documentation-evals/slug + +go 1.23 diff --git a/skills/go-documentation/evals/files/testable-examples/slug.go b/skills/go-documentation/evals/files/testable-examples/slug.go new file mode 100644 index 0000000..43a02e5 --- /dev/null +++ b/skills/go-documentation/evals/files/testable-examples/slug.go @@ -0,0 +1,61 @@ +// Package slug converts arbitrary text into URL-friendly slugs. +// +// Usage: +// ```go +// s := slug.New() +// fmt.Println(s.Make("Hello, World!")) // hello-world +// ``` +package slug + +import ( + "strings" + "unicode" +) + +// A Slugifier converts strings to slugs using a configurable separator. +type Slugifier struct { + // Separator is placed between words. Defaults to "-" when empty. + Separator string +} + +// New returns a Slugifier that joins words with "-". +func New() *Slugifier { + return &Slugifier{Separator: "-"} +} + +// Make converts s into a slug: it lowercases the text, splits it on any run of +// non-alphanumeric characters, and joins the pieces with the separator. +// +// @example +// New().Make("Hello, World!") // => "hello-world" +// +// @example WithNumbers +// New().Make("foo_bar 123") // => "foo-bar-123" +func (sl *Slugifier) Make(s string) string { + sep := sl.Separator + if sep == "" { + sep = "-" + } + var words []string + var cur strings.Builder + for _, r := range s { + if unicode.IsLetter(r) || unicode.IsDigit(r) { + cur.WriteRune(unicode.ToLower(r)) + } else if cur.Len() > 0 { + words = append(words, cur.String()) + cur.Reset() + } + } + if cur.Len() > 0 { + words = append(words, cur.String()) + } + return strings.Join(words, sep) +} + +// Make is a convenience wrapper that slugifies s with the default separator. +// +// @example +// slug.Make(" Go & TypeScript ") // => "go-typescript" +func Make(s string) string { + return New().Make(s) +} diff --git a/skills/go-documentation/references/comment-syntax.md b/skills/go-documentation/references/comment-syntax.md index b867afd..a1cc96c 100644 --- a/skills/go-documentation/references/comment-syntax.md +++ b/skills/go-documentation/references/comment-syntax.md @@ -15,6 +15,7 @@ The full markup grammar used by Go doc comments, as defined at https://go.dev/do - [Directives](#directives) - [How gofmt reformats comments](#how-gofmt-reformats-comments) - [Common pitfalls](#common-pitfalls) +- [Javadoc-style block comments](#javadoc-style-block-comments) ## Paragraphs @@ -89,6 +90,8 @@ Gofmt automatically moves all link definitions to the end of the comment, in up A URL in prose (e.g., `https://example.com`) is auto-linked in the HTML rendering. There is no Markdown-style `[Text](URL)` form — use the reference style if you need link text. +Symptom: `go doc` or pkg.go.dev shows a literal `[text](url)` in the output. That's leftover Markdown syntax carried over from JSDoc/TSDoc or a README — Go has no such link form, so it's preserved as plain text rather than rendered. Fix by converting to a bare URL or a reference-style link (above). + ## Doc links A doc link is a hyperlink to another Go identifier, written without a URL declaration: @@ -358,6 +361,27 @@ Go 1.19+ gofmt applies heuristics that merge unindented lines into adjacent code If you want a paragraph clearly separated from following non-paragraph content, insert a blank line between them. The blank line is the unambiguous separator and is preserved through every gofmt revision. +## Javadoc-style block comments + +A doc comment can be a `/* ... */` block comment as well as a `//` line comment — Go's attachment rule (immediately before the declaration, no blank line) doesn't care which style you use. This matters because a comment ported from Java/TypeScript often looks like: + +```go +/** + * The maximum number of tokens a bucket can hold by default. + */ +const DefaultCapacity = 100 +``` + +This attaches as the doc comment for `DefaultCapacity` — it is not silently dropped — but it renders badly. `//` line comments have their leading space stripped per line, but a `/* */` block comment's internal lines are used as written, so every ` * ` prefix becomes part of the text, and the leading space makes each line an indented (code-block) line. `go doc -all` on the example above prints: + +``` +const DefaultCapacity = 100 + * + - The maximum number of tokens a bucket can hold by default. +``` + +Worse, `gofmt` recognizes the leading-`*` Javadoc pattern and deliberately leaves it alone rather than guessing at a rewrite — unlike an ordinary indented block comment, which gofmt *will* reformat. So this pattern produces broken rendering that no `gofmt -d` diff will ever flag. Fix by converting to `//` on every line before deciding on wrapping or indentation. + ## Quick verification Before committing comment changes that include any of the markup above: diff --git a/skills/idiomatic-go/SKILL.md b/skills/idiomatic-go/SKILL.md index 3c2513d..652f258 100644 --- a/skills/idiomatic-go/SKILL.md +++ b/skills/idiomatic-go/SKILL.md @@ -1,13 +1,13 @@ --- name: idiomatic-go -description: Apply Go community idioms for API and package design, error and concurrency patterns, naming (initialisms, stutter, receivers), generics vs interfaces, and Go code review — including PR or diff review and questions about "the Go way", pointer vs value receivers, or channels vs mutexes, even when the user doesn't say "idiomatic." Skip trivial one-line Go edits with no design choices. Do not use for godoc or doc comments — use the `go-documentation` skill. +description: Apply Go community idioms for API and package design, error and concurrency patterns, naming (initialisms, stutter, receivers), generics vs interfaces, and Go code review — including PR or diff review, questions about "the Go way", pointer vs value receivers, channels vs mutexes, and building Go language tooling (parsers, language servers/LSP, type checkers, validators, DSLs, or toolkits built on a Go language framework) — even when the user doesn't say "idiomatic." Skip trivial one-line Go edits with no design choices. Do not use for godoc or doc comments — use the `go-documentation` skill. --- # Idiomatic Go The conventions here come from [Effective Go](https://go.dev/doc/effective_go) and [Go Code Review Comments](https://go.dev/wiki/CodeReviewComments) — the documents that codify what Go reviewers reject pull requests over. The content is filtered to the rules that strong models still slip on, so basic syntax, `gofmt` usage, and well-known mechanics (LIFO `defer`, `new` vs `make`, channel basics) are not repeated here. -The guiding principle is **clear is better than clever** (Rob Pike's phrasing, elaborated in [Dave Cheney's article of the same name](https://dave.cheney.net/2019/07/09/clear-is-better-than-clever)). When choosing between a concise/clever construction and an explicit/plain one, pick the explicit one. Code is decoded, not skimmed, and it outlives the person who wrote it. For godoc and doc-comment conventions, use the sibling `go-documentation` skill — this one stays focused on code style. +The guiding principle is **clear is better than clever** (Rob Pike's phrasing, elaborated in [Dave Cheney's article of the same name](https://dave.cheney.net/2019/07/09/clear-is-better-than-clever)). When choosing between a concise/clever construction and an explicit/plain one, pick the explicit one. Code is decoded, not skimmed, and it outlives the person who wrote it. APIs that require **Go 1.22 or later** are called out inline below. Older stable symbols (`errors.Is`, `fmt.Errorf` with `%w`, `slices.Clone`, and similar) are not annotated. @@ -17,7 +17,8 @@ Load these as needed — don't read them upfront. This file has the rules and de - `references/naming.md` — the full initialism table with the awkward cases (`OAuth`, `IPv4`, `gRPC`, `IDs`, `HTTPSProxy`), worked naming examples, deeper package-naming guidance (why `util`/`common`/`misc` collect garbage and how to split them), import grouping, and file naming with build constraints. Read this when naming anything exported or organizing a new package. - `references/interfaces-and-errors.md` — interface placement worked end-to-end, embedding, compile-time satisfaction checks; sentinel errors vs custom error types vs `fmt.Errorf`; the `errors.Is`/`As`/`Join` model and when *not* to wrap; structured errors; `panic`/`recover` discipline. Read this when designing an API, writing a non-trivial error type, or wrapping errors across abstraction boundaries. -- `references/concurrency-patterns.md` — the `context.Context` contract end-to-end, the four ways a goroutine exits, the `for select` skeleton, channel-direction signatures, semaphore and leaky-buffer patterns, `sync.Once`/`WaitGroup`/`errgroup` decisions, channel-closing rules, and the anti-patterns (sleeping in tests, polling, fire-and-forget goroutines). Read this when starting a goroutine, designing a cancellable operation, or reviewing concurrent code. +- `references/concurrency-patterns.md` — the `context.Context` contract end-to-end, the four ways a goroutine exits, the `for select` skeleton, channel-direction signatures, semaphore and leaky-buffer patterns, `sync.Once`/`WaitGroup`/`errgroup` decisions, channel-closing rules, idempotent shutdown against concurrent registration (a coordinator's shutdown safe to call twice and race-free against concurrent subscriber registration), and the anti-patterns (sleeping in tests, polling, fire-and-forget goroutines). Read this when starting a goroutine, designing a cancellable operation, or reviewing concurrent code. +- `references/language-engineering.md` — patterns specific to Go tools for *programming languages* (parsers, language servers/LSP, type checkers, validators, DSLs) that go beyond baseline idiomatic Go: nil-safe AST navigation, callback-vs-iterator traversal, optional capability interfaces, non-nil empty sentinels, typed source coordinates and UTF-16 columns, editor-overlay text handles, lazy scope chains, typed/untyped references and once-only resolution with cycle detection, phased incremental builds and the write-to-read lock downgrade, keeping the domain free of the LSP protocol, and completion as a parser capability. Read this when working on a language implementation, a DSL, or a toolkit built on a Go language framework — not for ordinary application code. ## Naming @@ -91,6 +92,8 @@ return fmt.Errorf("Failed to open %s: %v.", path, err) **Wrap with `%w` once** so `errors.Is` and `errors.As` work. Use `%v` only when you intentionally want to flatten — i.e., the inner error is an implementation detail not part of your contract. +**Wrapping is not branching.** `%w` only helps a caller who actually calls `errors.Is`/`errors.As` — it does nothing by itself. If *your own* function needs to treat one failure differently from the rest (not-found vs. everything else), check it with `errors.Is`/`errors.As` before you return; don't assume the branch happens further up the stack. + **Don't `_ = err`.** If you genuinely cannot act on an error, leave a comment explaining why. Silent discard is the default failure mode for `os.Setenv`, `f.Close()` on a read-only file, and similar cases. **Indent the error branch, not the happy path.** Early return on failure; let success flow down the page. @@ -113,7 +116,34 @@ func (c *Client) Fetch(ctx context.Context, id string) (*Doc, error) type Client struct { ctx context.Context; /* ... */ } ``` -**Prefer synchronous APIs.** A function that returns `(Result, error)` is composable, testable, and trivially wrappable in a goroutine by anyone who wants concurrency. A function that returns `<-chan Result` decides for the caller, leaks if the caller forgets to drain it, and complicates error reporting. Write the synchronous version first; let the caller add the goroutine. +**Prefer synchronous APIs.** A function that returns `(Result, error)` is composable, testable, and trivially wrappable in a goroutine by anyone who wants concurrency. A function that returns `<-chan Result` decides for the caller, leaks if the caller forgets to drain it, and complicates error reporting. Write the synchronous version first; let the caller add the goroutine — even a "streaming" API should be a thin wrapper over that synchronous core, not the primary shape. + +```go +// Good: synchronous core; concurrency is the caller's choice, not baked into the API. +func (c *Client) FetchOne(ctx context.Context) (Item, error) + +// A streaming form wraps the core — it doesn't replace it. +func (c *Client) FetchAll(ctx context.Context) <-chan Item { + out := make(chan Item) + go func() { + defer close(out) + for { + item, err := c.FetchOne(ctx) + if err != nil { + return + } + select { + case out <- item: + case <-ctx.Done(): + return + } + } + }() + return out +} +``` + +**Coming from TypeScript:** `async`/`await` makes every I/O call asynchronous by default, and the type system reflects it — `async function getUser(id): Promise` is simply how you write "fetch a user"; the language chose the shape, not the author. Porting that method to Go as `func (s *Service) GetUser(id string) chan User` reproduces the `Promise` shape in Go syntax, but Go never forced that choice. `func (s *Service) GetUser(ctx context.Context, id string) (User, error)` is the default; a caller who actually wants concurrency writes `go` in front of the call. **Unbuffered channels are synchronization; buffered channels are throughput tolerance.** `make(chan T)` blocks the sender until the receiver receives — that *is* the synchronization. `make(chan T, N)` lets the sender get ahead by N values; it doesn't "make things faster", it changes semantics. Pick the capacity by what the data flow requires, not by tuning. @@ -137,6 +167,34 @@ Use **value receivers** when: When in doubt, use a pointer receiver. The cost is one indirection; the cost of inconsistency is bugs. +## Value semantics: Go copies, TypeScript never does + +**Coming from TypeScript:** every object, array, and class instance is a reference — assignment, passing to a function, and storing something in a collection all point at the same underlying data, so there's no copy to reason about. **Go structs are value types.** Assignment, passing by value, and `for _, v := range` all copy. Code written on reference-semantics instincts keeps compiling in Go — it just quietly stops mutating anything. + +```go +// Looks right if "objects are always shared" — silently wrong in Go. +func deactivateStale(users []User, cutoff time.Time) { + for _, u := range users { // u is a copy of each element + if u.LastSeen.Before(cutoff) { + u.Active = false // mutates the copy; users is untouched + } + } +} + +// Correct: index into the slice so the assignment reaches the real element. +func deactivateStale(users []User, cutoff time.Time) { + for i := range users { + if users[i].LastSeen.Before(cutoff) { + users[i].Active = false + } + } +} +``` + +The same gap hides in a value-receiver method (`func (u User) Deactivate()` only ever mutates its own copy, never the caller's) and in a Redux-style "return the changed copy" update — natural coming from immutable-update habits, but in Go nothing changes until the caller replaces its own variable with the result; there's no in-place effect the way mutating a shared object would have. When a type needs a mutation to be visible to whoever holds the original, hold it by pointer: a pointer receiver, `[]*T` instead of `[]T`, or an index-based assignment like the fix above. + +**Fixing mutation-visibility does not fix slice-exposure, and vice versa — when one field backs both a mutator and an accessor, check both independently.** Switching `[]User` to `[]*User` (or indexing into the original) closes the mutation gap above, but any accessor that still returns a subslice of that same field (`return s.active[start:end]`) is exactly as unsafe as before — re-slicing `[]*User` still hands the caller a live view of the backing array, so `append`ing to the returned page can still corrupt the service's own next append, and every pointer in it is still the service's own `*User`. The two fixes live in different sections of this document (this one; "Data gotchas" below) because they are genuinely independent problems that happen to collide on the same field whenever a type both mutates and lists its own cache — applying one is not a side effect of applying the other, so re-check every reader of a field right after you change how it's mutated (and vice versa). + ## Pass values; avoid pointer-itis Don't reflexively reach for `*int`, `*string`, `*bool` to "save a copy" or to model optionality. Pointer-to-primitive forces nil-checks at every call site, hides intent, and costs more than the value would have. @@ -155,9 +213,9 @@ For optional configuration, use a zero-value-friendly struct, a functional `Opti **Slices share backing arrays — until they don't.** A slice is a header over an array; two slices into the same array see each other's writes. Then `append` exceeds capacity and reallocates, and the two slices silently diverge. When passing slices across an API boundary, document whether you retain or copy, and `slices.Clone` when in doubt. -**Composite literals: use field-name form.** `&File{fd: fd, name: name}` is robust to field reordering; `&File{fd, name, nil, 0}` breaks the day someone adds a field. +**Coming from TypeScript:** a getter that returns `this.items` is unremarkable — most callers never mutate through it, and JS arrays don't have a backing-array/capacity split to worry about. Go's equivalent, `func (s *Store) Items() []Item { return s.items }`, hands back the exact backing array: `items[0].Field = x` mutates the store's private state through it, and a caller's `append` may or may not corrupt the store's next append depending on spare capacity. Return `slices.Clone(s.items)` (or an equivalent copy) whenever a field must not leak a live handle to internal state. -**Nil maps panic on write.** See zero-value design above. +**Composite literals: use field-name form.** `&File{fd: fd, name: name}` is robust to field reordering; `&File{fd, name, nil, 0}` breaks the day someone adds a field. ## Control flow and structure @@ -196,22 +254,6 @@ default: For tokens, IDs, keys, session identifiers, or anything an attacker could exploit by predicting: use `crypto/rand` (`Read`, `Int(rand.Reader, max *big.Int)`, and from Go 1.24 `Text`). `math/rand` and `math/rand/v2` (Go 1.22) are for simulation, jitter, sampling, and tests — not security. The default autocomplete is `math/rand`; flag it on every use that produces a value an attacker would care about. -## Quick checklist before committing - -- [ ] Initialisms are single-cased: `URL`, `ID`, `HTTP`, `JSON` — `userID`, `ServeHTTP`, `parseJSON`. -- [ ] Package names are short, lowercase, single words; no `util`/`common`/`misc`. -- [ ] Receiver names are 1–2 letters, consistent across the type — never `self`/`this`/`me`. -- [ ] Interfaces are defined in the consumer; constructors return concrete types. -- [ ] Generics only where the same logic would otherwise be duplicated; interfaces kept for method-only APIs. -- [ ] Named slice types preserved (`S ~[]E`); `cmp`/`slices`/`maps` preferred over `x/exp/constraints`. -- [ ] Go 1.27+: generic methods not used where the type must implement a non-generic interface method. -- [ ] No `Get` prefix on getters. -- [ ] Error strings are lowercase, end with no punctuation, and wrap with `%w` when chained. -- [ ] No `_ = err` without an explaining comment. -- [ ] Every goroutine has a documented exit (finite work, `ctx.Done()`, close signal, or `WaitGroup`). -- [ ] `context.Context` is the first parameter where it appears; never stored in a struct. -- [ ] Receiver types are consistent within a type (all value or all pointer). -- [ ] No `*int`/`*string`/`*bool` parameters unless `nil` is genuinely distinct from the zero value. -- [ ] `crypto/rand` for anything security-sensitive; `math/rand` only for simulation/jitter. -- [ ] Happy path stays at the leftmost indent; error branches are early returns. -- [ ] `gofmt`/`goimports` clean. +## Before you're done + +Whether you wrote the code or are reviewing a diff, the section headings above are the agenda: walk every one, and flag each violation with the idiom it breaks and the concrete fix. Coverage — not the first few slips — is the bar. diff --git a/skills/idiomatic-go/evals/evals.json b/skills/idiomatic-go/evals/evals.json new file mode 100644 index 0000000..cea9094 --- /dev/null +++ b/skills/idiomatic-go/evals/evals.json @@ -0,0 +1,105 @@ +{ + "skill_name": "idiomatic-go", + "assertions_intent": "Each eval targets idioms the idiomatic-go skill calls out as ones strong models still slip on, chosen so a no-skill baseline is likely to produce working-but-non-idiomatic code. Two design rules keep the delta honest: (1) prompts state the user's goal and pain in behavioral terms and never name the idiom or the fix (no 'nil-safe', 'sentinel', 'errors.Is', 'context-keyed', 'synchronous core', 'consumer-side interface', 'crypto/rand', 'value semantics', 'aliasing', 'fan-in', 'WaitGroup'), so a passing run had to supply the pattern itself rather than echo the prompt; (2) fixtures compile and vet cleanly but deliberately deviate from the skill's idioms, and their comments describe the domain only — they never flag or hint the anti-pattern. Fixtures under evals/files/ are self-contained Go modules; the flawed ones compile but are non-idiomatic, and the two language-engineering fixtures provide only data structures plus builders so the agent designs the API. Assertions are tagged, based on the most recent measurement: [discriminating] when with_skill passed and the baseline failed; [regression] when both configs passed (the property is locked in and must not break; several regression checks would still separate on a weaker model); [regressed] when the baseline passed and with_skill failed — the skill's guidance produced a worse outcome than no guidance at all; [gap] when both configs failed but the check itself is sound — an unmet target for a future skill revision, not a broken assertion; [new, untested] when the assertion targets a fixture scenario with no measured result yet; and [sharpened, untested] when a prior assertion tied completely and its fixture was reworked to hide the same underlying bug behind a harder, more plausible-looking shape rather than being retired — also pending measurement.", + "evals": [ + { + "id": 0, + "name": "concurrency-poller-review", + "prompt": "Here is a small Go package, evals/files/concurrency-poller/ (poller.go). It backs a monitoring daemon that watches many targets at once. For each target the daemon needs the current reading whenever it asks, and it wants one place to read every target's incoming readings rather than juggling a channel per target. When it stops caring about a target it needs that target's background work to fully go away without disturbing the targets it's still watching — and starting and stopping individual targets while others keep running is routine, not a rare edge case. Please review this package and rewrite it to the standard you'd expect in a production Go service, then write up what you changed and why as a short REVIEW.md alongside the code so I have something to refer back to later.", + "expected_output": "A review that names the concurrency idioms the original breaks and a rewrite that fixes them, at both the single-target Poller level and the multi-target Manager level. Poller: the background goroutine gains a real exit path (it selects on <-ctx.Done() or a close/stop signal instead of looping forever), context is passed as a parameter (or the object stops via an explicit Stop()) rather than stored in the struct, and the periodic loop uses a time.Ticker with select rather than time.Sleep. Manager: it merges every watched target's samples onto one stream, and — because the original design has each per-target forwarding goroutine independently close that shared merged channel once its own source drains — the rewrite must coordinate the close so it happens exactly once across all current and future targets (e.g. a WaitGroup counted per forwarder with a single closer goroutine, or an equivalent scheme), not once per forwarder. Forgetting one target actually stops that target's background polling (by cancelling a per-target context or calling a per-target stop) rather than just deleting bookkeeping and leaking the goroutine, and does not disturb any other target. Any channel that remains is closed by its sender exactly once, or the channel is dropped in favor of a Last()/read accessor. The explanation ties each change to the idiom it restores — in particular it flags that the original stored context.Context in the struct, ran goroutines with no exit, and would double-close (or panic on) the shared merged channel as soon as more than one target was in play.", + "files": [ + "evals/files/concurrency-poller/poller.go", + "evals/files/concurrency-poller/go.mod" + ], + "assertions": [ + "context-not-stored [gap]: neither Poller nor Manager keeps a context.Context in a struct field; context is passed as a parameter (conventionally the first, named ctx) to the methods that need it, or the objects stop via an explicit Stop()/done channel instead.", + "review-flags-original-issues [gap]: a persisted write-up (not just the final chat reply) explicitly identifies the original problems (context stored in the struct, the never-terminating/leaking goroutines, time.Sleep polling, and the Manager's per-forwarder close of the shared merged channel) and names the idiom each violates, rather than silently rewriting.", + "fan-in-single-close [regressed]: the Manager's merged stream is closed exactly once by a single coordinating owner — e.g. a WaitGroup counting each per-target forwarder with a dedicated goroutine that closes only after every forwarder has finished, or an equivalent scheme — never by each per-target forwarder independently calling close on the shared channel; the design does not double-close (or panic) as targets are added and forgotten over the Manager's lifetime. A rewrite that keeps `close(m.merged)` inside the per-forwarder goroutine (the original shape) should fail this even if everything else is fixed.", + "forget-stops-target [regression]: calling the equivalent of Forget for one target actually terminates that target's background polling goroutine (e.g. by cancelling a per-target context or invoking a per-target Stop) rather than merely deleting it from a map and leaving the goroutine running; forgetting one target does not stop or otherwise disrupt any other target still being watched.", + "goroutine-has-exit [regression]: every goroutine (Poller's poll loop and Manager's per-target forwarders) has an explicit termination path — it returns on <-ctx.Done(), ranges a channel an owner closes, or is joined via WaitGroup — rather than an unconditional for{} that can never stop and leaks.", + "ticker-not-sleep [regression]: the periodic wait is driven by time.NewTicker (with defer ticker.Stop()) inside a for/select, not by time.Sleep in a bare loop.", + "sender-closes-channel [regressed]: every channel that remains (Poller.out and, if kept, Manager.merged) is closed by the sending side exactly once — never by a receiver, never by more than one sender without coordination, and safely even if the close operation is invoked more than once (e.g. via sync.Once) — or the design drops the channel entirely.", + "compiles-clean [regression]: the rewritten package still builds and vets cleanly (go build ./... and go vet ./... succeed) despite the added Manager/fan-in complexity." + ] + }, + { + "id": 1, + "name": "interfaces-errors-userstore", + "prompt": "In evals/files/interfaces-errors/ the orders package places an order only for buyers that already exist in userstore. Two things bug me. First, orders.Service is welded to the concrete userstore.Store, so I can't exercise PlaceOrder on its own. Second, when a buyer isn't a known user I want my code to be able to treat that outcome differently from an actual failure (a broken store, a bad lookup) — right now the two are hard to tell apart. Clean this up the way you'd do it in a real Go codebase, and add whatever test you'd want for the buyer-doesn't-exist case. Show me the updated files.", + "expected_output": "A refactor where the user lookup returns an explicit error (a sentinel such as ErrNotFound) instead of signaling absence with a zero-value User, and orders.PlaceOrder distinguishes not-found from other failures by branching with errors.Is before returning. The interface that orders depends on is declared in orders (the consumer) and is minimal; the pre-existing speculative userstore.Fetcher interface is removed or ignored rather than reused as the dependency, and userstore.NewStore still returns the concrete *Store. Error strings are lowercase with no trailing punctuation and wrap the cause with %w where it is part of the contract — importantly the original capitalized, %v-formatted fmt.Errorf strings in userstore.Load are corrected even though the prompt never mentions Load. A test in the orders package uses a fake implementation of the consumer interface to drive the not-found path.", + "files": [ + "evals/files/interfaces-errors/userstore/userstore.go", + "evals/files/interfaces-errors/orders/orders.go", + "evals/files/interfaces-errors/go.mod" + ], + "assertions": [ + "consumer-side-interface [discriminating]: the interface abstracting the user lookup is declared in the orders (consumer) package and is minimal; the speculative userstore.Fetcher is not reused as the dependency, and userstore does not keep/gain an exported lookup interface.", + "error-string-style [discriminating]: error strings introduced or kept are lowercase and carry no trailing punctuation, including the original \"Failed to ... .\" strings in userstore.Load — which the prompt never mentions.", + "wrap-with-%w [discriminating]: wrapped errors use %w (not %v, and not string concatenation) where the wrapped error is part of the contract, including userstore.Load.", + "not-found-as-error [regression]: the user lookup signals \"not found\" through the type system — a returned error (e.g. a sentinel) or a (User, bool) — instead of an in-band zero/empty User the caller must guess at.", + "service-branches-on-sentinel [regression]: orders.PlaceOrder itself distinguishes not-found from other failures via errors.Is/errors.As (not by inspecting u.Email == \"\" or matching an error message string), returning a distinguishable result for the two cases.", + "concrete-constructor-return [regression]: userstore.NewStore returns the concrete *Store, not an interface, so callers keep access to every method.", + "testable-with-fake [regression]: an orders test constructs the Service with a fake/stub implementation of the consumer interface (not the real *Store) and drives the not-found path through it." + ] + }, + { + "id": 2, + "name": "langeng-ast-navigation", + "prompt": "evals/files/ast-navigation/ (package doctree) is a document AST — a Document owning a tree of Nodes (document > section > paragraph); New and Assign already wire the parent and document back-pointers. It has the builders but none of the API the rest of the toolkit needs to read the tree. Please add that API: getting from a node to its parent, to its owning Document, and to its children; visiting everything beneath a node so callers can collect or search for nodes (a search normally wants to stop the moment it finds what it's after); and finding the closest enclosing node of a particular kind. Callers reach across the tree constantly — \"the Document that owns the section enclosing this paragraph\" is a typical hop — and this runs over large documents. Show me the code.", + "expected_output": "Navigation accessors that are nil-receiver-safe: called on a nil *Node they return the zero value (nil) so a chain like n.Container().Container().Doc() degrades to nil instead of panicking. Subtree traversal built on a callback primitive taking func(*Node) bool (return false to stop), with an ergonomic iter.Seq[*Node] wrapper layered on top for range loops; the stop signal is threaded back up the recursion so an early break/false actually halts the whole walk (including sibling subtrees) rather than being ignored. A nearest-enclosing-Kind helper walks the container chain and returns the first match (nil when none). The prompt deliberately avoids the words 'nil-safe', 'callback', 'iterator', and 'early stop'.", + "files": [ + "evals/files/ast-navigation/ast.go", + "evals/files/ast-navigation/go.mod" + ], + "assertions": [ + "nil-safe-accessors [discriminating]: navigation accessors guard a nil receiver and return the zero value instead of panicking, so chained hops on a nil *Node do not crash; the nil-safety is applied consistently across the accessors, not just one.", + "iterator-veneer [gap]: an ergonomic iter.Seq[*Node] wrapper is provided over the callback primitive so callers can range over descendants.", + "short-circuit-propagated [regression]: early termination is honored — the visit/yield bool return is threaded back up the recursion so a consumer break (or false return) stops the entire walk, including sibling subtrees, not just the current branch.", + "callback-traversal-primitive [regression]: subtree traversal is built on a callback primitive of the form func(visit func(*Node) bool) bool (return false to stop), rather than solely on a coroutine-style iterator, to avoid the per-step handoff cost on deep walks.", + "nearest-ancestor-helper [regression]: a helper returns the nearest enclosing node of a requested Kind by walking the container chain, returning the zero value (nil) when there is no match." + ] + }, + { + "id": 3, + "name": "langeng-reference-resolution", + "prompt": "evals/files/reference-resolution/ (package aliases) is a tiny type-alias language: \"type A = B\" makes A an alias of B, chains like A=B, B=C are allowed, and nothing stops someone from writing A=B, B=A. The Table, Symbol, and Ref types and their builders exist, but following a Ref to the Symbol it names isn't implemented — that's what I need. This model is shared across a language server's concurrent requests and the same references get followed again and again as the user edits, so following one has to hold up under that. Implement the resolution and show me the code.", + "expected_output": "A Resolve method that takes context.Context as its first parameter (not stored on a struct) and resolves a Ref to its target Symbol, returning an error on failure. Resolution is memoized so repeated calls do not recompute, and it is safe under concurrent callers (an atomic fast path plus double-checked locking, or an equivalent race-free scheme). Cycles (A=B, B=A) are detected and produce a clear, lowercase, punctuation-free error rather than infinite recursion, using per-resolution state (a visited set threaded through the recursion, or a context value keyed by the ref) rather than a mutable in-progress flag on the shared Ref/Table. The prompt avoids the words 'lazy', 'cached', 'memoized', 'cycle-safe', 'atomic', and 'context'.", + "files": [ + "evals/files/reference-resolution/types.go", + "evals/files/reference-resolution/go.mod" + ], + "assertions": [ + "context-first-param-not-stored [discriminating]: context.Context is the first parameter of Resolve and is not stored in the Ref/Symbol/Table struct.", + "lazy-once-memoized [gap]: a Ref resolves at most once even under concurrent callers — the result (target and/or error) is computed exactly once and returned from a cache on every subsequent call, with no redundant concurrent recomputation (e.g. an atomic fast path guarding a mutex-protected slow path with a double-check, sync.Once, or an equivalent scheme) and no data race under go test -race on a concurrent resolve.", + "cycle-detection-per-resolution [regression]: cycle detection uses per-call-stack state (a visited set threaded through the recursion, or a context value keyed by the ref) rather than an in-progress flag stored on the shared Ref/Table that would race or false-positive when independent chains resolve concurrently.", + "cycle-returns-error [regression]: a mutually recursive chain (A = B, B = A) resolves to a clear cyclic-reference error instead of infinite recursion or a stack overflow.", + "resolution-error-style [regression]: failures (unresolved name, cycle) are reported as returned errors with lowercase, non-punctuated messages." + ] + }, + { + "id": 4, + "name": "typescript-port-accounts", + "prompt": "I'm coming from TypeScript and evals/files/typescript-port/ (accounts.go) is the first Go service I've written mostly on my own. It compiles and the parts I've poked at seem to work, but I have a nagging feeling I'm just writing TypeScript in Go's syntax instead of real Go, and I'd rather learn the actual conventions now than keep building on habits I'll have to unlearn. Go through it and rewrite it the way an experienced Go developer would, and write up a REFACTORING.md alongside the code that walks me through what was off and why the Go way is different — I'd like to keep it for reference. Don't hold back — and don't assume something is fine just because it compiles.", + "expected_output": "A rewrite that removes the TypeScript accent across the board, plus an explanation that names each idiom and why Go differs. Expected corrections: initialisms single-cased (Id->ID, ApiToken->APIToken, AvatarUrl->AvatarURL, FindById->FindByID, ParseUserJson->ParseUserJSON); getters drop the Get prefix (GetEmail->Email) while setters may keep Set; receiver names become short and consistent (this/self -> u/s), never this/self; SCREAMING_SNAKE consts become MixedCaps (StatusActive, defaultPageSize); the Hungarian I-prefixed producer interfaces are removed/renamed and NewUserService returns a concrete type rather than an interface; pointer-to-primitive optional fields are reconsidered; error strings are lowercased and de-punctuated and wrap with %w instead of errors.New with string concatenation, and the dropped error in GetUserAsync is handled; GenerateApiToken uses crypto/rand rather than math/rand; the single-result channel method (GetUserAsync) is dropped or reduced to a thin wrapper over a synchronous lookup rather than kept as a primary channel-returning API. Two harder, silent gotchas: the fixture's DeactivateStale doesn't mutate a range-loop copy directly — it passes that copy to a deactivateUser(u User) helper (an ordinary function, not a method, so there's no pointer/value receiver inconsistency to spot by scanning signatures), which reads as a clean extracted-helper refactor but is exactly as inert as inline field writes would have been; the fix must give the mutation a real path back to the stored slice (index-based assignment, a pointer-receiver method invoked through an index or pointer element, or passing a pointer into the helper), not just rename the helper or tidy its parameter name. And the fixture's ActiveUsers doesn't hand back the whole internal slice directly — it re-slices into a page (`self.active[start:end]`), which looks like ordinary pagination code, but the returned page still shares the service's backing array; the fix must copy the page out (e.g. slices.Clone on the sliced range) rather than return the re-slice itself.", + "files": [ + "evals/files/typescript-port/accounts.go", + "evals/files/typescript-port/go.mod" + ], + "assertions": [ + "async-not-primary [regression]: the single-result channel-returning method (GetUserAsync) is removed, or reduced to a thin wrapper over a synchronous lookup — it is not kept as a primary API that hands the caller a channel for one value.", + "value-semantics-mutation-visible [regression]: the batch-update method that flips user state (originally DeactivateStale, which called a deactivateUser(u User) helper on each range-loop copy) mutates the stored users through a path that actually reaches the original slice — an index-based assignment (users[i].Field = ...), a pointer element (a []*User cache), or a helper/method taking a *User (or invoked on an addressable indexed element) — not a `for _, u := range` copy handed to a helper that takes User by value, whose writes never reach the original slice regardless of how cleanly the helper is named or factored. A rewrite that keeps deactivateUser taking a plain User parameter and merely renames it, or calls it from the same range-copy shape, should fail this even though the code still compiles and reads as a normal extracted helper.", + "no-internal-slice-exposure [regressed]: the method that lists the in-memory users (originally ActiveUsers, now paginated) returns a defensive copy of the page it computes (e.g. slices.Clone(self.active[start:end]), a freshly built slice, or an equivalent safeguard) rather than the re-sliced view of the live internal slice, so a caller cannot mutate or invalidate the service's private state by writing through the returned page or by appending to it. A rewrite that renames the method or tidies the bounds math but still returns `self.active[start:end]` (or an equivalent direct re-slice) should fail this.", + "initialisms-single-cased [regression]: identifiers use single-cased initialisms — ID (not Id), APIToken (not ApiToken), AvatarURL (not AvatarUrl), FindByID (not FindById), ParseUserJSON (not ParseUserJson); applied across the file.", + "no-getter-prefix [regression]: getter methods drop the Get prefix (Email() not GetEmail(), or the field is simply exported); a setter may keep Set to mark the asymmetry.", + "receiver-names-idiomatic [regression]: method receivers use a short, consistent 1-2 letter name reflecting the type, never this or self.", + "crypto-rand-for-token [regression]: the API-token generator uses crypto/rand (not math/rand), because a predictable token is a security hazard.", + "no-producer-side-interface [discriminating]: the I-prefixed producer interfaces are removed/renamed and any surviving abstraction is declared where it is consumed; NewUserService returns a concrete type rather than an interface.", + "no-pointer-itis [regression]: optional scalar fields are not reflexively modeled as *int/*bool/*string — they become value+zero, (value, ok), or keep a pointer only with a stated reason.", + "error-values-idiomatic [regression]: error strings are lowercase with no trailing punctuation and use fmt.Errorf with %w (or a sentinel) instead of errors.New with string concatenation; the dropped error in GetUserAsync is no longer discarded silently.", + "mixedcaps-consts [regression]: package constants use MixedCaps (StatusActive, defaultPageSize), not SCREAMING_SNAKE_CASE.", + "explanation-covers-categories [regressed]: a persisted write-up file (not just the final chat reply) explains the changes by idiom and covers most of the distinct problem categories (naming/initialisms, getters, receivers, interfaces/return-concrete, errors, pointer-itis, crypto/rand, synchronous-vs-channel, value-semantics/copy-mutation via a pass-by-value helper, slice-aliasing via a re-slice) rather than only the obvious renames.", + "compiles-clean [regression]: the rewritten package still builds and vets cleanly (go build ./... and go vet ./... succeed)." + ] + } + ] +} diff --git a/skills/idiomatic-go/evals/files/ast-navigation/ast.go b/skills/idiomatic-go/evals/files/ast-navigation/ast.go new file mode 100644 index 0000000..484cd89 --- /dev/null +++ b/skills/idiomatic-go/evals/files/ast-navigation/ast.go @@ -0,0 +1,59 @@ +// Package doctree is a miniature AST for a document markup language. +// +// A Document owns a tree of Nodes: the root is a "document", which contains +// "section" nodes, which contain "paragraph" (and nested "section") nodes. +// Every Node except the root has a container (its parent), and every Node knows +// the Document it belongs to. New and Assign build the tree and wire up the +// container and document back-pointers. +package doctree + +// Kind classifies a Node. +type Kind string + +const ( + KindDocument Kind = "document" + KindSection Kind = "section" + KindParagraph Kind = "paragraph" +) + +// Node is a single element in the document tree. +type Node struct { + Kind Kind + Text string + + container *Node + children []*Node + doc *Document +} + +// Document is a parsed document and the root of its Node tree. +type Document struct { + URI string + Root *Node +} + +// New assembles a Node of the given kind from already-built children, linking +// each child's container back to the new node. It does not set the Document; +// call Assign on the finished root for that. +func New(kind Kind, text string, children ...*Node) *Node { + n := &Node{Kind: kind, Text: text, children: children} + for _, c := range children { + c.container = n + } + return n +} + +// Assign builds a Document rooted at root and stamps every node in the subtree +// with a back-pointer to that Document. +func Assign(uri string, root *Node) *Document { + doc := &Document{URI: uri, Root: root} + var stamp func(*Node) + stamp = func(n *Node) { + n.doc = doc + for _, c := range n.children { + stamp(c) + } + } + stamp(root) + return doc +} diff --git a/skills/idiomatic-go/evals/files/ast-navigation/go.mod b/skills/idiomatic-go/evals/files/ast-navigation/go.mod new file mode 100644 index 0000000..0b86744 --- /dev/null +++ b/skills/idiomatic-go/evals/files/ast-navigation/go.mod @@ -0,0 +1,3 @@ +module example.com/idiomatic-go-evals/ast-navigation + +go 1.23 diff --git a/skills/idiomatic-go/evals/files/concurrency-poller/go.mod b/skills/idiomatic-go/evals/files/concurrency-poller/go.mod new file mode 100644 index 0000000..3ea032e --- /dev/null +++ b/skills/idiomatic-go/evals/files/concurrency-poller/go.mod @@ -0,0 +1,3 @@ +module example.com/idiomatic-go-evals/concurrency-poller + +go 1.23 diff --git a/skills/idiomatic-go/evals/files/concurrency-poller/poller.go b/skills/idiomatic-go/evals/files/concurrency-poller/poller.go new file mode 100644 index 0000000..33ed89f --- /dev/null +++ b/skills/idiomatic-go/evals/files/concurrency-poller/poller.go @@ -0,0 +1,108 @@ +// Package poller samples a probe function on an interval and can watch many +// targets at once through a Manager. +package poller + +import ( + "context" + "sync" + "time" +) + +// Sample is one probe result. +type Sample struct { + Target string + Value float64 + Err error +} + +// Poller calls a probe function repeatedly for a single target. +type Poller struct { + ctx context.Context + interval time.Duration + probe func() (float64, error) + out chan Sample + mu sync.Mutex + last Sample +} + +// NewPoller creates a Poller for target and starts it. +func NewPoller(ctx context.Context, target string, interval time.Duration, probe func() (float64, error)) *Poller { + p := &Poller{ + ctx: ctx, + interval: interval, + probe: probe, + out: make(chan Sample, 1), + } + go p.loop(target) + return p +} + +func (p *Poller) loop(target string) { + for { + time.Sleep(p.interval) + v, err := p.probe() + s := Sample{Target: target, Value: v, Err: err} + p.mu.Lock() + p.last = s + p.mu.Unlock() + p.out <- s + } +} + +// Results returns the channel that samples for this target are delivered on. +func (p *Poller) Results() <-chan Sample { + return p.out +} + +// Last returns the most recently observed sample for this target. +func (p *Poller) Last() Sample { + p.mu.Lock() + defer p.mu.Unlock() + return p.last +} + +// Manager watches many targets at once and folds every target's samples +// onto one merged stream, since callers generally want a single place to +// read incoming readings rather than juggling one channel per target. +type Manager struct { + mu sync.Mutex + pollers map[string]*Poller + merged chan Sample +} + +// NewManager creates an empty Manager. +func NewManager() *Manager { + return &Manager{ + pollers: make(map[string]*Poller), + merged: make(chan Sample), + } +} + +// Watch starts polling target and folds its samples into the Manager's +// merged stream. +func (m *Manager) Watch(ctx context.Context, target string, interval time.Duration, probe func() (float64, error)) { + p := NewPoller(ctx, target, interval, probe) + + m.mu.Lock() + m.pollers[target] = p + m.mu.Unlock() + + go func() { + for s := range p.Results() { + m.merged <- s + } + close(m.merged) + }() +} + +// Merged returns the channel carrying samples from every watched target. +func (m *Manager) Merged() <-chan Sample { + return m.merged +} + +// Forget stops watching target; the other targets keep running. +func (m *Manager) Forget(target string) { + m.mu.Lock() + delete(m.pollers, target) + m.mu.Unlock() +} diff --git a/skills/idiomatic-go/evals/files/interfaces-errors/go.mod b/skills/idiomatic-go/evals/files/interfaces-errors/go.mod new file mode 100644 index 0000000..b4e2c58 --- /dev/null +++ b/skills/idiomatic-go/evals/files/interfaces-errors/go.mod @@ -0,0 +1,3 @@ +module example.com/idiomatic-go-evals/interfaces-errors + +go 1.23 diff --git a/skills/idiomatic-go/evals/files/interfaces-errors/orders/orders.go b/skills/idiomatic-go/evals/files/interfaces-errors/orders/orders.go new file mode 100644 index 0000000..0fb1cae --- /dev/null +++ b/skills/idiomatic-go/evals/files/interfaces-errors/orders/orders.go @@ -0,0 +1,28 @@ +// Package orders places orders on behalf of known users. +package orders + +import ( + "errors" + + "example.com/idiomatic-go-evals/interfaces-errors/userstore" +) + +// Service places orders after checking that the buyer is a known user. +type Service struct { + users *userstore.Store +} + +// NewService returns a Service backed by the given user store. +func NewService(users *userstore.Store) *Service { + return &Service{users: users} +} + +// PlaceOrder records an order of item for the user identified by userID. +func (s *Service) PlaceOrder(userID, item string) error { + u := s.users.FetchUser(userID) + if u.Email == "" { + return errors.New("User not found.") + } + // ... record the order for u against item ... + return nil +} diff --git a/skills/idiomatic-go/evals/files/interfaces-errors/userstore/userstore.go b/skills/idiomatic-go/evals/files/interfaces-errors/userstore/userstore.go new file mode 100644 index 0000000..0c8cb09 --- /dev/null +++ b/skills/idiomatic-go/evals/files/interfaces-errors/userstore/userstore.go @@ -0,0 +1,55 @@ +// Package userstore loads user records and serves them by ID. +package userstore + +import ( + "encoding/json" + "fmt" + "os" +) + +// User is a single user record. +type User struct { + ID string + Email string +} + +// Fetcher is the lookup surface exposed for callers that only read users. +type Fetcher interface { + FetchUser(id string) User +} + +// Store holds the loaded users in memory. +type Store struct { + backend map[string]User +} + +var _ Fetcher = (*Store)(nil) + +// NewStore returns an empty Store. +func NewStore() *Store { + return &Store{backend: map[string]User{}} +} + +// Load reads a JSON array of users from path into the store. +func (s *Store) Load(path string) error { + f, err := os.Open(path) + if err != nil { + return fmt.Errorf("Failed to load users from %s: %v.", path, err) + } + defer f.Close() + + var users []User + if err := json.NewDecoder(f).Decode(&users); err != nil { + return fmt.Errorf("Could not decode users file %s: %v.", path, err) + } + for _, u := range users { + s.backend[u.ID] = u + } + return nil +} + +// FetchUser returns the user with the given ID. If no such user exists it +// returns the zero User. +func (s *Store) FetchUser(id string) User { + return s.backend[id] +} diff --git a/skills/idiomatic-go/evals/files/reference-resolution/go.mod b/skills/idiomatic-go/evals/files/reference-resolution/go.mod new file mode 100644 index 0000000..6f49b00 --- /dev/null +++ b/skills/idiomatic-go/evals/files/reference-resolution/go.mod @@ -0,0 +1,3 @@ +module example.com/idiomatic-go-evals/reference-resolution + +go 1.23 diff --git a/skills/idiomatic-go/evals/files/reference-resolution/types.go b/skills/idiomatic-go/evals/files/reference-resolution/types.go new file mode 100644 index 0000000..667cde9 --- /dev/null +++ b/skills/idiomatic-go/evals/files/reference-resolution/types.go @@ -0,0 +1,72 @@ +// Package aliases is a miniature type-alias language. +// +// A declaration "type A = B" makes A an alias of the type named B. Aliases can +// chain (A = B, B = C) and can form cycles (A = B, B = A). A Table holds the +// declared symbols; a Ref is the unresolved textual pointer from one alias to +// the name it targets. A Ref names a target by string; following it to the +// Symbol it points at is not implemented yet. +package aliases + +// Symbol is a declared type name. A primitive (int, string) has no alias; an +// alias declaration carries a Ref to the name it points at. +type Symbol struct { + Name string + alias *Ref +} + +// Alias returns the reference this symbol aliases, or nil for a primitive. +func (s *Symbol) Alias() *Ref { + if s == nil { + return nil + } + return s.alias +} + +// Ref is an unresolved reference to a Symbol by name. +type Ref struct { + target string + table *Table +} + +// Target returns the name this reference points at. +func (r *Ref) Target() string { + if r == nil { + return "" + } + return r.target +} + +// Table holds the declared symbols of one program, keyed by name. +type Table struct { + symbols map[string]*Symbol +} + +// NewTable returns an empty Table. +func NewTable() *Table { + return &Table{symbols: map[string]*Symbol{}} +} + +// Ref creates an unresolved reference to the symbol named target. +func (t *Table) Ref(target string) *Ref { + return &Ref{target: target, table: t} +} + +// Lookup returns the symbol declared under name, or nil if there is none. +func (t *Table) Lookup(name string) *Symbol { + return t.symbols[name] +} + +// DefinePrimitive declares a symbol with no alias (e.g. int, string). +func (t *Table) DefinePrimitive(name string) *Symbol { + s := &Symbol{Name: name} + t.symbols[name] = s + return s +} + +// DefineAlias declares "type name = target". The returned symbol's Alias() is a +// Ref that must be resolved to reach the target symbol. +func (t *Table) DefineAlias(name, target string) *Symbol { + s := &Symbol{Name: name, alias: t.Ref(target)} + t.symbols[name] = s + return s +} diff --git a/skills/idiomatic-go/evals/files/typescript-port/accounts.go b/skills/idiomatic-go/evals/files/typescript-port/accounts.go new file mode 100644 index 0000000..b2f73ae --- /dev/null +++ b/skills/idiomatic-go/evals/files/typescript-port/accounts.go @@ -0,0 +1,162 @@ +// Package accounts manages user accounts and registration. +package accounts + +import ( + "encoding/json" + "errors" + "math/rand" + "time" +) + +const ( + STATUS_ACTIVE = "active" + STATUS_DISABLED = "disabled" +) + +const DEFAULT_PAGE_SIZE = 20 + +// User is a user account record. +type User struct { + Id string + Email string + Age *int + Nickname *string + IsActive *bool + ApiToken string + AvatarUrl string + Status string + LastSeen time.Time +} + +func (this *User) GetId() string { + return this.Id +} + +func (this *User) GetEmail() string { + return this.Email +} + +func (this *User) SetEmail(email string) { + this.Email = email +} + +func (this *User) GetNickname() *string { + return this.Nickname +} + +// deactivateUser marks u inactive and disabled. Called on every user whose +// last-seen time is before a cutoff. +func deactivateUser(u User) { + inactive := false + u.IsActive = &inactive + u.Status = STATUS_DISABLED +} + +// IUserRepository reads and writes users from storage. +type IUserRepository interface { + FindById(id string) (*User, error) + Save(u *User) error +} + +// IUserService is the account service API. +type IUserService interface { + Register(email string) (*User, error) + GetUser(id string) (*User, error) + GetUserAsync(id string) chan *User + ActiveUsers(page int) []User + DeactivateStale(cutoff time.Time) +} + +type userService struct { + repo IUserRepository + active []User // users this service instance has loaded into memory +} + +// NewUserService builds a user service over the given repository. +func NewUserService(repo IUserRepository) IUserService { + return &userService{repo: repo} +} + +func (self *userService) Register(email string) (*User, error) { + if email == "" { + return nil, errors.New("Email is required!") + } + active := true + u := &User{ + Id: GenerateApiToken(), + Email: email, + IsActive: &active, + ApiToken: GenerateApiToken(), + Status: STATUS_ACTIVE, + LastSeen: time.Now(), + } + err := self.repo.Save(u) + if err != nil { + return nil, errors.New("Failed to save user: " + err.Error()) + } + self.active = append(self.active, *u) + return u, nil +} + +func (self *userService) GetUser(id string) (*User, error) { + u, err := self.repo.FindById(id) + if err != nil { + return nil, errors.New("Failed to get user: " + err.Error()) + } + if u == nil { + return nil, errors.New("User not found!") + } + return u, nil +} + +func (self *userService) GetUserAsync(id string) chan *User { + ch := make(chan *User) + go func() { + u, _ := self.repo.FindById(id) + ch <- u + }() + return ch +} + +// ActiveUsers returns one page of the users currently loaded in this +// service's in-memory cache, page 0 being the first page. +func (self *userService) ActiveUsers(page int) []User { + start := page * DEFAULT_PAGE_SIZE + if start >= len(self.active) { + return nil + } + end := start + DEFAULT_PAGE_SIZE + if end > len(self.active) { + end = len(self.active) + } + return self.active[start:end] +} + +// DeactivateStale marks cached users inactive if they haven't been seen since cutoff. +func (self *userService) DeactivateStale(cutoff time.Time) { + for _, u := range self.active { + if u.LastSeen.Before(cutoff) { + deactivateUser(u) + } + } +} + +// GenerateApiToken returns a random token used for API authentication. +func GenerateApiToken() string { + const chars = "abcdef0123456789" + b := make([]byte, 32) + for i := range b { + b[i] = chars[rand.Intn(len(chars))] + } + return string(b) +} + +// ParseUserJson decodes a User from JSON bytes. +func ParseUserJson(data []byte) (*User, error) { + var u User + err := json.Unmarshal(data, &u) + if err != nil { + return nil, errors.New("Invalid user JSON: " + err.Error()) + } + return &u, nil +} diff --git a/skills/idiomatic-go/evals/files/typescript-port/go.mod b/skills/idiomatic-go/evals/files/typescript-port/go.mod new file mode 100644 index 0000000..df94940 --- /dev/null +++ b/skills/idiomatic-go/evals/files/typescript-port/go.mod @@ -0,0 +1,3 @@ +module example.com/idiomatic-go-evals/typescript-port + +go 1.23 diff --git a/skills/idiomatic-go/references/concurrency-patterns.md b/skills/idiomatic-go/references/concurrency-patterns.md index 4124bcc..d564250 100644 --- a/skills/idiomatic-go/references/concurrency-patterns.md +++ b/skills/idiomatic-go/references/concurrency-patterns.md @@ -12,6 +12,7 @@ Contents: - [Mutex vs channel](#mutex-vs-channel) - [`sync.Once`, `sync.WaitGroup`, `errgroup`](#synconce-syncwaitgroup-errgroup) - [Channel-closing rules](#channel-closing-rules) +- [Idempotent shutdown against concurrent registration](#idempotent-shutdown-against-concurrent-registration) - [Pipelines and fan-out / fan-in](#pipelines-and-fan-out--fan-in) - [Anti-patterns](#anti-patterns) @@ -40,6 +41,8 @@ func (s *Service) ProcessAll(ctx context.Context, ids []string) error { Never store a `Context` in a struct field. The context belongs to the call, not the receiver. If a long-lived object needs to know when to stop, give it an explicit `Stop()` method or a `done` channel — not a stored context. +**This rule applies to every type in the design, not just the one under direct discussion.** A coordinating `Manager` that owns and starts several `Worker`s is bound by it exactly as much as the `Worker` itself — fixing a stored context on the leaf type while leaving it on the type that supervises the leaves is not a fix, it's a partial one. When reviewing or refactoring, re-check *every* struct in the file for a stored `context.Context` field, not only the one the task description names. + ## Every goroutine has an exit A goroutine that doesn't exit is a leak. Goroutines that block forever on a channel that no one will send to keep their stack and any captured references alive for the program's lifetime. Before writing `go ...`, name the exit: @@ -156,15 +159,157 @@ Capturing the loop variable in the closure is correct when the module uses Go 1. - **Closing means "no more values".** It does *not* mean "stop". Receivers continue to read buffered values; only after the buffer drains does `<-ch` return the zero value with `ok == false`. - **Don't close to "signal stop".** Use `ctx.Done()` or a dedicated `done chan struct{}` for stop signals. +## Idempotent shutdown against concurrent registration + +Any long-lived coordinator that hands out live channels tied to its own lifetime — a pub-sub hub, a file watcher, a streaming-RPC fan-out — and can also be torn down, has two obligations at once, and it's easy to satisfy only the obvious one: + +- **Shutdown must be safe to call twice.** Closing a closed channel panics, so a second shutdown call must be a no-op, not a repeat close. `sync.Once` is the standard tool. +- **Shutdown and registration must not race.** If registering a new subscriber can happen concurrently with (or after) shutdown closing every existing subscriber channel, the new one never gets closed, or — worse — a send to it after shutdown has already run panics with "send on closed channel". A closed flag checked *under the same lock* that guards registration and closing is what closes this gap; checking `ctx.Done()` alone does not, because there's a window between the context firing and every in-flight registration/send finishing. + +```go +type Broadcaster struct { + mu sync.Mutex + subscribers map[chan Event]struct{} + closed bool + closeOnce sync.Once +} + +func (b *Broadcaster) Subscribe() <-chan Event { + ch := make(chan Event, 1) + b.mu.Lock() + defer b.mu.Unlock() + if b.closed { + close(ch) // already shut down: hand back a closed channel, never panic + return ch + } + b.subscribers[ch] = struct{}{} + return ch +} + +func (b *Broadcaster) publish(e Event) { + b.mu.Lock() + defer b.mu.Unlock() + if b.closed { // Close already closed every channel below; don't send to them + return + } + for ch := range b.subscribers { + select { + case ch <- e: + default: // slow subscriber; drop rather than block the broadcaster + } + } +} + +func (b *Broadcaster) Close() { + b.closeOnce.Do(func() { // safe to call Close() any number of times + b.mu.Lock() + defer b.mu.Unlock() + b.closed = true + for ch := range b.subscribers { + close(ch) + } + b.subscribers = nil + }) +} +``` + +The two obligations reinforce each other: `closeOnce` makes the *close* idempotent, and checking `closed` under `mu` in both `Subscribe()` and `publish()` makes the *interaction* with concurrent callers race-free — neither alone is sufficient. The method names here are illustrative; the same shape applies under whatever a domain calls them (`Watch`/`Stop`, `Listen`/`Shutdown`, `Register`/`Close`) — the obligations attach to the *shape* (hand out a live channel, later close it, possibly concurrently), not to any particular name. + +**Do not split the guard check from the side effect it guards across two lock acquisitions.** A tempting-looking variant checks `closed` under the lock, unlocks, then does the "expensive-looking" work (starting a goroutine, allocating a worker) before re-locking to register it: + +```go +// Wrong: the guard and the registration are two separate critical sections. +func (m *Manager) Watch(ctx context.Context, name string, /* ... */) { + m.mu.Lock() + if m.closed { + m.mu.Unlock() + return + } + m.mu.Unlock() // <-- window opens here + + w := newWorker(ctx, name /* ... */) // Stop() can run entirely in this window + + m.mu.Lock() + m.workers[name] = w // too late: Stop() already closed the shared channel + m.mu.Unlock() +} +``` + +Between the two lock scopes, a concurrent `Stop()`/`Close()` can run to completion — snapshot the (still-empty-of-this-worker) registry, wait on it, and close the shared channel — before the new worker is ever registered, so its first send panics with "send on closed channel". Starting a goroutine is O(1) and non-blocking, so there is no performance reason to release the lock before doing it; the rule against long-held locks is about *blocking* work (I/O, the `probe()`/handler call itself), not about a `go` statement. Keep the check, the side effect, and the bookkeeping that lets shutdown find and wait for that side effect inside **one** uninterrupted critical section. + ## Pipelines and fan-out / fan-in A pipeline is a chain of stages connected by channels; each stage runs in its own goroutine, reads from an input channel, writes to an output channel, and closes the output when its input drains. Fan-out: many goroutines read from one channel and produce on their own (or a shared) output channel. -Fan-in: many goroutines write to a shared output channel; a single coordinator closes it once all producers signal done (typically via `WaitGroup`). +Fan-in: many goroutines write to a shared output channel; a single coordinator closes it once all producers signal done (typically via `WaitGroup`) — never by having each producer close the shared channel itself, which double-closes (panics) the moment more than one producer is active. + +The two previous sections compose directly into the common case of a coordinator that both fans in *and* accepts new producers for as long as it's running — e.g. a `Manager` that supervises an open-ended, add/remove-at-any-time set of named workers and merges their output onto one stream: + +```go +type Manager struct { + mu sync.Mutex + workers map[string]*Worker + merged chan Item + closed bool + wg sync.WaitGroup + stopOnce sync.Once +} + +func NewManager() *Manager { + return &Manager{workers: make(map[string]*Worker), merged: make(chan Item)} +} + +// Watch starts a worker called name and folds its output into the merged +// stream. It is a no-op once Stop has been called. +func (m *Manager) Watch(ctx context.Context, name string) { + m.mu.Lock() + if m.closed { + m.mu.Unlock() + return + } + w := newWorker(ctx, name) // cheap: allocates and starts a goroutine, doesn't block + m.workers[name] = w + m.wg.Add(1) + m.mu.Unlock() // check, start, and registration all happened under one lock + + go func() { + defer m.wg.Done() + for item := range w.Results() { + select { + case m.merged <- item: + case <-ctx.Done(): + return + } + } + }() +} + +// Stop stops every watched target and closes the merged stream exactly +// once. Safe to call any number of times, and race-free against a +// concurrent Watch: a racing Watch either wins the lock first (and Stop's +// WaitGroup then waits for it too) or sees m.closed and never starts. +func (m *Manager) Stop() { + m.stopOnce.Do(func() { + m.mu.Lock() + m.closed = true + workers := make([]*Worker, 0, len(m.workers)) + for _, w := range m.workers { + workers = append(workers, w) + } + m.mu.Unlock() + + for _, w := range workers { + w.Stop() + } + m.wg.Wait() // every forwarder goroutine above has returned from its range loop + close(m.merged) + }) +} +``` -A worked example is out of scope here; the patterns are documented in detail in Effective Go and Rob Pike's ["Go Concurrency Patterns"](https://go.dev/talks/2012/concurrency.slide) talk. +`sync.Once` makes `Stop` itself idempotent; the `closed` flag checked in the same critical section as registration makes it race-free against `Watch`; the `WaitGroup` makes the fan-in close-exactly-once regardless of how many targets were ever watched. Dropping any one of the three reopens one of the bugs above. Further depth on plain fan-out/fan-in (no concurrent registration) is in Effective Go and Rob Pike's ["Go Concurrency Patterns"](https://go.dev/talks/2012/concurrency.slide) talk. ## Anti-patterns @@ -173,7 +318,9 @@ Things to flag on review: - **`time.Sleep` in tests** to wait for a goroutine to do something. Replace with a synchronization primitive (channel, `WaitGroup`, or a deterministic test hook). - **Polling a channel** with a non-blocking `select { default: }` in a hot loop. Almost always wrong — use a blocking receive or `for select`. - **`go f()` with no exit plan.** Fire-and-forget goroutines leak. Either the function is finite, or it watches `ctx.Done()`, or it ranges over a closeable channel. -- **Sharing a `sync.Mutex` by value.** The zero value of `sync.Mutex` is an unlocked mutex; passing one by value creates two independent mutexes that don't protect the same critical section. Always hold a `sync.Mutex` via a pointer-receiver method or as a field in a struct accessed via pointer. +- **Sharing a `sync.Mutex` by value.** The zero value of `sync.Mutex` is an unlocked mutex; passing one by value creates two independent mutexes that don't protect the same critical section. Always hold a `sync.Mutex` via a pointer-receiver method or as a field in a struct accessed via pointer. This is a special case of SKILL.md's "Value semantics" gap: a mutex is exactly the kind of field a struct copy silently duplicates instead of sharing. - **Closing a channel from a receiver-side goroutine.** See channel-closing rules. -- **Storing `context.Context` in a struct.** See the context contract. +- **Storing `context.Context` in a struct.** See the context contract — and check every type in the file, not only the one the task names. +- **A shutdown method that panics on a second call, or that races a concurrent registration call.** See idempotent shutdown. +- **Checking a guard under a lock, releasing it, then performing and registering the guarded action under a second, later lock.** The window between the two critical sections is exactly where a concurrent shutdown can invalidate the guard. See idempotent shutdown and the fan-in worked example. - **Calling `WaitGroup.Add` from inside the goroutine it counts.** `Add` must be called *before* the goroutine starts, to avoid a race with `Wait`. diff --git a/skills/idiomatic-go/references/interfaces-and-errors.md b/skills/idiomatic-go/references/interfaces-and-errors.md index e256a49..0c5cd03 100644 --- a/skills/idiomatic-go/references/interfaces-and-errors.md +++ b/skills/idiomatic-go/references/interfaces-and-errors.md @@ -187,6 +187,24 @@ Do **not** wrap when: A useful rule: wrap when the inner error came from a public API of another package or from your own caller; replace when the inner error came from an internal helper whose existence is incidental. +**Wrapping and branching are independent decisions — do both when the caller needs to.** Returning `fmt.Errorf("...: %w", err)` preserves the chain for *some* future caller to inspect; it does not make the current function itself react to the failure. When your immediate caller needs one failure treated differently from the rest, check it with `errors.Is`/`errors.As` before you return, then wrap for whatever caller is further up: + +```go +// The service branches on the sentinel it cares about, then wraps for its own caller. +func (s *Service) Checkout(ctx context.Context, cartID string) error { + cart, err := s.carts.Find(ctx, cartID) + switch { + case errors.Is(err, store.ErrNotFound): + return fmt.Errorf("checkout: %w", ErrCartNotFound) // the service's own sentinel + case err != nil: + return fmt.Errorf("checkout: find cart %s: %w", cartID, err) + } + // ... +} +``` + +A wrap-only version compiles and looks identical to a caller who never checks — the gap only shows up when someone needs to branch and can't. + ## `errors.Is`, `errors.As`, `errors.Join` - `errors.Is(err, target)` — walks the chain looking for an error that equals `target`. Use for sentinel-error checks. diff --git a/skills/idiomatic-go/references/language-engineering.md b/skills/idiomatic-go/references/language-engineering.md new file mode 100644 index 0000000..99b2342 --- /dev/null +++ b/skills/idiomatic-go/references/language-engineering.md @@ -0,0 +1,352 @@ +# Language engineering patterns reference + +Patterns that recur in Go tools for *programming languages* — parsers, language servers (LSP), type checkers, validators, DSL implementations — and go beyond baseline idiomatic Go. The other references cover the general idioms; this one covers the design shapes specific to code that reads, analyzes, and serves source code. It assumes the vocabulary of language implementation (AST, scope, symbol, reference, diagnostic) and applies whether you build a toolkit from scratch or extend an existing Go framework. This is deliberate machinery — overkill for ordinary application code, load-bearing for a language implementation. + +Version-specific APIs are annotated inline: `iter.Seq` needs Go 1.23, `sync.WaitGroup.Go` needs Go 1.25. + +Contents: + +- [Nil-safe AST navigation](#nil-safe-ast-navigation) +- [Traversal: a callback primitive under an iterator veneer](#traversal-a-callback-primitive-under-an-iterator-veneer) +- [Optional capability interfaces](#optional-capability-interfaces) +- [Empty means empty: non-nil sentinels](#empty-means-empty-non-nil-sentinels) +- [Source positions as typed units](#source-positions-as-typed-units) +- [One text handle, editor buffer shadowing disk](#one-text-handle-editor-buffer-shadowing-disk) +- [Scopes: lazy chains along AST containment](#scopes-lazy-chains-along-ast-containment) +- [References: typed wrapper, untyped interface](#references-typed-wrapper-untyped-interface) +- [Once-only resolution with context-keyed cycle detection](#once-only-resolution-with-context-keyed-cycle-detection) +- [The build pipeline: phased, parallel within a phase](#the-build-pipeline-phased-parallel-within-a-phase) +- [Incremental state: a progressive aggregate and a phase bitmask](#incremental-state-a-progressive-aggregate-and-a-phase-bitmask) +- [Staying responsive: the write-to-read lock downgrade](#staying-responsive-the-write-to-read-lock-downgrade) +- [Keep the domain free of the protocol](#keep-the-domain-free-of-the-protocol) +- [Performance discipline and hot-path data structures](#performance-discipline-and-hot-path-data-structures) + +## Nil-safe AST navigation + +AST code chains accessors constantly — "the document of the container of the nearest enclosing block". If every hop panics on nil, every call site sprouts defensive checks. Make the core node and reference accessors *nil-receiver-safe*: guard `if n == nil` and return the zero value, so "not there" degrades to a zero value instead of a crash. + +```go +func (n *Node) Container() *Node { + if n == nil { + return nil // any nil hop yields nil, not a panic + } + return n.container +} + +doc := n.Container().Container().Document() // caller checks once, at the end +``` + +This is SKILL.md's "nil is a meaningful receiver value", applied as a *documented contract*: callers rely on it, so it must hold for every accessor on the type, not just the ones that happen to be safe today. + +## Traversal: a callback primitive under an iterator veneer + +Deep tree walks are a hot path. Build them on a callback primitive taking a `func(*Node) bool` (return `false` to stop), then layer ergonomic `iter.Seq` wrappers (Go 1.23) on top. The callback form avoids the per-step coroutine handoff a native iterator pays on a deep walk; the public API still reads as a range loop. + +```go +func (n *Node) ForEachDescendant(visit func(*Node) bool) bool { + for _, c := range n.children { + if !visit(c) || !c.ForEachDescendant(visit) { + return false // propagate the stop up the recursion + } + } + return true +} + +func (n *Node) Descendants() iter.Seq[*Node] { + return func(yield func(*Node) bool) { n.ForEachDescendant(yield) } +} +``` + +The subtlety: thread `yield`'s return value back up the recursion. `yield` returns `false` when the consumer `break`s the `for range`; ignore it and the walk keeps running after the consumer has stopped caring. The same discipline lets lazy operators (`Map`, `Filter`, `FlatMap`) compose over `iter.Seq` while still short-circuiting — which is what makes the lazy scope chains below cost nothing until consumed. + +**Apply the veneer to every traversal direction the API exposes, not just the one that comes up first.** A node type with both parent-ward and child-ward navigation needs *both* wrapped — shipping `Descendants()` while leaving upward traversal as a bare loop (or vice versa) is a half-finished version of this pattern, not a smaller one: + +```go +func (n *Node) Ancestors() iter.Seq[*Node] { + return func(yield func(*Node) bool) { + for p := n.Container(); p != nil; p = p.Container() { + if !yield(p) { + return // consumer broke out; stop walking upward + } + } + } +} +``` + +`Ancestors` has no recursion to thread `yield` through — the chain is already linear — but it's the same veneer-over-primitive shape: a plain loop underneath, `iter.Seq[*Node]` on top. The same question is worth asking of any bidirectional relationship a language toolkit models — parent/child, definition/reference, caller/callee: if one direction gets the callback-primitive-plus-veneer treatment, the other is under the same obligation, not a separate feature to add later. + +## Optional capability interfaces + +A generated AST has dozens of node types, and only some participate in any given concern (validation, symbol export, folding). Rather than force every node to implement a fat interface, define *small, optional* capability interfaces and check for them with a type assertion at the use site, falling back to a default when a node doesn't implement one. + +```go +type Validator interface { + Validate(ctx context.Context, report *Diagnostics) +} + +func validate(ctx context.Context, n *Node, report *Diagnostics) { + if v, ok := n.impl.(Validator); ok { + v.Validate(ctx, report) + return + } + defaultValidate(ctx, n, report) +} +``` + +A node opts into a behavior by implementing the interface on its implementation struct — no registration, no base class, no framework edit. The same move discovers lifecycle participants: instead of maintaining an explicit hook list, ask a registry for *every* service that satisfies a participation interface and call them. + +```go +for p := range registry.All[InitializeParticipant]() { + p.OnInitialize(ctx, params) +} +``` + +Behavior is discovered by what a value *is*, not by a list someone had to remember to populate. + +## Empty means empty: non-nil sentinels + +"Nothing here" — an empty scope, an empty symbol table, no diagnostics — recurs everywhere, and modeling it as `nil` forces every consumer to nil-check before use. Use a reusable *non-nil* sentinel that answers the interface with empty results, so consumers query it uniformly. + +```go +var EmptyScope Scope = emptyScope{} + +type emptyScope struct{} + +func (emptyScope) Lookup(name string) (Symbol, bool) { return Symbol{}, false } +func (emptyScope) All() iter.Seq[Symbol] { return func(func(Symbol) bool) {} } +``` + +Enforce the contract loudly: if a provider returns nil where a sentinel was required, panic with a message that names the fix rather than letting it surface later as a distant nil dereference. + +```go +func resolveIn(s Scope, name string) (Symbol, bool) { + if s == nil { + panic("nil scope: return EmptyScope for an empty result, never nil") + } + return s.Lookup(name) +} +``` + +This is one of the rare places a library should panic (see SKILL.md's errors section): a nil sentinel is a bug in the *provider*, worth failing on immediately. + +## Source positions as typed units + +Positions are the most-passed values in a toolkit, and a byte offset, a line, and a column are all "just an int" — which is why they get transposed. Give each its own named type so the compiler rejects the mistake, and pin the semantics that bite everyone in the doc comment. + +```go +type ( + ByteOffset int32 // 0-based offset into the UTF-8 source + Line int32 // 0-based line number + Column int32 // 0-based offset within the line, in UTF-16 code units +) + +type Position struct { + Line Line + Column Column +} + +// Half-open: End is the first position past the range. +type Range struct{ Start, End Position } +``` + +Two traps worth encoding in the types: LSP measures columns in *UTF-16 code units* by default (negotiable since LSP 3.17, but UTF-16 is what you get otherwise), so a column is neither a byte offset nor a rune count; and ranges are *half-open*, so boundary comparisons stay consistent. `int32` over `int` is a density choice — positions are stored in bulk, so halving their size matters at scale. + +## One text handle, editor buffer shadowing disk + +A language server reads text that may live on disk or in an unsaved editor buffer, and lexers, parsers, and resolvers shouldn't care which. Consume all text through one read interface, backed by two implementations — an immutable on-disk snapshot and an *overlay* layering unsaved edits over it. The store resolves a URI to the overlay when one is open, the file otherwise, so every consumer sees the *effective* content. + +```go +type Source interface { + Bytes() []byte + // The byte <-> UTF-16-position mapping the protocol needs lives here too, + // computed once against the effective content. + OffsetAt(Position) ByteOffset + PositionAt(ByteOffset) Position +} + +func (s *Store) Source(uri URI) Source { + if ov, ok := s.overlays[uri]; ok { + return ov // unsaved buffer shadows disk + } + return s.files[uri] +} +``` + +Concentrating the position-mapping logic behind this one handle means it's written and tested once, against whatever content is actually in effect. + +## Scopes: lazy chains along AST containment + +A name-resolution scope needn't be a precomputed symbol table. Assemble it *on lookup* by walking up the container chain: each container contributes its local symbols, chained to the enclosing container's scope, terminating in the global scope. + +```go +func ScopeOf(n *Node) Scope { + c := n.Container() + if c == nil { + return GlobalScope + } + locals := c.LocalSymbols() + if !locals.Any() { + return ScopeOf(c) // skip empty layers to keep the chain short + } + return chainedScope{locals: locals, parent: ScopeOf(c)} +} +``` + +Because lookups flow through the lazy sequence operators, an outer scope is traversed only if the inner ones miss — you pay for exactly the resolution depth a lookup needs. Merging symbols across documents follows the same laziness: wrap the per-document containers in an immutable view that `FlatMap`s over them on iteration, so the merge is O(1) to build and allocates nothing until read. + +## References: typed wrapper, untyped interface + +A cross-reference wants two audiences. Language-specific code wants compile-time safety — a reference to a `FunctionDecl` resolving to `*FunctionDecl`. Framework code iterating over *all* references wants to treat them uniformly without knowing the target type. Serve both from one value: a generic wrapper for the typed consumer that satisfies a non-generic interface for the framework. + +```go +type Reference[T Node] struct { /* target text, resolution state */ } + +func (r *Reference[T]) Resolve(ctx context.Context) (T, error) { /* ... */ } + +type UntypedReference interface { + Text() string + ResolveUntyped(ctx context.Context) (Node, error) +} + +var _ UntypedReference = (*Reference[Node])(nil) +``` + +One implementation, no duplication, no `any`-casting at call sites. The compile-time check keeps the two faces from drifting apart. + +## Once-only resolution with context-keyed cycle detection + +Reference resolution should be lazy, memoized (resolved at most once, even under concurrent access), and cycle-safe (mutually recursive declarations must not loop forever). The elegant part is cycle detection: key the `context` by the reference *itself*, so a cyclic chain is caught using the call graph — no separate visited set, no shared mutable state. + +```go +func (r *Reference[T]) Resolve(ctx context.Context) (T, error) { + if r.done.Load() { // atomic fast path + return r.target, r.err + } + return r.resolveSlow(ctx) +} + +func (r *Reference[T]) resolveSlow(ctx context.Context) (T, error) { + r.mu.Lock() + defer r.mu.Unlock() + if r.done.Load() { // double-check under the lock + return r.target, r.err + } + if ctx.Value(r) != nil { // r is already resolving further up the stack + var zero T + return zero, errCyclicReference + } + ctx = context.WithValue(ctx, r, true) + r.target, r.err = r.compute(ctx) // may recursively resolve other references + r.done.Store(true) + return r.target, r.err +} +``` + +This is a legitimate use of a context value — request-scoped, flowing down the call stack, never stored on a struct — exactly the distinction the concurrency reference draws. + +## The build pipeline: phased, parallel within a phase + +Analyzing a multi-file project has ordering constraints: you can't link references until every document has exported its symbols, nor validate until linking is done. Model this as *ordered phases* with a barrier between them, fanning out one goroutine per document *within* each phase. Check `ctx.Err()` between phases so a superseding edit cancels the build promptly. + +```go +func Build(ctx context.Context, docs []*Document) error { + phases := []func(context.Context, *Document){ + parseAndExport, // phase 1: per-doc; publishes exported symbols + resolveAndLink, // phase 2: reads other docs' exported symbols + validate, // phase 3: reads the fully linked model + } + for _, phase := range phases { + if err := ctx.Err(); err != nil { + return err + } + var wg sync.WaitGroup + for _, d := range docs { + wg.Go(func() { phase(ctx, d) }) // sync.WaitGroup.Go, Go 1.25 + } + wg.Wait() // barrier before the next phase + } + return nil +} +``` + +The barrier is the point: phase 2 reads data phase 1 produced *for other documents*, so it can't start until phase 1 finishes for *every* document. Within a phase there are no cross-document reads, so documents parallelize cleanly. + +## Incremental state: a progressive aggregate and a phase bitmask + +The `Document` is one struct whose fields fill in as phases run, not a fresh value per phase; each field's doc comment says which phase populates it. Track progress with a bitmask, and — the reusable trick — let `IsComplete` check only the framework-defined bits, leaving the high bits for adopters to define their own phases without the framework mistaking an incomplete document for a complete one. + +```go +type Document struct { + AST *Node // set by phase 1 (Parsed); nil before + Symbols SymbolTable // set by phase 2 (Linked); empty before + Diags []Diagnostic // set by phase 3 (Validated); nil before + state State +} + +type State uint32 + +const ( + Parsed State = 1 << iota + Linked + Validated + + frameworkComplete = Parsed | Linked | Validated // low bits only +) + +func (s State) Has(bits State) bool { return s&bits == bits } +func (s State) IsComplete() bool { return s.Has(frameworkComplete) } +``` + +This enables fine-grained incremental rebuilds: on an edit, clear only the fields whose phase must rerun, so a change to one function body needn't re-export symbols for the whole project. + +## Staying responsive: the write-to-read lock downgrade + +A language server must stay responsive while it rebuilds. An edit rebuilds under an exclusive lock, but read requests — hover, go-to-definition, completion — shouldn't wait for the slow tail (validation) to run against the already parsed-and-linked model. Three decisions make this work: + +- **Downgrade the write lock to a read lock atomically**, with no window for another writer to slip in. Reads unblock and observe exactly the state the writer produced, while validation continues under the shared lock. +- **Cancel superseded work, but let the cancelled unit finish its mutations.** A newer edit cancels an in-flight build via context, but the build only skips *expensive optional* work on `ctx.Err()` — dropping a mutation would desync the model from the text. +- **Schedule write-priority**, so the freshest edit always wins and readers never starve the writer. + +This is involved, and worth it only on the interactive request path. A batch tool (linter, compiler) has no concurrent readers and should just lock, build, release. + +## Keep the domain free of the protocol + +An LSP-backed toolkit is tempting to build directly on the protocol types (`lsp.Diagnostic`, `lsp.Position`). Resist it: if the core imports the transport's vocabulary, the domain can't be reused off the protocol (a CLI, a batch validator, a test) and is welded to one wire format. Mirror the protocol type in the core *without importing the protocol package*, and put a thin bridge at the edge. + +```go +// Core package: mirrors lsp.Diagnostic but imports nothing from the protocol. +type Diagnostic struct { + Range Range + Severity Severity + Message string +} + +// LSP layer only: +func (d Diagnostic) toLSP() lsp.Diagnostic { + return lsp.Diagnostic{ + Range: d.Range.toLSP(), + Severity: lsp.DiagnosticSeverity(d.Severity), + Message: d.Message, + } +} +``` + +The protocol layer adapts to the domain, not the other way around — SKILL.md's "define at the boundary" idea applied to a whole dependency. + +## Performance discipline and hot-path data structures + +Toolkits have real hot paths — lexing every byte, classifying every token, walking the tree, resolving references. Three habits keep them fit for purpose. + +**Annotate non-obvious optimizations with the alternative they beat and the measured factor.** An unexplained micro-optimization is indistinguishable from a mistake, and the next reader will "clean it up". + +```go +// atomic.Pointer, not sync.Once: measured ~2x faster here, since the resolved +// case is a single atomic load with no Once bookkeeping. +if cached := n.str.Load(); cached != nil { + return *cached +} +``` + +**Reach for a standard `comparable` map first; drop to a hashed bucket map only when keys genuinely aren't comparable** — e.g. structurally-equal types that are distinct pointers. A separate-chaining `map[uint64][]entry` keyed by a user-supplied `Hash() uint64` / `Equals(T) bool` unlocks map behavior for such keys. + +**On the parser hot path, make membership a bit test, not a scan.** Back token-type and token-group membership with a bitset — a group's set is the union of its members' — so "is this token in this group?" (and FIRST-set checks for error recovery) become word-wise bit tests. From 2008f58b7c1ab2a3a6f51d4e0e32a2fc18eb7663 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miro=20Spo=CC=88nemann?= Date: Mon, 20 Jul 2026 13:34:59 +0200 Subject: [PATCH 2/2] Review comments --- skills/go-documentation/SKILL.md | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/skills/go-documentation/SKILL.md b/skills/go-documentation/SKILL.md index 4b35add..c77de8c 100644 --- a/skills/go-documentation/SKILL.md +++ b/skills/go-documentation/SKILL.md @@ -1,6 +1,6 @@ --- name: go-documentation -description: Write idiomatic Go doc comments that render correctly on pkg.go.dev — package comments, `doc.go`, exported-symbol comments, testable `Example` functions, deprecation notices — and publish or debug modules on pkg.go.dev (`pkgsite`, the module proxy). Use when writing or reviewing Go documentation, or when a developer used to JSDoc/TSDoc/Markdown is unsure how Go doc comments differ. Not for general Go programming or non-Go languages. +description: Write idiomatic Go doc comments that render correctly on pkg.go.dev — package comments, `doc.go`, exported-symbol comments, testable `Example` functions, deprecation notices — and publish or debug modules on pkg.go.dev (`pkgsite`, the module proxy). Use when writing or reviewing Go documentation, or when a developer used to writing JSDoc/TSDoc/Markdown is unsure how Go doc comments differ. Not for general Go programming or non-Go languages. --- # Go Documentation @@ -11,20 +11,19 @@ Go treats documentation as part of the toolchain. `go doc` reads comments locall A Go doc comment is a plain `//` comment above a declaration — there are no tags, and almost none of the Markdown that JSDoc/TSDoc allow. These are the habits that silently produce wrong or unrendered docs; reach for the Go column instead. -| TypeScript / JSDoc habit | What Go actually does | +| TypeScript / JSDoc habit | What Go actually does | | ---------------------------------------- | --------------------------------------------------------------------------------------------------------- | | `@param`, `@returns`, `@throws` tags | No tags exist. Name params and results in prose ("It returns the number of bytes written"). Types are in the signature — don't restate them. | | `` `inline code` `` backticks | Backticks (and `'single quotes'`) render as curly ‘quotes’. There is no inline-code markup — use a doc link `[Name]` for a symbol, plain text otherwise. | | Markdown link `[text](url)` | Not a link. Use a bare URL, or reference style: `[text]` in prose plus `[text]: https://…` defined at the end of the comment. | -| `{@link Foo}` cross-reference | Not recognized, renders as literal text. Use a doc link: `[Foo]`. | -| Fenced ```` ``` ```` blocks, `**bold**` | No fences, no emphasis. A code block is any line indented by one tab; `**bold**` renders literally. | +| `{@link Foo}` cross-reference | Not recognized, renders as literal text. Use a doc link: `[Foo]`. | +| Fenced ```` ``` ```` blocks, `**bold**` | No fences, no emphasis. A code block is any line indented by one tab; `**bold**` renders literally. | | `/** ... */` Javadoc-style block comment | Valid Go syntax, and it *does* attach as the doc comment — but `gofmt` recognizes the leading-`*` pattern and deliberately leaves it untouched, so the un-stripped `*` on every line makes `go doc`/pkg.go.dev render it as a mangled code block. Use a plain `//` on every line instead. | | `@example` with a snippet | Examples are real compiled functions (`func ExampleFoo()`) in `_test.go`, run by `go test`. See `references/examples.md`. | -| `@deprecated` | A paragraph beginning with the exact prefix `Deprecated: `. | -| Summary in any phrasing | The first sentence must start with the symbol name: `Foo returns…`, `Package foo…` (see the table below). | -| README is the docs (npmjs.com) | pkg.go.dev and `go doc` show the **package comment**, not the README, above the API. Empty package docs = no docs. | -| `npm publish` to a registry | No push step. Tag `vX.Y.Z`; the module proxy indexes it. A published version is immutable. | -| Indent comment text freely | Any indented line becomes a code block. Keep prose flush left. | +| `@deprecated` | A paragraph beginning with the exact prefix `Deprecated: `. | +| Summary in any phrasing | The first sentence must start with the symbol name: `Foo returns…`, `Package foo…` (see the table below). | +| README is the docs (npmjs.com) | pkg.go.dev and `go doc` show the **package comment**, in addition to the README, above the API. | +| Indent comment text freely | Any indented line becomes a code block. Keep prose flush left. | These habits usually show up together. Porting one JSDoc comment typically means fixing several rows in the table at once. Before — reads fine in an editor, renders broken or missing on pkg.go.dev: @@ -35,7 +34,7 @@ These habits usually show up together. Porting one JSDoc comment typically means // @returns a new Cache func NewCache(size int) *Cache { ... } -// Get looks up `key` and returns true if found. +// Method to look up `key`, returns true if found. // See the [tuning guide](https://example.com/cache-tuning) for eviction details. func (c *Cache) Get(key string) (string, bool) { ... } ```