Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/complexity-antipatterns-and-refactoring-strategies.md
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,9 @@ engineering principles from the outset.

6. **Return Early / Guard Clauses:** To avoid deep nesting for validation or
pre-condition checks, process exceptional cases first and return early.8
This approach flattens the main logic path and reduces nesting. For example, instead of:
This approach flattens the main logic path and reduces nesting. For example,
instead of:

```cpp
void process(Data d) {
if (d.isValid()) { // +1 (if)
Expand Down
2 changes: 1 addition & 1 deletion docs/function-parsing-design.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ reached.
Missing colons between a parameter name and type trigger
`ParseError::MissingColon`. The span of the terminating comma or parenthesis is
attached, so diagnostics point at the error. Helper functions
`collect_parameter_name` and `finalise_parameter` keep the main loop small.
`collect_parameter_name` and `ParameterBuilder` keep the main loop small.

Empty names and types are reported with `ParseError::MissingName` and
`ParseError::MissingType`. Both `collect_parameter_name` and `parse_type_expr`
Expand Down
11 changes: 5 additions & 6 deletions src/parser/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

use rowan::SyntaxElement;

use self::parse_utils::is_trivia;
use crate::{DdlogLanguage, SyntaxKind};

#[cfg_attr(
Expand Down Expand Up @@ -100,12 +101,13 @@ fn take_first_ident(
) -> Option<String> {
use rowan::NodeOrToken;
for e in iter {
if is_trivia(&e) {
continue;
}
Comment on lines +104 to +106
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ New issue: Bumpy Road Ahead
take_first_ident has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function

Suppress

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leynos, the "Bumpy Road Ahead" issue occurs because take_first_ident now has nested conditional logic: the for loop contains both an if is_trivia(&e) check and a match expression, creating 2 levels of nesting.

Here's a fix that uses iterator combinators to eliminate the nested structure:

fn take_first_ident(
    iter: impl Iterator<Item = rowan::SyntaxElement<DdlogLanguage>>,
) -> Option<String> {
    use rowan::NodeOrToken;
    
    iter.filter(|e| !is_trivia(e))
        .find_map(|e| match e {
            NodeOrToken::Token(t) if t.kind() == SyntaxKind::T_IDENT => {
                Some(t.text().to_string())
            }
            _ => None,
        })
}

AI Coding Agent Prompt:

Refactor the `take_first_ident` function in src/parser/ast/mod.rs to reduce nested conditional logic by using iterator combinators instead of explicit loops and conditionals.

Current implementation has nested structure:
- for loop
  - if is_trivia check with continue
  - match expression

Replace with:
1. Use `.filter(|e| !is_trivia(e))` to skip trivia elements
2. Use `.find_map()` with the existing match logic to find and extract the first identifier
3. Remove the explicit for loop, if statement, and continue statement

The refactored function should maintain identical behavior while reducing cognitive complexity through functional programming patterns.

This approach eliminates the nested conditional logic by leveraging Rust's iterator combinators, making the code more declarative and easier to follow.

match e {
NodeOrToken::Token(t) if t.kind() == SyntaxKind::T_IDENT => {
return Some(t.text().to_string());
}
NodeOrToken::Token(t)
if matches!(t.kind(), SyntaxKind::T_WHITESPACE | SyntaxKind::T_COMMENT) => {}
_ => return None,
}
}
Expand All @@ -120,10 +122,7 @@ pub(super) fn skip_whitespace_and_comments<I>(iter: &mut std::iter::Peekable<I>)
where
I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
{
while matches!(
iter.peek().map(SyntaxElement::kind),
Some(SyntaxKind::T_WHITESPACE | SyntaxKind::T_COMMENT)
) {
while matches!(iter.peek(), Some(e) if is_trivia(e)) {
iter.next();
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
Expand Down
6 changes: 3 additions & 3 deletions src/parser/ast/parse_utils/errors.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
//! Error types and helpers for delimiter tracking during type parsing.
//!
//! The functions in `type_parsing` rely on these structures to report
//! mismatched or unclosed delimiters when reading parameter lists and type
//! expressions.
//! The functions in `params`, `outputs`, and `type_expr` rely on these
//! structures to report mismatched or unclosed delimiters when reading
//! parameter lists and type expressions.

use rowan::TextRange;

Expand Down
16 changes: 11 additions & 5 deletions src/parser/ast/parse_utils/mod.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,23 @@
//! Parsing helpers shared across AST modules.
//!
//! This module provides small utilities for collecting parameter names and
//! types from the CST and for recursively parsing type expressions. Both the
//! `Function` and `Relation` nodes import these helpers so they can share the
//! same logic when interpreting their declarations. See
//! types from the CST, recursively parsing type expressions, and parsing
//! transformer output identifiers. `Function`, `Relation`, and `Transformer`
//! nodes import these helpers so they can share the same logic when
//! interpreting their declarations. See
//! `docs/function-parsing-design.md` for an overview.

mod delimiter;
pub(crate) mod errors;
mod outputs;
mod params;
mod token_utils;
mod type_parsing;
mod type_expr;

pub use delimiter::extract_parenthesized;
pub(crate) use delimiter::paren_block_span;

pub(crate) use type_parsing::{parse_name_type_pairs, parse_output_list, parse_type_after_colon};
pub(crate) use outputs::parse_output_list;
pub(crate) use params::parse_name_type_pairs;
pub(crate) use token_utils::is_trivia;
pub(crate) use type_expr::parse_type_after_colon;
97 changes: 97 additions & 0 deletions src/parser/ast/parse_utils/outputs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
//! Output list parsing utilities.
//!
//! Parses comma-separated output relation names following a transformer
//! declaration's colon.

use rowan::SyntaxElement;

use crate::{DdlogLanguage, SyntaxKind};

use super::super::skip_whitespace_and_comments;

pub(crate) fn skip_to_top_level_colon<I>(iter: &mut std::iter::Peekable<I>)
where
I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
{
let mut depth = 0usize;
for e in iter.by_ref() {
match e.kind() {
SyntaxKind::T_LPAREN => depth += 1,
SyntaxKind::T_RPAREN => depth = depth.saturating_sub(1),
SyntaxKind::T_COLON if depth == 0 => break,
_ => {}
}
}
}

fn try_parse_identifier<I>(iter: &mut std::iter::Peekable<I>) -> Option<String>
where
I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
{
use rowan::NodeOrToken;

skip_whitespace_and_comments(iter);
match iter.next() {
Some(NodeOrToken::Token(t)) if t.kind() == SyntaxKind::T_IDENT => {
Some(t.text().to_string())
}
_ => None,
}
}

fn has_comma_separator<I>(iter: &mut std::iter::Peekable<I>) -> bool
where
I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
{
use rowan::NodeOrToken;

skip_whitespace_and_comments(iter);
if let Some(NodeOrToken::Token(t)) = iter.peek()
&& t.kind() == SyntaxKind::T_COMMA
{
iter.next();
true
} else {
false
}
}

pub(crate) fn parse_output_list<I>(iter: I) -> Vec<String>
where
I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
{
let mut iter = iter.peekable();
skip_to_top_level_colon(&mut iter);

let mut names = Vec::new();
while let Some(name) = try_parse_identifier(&mut iter) {
names.push(name);
if !has_comma_separator(&mut iter) {
break;
}
}

names
}

#[cfg(test)]
mod tests {
use super::*;
use crate::parser::ast::AstNode;
use crate::parser::parse;

#[test]
fn collects_output_names() {
let src = "extern transformer t(a: X): Out1, Out2";
let parsed = parse(src);
#[expect(clippy::expect_used, reason = "Using expect for clearer test failures")]
let tr = parsed
.root()
.transformers()
.first()
.cloned()
.expect("transformer missing");
let names = parse_output_list(tr.syntax().children_with_tokens());
assert_eq!(names, vec!["Out1".to_string(), "Out2".to_string()]);
}
}
Loading