Support custom recursion limits at build time #785

seanlinsley · 2022-12-15T22:22:42Z

This PR allows users to set a custom recursion limit for specific protobuf structures at build time:

let mut config = prost_build::Config::new();
config.recursion_limit("my_messages.MyMessageType", 1000);

seanlinsley · 2022-12-16T02:43:05Z

thoughts @LucioFranco @nrc @danburkert?

src/message.rs

seanlinsley · 2023-01-03T18:14:01Z

src/lib.rs

-// 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;


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.

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.

🤷 I'm not sure if a constants module adds much. If you still feel a constant is necessary, I'd lean towards simply adding back the constant here in lib.rs

seanlinsley · 2023-01-11T18:02:32Z

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

Overall LGTM! Some small suggestions and we can get this merged. Thanks for the patience I went on vacation in between :)

src/encoding.rs

src/message.rs

seanlinsley · 2023-02-27T15:10:06Z

@LucioFranco this should be ready for re-review

seanlinsley · 2023-04-04T15:04:28Z

@LucioFranco ping. happy to make any other changes if needed

LucioFranco

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!

LucioFranco · 2023-04-05T20:22:20Z

prost-derive/src/lib.rs

+    let recursion_limit: u32 = if let Some(attr) = input
+        .attrs
+        .iter()
+        .find(|attr| attr.path.is_ident("RecursionLimit"))


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

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.

Two years later and I finally got around to this. The code now uses #[prost(recursion_limit = 5)]

LucioFranco · 2023-04-05T20:22:50Z

src/encoding.rs

-    /// 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`,


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.

I've added a mention to the README

LucioFranco · 2023-04-05T20:24:15Z

src/lib.rs

-// 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;


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.

caspermeijn

Thank you for coming back to this after two years. I see why this is useful and this is a smart solution.

I am a bit concerned whether this could be a breaking change. I would like to see some prove that it is not.

caspermeijn · 2025-06-10T06:27:29Z

README.md

 - `derive`: Enable integration with `prost-derive`. Disable this feature to reduce compile times. This feature is enabled by default.
 - `prost-derive`: Deprecated. Alias for `derive` feature.
- `no-recursion-limit`: Disable the recursion limit. The recursion limit is 100 and cannot be customized. 
+- `no-recursion-limit`: Disable the recursion limit. The recursion limit is 100, and can be changed with `prost_build::recursion_limit`.


That should be prost_build::Config::recursion_limit, right?

caspermeijn · 2025-06-10T06:31:13Z

prost-derive/src/field/mod.rs


 /// Get the items belonging to the 'prost' list attribute, e.g. `#[prost(foo, bar="baz")]`.
-fn prost_attrs(attrs: Vec<Attribute>) -> Result<Vec<Meta>, Error> {
+pub(super) fn prost_attrs(attrs: Vec<Attribute>) -> Result<Vec<Meta>, Error> {


Just to verify: this doesn't make the function visible outside this crate, right?

Right: pub(super) makes an item visible to the parent module

caspermeijn · 2025-06-10T06:34:23Z

prost-derive/src/lib.rs

                #(#clear;)*
            }
+
+            fn recursion_limit() -> u32 {


Is this a breaking change for messages with a field named recursion_limit?

I think for enum fields, an accessor is created. However, this function is in a trait, so not directly accessible.

I would like to see a test case for that.

I'm not sure I follow: how could this possibly affect a field named recursion_limit? This is implemented on the Message trait, not on the struct itself.

caspermeijn · 2025-06-10T06:35:55Z

prost/src/encoding.rs

+    /// or it can be disabled entirely using the `no-recursion-limit` feature.
    #[cfg(not(feature = "no-recursion-limit"))]
-    recurse_count: u32,
+    #[doc(hidden)]


Why is this marked as doc hidden?

@LucioFranco previously suggested that it should be doc(hidden) because the field was changed to public. I assume a previous version of this PR needed it to be public but it seems like it's no longer needed. I'll revert this field back to private.

caspermeijn · 2025-06-10T06:43:19Z

prost/src/message.rs

+    /// The recursion limit for decoding protobuf messages.
+    ///
+    /// Defaults to 100. Can be customized in your build.rs or by using the no-recursion-limit crate feature.
+    fn recursion_limit() -> u32


Is this a breaking change? I feel like it is not, because you add a default implementation. However rust rules for breaking change are not always logical. Could you verify this with cargo-semver-checks?

My understanding is that it's not a breaking change because it has a default implementation.

I tried running cargo-semver-checks locally and it highlighted #1247 as being a breaking change, but doesn't mention anything about this PR:

--- failure feature_missing: package feature removed or renamed --- Description: A feature has been removed from this package's Cargo.toml. This will break downstream crates which enable that feature. ref: https://doc.rust-lang.org/cargo/reference/semver.html#cargo-feature-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/feature_missing.ron Failed in: feature prost-derive in the package's Cargo.toml Summary semver requires new major version: 1 major and 0 minor checks failed

Maybe this check should run on CI?

seanlinsley force-pushed the recursion-limit-macro branch 3 times, most recently from b2bb84c to 9a50224 Compare December 15, 2022 22:58

LucioFranco requested changes Dec 16, 2022

View reviewed changes

src/message.rs Outdated Show resolved Hide resolved

LucioFranco mentioned this pull request Dec 16, 2022

Support custom recursion limits at runtime #760

Closed

1 task

seanlinsley commented Jan 3, 2023

View reviewed changes

seanlinsley requested a review from LucioFranco January 3, 2023 18:36

seanlinsley mentioned this pull request Jan 11, 2023

Support parsing complex queries with deeply nested ASTs pganalyze/pg_query.rs#17

Open

LucioFranco requested changes Feb 4, 2023

View reviewed changes

src/encoding.rs Outdated Show resolved Hide resolved

src/message.rs Outdated Show resolved Hide resolved

seanlinsley requested a review from LucioFranco February 27, 2023 15:10

LucioFranco requested changes Apr 5, 2023

View reviewed changes

jszwedko mentioned this pull request May 7, 2024

fix(vector source protobuf codec disk buffering) Build prost with no-recursion-limit feature vectordotdev/vector#19413

Closed

seanlinsley force-pushed the recursion-limit-macro branch 4 times, most recently from 538a0ea to 08caf43 Compare May 29, 2025 17:40

Support custom recursion limits at build time

7fc6689

seanlinsley force-pushed the recursion-limit-macro branch from 08caf43 to 7fc6689 Compare May 29, 2025 17:44

seanlinsley requested a review from LucioFranco May 29, 2025 18:13

LucioFranco requested a review from caspermeijn June 9, 2025 20:47

caspermeijn requested changes Jun 10, 2025

View reviewed changes

review changes

86b9439

Support custom recursion limits at build time #785

Are you sure you want to change the base?

Support custom recursion limits at build time #785

Uh oh!

Conversation

seanlinsley commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanlinsley commented Dec 16, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanlinsley commented Jan 11, 2023

Uh oh!

LucioFranco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

seanlinsley commented Feb 27, 2023

Uh oh!

seanlinsley commented Apr 4, 2023

Uh oh!

LucioFranco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caspermeijn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seanlinsley commented Dec 15, 2022 •

edited

Loading