docs: discourage pointer-to-slice fields

Radek Antoniukrequested to merge
gitlab-community/gitlab-org/api/client-go:docs/2298-discourage-pointer-to-slice into main

What does this MR do?

Adds code style guidance discouraging pointer-to-slice types (*[]*T, *[]T) for struct fields and parameters, as proposed in #2298 (follow-up from the discussion in !3002 (merged)).

  • AGENTS.md: new "Slice Fields" section under "Common Patterns and Best Practices", with a bad vs good example and a small table showing how nil, empty and non-empty slices encode.
  • CONTRIBUTING.md: short paragraph in "Coding style" that links to the AGENTS.md section.

A nuance worth calling out

The issue says a nil slice distinguishes "unset" from an explicit empty slice for omitempty/omitzero. That holds for omitzero only. I checked how this repo encodes options (NewRequestToURL in gitlab.go: json.Marshal for POST/PUT/PATCH, go-querystring otherwise) with a quick script:

Value json:",omitempty" json:",omitzero" url:",omitempty" (go-querystring v1.2.0)
nil omitted omitted omitted
[]*T{} omitted [] omitted
&[]*T{} (today's *[]*T) [] [] omitted

So the guidance says: use a plain slice, and if the API needs an explicit empty list (for example, to clear something), use json:"...,omitzero", same as !3002 (merged) did for DefaultBranchProtectionDefaultsOptions. Query strings can't carry an empty list either way.

What this MR does not do

  • No existing fields are converted. Changing *[]*T to []*T is a breaking change, so that's deferred to 4.0 (similar to how breaking changes are being collected in #2301).
  • No lint rule. None of the enabled linters can flag pointer-to-slice struct fields without custom tooling. gocritic's ptrToRefParam only covers function parameters, and forbidigo matches identifiers, not types. I'd rather not add a custom analyzer just for this, but I'm happy to follow up if maintainers want one (a Duo review rule was also floated in !3002 (merged)).

Input for the 4.0 follow-up

There are currently 35 *[]*T fields outside tests (rg '\*\[\]\*' --type go -g '!*_test.go'):

File Count Fields
group_hooks.go 2 CustomHeaders
group_protected_branches.go 6 AllowedToPush, AllowedToMerge, AllowedToUnprotect
group_protected_environments.go 4 DeployAccessLevels, ApprovalRules
jobs.go 1 JobVariablesAttributes
pipelines.go 1 Variables
project_feature_flags.go 4 Strategies, Scopes
project_hooks.go 2 CustomHeaders
project_snippets.go 2 Files
protected_branches.go 6 AllowedToPush, AllowedToMerge, AllowedToUnprotect
protected_environments.go 4 DeployAccessLevels, ApprovalRules
protected_tags.go 1 AllowedToCreate
snippets.go 2 Files

All of them use json:",omitempty" today. When converting them, the JSON tag should switch to omitzero wherever sending an empty list is meaningful, otherwise callers lose the ability to send []. There are also about 118 *[]T (pointer to slice of values, for example *[]string) usages that the 4.0 audit may want to cover too.

Open questions

  • docs/guides/AddingAPISupport.md says "All fields should be pointers" for options structs. I left it alone to keep this MR scoped, but it might be worth adding a one-line exception for slices there too. Happy to do it here or in a follow-up.
  • Should the guidance also cover *[]T explicitly as part of the 4.0 audit, or only *[]*T as in the issue? The text currently discourages both.

Checks

  • markdownlint-cli2 on both files: no new findings (only pre-existing ones and the default line-length rule, which the repo doesn't enforce).
  • Docs-only change, no Go code touched.

Related to #2298

Merge request reports