Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ListCollector #3745

Merged
merged 8 commits into from Feb 9, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion src/Fable.Transforms/Replacements.Util.fs
Expand Up @@ -322,7 +322,6 @@ let (|BuiltinDefinition|_|) =
| Types.dateOnly -> Some BclDateOnly
| Types.timeOnly -> Some BclTimeOnly
| "System.Timers.Timer" -> Some BclTimer
| Types.decimal
| Types.fsharpSet -> Some(FSharpSet(Any))
| Types.fsharpMap -> Some(FSharpMap(Any, Any))
| Types.hashset -> Some(BclHashSet(Any))
Expand Down
3 changes: 3 additions & 0 deletions src/Fable.Transforms/Replacements.fs
Expand Up @@ -1148,6 +1148,8 @@ let tryEntityIdent (com: Compiler) entFullName =
makeImportLib com Any "AsyncReplyChannel" "AsyncBuilder" |> Some
| "Microsoft.FSharp.Control.FSharpEvent`1" -> makeImportLib com Any "Event" "Event" |> Some
| "Microsoft.FSharp.Control.FSharpEvent`2" -> makeImportLib com Any "Event$2" "Event" |> Some
| "Microsoft.FSharp.Core.CompilerServices.ListCollector`1" ->
makeImportLib com Any "ListCollector$1" "FSharp.Core.CompilerServices" |> Some
| _ -> None

let tryConstructor com (ent: Entity) =
Expand Down Expand Up @@ -4566,6 +4568,7 @@ let private replacedModules =
"Microsoft.FSharp.Collections.ListModule", listModule
"Microsoft.FSharp.Collections.HashIdentity", fsharpModule
"Microsoft.FSharp.Collections.ComparisonIdentity", fsharpModule
"Microsoft.FSharp.Core.CompilerServices.ListCollector`1", bclType
"Microsoft.FSharp.Core.CompilerServices.RuntimeHelpers", seqModule
"Microsoft.FSharp.Collections.SeqModule", seqModule
Types.keyValuePair, keyValuePairs
Expand Down
17 changes: 17 additions & 0 deletions src/fable-library/FSharp.Core.CompilerServices.fs
@@ -0,0 +1,17 @@
namespace Microsoft.FSharp.Core.CompilerServices

[<NoEquality; NoComparison>]
type ListCollector<'T>() =
let collector = ResizeArray<'T>()

member this.Add(value: 'T) = collector.Add(value)

member this.AddMany(values: seq<'T>) = collector.AddRange(values)

// In the particular case of closing with a final add of an F# list
// we can simply stitch the list into the end of the resulting list
member this.AddManyAndClose(values: seq<'T>) =
collector.AddRange(values)
Seq.toList collector

member this.Close() = Seq.toList collector
1 change: 1 addition & 0 deletions src/fable-library/Fable.Library.fsproj
Expand Up @@ -33,6 +33,7 @@
<Compile Include="Random.fs" />
<Compile Include="Set.fs" />
<Compile Include="Map.fs" />
<Compile Include="FSharp.Core.CompilerServices.fs" />
<TypeScriptCompile Include="Async.ts" />
<TypeScriptCompile Include="AsyncBuilder.ts" />
<TypeScriptCompile Include="BitConverter.ts" />
Expand Down
1 change: 1 addition & 0 deletions tests/Js/Main/Fable.Tests.fsproj
Expand Up @@ -78,6 +78,7 @@
<Compile Include="UriTests.fs" />
<Compile Include="TimeOnlyTests.fs" />
<Compile Include="TimeSpanTests.fs" />
<Compile Include="ListCollectorTests.fs" />
<Compile Include="Main.fs" />
</ItemGroup>

Expand Down
26 changes: 26 additions & 0 deletions tests/Js/Main/ListCollectorTests.fs
@@ -0,0 +1,26 @@
module Fable.Tests.ListCollector

open Util.Testing
open Fable.Tests.Util
open Microsoft.FSharp.Core.CompilerServices

let cutOffLast list =
let mutable headList = ListCollector<'a>()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code it seems like we can remove the mutable here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would not reflect the actual usage of shared code. In regular F# the collector needs to be mutable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know about that.


let rec visit list =
match list with
| []
| [ _ ] -> ()
| head :: tail ->
headList.Add(head)
visit tail

visit list
headList.Close()

let tests =
testList "ListCollector" [
testCase "ListCollector.Add and .Close" <| fun () ->
let result = cutOffLast [ 1; 2; 3 ]
result |> equal [ 1; 2 ]
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add tests for each method available and also edges cases like calling .close() on an empty ListCollector?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not overextend. I'm including only the parts that interest me, as this is largely experimental. The code is already solid—something we're both aware of—and this pull request improves its current state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree yes for this module it is probably enough, but it happens several times that doing so allowed us to catch behaviour we were not aware of.

There are still some APIs in fable-library that don't behave correctly because they didn't have a test associated for every usage. Having tests also allows us to catch on regressions.

I added the tests to cover the most common cases.

1 change: 1 addition & 0 deletions tests/Js/Main/Main.fs
Expand Up @@ -50,6 +50,7 @@ let allTests =
TypeTests.tests
UnionTypes.tests
Uri.tests
ListCollector.tests
|]

#if FABLE_COMPILER
Expand Down