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

Tuple unions created by proptest_derive should branch out more (stack overflow while generating values) #152

Open
sunshowers opened this issue Jun 24, 2019 · 3 comments
Assignees
Labels
quality-of-life This issue proposes a change that will improve the UX of proptest but isn't necessarily a "feature"

Comments

@sunshowers
Copy link
Contributor

Hi again! ☺️

So it turns out that we have an enum with 70+ different variants, and it's large enough that generating values for it appears to cause a stack overflow with the default Rust stack size 😕

At the moment, when there are a large number of variants, proptest_derive will create essentially a linear structure. For variants v<n>, the structure looks like:

[v0, v1, v2, v3, ... v8, ___ ]
                          |
                          v
                         [v9, v10, v11, ...]

and so on. Instead, what we could do is a bit more branchy:

[ ______________ , _______________ , ...]
       |                  |
       v                  v
[v0, v1, v2, ...] [v10, v11, v12, ...]

There are several possible distributions we could use, e.g. an exponential distribution (1 variant in the first slot, 2 in the second slot and so on). I'd love to hear your opinions on it! Thank you 😀

This isn't a problem with prop_oneof! since it just uses the Union strategy past 10 variants.

@sunshowers
Copy link
Contributor Author

I might give implementing this a shot if I have free time soon.

@Centril
Copy link
Collaborator

Centril commented Jun 25, 2019

(Might also be relevant to #151, cc @alessandrod)

Well... this was unexpected (for me) =P
I'm curious to hear more about why (and where) the stack overflow happens.

Instead, what we could do is a bit more branchy:

Did you try the more branchy expansion in a manual implementation to see that the issue is resolved with that?

There are several possible distributions we could use, e.g. an exponential distribution (1 variant in the first slot, 2 in the second slot and so on). I'd love to hear your opinions on it! Thank you 😀

Here's two alternatives that come to mind (the invariant here is that we have max 3 slots):

[ ____________, v4]
       |
       v
  [v0, v1, v2]

[ ____________, ____________]
       |             |
       v             v
  [v0, v1, v2]   [v4, v5]

(this seems most even tho possibly less efficient due to never having shortcuts)

vs.

[ ________, ________]
     |         |
     v         v
  [v0, v1]  [v3, v4]

[ ____________, v4, v5]
       |
       v
  [v0, v1, v2]

(this seems simplest; tho you might want to mirror the to put "extras" to the right instead...)

aalexandrov added a commit to aalexandrov/materialize that referenced this issue Apr 13, 2022
We need a custom implementation because the one synthesized
by the derive macro is hitting a known proptest issue[^1].

More arms need to be added as we build up the protobuf
support for `UnaryFunc` towards closing #11747.

[1]: proptest-rs/proptest#152
aalexandrov added a commit to aalexandrov/materialize that referenced this issue Apr 13, 2022
We need a custom implementation because the one synthesized
by the derive macro is hitting a known proptest issue[^1].

More arms need to be added as we build up the protobuf
support for `UnaryFunc` towards closing #11747.

[1]: proptest-rs/proptest#152
aalexandrov added a commit to aalexandrov/materialize that referenced this issue Apr 13, 2022
We need a custom implementation because the one synthesized
by the derive macro is hitting a known proptest issue[^1].

More arms need to be added as we build up the protobuf
support for `UnaryFunc` towards closing #11747.

[1]: proptest-rs/proptest#152
aalexandrov added a commit to aalexandrov/materialize that referenced this issue Apr 14, 2022
We need a custom implementation because the one synthesized
by the derive macro is hitting a known proptest issue[^1].

More arms need to be added as we build up the protobuf
support for `UnaryFunc` towards closing #11747.

[1]: proptest-rs/proptest#152
aalexandrov added a commit to aalexandrov/materialize that referenced this issue Apr 14, 2022
We need a custom implementation because the one synthesized
by the derive macro is hitting a known proptest issue[^1].

More arms need to be added as we build up the protobuf
support for `UnaryFunc` towards closing #11747.

[1]: proptest-rs/proptest#152
aalexandrov added a commit to aalexandrov/materialize that referenced this issue Apr 14, 2022
We need a custom implementation because the one synthesized
by the derive macro is hitting a known proptest issue[^1].

More arms need to be added as we build up the protobuf
support for `UnaryFunc` towards closing #11747.

[1]: proptest-rs/proptest#152
@matthew-russo matthew-russo added the quality-of-life This issue proposes a change that will improve the UX of proptest but isn't necessarily a "feature" label Dec 6, 2023
@matthew-russo matthew-russo self-assigned this Dec 23, 2023
@matthew-russo
Copy link
Member

matthew-russo commented Dec 23, 2023

an interesting note. I had a pretty old toolchain (1.72 nightly) and could repro this. updating to 1.77 nightly turned the stack overflow in to a segfault. then if i removed a few variants from my test then it turned back in to a stack overflow.

segfault might be from overjumping the guard page?

rexmas pushed a commit that referenced this issue Dec 23, 2023
For enums with a lot of variants the `Strategy` synthesized by the
`#[derive(Arbitrary)]` macro is a right-deep `TupleUnion`.

This commit adds a `boxed_union` feature, which when enabled will change
the `enum` expansion to use `Union<BoxedStrategy<Self>>`.

see github issues:
#249
#152

code included from #249 (comment)

Co-authored-by: Alexander Alexandrov <[email protected]>
rexmas pushed a commit that referenced this issue Dec 23, 2023
For enums with a lot of variants the `Strategy` synthesized by the
`#[derive(Arbitrary)]` macro is a right-deep `TupleUnion`.

This commit adds a `boxed_union` feature, which when enabled will change
the `enum` expansion to use `Union<BoxedStrategy<Self>>`.

see github issues:
#249
#152

code included from #249 (comment)

Co-authored-by: Alexander Alexandrov <[email protected]>
Co-authored-by: Sophie Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality-of-life This issue proposes a change that will improve the UX of proptest but isn't necessarily a "feature"
Projects
None yet
Development

No branches or pull requests

3 participants