Module functions for `'T option` ↔ `'T voption` by brianrourkeboll · Pull Request #17436 · dotnet/fsharp

brianrourkeboll · 2024-07-23T17:49:33Z

Description

Add module functions for converting between 'T option and 'T voption.
~~Remove the multiple existing internal copies of these functions in this repo and replace their usage with the FSharp.Core ones.~~ See Module functions for 'T option ↔ 'T voption #17436 (comment).

moduleOption =val inlineofValueOption:voption :'T voption ->'T optionval inlinetoValueOption:option :'T option ->'T voption

moduleValueOption =val inlineofOption:option :'T option ->'T voptionval inlinetoOption:voption :'T voption ->'T option

Checklist

Test cases added.
Release notes entry updated.

Notes

It seems to me like this ought to have been part of fsharp/fslang-suggestions#703, as @Thorium pointed out in fsharp/fslang-suggestions#703 (comment).

I can open a new language suggestion and RFC for this if needed — but perhaps we could just update RFC FS-1065 instead?

github-actions · 2024-07-23T17:50:15Z

❗ Release notes required

✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/FSharp.Core docs/release-notes/.FSharp.Core/9.0.100.md

brianrourkeboll · 2024-07-23T18:07:30Z

It looks like this bit of the CI

fsharp/azure-pipelines-PR.yml

Line 636 in 2eab49c

 - script: dotnet build ./FSharp.Compiler.Service.sln /bl:\"artifacts/log/$(_BuildConfig)/ServiceRegularBuild.binlog\"

means that f4ca9f4 might need to go in a separate PR?

…option

psfinaki · 2024-07-24T10:40:11Z

/azp run

azure-pipelines · 2024-07-24T10:40:26Z

Azure Pipelines successfully started running 2 pipeline(s).

psfinaki · 2024-07-24T12:28:55Z

@brianrourkeboll, I don't see how the CI stuff can be directly related here... can you maybe rerevert the changes so that we can investigate a bit? It would be nice to get rid of all those dupes of this. And yeah, our CI is quite flaky recently.

brianrourkeboll · 2024-07-24T12:55:41Z

@psfinaki The CI failed when it was directly invoking dotnet build on FSharp.Compiler.Service.sln (the "Regular rebuild of FSharp.Compiler.Service.sln" step in the Plain_Build_* runs), which meant that it wasn't using the modified version of FSharp.Core.

Since f4ca9f4 replaced the internal functions with calls to the new ones in FSharp.Core, but the new version of FSharp.Core wasn't being used, it wouldn't compile.

I believe this was the run: https://dev.azure.com/dnceng-public/public/_build/results?buildId=752388&view=results

Unfortunately, I guess my cancelling of the run by force-pushing cleared the results away.

psfinaki · 2024-07-24T13:03:11Z

Right, yeah I was looking at the wrong results then.
That's clear, I think we can remove the dupe in a followup.

psfinaki

@vzarytovskii @abelbraaksma should we update the RFC (before or after the merge)?

T-Gro · 2024-07-26T11:52:45Z

Thanks for this addition.
I will merge this in. @brianrourkeboll : Could you please update the existing RFC FS-1065 with the added signatures?

psfinaki · 2024-07-29T13:27:54Z

Thanks for the RFC as well @brianrourkeboll. Would be cool to remove all the dupes from the codebase now. If you don't have time for this, I can probably create a PR from the commit with those changes that you had in this PR.

brianrourkeboll · 2024-07-29T13:58:57Z

@psfinaki All right, I opened #17457.

If the CI step mentioned in #17436 (comment) is using a shipped version of FSharp.Core, though, then this change may need to wait (or we may need to modify that step, depending on its intent).

- 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.

* 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

brianrourkeboll added 3 commits July 23, 2024 12:58

Add functions for 'T option ↔ 'T voption
ae686f5

Add tests
16f0a9e

Update FSharp.Core surface area
59fd141

Update release notes
345a440

brianrourkeboll force-pushed the option-voption branch from 8060398 to 345a440Compare July 23, 2024 19:06

brianrourkeboll added 2 commits July 23, 2024 15:54

Remove for now
0960281

Merge branch 'main' ofhttps://github.com/dotnet/fsharpinto option-v…
4f4cbc7
…option

brianrourkeboll marked this pull request as ready for review July 23, 2024 20:45

