refactor: MemberDefinitions cleanup — AbstractSlot review + sweep#178
Merged
Conversation
Addresses unaddressed review feedback on PR #176 and the audit findings on AbstractSlot.fs, with parallel cleanup in AutoProperty.fs: - Drop the duplicate AbstractMemberModifiers extension methods that shadowed MemberDefnModifiers' canonical xmlDocs / attributes / attribute / toStatic for WidgetBuilder<MemberDefn>. The duplicates only "worked" because AbstractSlot.IsStatic aliased BindingNode.IsStatic, hiding the cross-widget coupling Copilot flagged. AbstractSlot now reads BindingNode.IsStatic directly at the call site so the canonical-key dependency is explicit. - Materialize the attributes seq with Seq.toArray at MemberDefnModifiers.attributes to eliminate the lazy Seq.map Gen.mkOak footgun (Copilot finding). - Extract MultipleTextsNode.CreateGetSet to Common.fs, collapsing ~50 duplicate lines of property-accessor rendering shared by AbstractSlot and AutoProperty. - Add ValueOption.toOption helper to Common.fs (FSharp.Core 8 lacks it); replace verbose `ValueOption.map Some |> defaultValue None` pipelines in both files. - AbstractSlot: materialize parameters with List.ofSeq before List.mapi so the inner Seq.length is O(1) and the source seq is walked exactly once (no lazy re-evaluation). - AbstractSlot: rename the HasGetterSetter scalar from "HasGetter" to "HasGetterSetter" so the debug name matches the stored type. - AbstractSlot: move the named-parameter empty-name check from a failwith deep in the widget compiler to invalidArg at builder construction time (in the funnel overload that all named-parameter variants delegate to). - AbstractSlot: flatten `Ast.LongIdent(tp) |> fun tp -> name, tp` to `name, Ast.LongIdent(tp)`; standardize defaultArg ordering across the eight AbstractMember overloads. - AutoProperty: drop dead IsStatic scalar (reader uses the canonical BindingNode.IsStatic). - Add tests covering `static abstract` properties and the named- parameter validation.
…gMember acc Follow-up to the audit of Widgets/MemberDefinitions/. Addresses the findings outside AbstractSlot/AutoProperty: - PropertyGetSet: delete four dead scalar definitions (IsInlined, MultipleAttributes, IsStatic, Accessibility). None were ever read in the WidgetKey — readers use the canonical BindingNode.* and MemberDefn.* keys. Same diagnosis as the AutoProperty.IsStatic dead-code finding. - PropertyGetSet: replace `tryGetNodeFromWidget ... + failwith "Getter is required"` with `getNodeFromWidget` for the required FirstBindingWidget. Eliminates the misplaced render-time validation; missing attribute now surfaces at the standard widget-attribute level rather than as a custom string. - Apply ValueOption.toOption sweep to PropertyGetSet, Method, ExplicitConstructor, ExternBinding, and Field (both Field and ValField widget keys). Replaces ~16 sites of the verbose `ValueOption.map Some |> ValueOption.defaultValue None` idiom and drops redundant `Some(...)` wrappers inside the map closure. - SigMember: add accessibility support to the get/set scalars. F# accepts `abstract Foo: int with public get, internal set` on signature members, but the SigMember builder previously only exposed bare booleans. Change HasGetter/HasSetter scalars from `bool` to `bool * AccessControl`, add optional getterAccessibility/setterAccessibility parameters to the builder, and route through the shared MultipleTextsNode.CreateGetSet helper so the rendering logic matches AbstractSlot/AutoProperty. Existing call sites (which only pass identifier ± hasGetter/hasSetter) continue to compile unchanged.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors src/Fabulous.AST/Widgets/MemberDefinitions/ to remove dead/duplicated widget attributes, centralize repeated rendering logic for property accessors, and improve validation/test coverage around abstract slots and signature members.
Changes:
- Extracts shared
with get/setrendering intoMultipleTextsNode.CreateGetSetand sweeps multiple member-definition widgets to use it. - Removes unused/dead scalar attributes (and redundant extension methods) and replaces verbose
voption -> optionconversions with a helper. - Extends
SigMemberto support accessor-level accessibility and adds regression tests forstatic abstractproperties + named-parameter validation.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Fabulous.AST/Widgets/MemberDefinitions/SigMember.fs | Changes getter/setter scalars to include AccessControl and routes rendering through the shared get/set helper. |
| src/Fabulous.AST/Widgets/MemberDefinitions/PropertyGetSet.fs | Deletes unused scalar definitions and simplifies required getter retrieval + voption handling. |
| src/Fabulous.AST/Widgets/MemberDefinitions/Method.fs | Simplifies voption mapping patterns using the new toOption helper. |
| src/Fabulous.AST/Widgets/MemberDefinitions/Field.fs | Simplifies voption mapping patterns using the new toOption helper (Field + ValField). |
| src/Fabulous.AST/Widgets/MemberDefinitions/ExternBinding.fs | Simplifies voption mapping patterns using the new toOption helper. |
| src/Fabulous.AST/Widgets/MemberDefinitions/ExplicitConstructor.fs | Simplifies voption mapping patterns using the new toOption helper. |
| src/Fabulous.AST/Widgets/MemberDefinitions/AutoProperty.fs | Removes dead IsStatic scalar and reuses the shared get/set helper. |
| src/Fabulous.AST/Widgets/MemberDefinitions/AbstractSlot.fs | Cleans up parameter rendering/perf, clarifies static handling, and moves named-parameter validation to builder-time. |
| src/Fabulous.AST/Widgets/MemberDefinitions/_BindingNode.fs | Materializes attributes sequence to avoid lazy-seq pitfalls. |
| src/Fabulous.AST/Widgets/Common.fs | Adds shared CreateGetSet helper + introduces ValueOption.toOption. |
| src/Fabulous.AST.Tests/MemberDefinitions/AbstractSlot.fs | Adds tests for static abstract property accessors and named-parameter validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add `net10.0` to the main library's TargetFrameworks (`net8.0;net10.0;netstandard2.1`) and to the test project's TargetFrameworks (`net8.0;net10.0`). - Bump FSharp.Core from 8.0.403 to 10.1.300 in Directory.Packages.props. - Add an explicit `<PackageReference Include="FSharp.Core" />` to src/Fabulous.AST/Fabulous.AST.fsproj. Without this, NuGet ignored the CPM pin and resolved FSharp.Core transitively through Fantomas.Core (minimum 8.0.100), so the existing pin was effectively dead. - Remove the custom `ValueOption.toOption` helper from Common.fs that was introduced in commit 5fe5f96. FSharp.Core 9.0+ ships `ValueOption.toOption` natively (added in dotnet/fsharp#17436), so the helper is now redundant and its module name collided with `Microsoft.FSharp.Core.ValueOption`. Pipeline (`dotnet fsi build.fsx`) green on all stages: lint, build (produces net8.0/net10.0/netstandard2.1 artifacts), test (780/780 on both net8.0 and net10.0), docs, pack.
samples/Playground.fsproj imports Fabulous.AST.Build.targets (which declares the FabulousAstJsonTask via <UsingTask>) but had no ProjectReference to Fabulous.AST.Build. With the solution built in parallel, Playground frequently evaluated the UsingTask before Fabulous.AST.Build.dll existed on disk, causing the task to silently fail to register and then crash with MSB4036 when the generation target invoked it. The race was already latent — it hit macOS-latest on PR #178's first run (then masked by a retry). Adding net10.0 to Fabulous.AST and Fabulous.AST.Tests added more parallel build edges, making the race deterministic on both macOS and Windows. Fix: add a ProjectReference from Playground to Fabulous.AST.Build with `ReferenceOutputAssembly="false"` so MSBuild enforces the build order without including the task assembly in Playground's runtime references.
The previous attempt to fix the FabulousAstJsonTask race by adding a ProjectReference from Playground to Fabulous.AST.Build made things worse — all three OS jobs failed on PR #178. Diagnosis: the <UsingTask Condition="Exists(...)"> in Fabulous.AST.Build.targets is evaluated at project-load time (when MSBuild parses Playground's imports), not at target-execution time. A ProjectReference enforces build order *during* a build, but the targets file is imported as part of project evaluation, which happens before any building. On a fresh checkout the task DLL doesn't exist yet, the UsingTask silently fails to register, and the generation target later crashes with MSB4036. Real fix: invoke `dotnet build` on Fabulous.AST.Build as a separate step in build.fsx before the solution build. The task DLL is then on disk when MSBuild loads Playground.fsproj for the main build. Verified locally by clearing all bin/obj directories and running the pipeline cold — green on the first attempt.
Summarizes the changes in this branch under [Unreleased]: - net10.0 / FSharp.Core 10.1.300 support - SigMember accessibility additions - builder-time validation for empty named-parameter names - shared withGetSetText helper, lazy-seq fix - duplicate AbstractMemberModifiers extensions removed - dead scalar definitions removed - parallel-build race fix in build.fsx
This was referenced May 30, 2026
edgarfgp
added a commit
that referenced
this pull request
May 30, 2026
Mechanical cleanup applying the patterns established in PR #178 to the rest of the Widgets/ tree. - Replace ~46 sites of `ValueOption.map Some |> defaultValue None` (in all four observed variants: bare `Some`, `map(Some)`, `map(fun x -> Some(x))`, `map(fun x -> Some(F x))`) with the canonical `ValueOption.toOption` form. Where the inner Some-wrapped expression was a simple constructor application, the `Some(F x)` is flattened to `F x` and chained into `ValueOption.toOption`. Two unusual sites (PropertyGetSetBinding's `Option<Option<...>>` collapse, and RecordField's multi-statement map) are rewritten to use direct `match` or simplified `map`+`toOption` accordingly. - Materialize ~28 lazy `Seq.map Gen.mkOak` sequences with `|> Seq.toArray` so the resulting `seq` stored in widget attributes isn't re-walked on each iteration and isn't sensitive to whether the source is a single-shot enumerable. Same fix the PR #178 audit applied to MemberDefnModifiers.attributes; just at scale. Pipeline green: lint, build (net8.0/net10.0/netstandard2.1), tests (780/780 on both TFMs), docs, pack.
This was referenced May 30, 2026
Merged
edgarfgp
added a commit
that referenced
this pull request
May 30, 2026
Updates the main CHANGELOG with the six PRs merged since 2.0.0-pre06: - #178: AbstractSlot/AutoProperty cleanup, net10.0 / FSharp.Core 10.1.300 target, shared CreateGetSet helper, parallel-build race fix - #179: three audit bug fixes (Delegate lazy-seq, ObjExpr failwith, Measure scalar-name typo) - #180: voption / lazy-seq sweep across all remaining widgets - #181: 21 specialized tests under src/Fabulous.AST.Tests/Specialized/ - #182: .toRecursive() no-op fix on Abbrev/Enum/TypeDefnExplicit/Measure - #183: new .toOverride() modifier for member definitions Extension CHANGELOGs (Fabulous.AST.Build, Fabulous.AST.Json) get a '2.0.0-pre07 - No changes' bump to keep versions aligned across the package family, following the cadence established from pre03.
edgarfgp
added a commit
that referenced
this pull request
May 30, 2026
…kflow (#185) The release workflow calls `dotnet build` directly on the solution, so it doesn't benefit from the pre-build step that `build.fsx` got in #178. Caused MSB4036 "FabulousAstJsonTask was not found" on the 2.0.0-pre07 release run because Playground's <UsingTask> in Fabulous.AST.Build.targets was evaluated before Fabulous.AST.Build.dll existed on the runner. Same fix as #178 / commit d17fa7b for build.fsx: build Fabulous.AST.Build first so the task DLL is on disk when MSBuild loads Playground.fsproj for the main build.
edgarfgp
added a commit
that referenced
this pull request
May 30, 2026
The release workflow's `dotnet pack ... -p:PackageReleaseNotes="${{
steps.changelog_reader.outputs.changes }}"` interpolates the literal
markdown content of the [version] changelog section directly into the
bash script body before bash parses it. Any backticks in the content
trigger command substitution; parens, semicolons, and other
metacharacters fragment the command into nonsense fragments. Failed
the 2.0.0-pre07 release because the new changelog entries are
multi-line and contain backticks (`FSharp.Core`, `Seq.length`, etc.)
and parens (`(#178)`).
Fix: pass the changelog content via env vars (RELEASE_NOTES,
JSON_RELEASE_NOTES, BUILD_RELEASE_NOTES). bash sees the variable
reference and substitutes the value at runtime as data, not as script
text — special characters are preserved without interpretation.
Failed run: https://github.com/edgarfgp/Fabulous.AST/actions/runs/26695038633/job/78678004419
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two-commit cleanup of
src/Fabulous.AST/Widgets/MemberDefinitions/covering:Net: −240 lines across 11 files, 1 new test, no behavioral regressions (780/780 tests pass, full
build.fsxpipeline green: lint + build + test + docs + pack).Commit 1 — AbstractSlot/AutoProperty cleanup + shared helper
AbstractMemberModifiersextension methods that shadowedMemberDefnModifiers.xmlDocs / attributes / attribute / toStatic. They only worked becauseAbstractSlot.IsStaticaliasedBindingNode.IsStatic, masking the cross-widget coupling Copilot flagged.AbstractSlotnow readsBindingNode.IsStaticdirectly at the call site so the canonical-key dependency is explicit.Seq.toArrayinMemberDefnModifiers.attributes(Copilot finding).MultipleTextsNode.CreateGetSettoCommon.fs, collapsing ~50 duplicate lines of property-accessor rendering shared betweenAbstractSlotandAutoProperty.ValueOption.toOptionhelper toCommon.fs(missing from FSharp.Core 8), replacing the verboseValueOption.map Some |> defaultValue Noneidiom across both files.AbstractSlot: materialize parameters withList.ofSeqbeforeList.mapi— eliminates the O(n²)Seq.lengthre-evaluation and the lazy-seq footgun.AbstractSlot: renameHasGetterSetterscalar from"HasGetter"to"HasGetterSetter"so the debug name matches the stored type.AbstractSlot: move the named-parameter empty-name check from afailwithin the widget compiler toinvalidArgat builder construction time.AbstractSlot: flatten the awkwardAst.LongIdent(tp) |> fun tp -> name, tp; standardizedefaultArgordering across the eightAbstractMemberoverloads.AutoProperty: drop deadIsStaticscalar (reader uses the canonicalBindingNode.IsStatic).static abstractproperties and the new named-parameter validation.Commit 2 — Broader MemberDefinitions sweep
IsInlined,MultipleAttributes,IsStatic,Accessibility). None were ever read in the WidgetKey — readers use the canonicalBindingNode.*/MemberDefn.*keys. Same diagnosis as theAutoProperty.IsStaticfinding.tryGetNodeFromWidget … + failwith "Getter is required"withgetNodeFromWidgetfor the requiredFirstBindingWidget. Missing attribute now surfaces at the standard widget-attribute level instead of a custom string deep in render.ValueOption.toOptionsweep across PropertyGetSet, Method, ExplicitConstructor, ExternBinding, and Field (both Field and ValField widget keys) — replaces ~16 sites of the verbose idiom and drops redundantSome(...)wrappers insidemapclosures.abstract Foo: int with public get, internal seton signature members but the builder previously only exposed bare booleans. ChangeHasGetter/HasSetterscalars frombooltobool * AccessControl, add optionalgetterAccessibility/setterAccessibilityparameters, and route through the sharedMultipleTextsNode.CreateGetSethelper. Existing call sites continue to compile unchanged (new optional params).Verification
dotnet fsi build.fsx(the project's CI pipeline) — all stages green: lint, build, test (780/780), docs, pack.AbstractSlot.fstest file cover the static-abstract-property render path and the builder-time name validation.Note
PR #177 (the earlier "address PR #176 review feedback" branch) is superseded by this PR and can be closed. The work was rebased onto a fresh branch from
mainto clean up history.