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

Consider providing a more explicit implementation of IArbMap #659

Closed
Smaug123 opened this issue Feb 23, 2024 · 6 comments
Closed

Consider providing a more explicit implementation of IArbMap #659

Smaug123 opened this issue Feb 23, 2024 · 6 comments

Comments

@Smaug123
Copy link
Contributor

Smaug123 commented Feb 23, 2024

The introduction of IArbMap is a great improvement on the implicit global mutable state we had before, but it's still kind of magic ("scrape the static members of an input type" is not a common operation!).

Have you considered also providing a more explicit implementation? Something like:

type TypeRegistrationVisitor<'ret> = abstract Eval<'a> : Arbitrary<'a> -> 'ret
type TypeRegistration = abstract Visit<'ret> : TypeRegistrationVisitor<'ret> -> 'ret

[<RequireQualifiedAccess>]
module TypeRegistration =
    let make<'a> (arb : 'a Arbitrary) : TypeRegistration =
        { new TypeRegistration with
            member _.Visit visitor =
                visitor.Eval arb
        }

    [<RequiresExplicitTypeArguments>]
    let force<'a> (reg : TypeRegistration) : Arbitrary<'a> =
        { new TypeRegistrationVisitor<_> with
            member _.Eval<'x> (arb : Arbitrary<'x>) =
                arb
                |> unbox<Arbitrary<'a>>
        }
        |> reg.Visit

    let private getUnboxedVisitor =
        { new TypeRegistrationVisitor<_> with
            member _.Eval<'x> (arb : Arbitrary<'x>) =
                arb
                |> unbox<Arbitrary<obj>>
        }

    let internal getObj (reg : TypeRegistration) : Arbitrary<obj> =
        reg.Visit getUnboxedVisitor

type ExplicitArbMap =
    private
        {
            // sigh, don't want to take a dep on System.Collections.Immutable
            /// Map of Type.FullName to registration of that type
            /// Invariant which is not present in the types:
            /// registrations.[t.FullName] contains an Arbitrary<t>.
            Registrations : Map<string, TypeRegistration>
        }

    interface IArbMap with
        member this.ArbFor (t : Type) : Arbitrary<obj> =
            match this.Registrations.TryGetValue t.FullName with
            | false, _ ->
                failwith "whatever we do here"
            | true, arb ->
                TypeRegistration.getObj arb

        member this.ArbFor<'c> () : Arbitrary<'c> =
            match this.Registrations.TryGetValue typeof<'c>.FullName with
            | false, _ ->
                failwith "whatever we do here"
            | true, arb ->
                TypeRegistration.force<'c> arb

        member this.MergeFactory<'a, 'b> (factory : Func<'a, Arbitrary<'b>>) : IArbMap =
            failwith "I don't understand what this member does so I haven't implemented it"

[<RequireQualifiedAccess>]
module ExplicitArbMap =
    let defaults : ExplicitArbMap =
        {
            Registrations = Map.empty
        }

    let add<'a> (arb : Arbitrary<'a>) (existing : ExplicitArbMap) : ExplicitArbMap =
        let registration = TypeRegistration.make arb
        {
            Registrations =
                existing.Registrations
                |> Map.add typeof<'a>.FullName registration
        }

    let addRange (regs : TypeRegistration seq) (existing : ExplicitArbMap) : ExplicitArbMap =
        (existing.Registrations, regs)
        ||> Seq.fold (fun existing reg ->
            { new TypeRegistrationVisitor<_> with
                member _.Eval<'a> (arb : 'a Arbitrary) =
                    Map.add typeof<'a>.FullName reg existing
            }
            |> reg.Visit
        )
        |> fun x -> { Registrations = x }

Then constructing an IArbMap looks like:

module Blah =
    let myArbMap =
        ExplicitArbMap.defaults
        |> ExplicitArbMap.add (Arb.fromGen (failwith<Gen<int>> "my generator"))

This has the advantage that the IArbMap is now manipulable at runtime rather than having to put all your generators up front on a particular type.

@bartelink
Copy link
Contributor

V quick drive by comment; could we way off...

I believe https://github.com/fscheck/FsCheck/blob/master/src/FsCheck/FSharp.ArbMap.fs more or less does that, but it's in the .FSharp namespace
I definitely agree that between lack of docs and no easy fluent discovery path, it's definitely hard to figure out the basics early on
#638
#644 (comment)

member this.MergeFactory<'a, 'b> (factory : Func<'a, Arbitrary<'b>>) : IArbMap =
            failwith "I don't understand what this member does so I haven't implemented it"

^ this is for the hook (used in the last link) where you can refer to the other arbs including modifications to the defaults and then decorate it. For me it's as ciritical as knowing how to get to the default arb map (I had a scenario where I want to honor customizations, but am doing a generic decorator on top that needs to be able to have those arbs in scope). (Yes, dont have time to write a pithier explanation - hope this makes some sense)

@Smaug123
Copy link
Contributor Author

Ah right - mergeArb was basically what I wanted, thanks!

@bartelink
Copy link
Contributor

bartelink commented Feb 23, 2024

If the .FSharp and .CSharp namespaces could be auto-opened on a global basis per language or something in that direction, it might help discoverability.
Tbh I only hit on this by delving into the source with ambition - With ArbMap.defaults as the my only mental point of entry to things, and the Factory syntax not being mentioned in the docs prevented me from looking for it/anticipating its existence for quite a while.
(I know the ultimate answer is for more people to do docs PRs to solve this over time...)

@kurtschelfthout
Copy link
Member

The introduction of IArbMap is a great improvement on the implicit global mutable state we had before, but it's still kind of magic ("scrape the static members of an input type" is not a common operation!).

I agree!

It's the "easiest" way I could think of at the time to help people put together arbitrary instances with generic arguments...if FsCheck needs to generate a type like Dictionary<K,V> based on a generic (i.e. no concrete K or V) entry in the ArbMap, then where does it get the K and V generators from? It can't necessarily pull them out of the ArbMap directly, because the number of concrete types that can be generated is effectively infinite. So if the user passes in IDictionary<int, List<string>> then List<string> is probably not in the map. But FsCheck can create it from its generic List<T> and string generator, which are in the map.

This is circular as you can imagine - the Dictionary<K,V> entry effectively calls itself when FsCheck is asked to generate Dictionary<int, Dictionary<int, int>> for example, and the ArbMap allows customizing

The functions in your implementations where you mentioned "didn't implement because didn't understand" are there to deal with this sort of stuff.

Nonetheless, it's very possible there is a better approach out there. Currently my time investement in FsCheck is minimal, so I'm not going to come up with it any time soon.

If the .FSharp and .CSharp namespaces could be auto-opened on a global basis per language or something in that direction, it might help discoverability.

Is that possible? Technically I mean in C# or F#.

(I know the ultimate answer is for more people to do docs PRs to solve this over time...)

Yes please submit docs PRs if you find something like this and have the time.

I have a feeling that FsCheck is pretty contribution-hostile at the moment because of legacy choices that make things harder than they need to be (paket etc). In principle I'd like for everything to just use nuget. Also the docs use fsformatting which is probably not everyone's cup of tea. Overhauling all that stuff is time-intensive though....

@bartelink
Copy link
Contributor

bartelink commented Feb 24, 2024

Is that possible? Technically I mean in C# or F#.

C# 10 has implicit global usings

F# does not yet - it's not an open and shut case as in general shadowing and aliasing mean that aggressive auto-opens (which is one way to pull stuff in only for F#) and/or implicit usings are not a good answer (and it was already debatable even for C# - one of those things that got pushed through by the ASP.NET team despite it not being a thing a mature language design team might have done as an organic thing)

Some meta assemblies like ASP.NET Core leverage that to auto-open things. Fs.Check could do that, but you'd need to do custom hacking to make it driven by the language of a project. Ultimately splitting the CSharp and FSharp and Fluent stuff etc is definitely for the best as there'd be nothing worse than having duplicates in your intellisense so not quibbling about it, but I am lamenting the fact that its a big discovery/learning impediment.

But if you think its a good idea, I do believe one can put MSBuildese in a .props file in the package to do language specific conditional things. Please make an issue and I will spike an impl if you are interested

However I think the practical answer is a prominent note near the top of the docs saying key things like

  • docs are for FSCheck 3.0 but are behind atm
  • for C#: using FsCheck, FsCheck.CSharp For F#: open FsCheck; open FSCheck.FSharp (which is not obvious to most people, esp people coming from V2 where it was mainly one namespace and there was not lang specific element IIRC)

In principle I'd like for everything to just use nuget.

Might be good to put a help wanted issue up and not move V3 out of RC until it is done then. If I can make time soon enough I might take it (but equally you never know is is lurking and just thought that they would not get a response if they tried to do it)

Also the docs use fsformatting which is probably not everyone's cup of tea.

FsFormatting is for the best for ease of light maintenance so prob no need to change it. But a lot can be achieved by customizing the ruleset to make ti more broadly palatable; some of the OOTB rules are just simply too aggressive and consume way too much vertical space eggregiously fsprojects/FSharp.AWS.DynamoDB#73

The bottom line on docs is that the vast majority are still learning and hence the imposter syndome is strong - I personally would have a hard time distinguishing between out of date docs, advice that I feel is wrong, and stuff that I just don't get yet, and I suspect there are lots of people in that boat.

But I absolutely get that the fact that you cant magic up the significant time needed to do even the most basic updates. If you can manufacture some time, perhaps a "docs updates needed checklist issue" with one liners saying "section x is very V2 specific, it should be rewritten to cover approach X" or "that section was a bad idea and should be moved to an appendix", "section X should be split intpo two".

@pblasucci and @mavnn have some great tutorial material that early sections of the docs should prob link to (I dont know of others that are relatively in date and meet a bar of usefulness personally, but I bet there are a few more worthy ones)

Overhauling all that stuff is time-intensive though....

Putting help wanted issues up is definitely useful in terms of suggesting where you'd take inputs. I's hard to guess a maintainers preferences without them. For instance, for all I'd have known you might love paket (until you suggested otherwise above). (I think paket is great for reasonable sized apps, even if NuGet Global Package Management is getting there, but for a library like this I think it's just bad news)

@kurtschelfthout
Copy link
Member

But if you think its a good idea, I do believe one can put MSBuildese in a .props file in the package to do language specific conditional things. Please make an issue and I will spike an impl if you are interested

Thanks for the offer! I'm a bit skeptical tbh.

Agree that better docs is a more practical answer at this point.

FsFormatting

Sorry, I meant fsdocs for the documentation. Not using fantomas/fsformatting at the moment, and don't think that's a priority atm.

I personally would have a hard time distinguishing between out of date docs, advice that I feel is wrong, and stuff that I just don't get yet, and I suspect there are lots of people in that boat.

Putting help wanted issues up is definitely useful in terms of suggesting where you'd take inputs. I's hard to guess a maintainers preferences without them.

That all makes sense. Good suggestions. I will try to organize issues and appeal to potential contributors in the readme. First order of business is making releases easier...fake/paket have been a constant pain these last years to be honest and it's in fact not working right now if you have .NET 8 installed or some such. I am personally completely burnt out on those kind of .NET tooling issues. Happily we have a PR in progress now that hopefully can address this in #662!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants