Skip to content

Fix internal Id type being selected in overload resolution #633

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.

Sign up for GitHub

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

Merged
gusty merged 2 commits into from
Jun 12, 2025
Merged

Fix internal Id type being selected in overload resolution #633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fca4d37
+ Failing test
gusty Jun 10, 2025
2268d29
Fix by adding ambiguity
gusty Jun 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/FSharpPlus/Control/Collection.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,7 @@ type DistinctBy =

type GroupBy =
static member GroupBy (x: Id<'T> , f: 'T->'Key, _: Id<'Key*Id<'T>> , [<Optional>]_impl: GroupBy) = let a = Id.run x in Id.create (f a, x)
static member GroupBy (x: Id2<'T> , f: 'T->'Key, _: Id2<'Key*Id2<'T>> , [<Optional>]_impl: GroupBy) = let a = Id2.run x in Id2.create (f a, x)
static member GroupBy (x: seq<'T> , f: 'T->'Key, _: seq<'Key*seq<'T>> , [<Optional>]_impl: GroupBy) = Seq.groupBy f x
static member GroupBy (x: list<'T>, f: 'T->'Key, _: list<'Key*list<'T>>, [<Optional>]_impl: GroupBy) = Seq.groupBy f x |> Seq.map (fun (x, y) -> x, Seq.toList y) |> Seq.toList
static member GroupBy (x: 'T [] , f: 'T->'Key, _: ('Key*('T [])) [] , [<Optional>]_impl: GroupBy) = Seq.groupBy f x |> Seq.map (fun (x, y) -> x, Seq.toArray y) |> Seq.toArray
Expand Down
10 changes: 10 additions & 0 deletions src/FSharpPlus/Internals.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,16 @@ module Id =
let map f (x: Id<_>) = Id (f x.getValue)
let create x = Id x

type Id2<'t> (v: 't) =
let value = v
member _.getValue = value

[<RequireQualifiedAccess>]
module Id2 =
let run (x: Id2<_>) = x.getValue
let map f (x: Id2<_>) = Id2 (f x.getValue)
let create x = Id2 x

type Id0 (v: string) =
let value = v
member _.getValue = value
Expand Down
7 changes: 7 additions & 0 deletions tests/FSharpPlus.Tests/Collections.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,3 +291,10 @@ module Collections =

let m = choose Some ((ofSeq :seq<_*_> -> Map<_,_>) (seq ["a", 1; "b", 2]))
Assert.IsInstanceOf<Option<Map<string,int>>> (Some m)

// Compile tests

let inline mapOfGroup (key: 'T -> 'Key when 'Key : equality) (sequence: '``Collection<'T>``) : Map<'Key, '``Collection<'T>``> =
Copy link
Member Author

@gusty gusty Jun 13, 2025

Choose a reason for hiding this comment

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

@fxdmhtt this function is kind of useful.

I already thought about grouping in the past and realized that ideally they should return non-empty collections.
Returning a map makes sense for the eager groupBy (as opposed to the lazy chunkBy), so maybe we should create an issue to discuss a function that groups into a NonEmptyMap.

Copy link
Contributor

@fxdmhtt fxdmhtt Jun 13, 2025

Choose a reason for hiding this comment

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

I don't understand how this works.

Why can I write the correct type signature by simply making the Id redundant?

Copy link
Contributor

@fxdmhtt fxdmhtt Jun 13, 2025

Choose a reason for hiding this comment

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

In fact, when I was writing the secondary encapsulation library, I encountered many situations where I had to delete the type signature to compile. But I thought it was caused by my weak F# ability, so I didn't report it to you.

Can you tell me what happened?

sequence
|> groupBy key
|> Map.ofSeq
Loading