xyncro / chiron

JSON for F#
https://xyncro.tech/chiron
MIT License
175 stars 41 forks source link

Extending FsCheck with extra Arbitraries causes StackOverflowException #21

Closed haf closed 9 years ago

haf commented 9 years ago

screen shot 2015-04-20 at 13 49 07

See 61cffa5

kolektiv commented 9 years ago

Hmmm... I have no idea :) I hope I might get time to dig in to it! I wonder whether @mavnn has any ideas, as he contributed the current FsCheck tests...

mavnn commented 9 years ago

Some quick experimentation shows that the default FsCheck DateTime generator never generates Utc DateTimes (or, at least, it generated several hundred in a row that weren't Utc - I haven't checked the source). I'm assuming it recursively generates values when you call Arb.filter and so you get the overflow.

Might be worth a pull request to FsCheck given it seems like the kind of thing that should be generated: @kurtschelfthout , you got any thoughts about that?

mavnn commented 9 years ago

DateTime generating code is here, by the way: https://github.com/fsharp/FsCheck/blob/master/src/FsCheck/Arbitrary.fs#L562

mavnn commented 9 years ago

And I'll stop spamming you after this, I promise! Why did you want only UTC DateTimes anyway? When I first added the property based tests, the fact that they weren't UTC actually showed up a bug :)

kolektiv commented 9 years ago

I believe a relevant chunk of PR is here: https://github.com/xyncro/chiron/pull/18/files#diff-1090e6da10894be536ef99858edf77ccR677

On 21 April 2015 at 12:55, Michael Newton notifications@github.com wrote:

And I'll stop spamming you after this, I promise! Why did you want only UTC DateTimes anyway? When I first added the property based tests, the fact that they weren't UTC actually showed up a bug :)

— Reply to this email directly or view it on GitHub https://github.com/xyncro/chiron/issues/21#issuecomment-94760016.

mavnn commented 9 years ago

Hmmm. I'm drugged up on painkillers so I'm not really at my best, but doesn't the PR enforce the user turning date times into UTC before persisting them? I'd rather have it convert to UTC for persistence, I think.

haf commented 9 years ago

Yes, it enforces UTC right now; to provide a baseline -- it's worse to have tests failing on different computers depending on their timezones than limiting the Kind-ness of DateTime, was the thinking. Now that we have a baseline (passing tests) we can expand how DateTime is handled (even though DateTimeOffset is better most of the time)

kurtschelfthout commented 9 years ago

Not clear to me why adding filter would cause a stackoverflow...filtering is not recursive. But maybe generating many many values has a recursive component somewhere.

kurtschelfthout commented 9 years ago

Can't repro the stackoverflow on simple tests, so not clear this is caused by FsCheck at the moment. However, if you use this definition of the generator things should work a lot better:

static member DateTime () =
    Arb.Default.DateTime ()
    |> Arb.mapFilter (fun dt -> dt.ToUniversalTime()) (fun dt -> dt.Kind = System.DateTimeKind.Utc)
kolektiv commented 9 years ago

Thanks @kurtschelfthout! I've made that change, we'll see how that goes - thanks for looking in to it for us, much appreciated.

haf commented 9 years ago

Getting this now:

 (00:00:00.3981883)
Stack overflow in unmanaged: IP: 0x2f567c, fault addr: 0xbf71efec

Perhaps either I'm getting broken memory and need to replace it, or there's a bug in some interop piece of mono... Have you guys never gotten errors like this?

It might be that I'm running in Debug mode -- and in that mode I don't have tailcall-optimisations on? Or perhaps it's that mono doesn't optimise tail-calls properly?

In lldb:

$  lldb -- mono --debug tests.exe
(lldb) run
Process 1947 launched: '/usr/bin/mono' (i386)
warning: (i386) /Library/Frameworks/Mono.framework/Versions/3.12.1/lib/mono/4.5/mscorlib.dll.dylib empty dSYM file detected, dSYM was created with an executable with no debug info.

