Grandiras/ATProto.NET

Fork 0

fix(lexgen): space types generate compilable C# #179

Merged

Grandiras merged 1 commit from fix/lexgen-space-collections into main

2026-09-27 11:07:41 +00:00

Grandiras commented

2026-09-27 11:04:44 +00:00

Owner

Part of #141 (found by the #138 docs work). The generated-code sample in docs/lexicon-codegen.md is now compiled with the other doc snippets instead of being marked partial.

What changed

atproto-lexgen csharp generated a "type": "space" def's holder class with:

public static IReadOnlyList<string> Collections => Declaration.Collections;

but SpaceTypeDeclaration.Collections (src/ATProtoNet/Spaces/SpaceTypeDeclaration.cs) is
IReadOnlyList<Nsid> — CS0266, so every generated space def failed to compile. The existing
emitter tests (CSharpEmitterTests) only assert on the emitted text, so nothing caught it.

Fix: tools/ATProtoNet.LexiconGenerator/CodeGen/CSharpEmitter.cs (EmitSpaceType) now
emits IReadOnlyList<ATProtoNet.Identity.Nsid> Collections, matching the fully-qualified style
the emitter already uses for Nsid.Parse(...) in the same method (space-def emission doesn't
set ctx.NeedsSdkIdentity, so the type is spelled out rather than relying on a using).
Audited every other forwarder on the space holder (Key, Name, LocalizedNames) and on the
permission-set holder (Nsid, Include) against their SDK counterparts
(SpaceTypeDeclaration, AtProtoScopes.Include) — no other mismatch found. LocalizedNames
returns a Dictionary<string,string> through an IReadOnlyDictionary<string,string>?-typed
property, which is a legal implicit interface conversion, so it was already fine.
New test: tests/ATProtoNet.Tests/Lexicon/CSharpEmitterCompileTests.cs compiles the
emitter's output, not just its text, with Roslyn (Microsoft.CodeAnalysis.CSharp 4.14.0 —
matched to the version already pinned by tests/ATProtoNet.DocSnippets.Generator on
origin/issue-138-docs). One test (Emit_RepresentativeLexiconSet_ProducesCompilableCSharp)
runs the emitter over 11 Lexicon documents — 4 reused from CSharpEmitterTests (bumped from
private to internal so they can be shared: RecipeDefs, RecipeRecord, ForumSpace,
AuthBasicSet) plus 7 new ones covering what those didn't — then compiles every generated
file together, referencing the trusted platform assemblies plus ATProtoNet.dll, with
nullable enabled, and asserts there are no error diagnostics (printing them on failure).
Coverage: records; every primitive type and every string format; arrays; an open union and a
closed union of object defs; a knownValues string and a ref to a string-enum def; refs across
documents both within the same C# namespace (sibling NSID authority) and across namespaces
(exercising both branches of CSharpEmitter.Shorten, including its global:: qualification);
token families; a query and a procedure with parameters/input/output; a subscription with
a closed union message.schema; a space def with collections and localized names, and one
without either; and a permission set.
CHANGELOG.md: added a Fixed bullet under ## [Unreleased].

Verified

Confirmed the new test fails with the original IReadOnlyList<string> Collections line
(two CS0266 diagnostics, on the reused ForumSpace fixture and the new BareSpace fixture),
and passes once the property is retyped.
Full solution build: dotnet build -p:EnableSourceControlManagerQueries=false — 0 warnings,
0 errors.
Unit tests: dotnet test tests/ATProtoNet.Tests/ -p:EnableSourceControlManagerQueries=false —
5059 passed, 0 failed, 5 skipped (the 5 skips are pre-existing Aspire manifest tests,
unrelated to this change).

Deferred / not in scope

Queries, procedures and subscriptions still generate no C# client code (case "query": case "procedure": break; / case "subscription": break; in CSharpEmitter.EmitDocument) — that's
the documented Issue #45 scope, not part of this bug. The new compile test includes fixtures
for all three only to prove the emitter doesn't choke when they sit alongside emittable defs.
No other generator bugs turned up in the compile pass — the CS0266 above was the only one.
Kept tools/** edits to the single line described above, and src/** untouched, per the
note that PR #177 is doing a mechanical pass over tools/ and src/.

🤖 Generated with Claude Code

Part of #141 (found by the #138 docs work). The generated-code sample in `docs/lexicon-codegen.md` is now compiled with the other doc snippets instead of being marked `partial`. ## What changed `atproto-lexgen csharp` generated a `"type": "space"` def's holder class with: ```csharp public static IReadOnlyList<string> Collections => Declaration.Collections; ``` but `SpaceTypeDeclaration.Collections` (`src/ATProtoNet/Spaces/SpaceTypeDeclaration.cs`) is `IReadOnlyList<Nsid>` — CS0266, so every generated space def failed to compile. The existing emitter tests (`CSharpEmitterTests`) only assert on the emitted text, so nothing caught it. - **Fix**: `tools/ATProtoNet.LexiconGenerator/CodeGen/CSharpEmitter.cs` (`EmitSpaceType`) now emits `IReadOnlyList<ATProtoNet.Identity.Nsid> Collections`, matching the fully-qualified style the emitter already uses for `Nsid.Parse(...)` in the same method (space-def emission doesn't set `ctx.NeedsSdkIdentity`, so the type is spelled out rather than relying on a using). - Audited every other forwarder on the space holder (`Key`, `Name`, `LocalizedNames`) and on the permission-set holder (`Nsid`, `Include`) against their SDK counterparts (`SpaceTypeDeclaration`, `AtProtoScopes.Include`) — no other mismatch found. `LocalizedNames` returns a `Dictionary<string,string>` through an `IReadOnlyDictionary<string,string>?`-typed property, which is a legal implicit interface conversion, so it was already fine. - **New test**: `tests/ATProtoNet.Tests/Lexicon/CSharpEmitterCompileTests.cs` compiles the emitter's *output*, not just its text, with Roslyn (`Microsoft.CodeAnalysis.CSharp` 4.14.0 — matched to the version already pinned by `tests/ATProtoNet.DocSnippets.Generator` on `origin/issue-138-docs`). One test (`Emit_RepresentativeLexiconSet_ProducesCompilableCSharp`) runs the emitter over 11 Lexicon documents — 4 reused from `CSharpEmitterTests` (bumped from `private` to `internal` so they can be shared: `RecipeDefs`, `RecipeRecord`, `ForumSpace`, `AuthBasicSet`) plus 7 new ones covering what those didn't — then compiles every generated file together, referencing the trusted platform assemblies plus `ATProtoNet.dll`, with nullable enabled, and asserts there are no error diagnostics (printing them on failure). Coverage: records; every primitive type and every `string` format; arrays; an open union and a closed union of object defs; a `knownValues` string and a ref to a string-enum def; refs across documents both within the same C# namespace (sibling NSID authority) and across namespaces (exercising both branches of `CSharpEmitter.Shorten`, including its `global::` qualification); token families; a query and a procedure with `parameters`/`input`/`output`; a subscription with a closed union `message.schema`; a space def with collections and localized names, and one without either; and a permission set. - **CHANGELOG.md**: added a `Fixed` bullet under `## [Unreleased]`. ## Verified - Confirmed the new test **fails** with the original `IReadOnlyList<string> Collections` line (two `CS0266` diagnostics, on the reused `ForumSpace` fixture and the new `BareSpace` fixture), and **passes** once the property is retyped. - Full solution build: `dotnet build -p:EnableSourceControlManagerQueries=false` — **0 warnings, 0 errors**. - Unit tests: `dotnet test tests/ATProtoNet.Tests/ -p:EnableSourceControlManagerQueries=false` — **5059 passed, 0 failed, 5 skipped** (the 5 skips are pre-existing Aspire manifest tests, unrelated to this change). ## Deferred / not in scope - Queries, procedures and subscriptions still generate no C# client code (`case "query": case "procedure": break;` / `case "subscription": break;` in `CSharpEmitter.EmitDocument`) — that's the documented Issue #45 scope, not part of this bug. The new compile test includes fixtures for all three only to prove the emitter doesn't choke when they sit alongside emittable defs. - No other generator bugs turned up in the compile pass — the CS0266 above was the only one. - Kept `tools/**` edits to the single line described above, and `src/**` untouched, per the note that PR #177 is doing a mechanical pass over `tools/` and `src/`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit

2026-09-27 11:04:44 +00:00

fix(lexgen): space types generate compilable C# (#141 )

CI / pds-integration (pull_request) Successful in 30s

Details

CI / build-and-test (pull_request) Successful in 1m4s

Details

Sync Closures to GitHub / sync-closure (pull_request) Successful in 7s

Details

ac4244e52e

The space holder's Collections property was emitted as
IReadOnlyList<string> over SpaceTypeDeclaration.Collections, which is
IReadOnlyList<Nsid>, so every generated space def failed with CS0266.
The emitter tests only compared text; a new test compiles the emitter's
output for a representative lexicon set with Roslyn against the SDK.
The generated-code sample in docs/lexicon-codegen.md is now compiled
with the other doc snippets instead of being marked partial.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

opencode-bot commented

2026-09-27 11:04:46 +00:00

Collaborator

🤖 Reviewing this PR — the review will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the review runs.

🤖 Reviewing this PR — the review will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-179/run-2026-09-27T11-04-45-662Z.md) — updating every ~5 minutes while the review runs.

opencode-bot commented

2026-09-27 11:04:48 +00:00

Collaborator

⏳ Held on the OpenCode Go window — the 5-hour window is at 0% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-27T16:04:46.098Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 0% (the bot holds at 95%). This continues automatically after the window resets (2026-09-27T16:04:46.098Z) — nothing to re-trigger.

Grandiras commented

2026-09-27 11:04:51 +00:00

Author

Owner

@claude /review. This is a small fix plus a new compile test. Please check four things:

The emitter fix in tools/ATProtoNet.LexiconGenerator/CodeGen/CSharpEmitter.cs.
That tests/ATProtoNet.Tests/Lexicon/CSharpEmitterCompileTests.cs really compiles the emitter output against the SDK: trusted platform assemblies plus ATProtoNet.dll, nullable enabled, and a failure on any error diagnostic. Also that its 11 fixtures cover the def kinds the emitter supports.
The Microsoft.CodeAnalysis.CSharp 4.14.0 reference in the test project, which matches the doc-snippet generator's pin.
The un-marked docs sample in docs/lexicon-codegen.md.

@claude /review. This is a small fix plus a new compile test. Please check four things: 1. The emitter fix in `tools/ATProtoNet.LexiconGenerator/CodeGen/CSharpEmitter.cs`. 2. That `tests/ATProtoNet.Tests/Lexicon/CSharpEmitterCompileTests.cs` really compiles the emitter output against the SDK: trusted platform assemblies plus ATProtoNet.dll, nullable enabled, and a failure on any error diagnostic. Also that its 11 fixtures cover the def kinds the emitter supports. 3. The `Microsoft.CodeAnalysis.CSharp` 4.14.0 reference in the test project, which matches the doc-snippet generator's pin. 4. The un-marked docs sample in `docs/lexicon-codegen.md`.

claude-bot commented

2026-09-27 11:04:54 +00:00

Collaborator

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/632); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-179/run-632.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-09-27 11:06:22 +00:00

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

The fix is correct: SpaceTypeDeclaration.Collections in src/ATProtoNet/Spaces/SpaceTypeDeclaration.cs is indeed IReadOnlyList<Nsid> (confirmed by reading the source), so the generated forwarder's old IReadOnlyList<string> was a genuine CS0266 bug. The one-line fix to ATProtoNet.Identity.Nsid matches the fully-qualified style already used elsewhere in EmitSpaceType (which never sets ctx.NeedsSdkIdentity), so it's consistent rather than a one-off.

The new CSharpEmitterCompileTests is a solid regression guard — compiling the emitter's actual output with Roslyn against trusted platform assemblies plus the real ATProtoNet.dll closes the exact gap the PR description identifies (the existing tests only diffed emitted text). Fixture coverage is broad and well-targeted: every primitive/format, closed and open unions, cross-document/cross-namespace refs exercising both branches of Shorten, a space def with and without collections, and query/procedure/subscription defs to confirm they don't break compilation of neighboring emittable defs. Bumping the four reused fixtures from private to internal is the minimal change needed to share them.

The doc snippet change (dropping partial from the ForumSpace block in docs/lexicon-codegen.md) is consistent with CONTRIBUTING.md's guidance to use partial sparingly — the block is a complete, self-contained class and now compiles cleanly, so marking it non-partial is correct and keeps the sample from going stale silently. The audit of the other forwarders (Key, Name, LocalizedNames on the space holder; Nsid, Include on the permission-set holder) checks out against their SDK counterparts. CHANGELOG entry is properly scoped under Fixed with the issue reference. Scope is appropriately narrow, matching the stated constraint of not touching src/** beyond the doc snippet and only touching the one generator line in tools/**.

⏱ 1m 11s · 18 turns · tokens: 29.4k in / 4.3k out (+723.9k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. The fix is correct: `SpaceTypeDeclaration.Collections` in `src/ATProtoNet/Spaces/SpaceTypeDeclaration.cs` is indeed `IReadOnlyList<Nsid>` (confirmed by reading the source), so the generated forwarder's old `IReadOnlyList<string>` was a genuine CS0266 bug. The one-line fix to `ATProtoNet.Identity.Nsid` matches the fully-qualified style already used elsewhere in `EmitSpaceType` (which never sets `ctx.NeedsSdkIdentity`), so it's consistent rather than a one-off. The new `CSharpEmitterCompileTests` is a solid regression guard — compiling the emitter's actual output with Roslyn against trusted platform assemblies plus the real `ATProtoNet.dll` closes the exact gap the PR description identifies (the existing tests only diffed emitted text). Fixture coverage is broad and well-targeted: every primitive/format, closed and open unions, cross-document/cross-namespace refs exercising both branches of `Shorten`, a space def with and without collections, and query/procedure/subscription defs to confirm they don't break compilation of neighboring emittable defs. Bumping the four reused fixtures from `private` to `internal` is the minimal change needed to share them. The doc snippet change (dropping `partial` from the `ForumSpace` block in `docs/lexicon-codegen.md`) is consistent with CONTRIBUTING.md's guidance to use `partial` sparingly — the block is a complete, self-contained class and now compiles cleanly, so marking it non-partial is correct and keeps the sample from going stale silently. The audit of the other forwarders (`Key`, `Name`, `LocalizedNames` on the space holder; `Nsid`, `Include` on the permission-set holder) checks out against their SDK counterparts. CHANGELOG entry is properly scoped under Fixed with the issue reference. Scope is appropriately narrow, matching the stated constraint of not touching `src/**` beyond the doc snippet and only touching the one generator line in `tools/**`. --- ⏱ 1m 11s · 18 turns · tokens: 29.4k in / 4.3k out (+723.9k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-179/run-632.md)

Grandiras merged commit 573747c36c into main

2026-09-27 11:07:41 +00:00

Grandiras referenced this pull request from a commit

2026-09-27 11:07:41 +00:00

fix(lexgen): space types generate compilable C# (#179)

Grandiras deleted branch fix/lexgen-space-collections

2026-09-27 11:07:41 +00:00