fix(parser): repeated ( / parenthesis / opened sub-expressions causes memory leak#16204
fix(parser): repeated ( / parenthesis / opened sub-expressions causes memory leak#16204Bahex merged 2 commits intonushell:mainfrom
( / parenthesis / opened sub-expressions causes memory leak#16204Conversation
| .iter() | ||
| .any(|e| match e { | ||
| ParseError::Unclosed(right, _) if right == ")" => true, | ||
| ParseError::Unbalanced(left, right, _) if left == "(" && right == ")" => true, | ||
| _ => false, | ||
| }); |
There was a problem hiding this comment.
Looking back at this, do we need the change to .first().is_some_and() from .any() at all?
If the first error is one of the paren related ones (expanded back in #16235) it will short circuit the same. If there is another error in the given span that is handled by parse_paren_expr it would also stop trying if there is a balancing error following. Would that be more tolerant?
There was a problem hiding this comment.
If one of these errors appear after any other error, it means the outer expression was parsed successfully but actually parsing its content raised another error before raising one of these two.
In that case, attempting to parse the expression as a bare word interpolation wouldn't make sense.
| if malformed_subexpr { | ||
| working_set.parse_errors.truncate(starting_error_count); | ||
| parse_string(working_set, span) | ||
| parse_string_interpolation(working_set, span) |
There was a problem hiding this comment.
Can you explain this particular change?
There was a problem hiding this comment.
Any bare word expression that's passed to parse_paren_expr always includes a sub-expression, so parse_string would call parse_string_interpolation with any input it could be called with here.
I'm not sure why I initially implemented it with parse_string in the first place, but that contributed to deep call stack problem this PR solves. Calling parse_string_interpolation should make what happens here a little more clear while avoiding an unnecessary call.
|
|
||
| let expr = parse_full_cell_path(working_set, None, span); | ||
| output.push(expr); | ||
| if delimiter_stack.is_empty() { |
There was a problem hiding this comment.
Making this conditional makes intuitive sense to me :D
Regression from #16204 Before:  After:  # Tests + Formatting +1 --------- Co-authored-by: Bahex <17417311+Bahex@users.noreply.github.com>
Regression from #16204 Before:  After:  # Tests + Formatting +1 --------- Co-authored-by: Bahex <17417311+Bahex@users.noreply.github.com>
Regression from nushell#16204 Before:  After:  # Tests + Formatting +1 --------- Co-authored-by: Bahex <17417311+Bahex@users.noreply.github.com>
…es memory leak (nushell#16204) - fixes nushell#16186 # Description Don't attempt further parsing when there is no complete sub-expression. --------- Co-authored-by: Bahex <17417311+Bahex@users.noreply.github.com>
Regression from nushell#16204 Before:  After:  # Tests + Formatting +1 --------- Co-authored-by: Bahex <17417311+Bahex@users.noreply.github.com>
(/ opened subexpressions goes OOM #16186Description
Don't attempt further parsing when there is no complete sub-expression.