Enforce AST node coverage, then close the declaration-level gaps by matt-edmondson · Pull Request #60 · ktsu-dev/Coder

matt-edmondson · 2026-09-12T12:32:36Z

Seven commits. The first closes #49; the next four use it; the last two address this PR's own review comment and analyzer findings. The exhaustiveness test is what made the rest safe to write, and it demanded the schema, inspector and palette entries for the new nodes rather than my remembering them — which is why they are one PR.

Together they close the declaration-level gap: public readonly partial record struct Mass<T> : IVector0<Mass<T>, T> where T : struct, INumber<T> is now reachable in full, in all seven targets, which is what emitting ktsu.Semantics from this AST needs.

843/843 tests pass, including the C, Rust and Go toolchain tests, which compile what the generators write.

1. Make a node type that misses `AstSchema` or `AstFields` fail a test (closes #49)

Both are hand-written switches ending in a silent default. That is the right shape — a node's structure is part of the library's contract, and a reflective walk would start exposing whatever property was added next — but it means a node type left out of either produces nothing failing. It produces a node that draws an empty inspector.

So the production code stays hand-written and the test is the reflective one. It walks every concrete AstNode subclass and asks three things: that AstSchema.SlotsOf gives it slots or that it is on a Childless list saying it has none on purpose; that AstFields.Of offers a field when the type has a public settable property holding a value; and that every field it offers can be written with a value it does not already hold and read back unchanged.

It found the three gaps that were already there — MemberInitialiser had no Name field, ConstructionExpression no Type field, and neither was writable, both switches having missed them together.

Childless holds Type objects, so renaming a node type is a compile error rather than a stale entry. The one property excluded from "something to edit" is Expression.ExpectedType: every expression carries it and the inspector offers it on none, so counting it would demand a new field on every expression rather than catch the gap this is for.

2. What a type declaration says about itself

Interfaces, IsRecord, IsPartial, IsReadOnly.

Interfaces are separate from BaseType because what each target does with the two differs enough that a generator handed one list would be guessing which entry was the class. C# writes them in one list, base first; C++ writes public before each and does not distinguish them; C embeds each as a member and gives the layout-compatible first position to the base, because it has exactly one to give; Rust makes both supertraits of a trait and writes a struct's down, an impl block needing bodies the declaration lacks; Python takes them all as bases; JavaScript extends one thing.

Go is the interesting one. It satisfies an interface structurally — a type implements one by having its methods and never says so — which leaves a declaration that meant to implement something with nothing in the file to show for it. So it writes var _ Contract = (*Type)(nil), which is a check rather than a comment. GoGeneratedSourceCompilesTests now declares that Circle implements Shape and compiles it, so the claim is one the test can break.

The modifiers split along one line: IsRecord and IsReadOnly are claims about the type, so a target with no word for one writes it down (Rust has a word for the first and uses it — #[derive(Clone, Debug, PartialEq)]); IsPartial claims nothing about the type and is dropped in silence.

3. The types a declaration is written over

TypeParameter and TypeConstraint, on both ClassDeclaration and FunctionDeclaration. Values rather than nodes, like SpecialisationArguments, with Parse/ToString inverses so a document carries a whole parameter on one line.

This is the one the design question was about, and settling it was the work:

A parameter's name travels everywhere. What a language can say about that name does not.

So TypeConstraintKind names four intents and stops — a C++ concept is a predicate that can ask anything at all, which is the same reason CompileTimeAssertion.Condition is text. Then C# spells all four and reorders them (the language requires struct/class first and new() last; the AST has no reason to know that). Rust spells two and carries the parameters onto every impl block, which is what makes impl<T: Bound> Mass<T> compile where impl Mass would not — RustGeneratedSourceCompilesTests compiles a generic struct whose bound the body depends on. Go spells one and only for a function, a method on a generic type needing the parameters spelled two ways. C++ writes the template head and notes the constraints, the standard concepts needing an include the AST does not carry.

4. Metadata

An Annotation on a class, function or field. Name and arguments are text written verbatim, for the reason CallExpression.Callee is; what each generator supplies is the syntax around them, the same split SpellImport makes.

C#      [SuppressMessage("Usage", "CA2225:…")]      Rust    #[SuppressMessage(…)]
C++     [[SuppressMessage("Usage", "CA2225:…")]]    Python  @SuppressMessage(…)

C, JavaScript and Go write it down instead. Arguments are a sequence so a comma inside one stays inside it.

5. Properties, and where one lands

The decision here is not the syntax but where a property lands. A property whose accessors have no bodies is a field with a storage location the compiler supplies; one with bodies is a pair of functions. Neither is an approximation, so the four targets without properties write the field, or the pair.

Rust   pub Value: i32,   /  pub fn doubled(&self) -> i32
Go     Value int         /  func (self Box) Doubled() int
C++    int Value{};      /  int doubled() const
C      int Value;        /  int Box_doubled(const Box* self)

The separation happens to the member list, not at the point each member is written, and that is why it works: Rust routes data to a struct and behaviour to an impl, Go writes fields in the type and methods beside it, C lowers a method to a free function — and every one of those reads the member list first. Separated afterwards, a property arrives too late to be routed and comes out wherever it was standing, which is what the first attempt did and what put a member function inside a C struct.

Two things fall out. A property may be readable and not writable and a field is neither or both, so the field carries that in its own documentation and it travels to wherever the target puts the field. And a separated setter has no name until one is invented, so NamingStyle gives each target its own convention — rustc warns on a member name that is not snake case and an unexported Go name cannot be called from outside its package, so two of the four are more than taste. The AST still renames nothing it was given.

6. This PR's own review comment and findings

The github-code-quality comment on ReadAnnotation (the trim is a mapping and now says so). CGenerator.GenerateClassDeclaration went over its cognitive-complexity limit when the embedded bases went in, so that block is WriteEmbeddedBases and the paragraph about why the first member is first lives with the code that puts it there. AstFields gets a constant for "ReadOnly", which earns one: it is the same word for two different promises. And two Assert.AreEqual(n, x.Count) become Assert.HasCount.

7. The two Sonar findings that followed

AstFields.TryWriteDeclaration came out at cognitive complexity 20 against a limit of 15, and every point of it was one &&: a TryParse(…) && Assign(…) pair per field. There are forty such pairs across the three switches, so the shape was going to keep pushing a method over the line each time nodes were added — it is what forced the last split. Parsing and writing are now one call named for the kind of field (AssignFlag, AssignInteger, AssignNumber, AssignMember<TEnum>), which takes that method to 1 and the other two switches with it. Four names rather than four overloads, because IDE0350 wants an inferred lambda parameter and an overload set is exactly what stops one being inferred.

That made every enumeration read case-insensitively. Visibility already did, with a comment saying why — a document hand-edited with public should read back the same as the inspector's own Public — and there was no reason for it to be the one that did.

JavaScriptGenerator spelled "static " five times; it is a constant now.

MSTEST0046 is deliberately not adopted. It wants Assert.Contains over StringAssert.Contains, and this test project is 290 to 111 the other way; Sonar reports it as INFO and the quality gate passes with it outstanding.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QGCMUrT3jBgmANHNHcPmBf

Closes #49. Both files are hand-written switches ending in a silent default, which is the right shape for them — a node's structure is part of the library's contract and a reflective walk would start exposing whatever property was added next — but it means a node type left out of either produces no error, no warning and nothing failing. It produces a node that draws an empty inspector. So the production code stays hand-written and the test is the reflective one. `AstNodeCoverageTests` walks every concrete `AstNode` subclass in the assembly, closing the two generic ones over the four storage types the inspector matches on, and asks three things of each: that `AstSchema.SlotsOf` gives it slots or that it is on a `Childless` list saying it has none on purpose, that `AstFields.Of` offers a field when the type has a public settable property holding a value rather than a child, and that every field it offers can be written with a value it does not already hold and read back unchanged. Thirty node types, ninety cases, and a failure names the type. It found the three gaps that were already there: - `MemberInitialiser` had no `Name` field, so the node that says which member an argument is for could not be told which one. - `ConstructionExpression` had no `Type` field, so the one expression that names a type rather than a name could not be given it. With no type it is a braced list, which is a real state, so the field is allowed to be empty. - Neither was writable either, both switches having missed them together. `Childless` holds types rather than names, so renaming one is a compile error here rather than a stale entry. The one property excluded from "something to edit" is `Expression.ExpectedType`: every expression carries it and the inspector offers it on none of them, so counting it would demand a new field on every expression in the AST rather than catch the gap this is for. Excluding the property rather than the two nodes that have nothing else is deliberate — the reason is one property shared by every expression, not two nodes that happen to be bare. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGCMUrT3jBgmANHNHcPmBf

[minor] `ClassDeclaration` carried a name, a kind and one base type. Four things a generated type routinely says about itself had nowhere to go: which interfaces it implements, whether the language should supply its value semantics, whether the rest of it may be declared elsewhere, and whether any member of it modifies it. `public readonly partial record struct Mass<T> : IVector0<Mass<T>, T>` needs all four, and none of them was reachable. `Interfaces` is separate from `BaseType` rather than one list, because what each target does with the two is different often enough that a generator handed one list would be guessing which entry was the class: - C# writes them in one list, base first, which is the order the language requires and the order a generator could not recover. - C++ writes `public` before each and does not distinguish them at all, an interface being a class whose members are pure virtual. - C embeds each as a member. The first position is what makes a pointer to the whole a pointer to the member, C has exactly one of those to give, so the base takes it and the interfaces after it are reached by address. The declaration says which one is first and why. - Rust makes both supertraits of a trait, which is the one place a target answers the question exactly. A struct's interfaces are written down instead: an impl block needs the bodies the declaration does not have. - Python takes them all as bases, having no separate notion of an interface. - JavaScript extends one thing, so the rest are written down. - Go embeds them in an interface, and for a struct writes `var _ Contract = (*Type)(nil)` — the language's own way to say it, and a check rather than a comment: the file stops compiling when the type stops implementing the interface. `GoGeneratedSourceCompilesTests` now declares that `Circle` implements `Shape` and compiles the result, so the claim is one that test can break. The three modifiers split along a line worth stating once. `IsRecord` and `IsReadOnly` are claims about the type — it compares by value, no member of it modifies it — so a target with no word for one writes it down, the same as a `CompileTimeAssertion`. Rust has a word for the first and uses it: `#[derive(Clone, Debug, PartialEq)]` is exactly what a record asks for. Python's `@dataclass` is the obvious answer and is deliberately not taken, because the decorator needs an import and a class is generated on its own as readily as inside a file whose imports the AST carries; emitting one would change how that generator writes a file rather than how it writes a class. `IsPartial` is the one dropped in silence, and the reason is what it says. It claims nothing about the type: it is permission to declare the rest of it in another file, and a generator that has written the whole declaration has not used that permission for anything a reader of this file could be missing. Twenty new tests over the seven targets, plus the YAML round trip and the clone. The interfaces are serialized as a sequence rather than one joined string, for the reason the specialisation arguments already are: an interface can have type arguments, so a comma inside one is part of it as often as it separates two — which is also why both lists now read through one `DeserializeTypeList`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGCMUrT3jBgmANHNHcPmBf

[minor] `ClassDeclaration` and `FunctionDeclaration` now carry type parameters, each with the requirements on it. `public readonly partial record struct Mass<T> : IVector0<Mass<T>, T> where T : struct, INumber<T>` is now reachable in full, which was the point. `TypeParameter` and `TypeConstraint` are values rather than nodes, like `SpecialisationArguments` and for the same reason: a type parameter is part of the thing being declared rather than a member of it, so there is nothing in the editor for it to be a node of. `Parse` and `ToString` are inverses, so a document carries a whole parameter — constraints and all — on one line a person can read, and the split respects the brackets because the comma in `IComparer<T, U>` belongs to it rather than separating two constraints. The constraints are where the targets part, and settling that was the work: A parameter's *name* travels everywhere. What a language can say about that name does not. So `TypeConstraintKind` names four intents — implements a type, is a value, is a reference, is constructible — and stops. Those are the ones with a shared idea underneath. A C++ concept is a predicate that can ask anything at all (`requires (T a) { a.begin(); }`), which is the same reason `CompileTimeAssertion.Condition` is text: there is no idea to model, only a language's own way of asking a question. Each target then spells what it has a word for and writes down the rest, the way `WriteTypePromises` already does: - C# spells all four, and *reorders* them. The language requires the class or struct constraint first and `new()` last and rejects any other order; the AST has no reason to know that, and a caller listing them as they think of them should still get a file that compiles. - Rust spells two — a trait bound is exactly `Implements`, `Default` is exactly `Constructible` — and carries the parameters onto every `impl` block it opens: the inherent one, an operator, a conversion and `Drop`. That is the part that had to be right rather than plausible, since `impl Mass` beside a `struct Mass<T>` reads correctly and does not build. `RustGeneratedSourceCompilesTests` now compiles a generic struct whose bound the body depends on, so rustc checks it. - Go spells one, `Implements` being exactly a Go constraint interface, and only for a function. A method on a generic type needs the parameters in three places and spelled two ways — `NewPoint` for the constructor's name but `Point[T]` for its receiver and its result — so a generic type is written down instead, and the interface assertion is suppressed with it rather than asserting something about a name that is not a type. - C++ writes `template <typename T>` and notes every constraint. The standard concepts need an include the AST does not carry for a declaration generated on its own, which is the reason Python does not get its `@dataclass` either. - C, Python and JavaScript write the whole parameter down. Eleven more tests, over what each target writes and over the parse/print round trip, plus the rustc one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGCMUrT3jBgmANHNHcPmBf

[minor] An `Annotation` on a `ClassDeclaration`, a `FunctionDeclaration` or a `FieldDeclaration`. C# calls it an attribute, C++ calls it an attribute and spells it differently, Rust calls it an attribute macro and Python calls it a decorator; the name here is the one that is nobody's keyword. Its `Name` and its `Arguments` are text, written verbatim, for the reason `CallExpression.Callee` is: `[Obsolete]`, `#[serde(rename = "x")]` and `@staticmethod` have nothing underneath them for the AST to hold, and one of them usually means nothing at all in the others. What *is* shared — and is the whole of what the four targets with metadata disagree about — is the syntax around them, so that is what each generator supplies through `SpellAnnotation`. The same split `SpellImport` already makes, and the same shape. C# [SuppressMessage("Usage", "CA2225:…")] C++ [[SuppressMessage("Usage", "CA2225:…")]] Rust #[SuppressMessage("Usage", "CA2225:…")] Python @SuppressMessage("Usage", "CA2225:…") C, JavaScript and Go have no metadata syntax and write the annotation down rather than dropping it, which is what `WriteInexpressible` is for: a file that quietly loses its `[Obsolete]` looks like a file that never had one. The arguments are a sequence rather than one string so that a comma inside an argument stays inside it — `SuppressMessage("Usage", "CA2225:Operator overloads have named alternates")` has two arguments and three commas — and the reader that splits them back out of a document respects quotes and brackets for the same reason. Nine more tests, including the round trip that proves the commas survive it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGCMUrT3jBgmANHNHcPmBf

[minor] A `PropertyDeclaration`: a named, typed member read and written through code rather than stored. C# spells it `T Name { get; set; }`, Python `@property`, JavaScript `get name()`. The other four have the two halves of what it is and no word joining them, which makes the decision here not the syntax but **where a property lands**. It lands in one of two places, and neither is an approximation: A property whose accessors have no bodies IS a field with a storage location the compiler supplies. One with bodies IS a pair of functions. So a target with no properties writes the field, or writes the pair, and both are what a person would have written. That is also why `FieldDeclaration` is not reused for the first: a field says where a value is kept and this says how it is reached, a difference that is invisible in C# and load-bearing everywhere else. The separation happens to the *member list*, in `StandardLanguageGenerator`, and that is the whole of why it works. Rust puts data in a `struct` and behaviour in an `impl`; Go writes fields in the type and methods beside it; C lowers a method to a free function taking the instance. Every one of those routes reads the member list before anything is written, so a property separated at the point it is written arrives after the routing that decides all of that and comes out wherever it happened to be standing — which is what the first attempt did, and what put a member function inside a C struct. Separated first, each generator's existing routing sees an ordinary field or an ordinary function and needs to know nothing about properties at all. Rust pub Value: i32, / pub fn doubled(&self) -> i32 Go Value int / func (self Box) Doubled() int C++ int Value{}; / int doubled() const C int Value; / int Box_doubled(const Box* self) Two things fall out of it. A property may be readable and not writable and a field is neither or both, so the field carries that in its own documentation rather than in a note beside it — which means it travels to wherever the target puts the field. And a separated setter has no name until somebody invents one, so `NamingStyle` gives each target its own convention: rustc warns on a member name that is not snake case, and an unexported Go name cannot be called from outside its package, so two of the four are more than taste. The AST still renames nothing it was given; this is only for the names a generator has to make up. `AstSchema` gives the property two statement slots, `AstFields` its seven editable properties, and the palette an entry — all three of which the exhaustiveness test from the first commit here demanded rather than my remembering. `TryDetachAt` is split in two along the same line the three attach methods already are, the sequences having outgrown one switch. Eighteen more tests, over what each of the seven writes for both kinds and over the round trip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGCMUrT3jBgmANHNHcPmBf

Four things, each from a bot on this PR rather than from taste. The review comment on `ReadAnnotation`: the trim is a mapping and now says so, with the emptiness check moving into the same chain. Behaviour is identical and the loop does one thing. `CGenerator.GenerateClassDeclaration` went over its cognitive-complexity limit when the embedded bases went in, which is fair: the block decides how many there are, which of them is first, what to say about the position, and whether a blank line follows. It is `WriteEmbeddedBases` now, and the paragraph explaining why the first member is the first member lives with the code that puts it there. `AstFields` spelled "ReadOnly" four times. It is a constant beside the three that were already there, and worth one: it is one name for two different promises — on a function that the call does not modify the receiver, on a type that none of its members does — so a reader of the inspector should see the same word for the same idea. And two `Assert.AreEqual(n, x.Count)` become `Assert.HasCount`, which the same file already used three lines away. MSTEST0046 is not adopted, and that is deliberate rather than an oversight: it wants `Assert.Contains` over `StringAssert.Contains`, and this test project is 290 to 111 the other way. Sonar reports it as INFO and the quality gate passes with it outstanding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGCMUrT3jBgmANHNHcPmBf

Both are on code this PR added, and both were worth taking. `TryWriteDeclaration` came out at cognitive complexity 20 against a limit of 15, and every point of it was one `&&`: a `TryParse(…) && Assign(…)` pair per field. There are forty such pairs across the three switches, so the shape was going to keep pushing a method over the line each time nodes were added — it is what forced the last split. Parsing and writing are now one call named for the kind of field, which takes that method to 1 and the other two with it. Four names rather than four overloads, because IDE0350 wants an inferred lambda parameter and an overload set is exactly what stops one being inferred. Every enumeration now reads case-insensitively. Visibility already did, with a comment saying why — a document hand-edited with "public" should read back the same as the inspector's own "Public" — and there was no reason for it to be the one that did. `JavaScriptGenerator` spelled `"static "` five times; it is a constant now. 843/843 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGCMUrT3jBgmANHNHcPmBf

sonarqubecloud · 2026-09-12T13:50:46Z

Quality Gate passed

Issues
46 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

claude added 2 commits September 12, 2026 12:32

matt-edmondson changed the title ~~Make a node type that misses AstSchema or AstFields fail a test~~ Enforce AST node coverage, then teach a type declaration what it implements Sep 12, 2026

claude added 2 commits September 12, 2026 13:02

github-code-quality Bot found potential problems Sep 12, 2026

View reviewed changes

Comment thread Coder/Serialization/YamlDeserializer.cs Fixed

matt-edmondson changed the title ~~Enforce AST node coverage, then teach a type declaration what it implements~~ Enforce AST node coverage, then close the declaration-level gaps Sep 12, 2026

claude added 2 commits September 12, 2026 13:24

matt-edmondson merged commit 15a3a1e into main Sep 13, 2026
12 checks passed

matt-edmondson deleted the claude/bold-planck-mxarux branch September 13, 2026 00:07

matt-edmondson mentioned this pull request Sep 13, 2026

MemberInitialiser and ConstructionExpression have no editable fields, so their name and type cannot be set from the editor #48

Closed

Navigation Menu

Uh oh!

Enforce AST node coverage, then close the declaration-level gaps - #60

Enforce AST node coverage, then close the declaration-level gaps#60
matt-edmondson merged 7 commits into
mainfrom
claude/bold-planck-mxarux

matt-edmondson commented Sep 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

sonarqubecloud Bot commented Sep 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

matt-edmondson commented Sep 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Make a node type that misses AstSchema or AstFields fail a test (closes #49)

2. What a type declaration says about itself

3. The types a declaration is written over

4. Metadata

5. Properties, and where one lands

6. This PR's own review comment and findings

7. The two Sonar findings that followed

Uh oh!

Uh oh!

sonarqubecloud Bot commented Sep 12, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

matt-edmondson commented Sep 12, 2026 •

edited

Loading

1. Make a node type that misses `AstSchema` or `AstFields` fail a test (closes #49)