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

random_choice missing from fable library #3966

Open
joprice opened this issue Dec 2, 2024 · 4 comments
Open

random_choice missing from fable library #3966

joprice opened this issue Dec 2, 2024 · 4 comments

Comments

@joprice
Copy link
Contributor

joprice commented Dec 2, 2024

Description

A runtime error is hit when trying to use Seq.randomChoice:

ImportError: cannot import name 'random_choice' from 'fable_modules.fable_library.seq'

It looks like this is a group of relatively recent helpers from this rfc (https://github.com/fsharp/fslang-design/blob/main/RFCs/FS-1135-random-functions-for-collections.md), and need to be added to the replacements:
dotnet/fsharp@54a1997#diff-e145f82a233516da994063cddbbe920e616e734ee6ab652eb4c8a21f76382ef9

Related information

  • Fable version: 4.23.0
  • Operating system: OSX
@MangelMaxime
Copy link
Member

Thanks for the report, we should indeed add them. It looked like really interesting API when I was reading about them last week.

If I remember correctly, Seq, List and Array in Fable are implemented using F# directly so perhaps it will not be too difficult to do. I think the things we need to be careful to test is the that the Random seed works for reproducing the randomness.

On another note, I am a bit surprise that you have this error and not a "nicely formatted" Fable error message about not all BCL API being supported. But this perhaps, this is because we just forward the call without checking if we actually have an implementation in the Seq.fs, List.fs, etc.

@joprice
Copy link
Contributor Author

joprice commented Dec 2, 2024

Yes I expected the typical unsupported error as well. I guess to handle that every supported method needs to be listed out in the replacements to avoid new passing through.

@MangelMaxime
Copy link
Member

I guess to handle that every supported method needs to be listed out in the replacements to avoid new passing through.

Indeed, I often think about if we should or should not do it.

On one side, have an exhaustive list of the support API make the DX better because Fable can capture the error.

On the other side, it makes adding a new API a bit more verbose and also means that for every API call Fable will need to check the list of supporting API. We would probably not want to use a list for that but a Set or Hashmap if I remember correctly to get a O(1) search.

IHMO I think the pros are worth considering.

@joprice
Copy link
Contributor Author

joprice commented Dec 3, 2024

Yes, luckily it results in a hard error that prevents the script from executing, but having a compile time error benefits library development as well to find unsupported usages without requiring full coverage.

It could end up being a lot of string comparisons in a large codebase. A hash or treemap/set lookup or even a trie could be used to optimize it.

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

No branches or pull requests

2 participants