When parsing, don't swallow recursion depth errors. #284
Conversation
This is necessary for both performance and correctness reasons. At many points when parsing symbols a failure to parse as one construct is retried as a different construct. This makes sense when parsing fails because the input does not match the first construct. It does not make sense when the input triggers the recursion limits. 1. Because the input might have been correctly parsed on the path that was aborted due to the recursion limits, any other output produced may be wrong. 2. Even the error returned might be wrong, if the correct path was aborted due to recursion limits and the last path tried was aborted for a different error. 3. Attempting to try additional constructs after a recursion error increases the runtime, in some cases significantly.
| if let Ok((name, tail)) = try_recurse!(Name::parse(ctx, subs, input)) { | ||
| if let Ok((ty, tail)) = try_recurse!(BareFunctionType::parse(ctx, subs, tail)) { |
There was a problem hiding this comment.
Ah it is unfortunate that we can't use ? at all because of the if let Ok pattern. I don't see a better approach though.
There was a problem hiding this comment.
I guess another option would be some sort of helper like
fn parse<'a, 'b, P: Parse>(ctx: &'a ParseContext, subs: &'a mut SubstitutionTable, input: IndexStr<'b>) -> Result<Result<(P, IndexStr<'b>)>> {
match P::parse(ctx, subs, input) {
Err(Error::TooMuchRecursion) => Err(Error::TooMuchRecursion),
x => Ok(x),
}
}and then we could use it like
if let Ok((name, tail)) = parse::<Name>(ctx, subs, input)? {
// ...
}This is a little less macro-y and lets us use ?.
What do you think of that?
There was a problem hiding this comment.
I'm not really a fan of this because there are many sites where all Foo::parse errors are propagated immediately, and then it's not clear where you need to use this helper and where you don't.
I think what we really want (and what doesn't exist) is a tri-state Result with Ok/Recoverable/Irrecoverable errors and two ? operators one that propagates Irrecoverable only and one that propagates both.
There was a problem hiding this comment.
I think what we really want (and what doesn't exist) is a tri-state Result with Ok/Recoverable/Irrecoverable errors and two ? operators one that propagates Irrecoverable only and one that propagates both.
I guess I was imagining that Result<Result<T>> would be that tri-state where
Ok(Ok(_))was successOk(Err(_))was recoverable error- and
Err(_)was irrecoverable error
But again, no strong opinions!
This is necessary for both performance and correctness reasons. At many points when parsing symbols a failure to parse as one construct is retried as a different construct. This makes sense when parsing fails because the input does not match the first construct. It does not make sense when the input triggers the recursion limits.
This would fix the performance issue in #282 and probably in #277 (though the demangling failure there was caused by a cpp_demangle bug not the recursion limits). This should probably be reviewed though.