-
Notifications
You must be signed in to change notification settings - Fork 520
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
Support custom recursion limits at build time #785
base: master
Are you sure you want to change the base?
Changes from 4 commits
eb5e602
f4604c4
9a50224
9a4d460
46c9a1c
03c544e
4f02d84
836a0c6
7f374c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -195,29 +195,25 @@ pub struct DecodeContext { | |
/// How many times we can recurse in the current decode stack before we hit | ||
/// the recursion limit. | ||
/// | ||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
/// or it can be disabled entirely using the `no-recursion-limit` feature. | ||
#[cfg(not(feature = "no-recursion-limit"))] | ||
recurse_count: u32, | ||
pub recurse_count: u32, | ||
seanlinsley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
#[cfg(not(feature = "no-recursion-limit"))] | ||
impl Default for DecodeContext { | ||
#[inline] | ||
fn default() -> DecodeContext { | ||
impl DecodeContext { | ||
pub(crate) fn new(recursion_limit: u32) -> DecodeContext { | ||
DecodeContext { | ||
recurse_count: crate::RECURSION_LIMIT, | ||
#[cfg(not(feature = "no-recursion-limit"))] | ||
recurse_count: recursion_limit, | ||
} | ||
} | ||
} | ||
|
||
impl DecodeContext { | ||
/// Call this function before recursively decoding. | ||
/// | ||
/// There is no `exit` function since this function creates a new `DecodeContext` | ||
/// to be used at the next level of recursion. Continue to use the old context | ||
// at the previous level of recursion. | ||
/// at the previous level of recursion. | ||
#[cfg(not(feature = "no-recursion-limit"))] | ||
#[inline] | ||
pub(crate) fn enter_recursion(&self) -> DecodeContext { | ||
|
@@ -1503,7 +1499,7 @@ mod test { | |
wire_type, | ||
&mut roundtrip_value, | ||
&mut buf, | ||
DecodeContext::default(), | ||
DecodeContext::new(100), | ||
) | ||
.map_err(|error| TestCaseError::fail(error.to_string()))?; | ||
|
||
|
@@ -1575,7 +1571,7 @@ mod test { | |
wire_type, | ||
&mut roundtrip_value, | ||
&mut buf, | ||
DecodeContext::default(), | ||
DecodeContext::new(100), | ||
) | ||
.map_err(|error| TestCaseError::fail(error.to_string()))?; | ||
} | ||
|
@@ -1594,7 +1590,7 @@ mod test { | |
WireType::LengthDelimited, | ||
&mut s, | ||
&mut &buf[..], | ||
DecodeContext::default(), | ||
DecodeContext::new(100), | ||
); | ||
r.expect_err("must be an error"); | ||
assert!(s.is_empty()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,11 +23,6 @@ use bytes::{Buf, BufMut}; | |
|
||
use crate::encoding::{decode_varint, encode_varint, encoded_len_varint}; | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually kinda like having this as a constant. I wonder if we could just make a |
||
|
||
/// Encodes a length delimiter to the buffer. | ||
/// | ||
/// See [Message.encode_length_delimited] for more info. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should make this one camel case like serde does https://serde.rs/container-attrs.html
so
#[prost(recursion_limit = 5)]
etcThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.