fsharp-hedgehog icon indicating copy to clipboard operation
fsharp-hedgehog copied to clipboard

Simplify random

Open TysonMN opened this issue 4 years ago • 19 comments

This PR simplifies the code in Random.fs. This will make it easier to implement the fix for #289.

The changes are clearly separated in three commits.

The last commit removes Random.replicate. The migration paths is to replace

r
|> Random.replicate 10

with

r
|> List.replicate 10
|> ListRandom.sequence

TysonMN avatar Feb 08 '21 14:02 TysonMN

(For the those of you that got the email, the first build failure was my fault. I had a compile error because some projects were unloaded.)

The current build error is with the fable tests. However, the build logs are weird. It sort of looks like it didn't finish.

Can someone with fable expertise look into that?

TysonMN avatar Feb 08 '21 14:02 TysonMN

The current build error is with the fable tests. However, the build logs are weird. It sort of looks like it didn't finish.

Can someone with fable expertise look into that?

Looks like a stack overflow, it's all in the Random.fs.js file. I'll try running it again, but it may be due to the changes in this PR.

   1) All tests
       Gen tests
         unicode doesn't return any surrogate:
     RangeError: Maximum call stack size exceeded
      at BigNatModule_scaleAddInPlace (webpack:///./.fable/fable-library.3.1.1/BigInt/n.js?:1177:38)
      at BigNatModule_divmod (webpack:///./.fable/fable-library.3.1.1/BigInt/n.js?:1263:17)
      at BigInteger_DivRem_56F059C0 (webpack:///./.fable/fable-library.3.1.1/BigInt/z.js?:698:95)
      at BigInteger_op_Modulus_56F059C0 (webpack:///./.fable/fable-library.3.1.1/BigInt/z.js?:753:12)
      at op_Modulus (webpack:///./.fable/fable-library.3.1.1/BigInt.js?:234:95)
      at SeedModule_nextBigInt (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Seed.fs.js?:133:243)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Gen.fs.js?:562:95)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:84:46)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:84:46)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Random_run (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:50:12)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:128:45)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:84:46)```

ghost avatar Feb 10 '21 22:02 ghost

Since the tests pass when compiled to .NET/MSIL, this makes me think the problem is a missing feature or bug in the Fable compiler related to recursion.

TysonMN avatar Feb 11 '21 04:02 TysonMN

Perhaps @ThisFunctionalTom knows why the fable equivalent crashes?

ghost avatar Feb 15 '21 00:02 ghost

The current build error is with the fable tests. However, the build logs are weird. It sort of looks like it didn't finish. Can someone with fable expertise look into that?

Looks like a stack overflow, it's all in the Random.fs.js file. I'll try running it again, but it may be due to the changes in this PR.

   1) All tests
       Gen tests
         unicode doesn't return any surrogate:
     RangeError: Maximum call stack size exceeded
      at BigNatModule_scaleAddInPlace (webpack:///./.fable/fable-library.3.1.1/BigInt/n.js?:1177:38)
      at BigNatModule_divmod (webpack:///./.fable/fable-library.3.1.1/BigInt/n.js?:1263:17)
      at BigInteger_DivRem_56F059C0 (webpack:///./.fable/fable-library.3.1.1/BigInt/z.js?:698:95)
      at BigInteger_op_Modulus_56F059C0 (webpack:///./.fable/fable-library.3.1.1/BigInt/z.js?:753:12)
      at op_Modulus (webpack:///./.fable/fable-library.3.1.1/BigInt.js?:234:95)
      at SeedModule_nextBigInt (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Seed.fs.js?:133:243)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Gen.fs.js?:562:95)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:84:46)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:84:46)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Random_run (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:50:12)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:128:45)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:84:46)```

The current build error is with the fable tests. However, the build logs are weird. It sort of looks like it didn't finish. Can someone with fable expertise look into that?

Looks like a stack overflow, it's all in the Random.fs.js file. I'll try running it again, but it may be due to the changes in this PR.

   1) All tests
       Gen tests
         unicode doesn't return any surrogate:
     RangeError: Maximum call stack size exceeded
      at BigNatModule_scaleAddInPlace (webpack:///./.fable/fable-library.3.1.1/BigInt/n.js?:1177:38)
      at BigNatModule_divmod (webpack:///./.fable/fable-library.3.1.1/BigInt/n.js?:1263:17)
      at BigInteger_DivRem_56F059C0 (webpack:///./.fable/fable-library.3.1.1/BigInt/z.js?:698:95)
      at BigInteger_op_Modulus_56F059C0 (webpack:///./.fable/fable-library.3.1.1/BigInt/z.js?:753:12)
      at op_Modulus (webpack:///./.fable/fable-library.3.1.1/BigInt.js?:234:95)
      at SeedModule_nextBigInt (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Seed.fs.js?:133:243)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Gen.fs.js?:562:95)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:84:46)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:84:46)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Random_run (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:50:12)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:128:45)
      at Random_unsafeRun (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:46:27)
      at Array.eval (webpack:///D:/a/fsharp-hedgehog/fsharp-hedgehog/src/Hedgehog/Random.fs.js?:84:46)```

I tried running the tests with dotnet test and with dotnet run (both are .NET/MSIL) and I also get StackOverflow exception. Can you please check again on your system if the tests are really running? image

I then had a thought that maybe .NET 5 is the reason and tried running the tests in the devcontainer with .NET SDK 3.1 but I get the same StackOverflow Exception. image

ThisFunctionalTom avatar Feb 15 '21 05:02 ThisFunctionalTom

@ThisFunctionalTom, the c|configuration argument defaults to Debug. To reproduce the behavior on the build server, you need to set the configuration to Release. https://github.com/hedgehogqa/fsharp-hedgehog/blob/e1f87f35d6cbe7273a4b2f942996d6ae92c808c5/.github/workflows/pull_request.yml#L29 (You can drop --no-build. That just makes it easier to troubleshoot failed builds on the build server.)

When compiling with configuration=Debug, the (standard) F#-to-MSIL compiler does not emit tail calls but does do so in some cases when compiling with configuration=Release. That is why I said..

Since the tests pass when compiled to .NET/MSIL, this makes me think the problem is a missing feature or bug in the Fable compiler related to recursion.

I just did some searches. I think my understanding has improved.

My impression is that tail call optimization depends on the JavaScript compiler, not the Fable compiler.

TysonMN avatar Feb 15 '21 15:02 TysonMN

@TysonMN Oh, sorry. Haven't thought about configuration. You are right with 'Release' config it works. I am not sure if fable can do TCO. We can ask in F# slack. I ran the fable tests locally on your branch and a lot of tests are failing, not just the StackOverflow ones. If you would like to try it out yourself you need to have nodejs installed and from the tests/Hedgehog.Tests directory call:

  • npm install
  • npm test

ThisFunctionalTom avatar Feb 15 '21 17:02 ThisFunctionalTom

I ran the fable tests locally on your branch and a lot of tests are failing, not just the StackOverflow ones. If you would like to try it out yourself you need to have nodejs installed and from the tests/Hedgehog.Tests directory call:

  • npm install
  • npm test

Thanks for those instructions. I ran the Fable tests locally. I don't have any additional failures.

The full test results are

196 passing (6s)
2 pending
3 failing

TysonMN avatar Feb 15 '21 20:02 TysonMN

Yeah, you are right, with fable 3.1.1 there are only 3 failing tests. I was actually running the tests with the newest fable compiler (3.1.4) because I thought maybe they already fixed something but then i got 12 failing tests.

187 passing (4s) 2 pending 12 failing

I also tried running the tests in the browser environment (npm test runs in nodejs environment, npm start runs it in the browser if you open http://localhost:8085/) and I got the same result. The failing tests are: image

Hmmm. I am not sure how we should proceed with this. I think we should ask for help of the fable experts. What do you think?

ThisFunctionalTom avatar Feb 16 '21 07:02 ThisFunctionalTom

What do you think?

I contribute to fsharp-hedgehog because I want to get better at functional programming and fsharp-hedgehog contains opportunities for improvement with the right level of difficulty to make that happen. Fable is not helping me achieve this goal. At the moment, I think it is getting in the way of my goal. I am not psyched about the idea of compromising the F# code to make it compatible with "every" JavaScript compiler, particularly those without tail-call optimization.

TysonMN avatar Feb 16 '21 13:02 TysonMN

What do you think?

Can't we call the failing test(s) with fableIgnore so they're ignored from the JS test-runner?

moodmosaic avatar Feb 16 '21 13:02 moodmosaic

I do not have a problem if we remove support for fable if it stands in the way, but I will be sad a little 😢. I added the support for it because Hedgehog had no dependencies and I saw an opportunity to easily add support for property based testing to fable (javascript) projects. If it stands in the way let's talk to other contributors and see what they think.

The other possibility is to exclude functionality not supported by fable with #if !FABLE_COMPILER/#endif but I guess if there is a lot of functionality not supported by fable it will soon become tedious.

ThisFunctionalTom avatar Feb 16 '21 13:02 ThisFunctionalTom

Can't we call the failing test(s) with fableIgnore so they're ignored from the JS test-runner?

@ThisFunctionalTom, wouldn't this be an accepted solution?

moodmosaic avatar Feb 16 '21 14:02 moodmosaic

Can't we call the failing test(s) with fableIgnore so they're ignored from the JS test-runner?

@ThisFunctionalTom, wouldn't this be an accepted solution?

Yes, of course this will accepted, but if I understood it correctly this PR changes the core of Gen so I am not sure if it will still work with fable or not. If, for example, list generation does not work with fable I am not sure if Hedgehog will be usable with fable. Am I right here or did I misunderstand something?

ThisFunctionalTom avatar Feb 16 '21 14:02 ThisFunctionalTom

You are correct. I wouldn't use a dependency that handles failing tests by ignoring them.

TysonMN avatar Feb 16 '21 15:02 TysonMN

@TysonMN Do you think it would be possible to use trampoline for TCO in fable?

I think fable uses it for async CE.

We could still guard the implementation with trampoline just for Fable with #if FABLE_COMPILER.

What do you think?

ThisFunctionalTom avatar Feb 18 '21 06:02 ThisFunctionalTom

That is very interesting. Thanks for the link. I will look into this.

TysonMN avatar Feb 18 '21 13:02 TysonMN

I don't think we should stop supporting Fable. We may have to limit parts of the API from being available (and therefor tested) in Fable, if they are unstable in browsers. We also can't let Fable support hold us back, that's why I say we only expose parts of the API that work in Fable for Fable users. That way, Hedgehog can continue to evolve and we can continue to support Fable wherever possible.

ghost avatar Feb 20 '21 00:02 ghost

I created this issue in the Fable GitHub. Let's see if someone there can help us.

TysonMN avatar Feb 20 '21 19:02 TysonMN