Simplify random #314

TysonMN · 2021-02-08T14:06:31Z

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 · 2021-02-08T14:30:42Z

(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?

ghost · 2021-02-10T22:52:37Z

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)```

TysonMN · 2021-02-11T04:18:23Z

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.

ghost · 2021-02-15T00:42:25Z

Perhaps @ThisFunctionalTom knows why the fable equivalent crashes?

ThisFunctionalTom · 2021-02-15T05:26:45Z

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?

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.

TysonMN · 2021-02-15T15:36:21Z

@ThisFunctionalTom, the c|configuration argument defaults to Debug. To reproduce the behavior on the build server, you need to set the configuration to Release.

fsharp-hedgehog/.github/workflows/pull_request.yml

Line 29 in e1f87f3

    
           run: dotnet test --no-build --configuration Release --verbosity normal tests/Hedgehog.Linq.Tests/Hedgehog.Linq.Tests.csproj

(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.

ThisFunctionalTom · 2021-02-15T17:25:52Z

@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

TysonMN · 2021-02-15T20:49:51Z

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

The two pending tests are the two tests Fable is supposed to ignore.
The three failing tests are all tests that fail due to StackOverflow.

ThisFunctionalTom · 2021-02-16T07:10:02Z

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:

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?

TysonMN · 2021-02-16T13:43:37Z

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.

moodmosaic · 2021-02-16T13:53:39Z

What do you think?

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

ThisFunctionalTom · 2021-02-16T13:54:36Z

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.

moodmosaic · 2021-02-16T14:20:44Z

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?

ThisFunctionalTom · 2021-02-16T14:25:49Z

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?

TysonMN · 2021-02-16T15:08:40Z

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

ThisFunctionalTom · 2021-02-18T06:12:54Z

@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?

TysonMN · 2021-02-18T13:06:49Z

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

ghost · 2021-02-20T00:27:11Z

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.

TysonMN · 2021-02-20T19:32:51Z

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

TysonMN force-pushed the simplify_random branch from 800ed44 to d95c9ae Compare February 8, 2021 14:21

TysonMN mentioned this pull request Feb 8, 2021

Random.map without stack overflow #289

Open

TysonMN added 3 commits February 15, 2021 09:03

improved naming and formatting

1498741

added Random.join, implemented Random.bind via Random.join

b6e3edc

replaced Random.replicate with ListRandom traverse and sequence

997a25a

TysonMN force-pushed the simplify_random branch from d95c9ae to 997a25a Compare February 15, 2021 15:03

TysonMN mentioned this pull request Feb 20, 2021

StackOverflow with Fable but not with fsc in release fable-compiler/Fable#2392

Open

TysonMN force-pushed the master branch from d74c820 to 03c7125 Compare July 23, 2022 04:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify random #314

Simplify random #314

TysonMN commented Feb 8, 2021

TysonMN commented Feb 8, 2021

ghost commented Feb 10, 2021

TysonMN commented Feb 11, 2021

ghost commented Feb 15, 2021

ThisFunctionalTom commented Feb 15, 2021 •

edited

TysonMN commented Feb 15, 2021

ThisFunctionalTom commented Feb 15, 2021

TysonMN commented Feb 15, 2021

ThisFunctionalTom commented Feb 16, 2021

TysonMN commented Feb 16, 2021

moodmosaic commented Feb 16, 2021

ThisFunctionalTom commented Feb 16, 2021

moodmosaic commented Feb 16, 2021

ThisFunctionalTom commented Feb 16, 2021 •

edited

TysonMN commented Feb 16, 2021

ThisFunctionalTom commented Feb 18, 2021 •

edited

TysonMN commented Feb 18, 2021

ghost commented Feb 20, 2021

TysonMN commented Feb 20, 2021

Simplify random #314

Are you sure you want to change the base?

Simplify random #314

Conversation

TysonMN commented Feb 8, 2021

TysonMN commented Feb 8, 2021

ghost commented Feb 10, 2021

TysonMN commented Feb 11, 2021

ghost commented Feb 15, 2021

ThisFunctionalTom commented Feb 15, 2021 • edited

TysonMN commented Feb 15, 2021

ThisFunctionalTom commented Feb 15, 2021

TysonMN commented Feb 15, 2021

ThisFunctionalTom commented Feb 16, 2021

TysonMN commented Feb 16, 2021

moodmosaic commented Feb 16, 2021

ThisFunctionalTom commented Feb 16, 2021

moodmosaic commented Feb 16, 2021

ThisFunctionalTom commented Feb 16, 2021 • edited

TysonMN commented Feb 16, 2021

ThisFunctionalTom commented Feb 18, 2021 • edited

TysonMN commented Feb 18, 2021

ghost commented Feb 20, 2021

TysonMN commented Feb 20, 2021

ThisFunctionalTom commented Feb 15, 2021 •

edited

ThisFunctionalTom commented Feb 16, 2021 •

edited

ThisFunctionalTom commented Feb 18, 2021 •

edited