recursion_limit intended use is unclear

[allancalix]

Description

This issue may not actually be a bug but i've found the recursion_limit option on the parser to not work the way I expect. My expectation is that this feature is guarding against stack overflows and should correlate to the number of stack frames deep the parser is while parsing but this doesn't appear to be the case.

For example, the following query fails when the recursion limit is set to 5:

    {
        field
        field2
        field3
        field4
        field5
        field6
    }

    let parser = Parser::new(QUERY)
        .recursion_limit(5)
        .parse()

When digging into the code, it appears that sibling fields happen sequentially (allocating a field(p) stack, returning, and then allocating another one for the next field) meaning that I would expect the maximum recursion depth to be ~2 for this query but instead I get Iter([ERROR@75:75 "parser limit(5) reached" ]).

I admit this could be my misunderstanding this feature but I'd appreciate some clarity on how this feature should be used.

Steps to reproduce

You can see the full example here.

Expected result

Sibling fields should reset the recursion depth after returning for a single selection set.

Actual result

Sibling fields do not reset the recursion depth and selection sets with many fields (possibly an arbitrarily large number given the ability to use aliases) hit the recursion limit.

Environment

• Operating system and version: macos
• Shell (bash/zsh/powershell): fish
• apollo-rs crate: parser
• Crate version: HEAD as of 2022/04/04

[allancalix]

It seems like this is fixed by the pending PR: #462.

[goto-bus-stop]

You're correct about what the recursion limit is for. Currently field parsing is implemented with recursion:

apollo-rs/crates/apollo-parser/src/parser/grammar/field.rs

Lines 45 to 50 in 8d071eb

	match p.peek() {
	Some(TokenKind::Name) => {
	guard.finish_node();
	field(p)
	}

Inside field(), after parsing one field, it parses the next one by calling itself. So it's intended that the recursion count increases on sibling fields.

I do want to get rid of that recursion though, ideally the field() parser would only parse one field at a time and multiple fields would be handled by the caller in a simple loop.

[goto-bus-stop]

I see the code you linked in selection.rs now--maybe it would already work if we just got rid of the recursion in field(), 😆

PR: #519