-
Notifications
You must be signed in to change notification settings - Fork 89
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 a recursion limit to background evaluation #1878
Conversation
core/src/eval/mod.rs
Outdated
pub fn eval_permissive(&mut self, rt: RichTerm) -> Vec<EvalError> { | ||
/// - We support a recursion limit, to limit the number of times we recurse into | ||
/// arrays or records. | ||
pub fn eval_permissive(&mut self, rt: RichTerm, recursion_limit: u64) -> Vec<EvalError> { |
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.
Nitpick: should we use usize
instead of u64
? I guess in practice u16
is already amply sufficient, but you might prefer to use a word-aligned value. Using u64
sounds like we need the guarantee to have larger values than u32::MAX
.
core/src/eval/mod.rs
Outdated
@@ -966,7 +969,9 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> { | |||
attrs.pending_contracts.iter().cloned(), | |||
t.pos, | |||
); | |||
inner(slf, acc, value_with_ctr); | |||
if let Some(r) = recursion_limit.checked_sub(1) { |
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.
nitpick: if we test at the very beginning of the function that recursion_limit > 0
(and return immediatly instead), we have to perform the check only once, instead of at each call site (using saturating sub instead of checked_sub)
Adds a recursion limit (currently hardcoded to 128) to the background evaluator in nls. This limit applies to recursion in arrays or records, not to recursive function calls.
This might fix #1875 (it's hard to tell, as I can't reproduce it). But independent of that, it avoids a crash and preserves eval diagnostics.