Add ability to customize max default SizeRange via env var.

[nipunn1313]

The implementation directly accesses Config::default() from SizeRange::default().

I don't love the implementation for two reasons

• It clones the full config on every call to SizeRange::default()
• It is hard to test.

However, it does achieve the goal. I am open to better ideas!

We could avoid the clone by changing the behavior of Config::default() (perhaps (&Config)::default() - or by making DEFAULT_CONFIG lazystatic public.

Fixes #320

After implementing this, I am starting to feel more partial to the alternate approach of reducing the default max sizerange drastically to 0..8 and making people increase it manually via a non-default strategy - if that's the behavior they want. The perf of nested vecs snowballs out of control quite fast IMO and a bit of a surprise.

[tzemanovic]

Thanks! Personally, I'd be in favor of changing the behavior of Config::default(). We could just remove the static ref DEFAULT_CONFIG that calls contextualize_config to resolve env vars and instead call contextualize_config from TestRunner::new to preserve the behavior.

When using proptest! it will already call contextualize_config - I've added this in #318 to allow overriding non-default config and it's also a bit sub-optimal because the env vars are being resolved twice. With the proposed change to TestRunner::new we could undo the changes in sugar.rs (https://github.com/proptest-rs/proptest/pull/318/files#diff-80b8c1ddfc4dfa65d2e43d283c01f08dc1b47e77b79a69ae9a1e947894488b70).

I would also not oppose reducing the default.

[tzemanovic]

I've added the changes I proposed here https://github.com/nipunn1313/proptest/compare/nipunn/proptest_max_size...tzemanovic:proptest:nipunn/proptest_max_size_2?expand=1. Please lmk if you're happy with the changes and I can push them back here

[matthew-russo]

I'll give this a look this evening after work. If it's reusing existing config mechanisms rather than plunking env vars in the impl of Default im more in favor

[matthew-russo]

Agh I never published my comment. I didn't realize what the original proposal was. I'm fine with the original PR opened here. Tomas for your alternative changes, I'd want the default impl of size range to use the config contextualization rather than directly reading env vars. That way we isolate all env-based configuration to just Config.

Whats the rationale behind removing the contextualization from Config::Default?

[tzemanovic]

@matthew-russo My main motivation is to remove the cloning from impl Default for Config. The other options @nipunn1313 suggested were to either:

• impl Default for &Config where for no-std were we'd need to add another static be able to impl this as there is non-const rng_algorithm: RngAlgorithm::default() used in fn default_default_config
• make static ref DEFAULT_CONFIG pub, but this is also not available in no-std

[matthew-russo]

If this is perf related are there any numbers showing removing the clones improves test runtime? I'd be pretty surprised if the config generation is where times being spent. I haven't spent much time with this area of the code though.

This would change the behavior of existing users which I'm skeptical of doing without a breaking version change.

[tzemanovic]

If this is perf related are there any numbers showing removing the clones improves test runtime? I'd be pretty surprised if the config generation is where times being spent. I haven't spent much time with this area of the code though.

This would change the behavior of existing users which I'm skeptical of doing without a breaking version change.

On my machine it is slightly better than the previous impl that was cloning the whole Config just to get this single field, though I agree that this is not likely a bottleneck of any test.

The change itself however is not externally a breaking change. Even though the behavior of impl Default for Config is changed, the only places where the Config appears in public API is TestRunner::new/new_with_rng which with the added call to contextualize_config inside them remain the same. The only difference would be if someone were to use Config::default() without the proptest! macro, manually override some field on it while at the same time having an env var also overriding this field. Previously, the first override would take precedence, now it would be the second override from an env var. I'm not sure if I'd go as far as call the current precedence order a bug and I doubt anyone is relying on it, but I think the new order makes more sense anyway.

[rex-remind101]

I took a look over this myself and just wanted to weigh in with my opinion

• This pr to me seems good as-is and I'm fine merging it.
• The default of 100 has been a nuisance to me before, however I'd consider it a breaking change to change our default cardinality.

I read through @tzemanovic 's pr and though I agree with some of the motivation I think there's some things to consider

I'd want the default impl of size range to use the config contextualization rather than directly reading env vars. That way we isolate all env-based configuration to just Config.

• I agree with @matthew-russo here, this line doesn't follow the preexisting pattern the rest of the codebase seems to use by accessing Config::default() directly, moving away from that pattern seems like a larger change to me outside of scope of this pr.
• However, "Previously, the first override would take precedence, now it would be the second override from an env var. I'm not sure if I'd go as far as call the current precedence order a bug", this does seem like a bug to me. It's inconsistent behavior even if unlikely to be encountered. If yall don't think it's a bug then I'd agree with matthew that it's a breaking change, but sounds like a bug to me. Also sounds like it could be addressed separately however with the rest of tzemanovic's proposed changes.

I hope this helps move this forward in some direction. Just to summarize

1. I'm in favor of merging
   1. (barring someone has evidence of a big performance regression, I know that was mentioned earlier)
2. I'm also in favor of the direction tzemanovic's pr is taking and the (what think is a) bug it also addresses, but I think this can be addressed separately.
3. Finally I'm in favor of lowering this default count dramatically but I think that's a breaking change (open to other opinions on that though).

I'll be on vacay for a couple weeks so that's all input I have for the time being

[tzemanovic]

unrelated change broke nightly build - fixing it in https://github.com/proptest-rs/proptest/pull/389

[nipunn1313]

I merged master here to reconcile the merge conflicts with master. Should be good to go!

[matthew-russo]

Nightly build seems broken for an unrelated reason. will try to fix that today and then get this merged