// some logs

Process 1947 stopped
* thread #1: tid = 0x3bdc, 0x004d276c, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0xbf808fe4)
    frame #0: 0x004d276c
-> 0x4d276c:  movl   %eax, 0x4(%esp)
   0x4d2770:  movl   %esi, (%esp)
   0x4d2773:  calll  0x21c560                  ; mono_gc_alloc_obj
   0x4d2778:  movl   %eax, %ecx

It might also be that this is a ABI error - I might be using different versions of assemblies - currently checking that. -- Edit: tried updating latest everything, no luck.

kolektiv commented 9 years ago

Hmmm. Wow, nope. However... I've never tried it on mono either! Could it be possible that something that FsCheck relies on behaves quite differently under mono? @kurtschelfthout - sorry to bother you but does that change any opinion on your side? :)

haf commented 9 years ago

This code causes stack overflow exception:


  static member Floats () : Arbitrary<float> =
    Arb.generate<float>
    |> Arb.fromGen
    |> Arb.filter (fun f -> not <| System.Double.IsNaN(f) &&
                            not <| System.Double.IsInfinity(f)) 

With stack trace snippet:

  at FsCheck.TypeClass+TypeClass`1[FsCheck.Arbitrary`1[System.Object]].GetInstance (System.Type instance, Microsoft.FSharp.Core.FSharpOption`1 arguments) [0x00000] in <filename unknown>:0
  at FsCheck.TypeClass+TypeClass`1[FsCheck.Arbitrary`1[System.Object]].InstanceFor[Double,Arbitrary`1] (Microsoft.FSharp.Core.FSharpOption`1 arguments) [0x00000] in <filename unknown>:0
  at FsCheck.Arb.generate[Double] () [0x00000] in <filename unknown>:0
  at Utilities+Arbs.Floats () [0x00000] in <filename unknown>:0
  at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <filename unknown>:0
  --- End of inner exception stack trace ---
  at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <filename unknown>:0
  at System.Reflection.MethodBase.Invoke (System.Object obj, System.Object[] parameters) [0x00000] in <filename unknown>:0
  at FsCheck.TypeClass+GetInstance@178-1.Invoke (IEnumerable`1 arguments) [0x00000] in <filename unknown>:0
  at FsCheck.TypeClass+TypeClass`1[FsCheck.Arbitrary`1[System.Object]].GetInstance (System.Type instance, Microsoft.FSharp.Core.FSharpOption`1 arguments) [0x00000] in <filename unknown>:0
  at FsCheck.TypeClass+TypeClass`1[FsCheck.Arbitrary`1[System.Object]].InstanceFor[Double,Arbitrary`1] (Microsoft.FSharp.Core.FSharpOption`1 arguments) [0x00000] in <filename unknown>:0
  at FsCheck.Arb.generate[Double] () [0x00000] in <filename unknown>:0
  at Utilities+Arbs.Floats () [0x00000] in <filename unknown>:0
  at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <filename unknown>:0
  --- End of inner exception stack trace ---
  at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <filename unknown>:0
  at System.Reflection.MethodBase.Invoke (System.Object obj, System.Object[] parameters) [0x00000] in <filename unknown>:0
  at FsCheck.TypeClass+GetInstance@178-1.Invoke (IEnumerable`1 arguments) [0x00000] in <filename unknown>:0
kurtschelfthout commented 9 years ago

Yes, it would. You are overriding the float generator in terms of itself by using Arb.generate. If you want to use the default generator, you can refer to it directly using Arb.Default().Float() instead.

haf commented 9 years ago

Yeah. I'm just wondering what's causing the stack overflow in the linked PR.

kurtschelfthout commented 9 years ago

What's the stack trace of that one? Can you break before it totally crashes, that might indicate what is going on?

haf commented 9 years ago

Can't repro anymore.