Skip to content
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

Limit nested filters to avoid stack overflow #979

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

djc
Copy link
Owner

@djc djc commented Mar 7, 2024

No description provided.

@GuillaumeGomez
Copy link
Contributor

Can you add a ui test to see the error output too please?

@GuillaumeGomez
Copy link
Contributor

I think you forgot the ui stderr file. ;)

@djc
Copy link
Owner Author

djc commented Mar 7, 2024

I haven't actually used the ui tests before... I finally figured out how to work the system, but not sure how to get a better error message.

@GuillaumeGomez
Copy link
Contributor

Like I did in #954: by providing your own. :)

@djc
Copy link
Owner Author

djc commented Mar 7, 2024

That's providing a separate error, but not inspecting the nom error to refine the error. Made an attempt which doesn't seem to work. Not sure how much more time I want to spend on this.

@Kijewski
Copy link
Contributor

Kijewski commented Mar 7, 2024

Can you try

    fn filtered(i: &'a str, mut level: Level) -> ParseResult<'a, Self> {
        #[allow(clippy::type_complexity)]
        fn filter<'a>(
            i: &'a str,
            level: &mut Level,
        ) -> ParseResult<'a, (&'a str, Option<Vec<Expr<'a>>>)> {
            let (i, _) = char('|')(i)?;
            *level = level.nest(i)?.1;
            pair(ws(identifier), opt(|i| Expr::arguments(i, *level, false)))(i)
        }

        let (mut i, mut res) = Self::prefix(i, level)?;
        while let (j, Some((fname, args))) = opt(|i| filter(i, &mut level))(i)? {
            i = j;
            res = Self::Filter(fname, {
                let mut args = match args {
                    Some(inner) => inner,
                    None => Vec::new(),
                };
                args.insert(0, res);
                args
            })
        }
        Ok((i, res))
    }

@GuillaumeGomez
Copy link
Contributor

If it becomes too much of a hassle for you, I can take over this PR. ;)

@djc
Copy link
Owner Author

djc commented Apr 5, 2024

@Kijewski that worked well, thanks!

I don't trust that the new ui test is actually working well? Feels like it shouldn't be triggering here (since the Level-based nesting should allow greater depth than the previous approach).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants