Add faillible version of size_hint to properly handle recursive structures #185
Conversation
|
I'd have hoped that LLVM would inline the recursive |
|
My other thought is that, if we do need to define a new |
|
It's unlikely that LLVM would be able to inline a function call that is that slow, even though it could just be a constant calculation, Regarding keeping the lower bound: yes that is possible, by keeping it in the error type, but that would not be that useful, since this would still just return the first value that fails, and it would also complicate the implementation. |
sosthene-nitrokey
force-pushed
the
wide-recursive
branch
from
b5e2b9c
to
d1a7282
Compare
|
Rebased on top of main. |
src/foreign/core/cell.rs
Outdated
| #[inline] | ||
| fn size_hint(depth: usize) -> (usize, Option<usize>) { | ||
| <A as Arbitrary<'a>>::size_hint(depth) | ||
| Self::faillible_size_hint(depth).unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
The old size_hint can be default-implemented with fallible_size_hint or the default right? No need to repeat this incantation in every implementation.
There was a problem hiding this comment.
Not really, because of backwards compatibility.
If we make the default size_hint delegate to faillible_size_hint, this means we can't provide a default implementation for faillible_size_hint that calls back to size_hint, otherwise there would be an infinite loop.
We could have a default faillible_size_hint implementation that returns Ok((0, None)), but then, when libraries migrate to calling faillible_size_hint, it will break for out of date dependencies that only implement size_hint.
| /// ## How to implement this | ||
| /// | ||
| /// If the size hint calculation is a trivial constant and does not recurse into any other `size_hint` call, you should implement it in `size_hint`: |
There was a problem hiding this comment.
Let's not muddy the waters by suggesting that sometimes people use the old size hint method and sometimes the new one.
Let's deprecate the old one and always recommend using the new one.
There was a problem hiding this comment.
See #185 (comment)
We could say that this is "just" an optimization and that it's ok to break it with a minor release.
Arguably a lot of users use the derive macro or don't implement size_hint, so it could be justified. And marking it as deprecated would make this pretty visible to downstream.
I also don't like the duplicate API.
There was a problem hiding this comment.
The other comment about defaults and backwards compat makes sense, thanks for pointing that out.
That said, we should suggest that everyone move over to try_size_hint and ignore size_hint and/or implement it with try_size_hint().unwrap_or_default() and then avoid mentioning the deprecated method again.
There was a problem hiding this comment.
One of the things with my approach is that it does not require dependents to change what they call.
For example libfuzzer_sys does not care about the size hint being fallible, so it would just have been able size_hint and let the implementation deal with recursion internally.
I think the ideal setup would be to only have try_size_hint be part of the trait, and have an infallible size_hint in an extension trait (so users are not given the opportunity to override it) that does not even take a depth parameter for use in libfuzzer_sys for example. But that seems like adding more complexity and bring in naming conflicts.
There was a problem hiding this comment.
The backward compatibility issues will also mean that users will need to implement size_hint to call try_size_hint instead, and have to silence the deprecated warning to do so.
I think the issue this fixes most likely only affects users of the derive macros, and therefore I wanted to minimize the impact on other users. Especially given that a lot of types that can implement arbitrary will likely implement it on single, non recursive types and therefore they could still implement only size_hint.
Also, it's still possible to add deprecation attributes in the future if this does not happen to be the case.
sosthene-nitrokey
force-pushed
the
wide-recursive
branch
2 times, most recently
from
8562300
to
a8907c9
Compare
…tures The default implementation forwards to the current size_hint, so that it can be relied upon also for structures that still use the default implementation. The default implementation of `size_hint` is not changed, so that it does not break existing implementations
sosthene-nitrokey
force-pushed
the
wide-recursive
branch
from
a8907c9
to
e5897a1
Compare
sosthene-nitrokey
force-pushed
the
wide-recursive
branch
from
08baf88
to
b6991ce
Compare
The default implementation forwards to the current size_hint, so that it can be relied upon also for structures that still use the default implementation.
The default implementation of
size_hintis not changed, so that it does not break existing implementationsFix #144