basecamp / basecamp/basecamp-sdk

check-wrapper-drift: embedded-field promotion analysis is best-effort — known-unhandled shapes

Open
#741 2 comments 0 reactions 0 assignees View on GitHub
documentation go
Dominant language
Go
Stars
49
Forks
12
Avg merge
20h 47m
Merged PRs (30d)
89

Description

`scripts/check-wrapper-drift` gained embedded-field support in #721, which fixed #599: a wrapper that embeds a struct used to read as **fully drifted**, reporting every promoted field as a missing JSON tag. That is fixed and proven — 0 phantom reports on an embedding wrapper that is genuinely correct, while a genuinely missing field is still reported.

Certifying promoted fields **correctly** through arbitrary embeds is a different and much larger problem, and this issue records where that analysis is known to be wrong.

## Status: best-effort, and unreachable today

**No wrapper in a checked pair embeds a struct.** The gate says so on every run:

```
Wrapper drift check: 92 pairs walked, 1245 generated fields verified (…)
Embedded-field promotion: no pair embeds a struct, so nothing above was verified through it.
```

Every defect below is therefore latent by construction. The moment someone introduces an embedding wrapper into a checked pair, they become live — so **harden or restrict this path before doing that**, and use this list as the starting inventory rather than rediscovering it.

## Why this is a list and not a fix

#721 ran to twenty review rounds. Every finding fell into one of two families:

1. **Visited/arrival identity** — which types the promotion walk considers "the same type", which decides whether `encoding/json`'s same-depth annihilation fires.
2. **Crediting an assignment shape** — deciding what a right-hand side populates underneath the field it is assigned to.

Each round instantiated those two families in new Go syntax. #721 landed a conservative default for exactly this reason — an assignment shape or embed the walker cannot classify is now *reported* rather than credited, via a single chokepoint (`classifyValue`) whose default arm is "unrecognised".

**That default cannot reach the defects below, and this is the single most useful sentence here for whoever picks this up.** A conservative default only helps where the code recognises its own ignorance. In every case below the walker is not unsure — it is *confidently wrong*: it resolves a name, classifies a shape, and returns a definite answer that differs from the one `encoding/json` or `go/types` would give. There is no "unknown" branch for the default to catch. Closing them means enumerating Go's type identity and conversion rules, which has no natural end in an AST-only walk. **Do not re-attempt the general fix by widening the default; it does not generalise.** Either restrict what the wrapper package is allowed to embed, or move this analysis onto `go/types`.

## Silent under-reports (a wrapper can drop a generated field with no report)

These are the dangerous ones: the gate passes while the wire output omits a field the generated struct declares.

1. **Defined types collapse to one visited identity.** The walk keys arrival on the resolved underlying struct, so with `type Left Base`, `type Right Base` and `Base` embedding `Deep`, `encoding/json` reaches `Deep` twice and annihilates its tags while the walk reaches it once and certifies them. (Copilot, #721 `main.go:1886`; Codex raised the same family earlier from the arrival-count side.)
2. **Invalid JSON tag names are read as names.** `Base \`json:"bad\\name"\`` — `reflect.StructTag.Lookup` returns the non-empty string, so the walk treats the anonymous field as an ordinary tagged field, while `encoding/json` rejects the invalid name and *still promotes the embedded struct's fields*. On the generated side this omits real promoted keys from the expected set, so a wrapper may drop them. (Codex, `main.go:1135`.)
3. **Struct conversions are credited as whole values.** `w.Base = Base(Raw{ID: g.ID})` where `Raw` shares `Base`'s underlying type: the conversion is an `ast.CallExpr`, classified as a complete value, so the whole subtree is credited even though the inner keyed literal omits fields. Distinguishing a conversion from a call needs type identity. (Codex, `main.go:2390`.)
4. **Untagged exported fields are outside dominance.** An untagged exported field contributes its Go name as a JSON key and can dominate a promoted tag. Declined three times in #721 with evidence: it needs a *generated* tag spelled like a Go identifier, and 0 of 344 distinct tags in `client.gen.go` contain an uppercase letter, because that file is oapi-codegen output from snake_case wire names. Reachable only if the generated side ever grows an identifier-shaped tag.

## Loud over-reports (drift reported where Go would accept the code)

Safe direction — the gate names the field and someone rewrites the spelling — but they make embedding annoying in practice:

5. **Skipped-segment selectors.** `w.Base.CreatedAt` where an embed sits between `Base` and the field is legal Go and is not among the recognised spellings. The bounded fix is to resolve recorded assignment paths against the promotion tree and compare canonical identities, rather than enumerating spellings.
6. **A name chain reaching a marshaller is refused whichever declaration form it used.** `type Safe Stamp` has an empty method set and Go would walk its fields; the closure treats `type A B` and `type A = B` alike on purpose, to avoid tracking which hop carries a method set.
7. **Sibling embeds that both declare `MarshalJSON`.** The promoted method is ambiguous, so the encoder *does* walk the fields, but the walk refuses.
8. **Diamond alternate-path population targets.** With a duplicated `Common`, only the retained path's spellings count, so a write through the other leg reads as unpopulated.
9. **Decode-unsafe fires for fields that annihilated anyway.**

## Verified impossible, recorded so nobody re-files it

10. **Embedded pointer aliases** (`type P = *Base`, `type P *Base`). `gc` rejects both with `embedded field type cannot be a pointer`, per the spec's "T itself may not be a pointer type", so the shape cannot occur in code that compiles.

## What "fixing this" would mean

Not another round of spelling rules. Either:

- **Restrict the input** — decide the wrapper package may embed only plainly-declared local structs with no method sets, and make the gate refuse anything else (it already refuses several such shapes); or
- **Change the instrument** — run this analysis on `go/types` instead of the AST, where type identity, conversions, method sets and tag validity are answered by the compiler's own rules rather than re-derived.

The second is the real fix and is a rewrite, not a patch. Neither is worth doing until a wrapper actually embeds something.

## References

- #599 — the phantom-drift bug this analysis was added for
- #721 — the PR that added it, with the full round-by-round reasoning in its description and threads
- #722 — the sibling guard (`collectTimestampFields`) with the same original blind spot
- The `CAN / CANNOT` list and known-over-reports inventory in `scripts/check-wrapper-drift/main.go`'s header, which is the in-code counterpart of this issue

Contributor guide

Open the contributing guide

Research direction

Start with the CAN/CANNOT list and known-over-reports inventory in scripts/check-wrapper-drift/main.go, then read the embedded-field analysis and classifyValue path described in #721. Decide whether the wrapper input should be restricted or the analysis moved to go/types. Done means embedded wrappers cannot produce silent under-reports or known false refusals.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.