brianrourkeboll requested a review from a team as a code owner July 23, 2024 20:45

psfinaki approved these changes Jul 24, 2024
View reviewed changes

edgarfgp approved these changes Jul 24, 2024
View reviewed changes

T-Gro approved these changes Jul 26, 2024
View reviewed changes

T-Gro merged commit 302169c into dotnet:mainJul 26, 2024

brianrourkeboll deleted the option-voption branch July 26, 2024 12:31

brianrourkeboll mentioned this pull request Jul 26, 2024
[RFC FS-1065.1] Addendum to RFC FS-1065: 'T option ↔ 'T voption conversion functions fsharp/fslang-design#779
Merged

brianrourkeboll mentioned this pull request Jul 29, 2024
Replace internal option conversion funcs with FSharp.Core ones #17457
Closed

This was referenced Jul 29, 2024
Cleanup: replace internal option conversion funcs with FSharp.Core ones once shipped #17460
Closed
ValueOption parity with options fsharp/fslang-suggestions#703
Closed

This was referenced Jul 31, 2024
[Automated] PRs inserted in VS build main-35130.30 #17468
Closed
[Automated] PRs inserted in VS build feature.debugger.main-35201.175 #17483
Closed

dotnet-bot mentioned this pull request Aug 28, 2024
[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35227.190 #17620
Closed

brianrourkeboll mentioned this pull request Jan 25, 2025
Replace internal option–voption conversions with FSharp.Core funcs #18269
Merged

This was referenced Sep 8, 2025
Bump FSharp.Core from 8.0.100 to 9.0.303 NicoVIII/RunHelpers#22
Closed
Bump FSharp.Core from 8.0.100 to 9.0.303 NicoVIII/SimpleOptics#10
Closed

This was referenced Oct 10, 2025
Bump FSharp.Core from 4.5.0 to 9.0.303 G-Research/TypeEquality#35
Closed
Bump FSharp.Core from 4.5.0 to 9.0.303 G-Research/TypeEquality#37
Closed

dependabotBot mentioned this pull request Oct 27, 2025
Bump FSharp.Core from 8.0.100 to 9.0.303 NicoVIII/RunHelpers#25
Closed

Navigation Menu

Uh oh!

Module functions for `'T option` ↔ `'T voption` - #17436

Module functions for `'T option` ↔ `'T voption`#17436
T-Gro merged 6 commits into
dotnet:mainfrom
brianrourkeboll:option-voption

brianrourkeboll commented Jul 23, 2024•
edited
Loading

Uh oh!

github-actionsBot commented Jul 23, 2024•
edited
Loading

Uh oh!

brianrourkeboll commented Jul 23, 2024•
edited
Loading

Uh oh!

psfinaki commented Jul 24, 2024

Uh oh!

azure-pipelinesBot commented Jul 24, 2024

Uh oh!

psfinaki commented Jul 24, 2024

Uh oh!

brianrourkeboll commented Jul 24, 2024•
edited
Loading

Uh oh!

psfinaki commented Jul 24, 2024•
edited
Loading

Uh oh!

psfinaki left a comment

Uh oh!

T-Gro commented Jul 26, 2024

Uh oh!

psfinaki commented Jul 29, 2024

Uh oh!

brianrourkeboll commented Jul 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

brianrourkeboll commented Jul 23, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Notes

Uh oh!

github-actionsBot commented Jul 23, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❗ Release notes required

Uh oh!

brianrourkeboll commented Jul 23, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psfinaki commented Jul 24, 2024

Uh oh!

azure-pipelinesBot commented Jul 24, 2024

Uh oh!

psfinaki commented Jul 24, 2024

Uh oh!

brianrourkeboll commented Jul 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psfinaki commented Jul 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psfinaki left a comment

Choose a reason for hiding this comment

Uh oh!

T-Gro commented Jul 26, 2024

Uh oh!

psfinaki commented Jul 29, 2024

Uh oh!

brianrourkeboll commented Jul 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brianrourkeboll commented Jul 23, 2024•
edited
Loading

github-actionsBot commented Jul 23, 2024•
edited
Loading

brianrourkeboll commented Jul 23, 2024•
edited
Loading

brianrourkeboll commented Jul 24, 2024•
edited
Loading

psfinaki commented Jul 24, 2024•
edited
Loading