From e828968586082447e3d15e79499fd6205aafcefa Mon Sep 17 00:00:00 2001 From: Matjaz Domen Pecan Date: Fri, 17 Apr 2026 15:46:59 +0200 Subject: [PATCH] refactor(parser): extract fill_heredoc_contents visitor helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fill_heredoc_contents in src/parser/helpers.rs was a ~100-line recursive AST visitor with ~12 arms, gated by #[allow(clippy::too_many_lines, clippy::match_same_arms)]. Many arms shared boilerplate: - The `for r in redirects { fill_heredoc_contents(r, lexer); }` loop appeared in 7 arms. - Subshell, BraceGroup, For, and Select all follow the body+redirects shape but were split across two match arms. The refactor introduces six module-private helpers: - fill_each: generic walk over a node slice (absorbs redirect-walks and Pipeline's command-walk) - fill_command: Command arm - fill_if: If arm (takes Option<&mut Node> for else_body via as_deref_mut()) - fill_body_and_redirects: Subshell / BraceGroup / For / Select (one OR-pattern arm) - fill_cond_body_redirects: While / Until - fill_case: Case arm Dispatcher shrinks to ~60 lines. Both #[allow] attributes are dropped — the remaining arms are textually distinct and the function now fits under the 60-line clippy cap. Also adds 3 parable test cases in tests/parable/14_here_documents.tests to pin behavior against future visitor edits: - heredoc in if else branch (exercises else_body.as_deref_mut()) - heredoc in case pattern body (exercises fill_case) - heredoc nested while inside if else (exercises multi-level recursion through the lexer heredoc queue) AST output is byte-identical; all 252 unit/integration + 12 oracle tests pass. Part of #61 (v0.2.0 cycle). Closes #57 Co-Authored-By: Claude Opus 4.7 (1M context) --- src/parser/helpers.rs | 147 ++++++++++++++++---------- tests/parable/14_here_documents.tests | 42 ++++++++ 2 files changed, 132 insertions(+), 57 deletions(-) diff --git a/src/parser/helpers.rs b/src/parser/helpers.rs index b9f0bad..5c6c4d8 100644 --- a/src/parser/helpers.rs +++ b/src/parser/helpers.rs @@ -105,7 +105,6 @@ pub(super) fn make_stderr_redirect() -> Node { } /// Walks an AST node and fills in empty `HereDoc` content from the lexer queue. -#[allow(clippy::too_many_lines, clippy::match_same_arms)] pub(super) fn fill_heredoc_contents(node: &mut Node, lexer: &mut crate::lexer::Lexer) { match &mut node.kind { NodeKind::HereDoc { content, .. } if content.is_empty() => { @@ -117,22 +116,8 @@ pub(super) fn fill_heredoc_contents(node: &mut Node, lexer: &mut crate::lexer::L assignments, words, redirects, - } => { - for a in assignments { - fill_heredoc_contents(a, lexer); - } - for w in words { - fill_heredoc_contents(w, lexer); - } - for r in redirects { - fill_heredoc_contents(r, lexer); - } - } - NodeKind::Pipeline { commands, .. } => { - for c in commands { - fill_heredoc_contents(c, lexer); - } - } + } => fill_command(assignments, words, redirects, lexer), + NodeKind::Pipeline { commands, .. } => fill_each(commands, lexer), NodeKind::List { items } => { for item in items { fill_heredoc_contents(&mut item.command, lexer); @@ -143,16 +128,13 @@ pub(super) fn fill_heredoc_contents(node: &mut Node, lexer: &mut crate::lexer::L then_body, else_body, redirects, - } => { - fill_heredoc_contents(condition, lexer); - fill_heredoc_contents(then_body, lexer); - if let Some(eb) = else_body { - fill_heredoc_contents(eb, lexer); - } - for r in redirects { - fill_heredoc_contents(r, lexer); - } - } + } => fill_if( + condition, + then_body, + else_body.as_deref_mut(), + redirects, + lexer, + ), NodeKind::While { condition, body, @@ -162,44 +144,20 @@ pub(super) fn fill_heredoc_contents(node: &mut Node, lexer: &mut crate::lexer::L condition, body, redirects, - } => { - fill_heredoc_contents(condition, lexer); - fill_heredoc_contents(body, lexer); - for r in redirects { - fill_heredoc_contents(r, lexer); - } - } - NodeKind::Subshell { body, redirects } | NodeKind::BraceGroup { body, redirects } => { - fill_heredoc_contents(body, lexer); - for r in redirects { - fill_heredoc_contents(r, lexer); - } - } - NodeKind::For { + } => fill_cond_body_redirects(condition, body, redirects, lexer), + NodeKind::Subshell { body, redirects } + | NodeKind::BraceGroup { body, redirects } + | NodeKind::For { body, redirects, .. } | NodeKind::Select { body, redirects, .. - } => { - fill_heredoc_contents(body, lexer); - for r in redirects { - fill_heredoc_contents(r, lexer); - } - } + } => fill_body_and_redirects(body, redirects, lexer), NodeKind::Case { patterns, redirects, .. - } => { - for p in patterns { - if let Some(body) = &mut p.body { - fill_heredoc_contents(body, lexer); - } - } - for r in redirects { - fill_heredoc_contents(r, lexer); - } - } + } => fill_case(patterns, redirects, lexer), NodeKind::Negation { pipeline } | NodeKind::Time { pipeline, .. } => { fill_heredoc_contents(pipeline, lexer); } @@ -209,3 +167,78 @@ pub(super) fn fill_heredoc_contents(node: &mut Node, lexer: &mut crate::lexer::L _ => {} } } + +/// Recurses `fill_heredoc_contents` into every node slot. +fn fill_each(nodes: &mut [Node], lexer: &mut crate::lexer::Lexer) { + for n in nodes { + fill_heredoc_contents(n, lexer); + } +} + +/// Walks `Command` constituents: assignments, then words, then redirects. +fn fill_command( + assignments: &mut [Node], + words: &mut [Node], + redirects: &mut [Node], + lexer: &mut crate::lexer::Lexer, +) { + fill_each(assignments, lexer); + fill_each(words, lexer); + fill_each(redirects, lexer); +} + +/// Walks `If` constituents: condition, then-body, optional else-body, +/// then redirects. +fn fill_if( + condition: &mut Node, + then_body: &mut Node, + else_body: Option<&mut Node>, + redirects: &mut [Node], + lexer: &mut crate::lexer::Lexer, +) { + fill_heredoc_contents(condition, lexer); + fill_heredoc_contents(then_body, lexer); + if let Some(eb) = else_body { + fill_heredoc_contents(eb, lexer); + } + fill_each(redirects, lexer); +} + +/// Walks `body` then its trailing redirects. Used by compound commands +/// that carry exactly a body + redirects pair (`Subshell`, `BraceGroup`, +/// `For`, `Select`). +fn fill_body_and_redirects( + body: &mut Node, + redirects: &mut [Node], + lexer: &mut crate::lexer::Lexer, +) { + fill_heredoc_contents(body, lexer); + fill_each(redirects, lexer); +} + +/// Walks `condition`, `body`, then the trailing redirects. Used by +/// `While` and `Until`. +fn fill_cond_body_redirects( + condition: &mut Node, + body: &mut Node, + redirects: &mut [Node], + lexer: &mut crate::lexer::Lexer, +) { + fill_heredoc_contents(condition, lexer); + fill_heredoc_contents(body, lexer); + fill_each(redirects, lexer); +} + +/// Walks each case pattern's optional body, then the trailing redirects. +fn fill_case( + patterns: &mut [crate::ast::CasePattern], + redirects: &mut [Node], + lexer: &mut crate::lexer::Lexer, +) { + for p in patterns { + if let Some(body) = &mut p.body { + fill_heredoc_contents(body, lexer); + } + } + fill_each(redirects, lexer); +} diff --git a/tests/parable/14_here_documents.tests b/tests/parable/14_here_documents.tests index 0f53fca..ca92898 100644 --- a/tests/parable/14_here_documents.tests +++ b/tests/parable/14_here_documents.tests @@ -557,3 +557,45 @@ EOF next ")) --- + + +=== heredoc in if else branch +if true; then cat <