Support custom recursion limits at build time #785
Conversation
seanlinsley
force-pushed
the
recursion-limit-macro
branch
2 times, most recently
from
e9633ff
to
b2bb84c
Compare
seanlinsley
force-pushed
the
recursion-limit-macro
branch
from
b2bb84c
to
9a50224
Compare
|
thoughts @LucioFranco @nrc @danburkert? |
| // See `encoding::DecodeContext` for more info. | ||
| // 100 is the default recursion limit in the C++ implementation. | ||
| #[cfg(not(feature = "no-recursion-limit"))] | ||
| const RECURSION_LIMIT: u32 = 100; |
There was a problem hiding this comment.
Since it sounds like this will never change, I opted to inline it everywhere it's used. That allows documentation to say explicitly that the default recursion limit is 100 instead of requiring that users look up this constant.
There was a problem hiding this comment.
I actually kinda like having this as a constant. I wonder if we could just make a constants module that contains just this one. And we can then have all the deep dive docs on the recursion implementation there and then we just need to link to there from the lib doc page. I feel like that would make it easier to maintain down the line and centralize it a bit.
|
Here's a downstream PR using this feature: pganalyze/pg_query.rs#17 Note that to prevent a stack overflow from the recursion, I had to increase the stack size in a separate thread. I wonder if that should be documented here. |
|
@LucioFranco this should be ready for re-review |
|
@LucioFranco ping. happy to make any other changes if needed |
There was a problem hiding this comment.
Sorry for the super long delay on the review here and thank you for the ping (I generally don't mind them if you're waiting for a review). I left a few comments that I would like to see changed but they are pretty minor and I think once those are good we are good to merge. Thank you for pushing through on this!
| let recursion_limit: u32 = if let Some(attr) = input | ||
| .attrs | ||
| .iter() | ||
| .find(|attr| attr.path.is_ident("RecursionLimit")) |
There was a problem hiding this comment.
I wonder if we should make this one camel case like serde does https://serde.rs/container-attrs.html
so #[prost(recursion_limit = 5)] etc
There was a problem hiding this comment.
I'm having trouble getting a bespoke attribute parser working. Would it be okay to pull in darling as a dependency? https://crates.io/crates/darling
Another option is to skip proper attribute parsing and only handle the single attribute we have for now.
| /// The recursion limit is defined by `RECURSION_LIMIT` and cannot be | ||
| /// customized. The recursion limit can be ignored by building the Prost | ||
| /// crate with the `no-recursion-limit` feature. | ||
| /// It defaults to 100 and can be changed using `prost_build::recursion_limit`, |
There was a problem hiding this comment.
So because this is hidden this doc won't actually be readable. So we need to make sure this is documented at the lib level of prost.
| // See `encoding::DecodeContext` for more info. | ||
| // 100 is the default recursion limit in the C++ implementation. | ||
| #[cfg(not(feature = "no-recursion-limit"))] | ||
| const RECURSION_LIMIT: u32 = 100; |
There was a problem hiding this comment.
I actually kinda like having this as a constant. I wonder if we could just make a constants module that contains just this one. And we can then have all the deep dive docs on the recursion implementation there and then we just need to link to there from the lib doc page. I feel like that would make it easier to maintain down the line and centralize it a bit.
This PR allows users to set a custom recursion limit for specific protobuf structures at build time: