System.ArgumentException in memoizeWith

[svdijk]

I recently ran into a System.ArgumentException : An item with the same key has already been added. in Common.memoizeWith with some custom generators/arbitraries. Below is minimal example test that triggers this (I know that Arb.generate should be deferred here, and that indeed also avoids this issue, but it probably shouldn't crash with this code nonetheless). It seems that Common.memoizeWith first concludes that n is not in the memo, then calls f, and then when it tries to add n to the memo this exception is raised because n is already in the memo. This can be worked around by, after calling f, either checking again if n is already in the memo and only adding it if it's not, or simply removing and than (re-)adding it, Both seem to work, but I'm not sure which (if either) is the correct solution, so I'm making this just a report instead of a pull request for now.

module Test

open FsCheck
open FsCheck.Xunit

type Inner = Inner
type Outer = Outer of Inner

let inner = Gen.constant Inner
let outer = Arb.generate<Inner> |> Gen.map Outer

type Arbs =
    static member Inner() = Arb.fromGen inner
    static member Outer() = Arb.fromGen outer

[<Property(Arbitrary = [| typeof<Arbs> |])>]
let test (_: Inner) = ()

[kurtschelfthout]

I think either option is more confusing than failing, because either way which generator "wins" is dependent on the order in which the generators are used (because the whole thing is lazy).

So I would suggest going through methods explicitly instead of calling Arb.generate, e.g. use Arb.Default.Derive if you want to use reflection based generation and/or call Arbs.Inner() explicitly if you want your custom Arbitrary instance.

At least the error message can be improved though!

[svdijk]

Yeah I know the example code is bad, it was meant just as a minimal reproducible example that triggers the issue, i.e. that Common.memoizeWith doesn't take into account (at least not explicitly 😄) that f may end up recursively calling it again, thus changing the memo between memo.TryGetValue and memo.Add.

[kurtschelfthout]

I understand the example is very specific, but this problem is “known” to me in general. I understand you can fix it using one of the approaches you suggested, but my point the fix would make things potentially more confusing in many cases - at least when people are forced to use Arb.Default.String or some such directly instead of Arb.generate<string> it`s very clear what is being generated.

It’s in some sense similar to the aliasing problem with mutable data structures - if you write Arb.generate you don’t really know what you’re going to get, as anyone can come in after and override the generator for that type. So esp. when writing Arbitray instances it’s best avoided.

[kurtschelfthout]

Closing after inactivity. Feel free to re-open if more questions.