feat: add support for abstract slots to be static; fix xml docs for static slots - Fixes #175 - #176
Merged
Merged
Conversation
fix: xmldocs do not require direct reference to AbstractMemberModifiers to correctly render
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for marking abstract slots as static and corrects/aligns XML docs handling for abstract slots, plus new modifier extensions and regression tests.
Changes:
- Reuses
MemberDefn.XmlDocsforAbstractSlot.XmlDocsand updates rendering to includestatic abstractwhen requested. - Adds
attributes/attribute/toStaticextensions toAbstractMemberModifiers. - Adds tests covering
toStaticoutput and XML documentation output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Fabulous.AST/Widgets/MemberDefinitions/AbstractSlot.fs | Adds IsStatic handling in abstract slot rendering; aligns XmlDocs attribute; introduces new modifier extensions. |
| src/Fabulous.AST.Tests/MemberDefinitions/AbstractSlot.fs | Adds test coverage for static abstract and XML docs rendering to prevent regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let Identifier = Attributes.defineScalar<string> "Identifier" | ||
| let ReturnType = Attributes.defineWidget "Type" | ||
| let Parameters = Attributes.defineScalar<MethodParamsType> "Parameters" | ||
| let IsStatic = BindingNode.IsStatic |
There was a problem hiding this comment.
AbstractSlot.IsStatic is currently aliased to BindingNode.IsStatic, which couples abstract-slot member rendering to a binding-node attribute key and risks accidental key reuse/collisions across unrelated widgets. Prefer defining a dedicated IsStatic scalar for abstract slots (or adding a MemberDefn.IsStatic attribute if that’s the intended shared contract) and have toStatic set that dedicated key.
Suggested change
| let IsStatic = BindingNode.IsStatic | |
| let IsStatic = Attributes.defineScalar<bool> "AbstractSlotIsStatic" |
Comment on lines
+695
to
+696
| static member toStatic(this: WidgetBuilder<MemberDefn>) = | ||
| this.AddScalar(AbstractSlot.IsStatic.WithValue(true)) |
There was a problem hiding this comment.
AbstractSlot.IsStatic is currently aliased to BindingNode.IsStatic, which couples abstract-slot member rendering to a binding-node attribute key and risks accidental key reuse/collisions across unrelated widgets. Prefer defining a dedicated IsStatic scalar for abstract slots (or adding a MemberDefn.IsStatic attribute if that’s the intended shared contract) and have toStatic set that dedicated key.
| /// <param name="attributes">The attributes to set.</param> | ||
| [<Extension>] | ||
| static member attributes(this: WidgetBuilder<MemberDefn>, attributes: WidgetBuilder<AttributeNode> seq) = | ||
| this.AddScalar(MemberDefn.MultipleAttributes.WithValue(attributes |> Seq.map Gen.mkOak)) |
There was a problem hiding this comment.
attributes |> Seq.map Gen.mkOak is lazy; depending on how WithValue stores/consumes the sequence, this can lead to repeated enumeration or surprises if the input sequence is not stable. Consider materializing to an array/list before storing (e.g., map then Seq.toList/Seq.toArray) to ensure deterministic, single-pass evaluation.
Suggested change
| this.AddScalar(MemberDefn.MultipleAttributes.WithValue(attributes |> Seq.map Gen.mkOak)) | |
| this.AddScalar( | |
| MemberDefn.MultipleAttributes.WithValue( | |
| attributes |> Seq.map Gen.mkOak |> Seq.toArray | |
| ) | |
| ) |
Comment on lines
+690
to
+695
| /// <summary> | ||
| /// Sets the current member definition widget to be static. | ||
| /// </summary> | ||
| /// <param name="this">Current widget.</param> | ||
| [<Extension>] | ||
| static member toStatic(this: WidgetBuilder<MemberDefn>) = |
There was a problem hiding this comment.
The XML doc for toStatic says it applies to a generic member definition widget, but the implementation is wired specifically to abstract-slot rendering via AbstractSlot.IsStatic. It would be clearer to document that this modifier is intended for abstract members/slots (and may have no effect on other MemberDefn kinds).
Contributor
Author
|
The original PR is suggested based on the current implementation which diverts to other defined attributes; it maintains the current pattern but corrects the implementation to utilise it appropriately. The suggested changes imply separating the abstract slot attribute key definitions from the bindingnode definitions. Overall I think the current PR maintains backwards compatibility the best (although the current implementation pre the PR does not correctly render the operations) |
edgarfgp
approved these changes
Feb 14, 2026
edgarfgp
left a comment
edgarfgp
left a comment
Owner
There was a problem hiding this comment.
Thanks. Will back ported to v1
3 tasks
edgarfgp
added a commit
that referenced
this pull request
May 30, 2026
Make the cross-widget dependency explicit at the call site rather than hiding it behind a local IsStatic alias. This aligns with Copilot's intent in the PR #176 review by surfacing exactly which canonical attribute key abstract-slot rendering consults. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
edgarfgp
added a commit
that referenced
this pull request
May 30, 2026
* refactor: AbstractSlot/AutoProperty cleanup + shared helper 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. * refactor: MemberDefinitions sweep — dead scalars, voption cleanup, SigMember 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. * build: target net10.0 and bump FSharp.Core to 10.1.300 - 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. * build: declare Playground -> Fabulous.AST.Build dependency 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. * build: pre-build Fabulous.AST.Build before solution build 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. * docs: add CHANGELOG entry for PR #178 cleanup 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 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.
Description
AbstractSlot XmlDocs attribute references the MemberDefn XmlDocs attribute so that the modifier can be used without explicit reference to
AbstractMemberModifiersextension method.The PR also adds a reference to
IsStatic, and rounds out the explicitAbstractMemberModifiersby adding explicit extensions for xmldocs, attributes, and toStatic.Tests are added to prevent regression for xml docs, and to confirm functionality of toStatic.
Fixes #175