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

Add config option to not cut tracing at recursion points #41

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

juntyr
Copy link

@juntyr juntyr commented Jun 6, 2024

Summary

The tracer thus far makes conservative assumptions to ensure that even tracing infinitely-expanding containers will be traced with termination. Sometimes, however, the user knows that their data type is non-infinite and that these conservative assumptions may be broken.

This PR adds four config options, one per recursive tracing cut-off point, to disable the safeguards and trace a type exhaustively.

Test Plan

I'm happy to add tests where needed :)

@juntyr juntyr requested a review from ma2bd as a code owner June 6, 2024 11:24
@ma2bd
Copy link
Contributor

ma2bd commented Jun 6, 2024

Do you have an example where this is useful?

@juntyr
Copy link
Author

juntyr commented Jun 6, 2024

Do you have an example where this is useful?

Yes - I’m using it to translate Rust types into Python types at ~runtime. In particular, I have large Rust library that exposes config through deep serde data structures. I’m also developing a Python wrapper for this library. Translating all types over manually is not scalable. While serialising and deserialising from/to duck-typed Python data is possible using the pythonizer crate, I also want to generate nice Python documentation.

Here is where serde-reflection comes in. My Python extension module written in Rust traces all config data structures exhaustively (which is needed since they contain private enums inside options inside publicly exposed structs), from which I generate Python types that are exported in the Python modules, which are then picked up during an auto doc step to generate native Python docs.

@ma2bd
Copy link
Contributor

ma2bd commented Jun 6, 2024

Do you have an example where this is useful?

Yes - I’m using it to translate Rust types into Python types at ~runtime. In particular, I have large Rust library that exposes config through deep serde data structures. I’m also developing a Python wrapper for this library. Translating all types over manually is not scalable. While serialising and deserialising from/to duck-typed Python data is possible using the pythonizer crate, I also want to generate nice Python documentation.

Here is where serde-reflection comes in. My Python extension module written in Rust traces all config data structures exhaustively (which is needed since they contain private enums inside options inside publicly exposed structs), from which I generate Python types that are exported in the Python modules, which are then picked up during an auto doc step to generate native Python docs.

Thanks for sharing. Actually, I meant a minimal sample of Rust code that shows why this feature is useful.

@ma2bd
Copy link
Contributor

ma2bd commented Jun 6, 2024

Mmm I guess your answer is the private enums.

@juntyr
Copy link
Author

juntyr commented Jun 13, 2024

@ma2bd What are your current thoughts on this? In my opinion, it's a good small step forward that simply exposes more current functionality. Any substantive improvements to the actual tracing could be done in future PRs but should not block this minor change.

@juntyr
Copy link
Author

juntyr commented Jun 26, 2024

gentle ping

@juntyr
Copy link
Author

juntyr commented Jul 26, 2024

@ma2bd Do you have any thoughts?


/// Whether an optional value is *not* explored again after encountering it once.
///
/// Warning: Disabling this option may lead to the tracing not terminating.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add "in case of recursive data-structures" ?

@ma2bd
Copy link
Contributor

ma2bd commented Aug 2, 2024

@ma2bd Do you have any thoughts?

@juntyr Thanks for your patience, so here is a path forward:

  • Can you double-check that this approach is guaranteed to work for recursion-free data-structures? (If not, is there something we can do?)
  • Maybe improve the warning (see comment inline)
  • Can you add unit tests to cover the new feature?

@ma2bd
Copy link
Contributor

ma2bd commented Aug 2, 2024

On a different note, I don't think further improving the tracing algorithm is going to be easy. It will probably cause massive complications and limitations (like a thread-unsafe global state). Personally, what I'd love for the Rust community is a simpler serde crate specialized in binary formats (no JSON hacks) and where format tracing (for various purposes) is officially supported.

@juntyr
Copy link
Author

juntyr commented Aug 2, 2024

On a different note, I don't think further improving the tracing algorithm is going to be easy. It will probably cause massive complications and limitations (like a thread-unsafe global state). Personally, what I'd love for the Rust community is a simpler serde crate specialized in binary formats (no JSON hacks) and where format tracing (for various purposes) is officially supported.

Yes, I absolutely agree with you on that. I’m a maintainer of ron and last year I added fuzzing support for serde attributes and 99% of its findings are about serde and its limitations for any data format that isn’t like JSON. I think a serde 2.0 could provide per-type shallow type information (since it’s the only library with this much ecosystem penetration to gain proc macro access to most type definitions), which a tracer could then combine into a deep layout.

But I think that may unfortunately be wishful thinking for a while and until then this crate provides the best solution I’ve come across (huge props for that to you)

@juntyr
Copy link
Author

juntyr commented Aug 2, 2024

I’ll look at and implement your suggestions in the next few days :)

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

Successfully merging this pull request may close these issues.

3 participants