Implement transformer parser#66
Merged
Merged
Conversation
Contributor
Reviewer's GuideThis PR adds full support for parsing extern transformer declarations by extending span tracking, introducing a generic extern-declaration collector, implementing transformer-specific parsing logic, exposing a Transformer AST wrapper with accessors, updating parse utilities, and adding comprehensive tests. Class diagram for new and updated AST types (Transformer support)classDiagram
class Root {
+transformers() Vec~Transformer~
}
class Transformer {
+syntax() SyntaxNode
+name() Option~String~
+inputs() Vec~(String, String)~
+outputs() Vec~String~
}
Root --> "*" Transformer
Class diagram for updated ParsedSpans struct (transformer span tracking)classDiagram
class ParsedSpans {
+transformers: Vec~Span~
+transformers() &[Span]
}
Flow diagram for collecting extern transformer declaration spansflowchart TD
A[Tokens + Source] --> B[collect_extern_declarations]
B --> C[collect_transformer_spans]
C --> D[ParsedSpans.transformers]
D --> E[build_green_tree]
E --> F[AST with Transformer nodes]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Summary by CodeRabbit
WalkthroughIntegrate support for parsing, collecting, and handling Changes
Sequence Diagram(s)sequenceDiagram
participant Source
participant Parser
participant CSTBuilder
participant AST
Source->>Parser: Provide tokens (with potential transformer decls)
Parser->>Parser: Collect transformer spans via collect_transformer_spans
Parser->>CSTBuilder: Pass transformer spans to build_green_tree
CSTBuilder->>CSTBuilder: Add transformer nodes to CST
CSTBuilder->>AST: Provide CST with transformer nodes
AST->>AST: Expose transformer nodes via Root::transformers()
AST->>AST: Allow access to name, inputs, outputs of each Transformer
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.rsInstructions used from: Sources: 🧬 Code Graph Analysis (1)src/parser/mod.rs (4)
🔇 Additional comments (7)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/parser/mod.rs:1880` </location>
<code_context>
+
+ /// Name of the transformer if present.
+ #[must_use]
+ pub fn name(&self) -> Option<String> {
+ self.syntax
+ .children_with_tokens()
</code_context>
<issue_to_address>
The name() method assumes a specific token order; consider resilience to formatting.
If there are extra tokens (e.g., whitespace or comments) between K_TRANSFORMER and T_IDENT, this logic may not work as intended. Please verify this aligns with the parser's output in all scenarios.
</issue_to_address>
### Comment 2
<location> `src/parser/mod.rs:834` </location>
<code_context>
}
+/// Collects the spans of transformer declarations in the token stream.
+fn collect_transformer_spans(
+ tokens: &[(SyntaxKind, Span)],
+ src: &str,
</code_context>
<issue_to_address>
Consider refactoring the various extern declaration collectors into a single generic helper function to eliminate duplicated boilerplate.
Here is one way to collapse all of the “extern X‐decl” collectors (functions, transformers, indexes, …) into a single helper, so that `collect_transformer_spans` simply instantiates that helper with its own parser. This preserves every bit of behavior but removes nearly all of the duplicated boilerplate.
```rust
// new in your module
fn collect_extern_declarations<P>(
tokens: &[(SyntaxKind, Span)],
src: &str,
decl_kind: SyntaxKind,
decl_parser: P,
) -> (Vec<Span>, Vec<Simple<SyntaxKind>>)
where
P: Parser<SyntaxKind, Span, Error = Simple<SyntaxKind>> + Clone,
{
type State<'a> = SpanCollector<'a, Vec<Simple<SyntaxKind>>>;
let mut st = State::new(tokens, src, Vec::new());
// this closure handles both the `extern` prefix and skipping
let handler = move |st: &mut State<'_>, span: Span| {
// only parse if the *next* token is the decl we expect
if !matches!(
st.stream.peek_after_ws_inline().map(|t| t.0),
Some(`decl_kind`)
) {
st.skip_line();
return;
}
// build a parser that eats `extern` and then our decl
let ws = inline_ws().repeated();
let parser = just(SyntaxKind::K_EXTERN)
.padded_by(ws.clone())
.ignore_then(decl_parser.clone());
let (res, err) = st.parse_span(parser, span.start);
if let Some(sp) = res {
st.spans.push(span.start..sp.end);
st.stream.skip_until(sp.end);
} else {
st.extra.extend(err);
st.skip_line();
}
};
token_dispatch!(st, { SyntaxKind::K_EXTERN => handler });
st.into_parts()
}
// now your transformer‐specific collector is just one line:
fn collect_transformer_spans(
tokens: &[(SyntaxKind, Span)],
src: &str
) -> (Vec<Span>, Vec<Simple<SyntaxKind>>) {
collect_extern_declarations(
tokens,
src,
SyntaxKind::K_TRANSFORMER,
transformer_decl(), // your existing parser fn
)
}
```
Apply the same pattern to the other `collect_*_spans` funcs: factor out the shared `SpanCollector` setup and `token_dispatch!` logic into `collect_extern_declarations`, and leave each domain‐specific collector as a 1‐liner.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Use a generic helper to parse extern declarations and simplify transformer span collection. Adjust transformer name parsing for whitespace tolerance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
Author
|
@sourcery-ai resolve |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- The manual traversal in Transformer::name and outputs is brittle—consider reusing your existing parse_name_type_pairs or other parser combinators to simplify and harden the AST wrapper logic.
- collect_extern_declarations currently boxes the parser and requires Clone bounds—refactor it to accept parser closures or references to avoid boxing overhead and simplify its signature.
- The AST accessor methods allocate new Strings on each call; consider returning &str slices or text ranges from the underlying syntax to reduce unnecessary allocations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The manual traversal in Transformer::name and outputs is brittle—consider reusing your existing parse_name_type_pairs or other parser combinators to simplify and harden the AST wrapper logic.
- collect_extern_declarations currently boxes the parser and requires Clone bounds—refactor it to accept parser closures or references to avoid boxing overhead and simplify its signature.
- The AST accessor methods allocate new Strings on each call; consider returning &str slices or text ranges from the underlying syntax to reduce unnecessary allocations.
## Individual Comments
### Comment 1
<location> `tests/parser.rs:908` </location>
<code_context>
+
+#[rstest]
+#[expect(clippy::expect_used, reason = "Using expect for clearer test failures")]
+fn transformer_single_parsed(transformer_single_io: &str) {
+ let parsed = parse(transformer_single_io);
+ assert!(parsed.errors().is_empty());
+ let transformers = parsed.root().transformers();
+ assert_eq!(transformers.len(), 1);
+ let t = transformers.first().expect("transformer missing");
+ assert_eq!(t.name(), Some("normalize".into()));
+ assert_eq!(
+ t.inputs(),
+ vec![("input".into(), "UnnormalizedData".into())]
+ );
+ assert_eq!(t.outputs(), vec![String::from("NormalizedData")]);
+}
+
</code_context>
<issue_to_address>
Consider adding tests for edge cases in transformer parsing.
Please add tests for cases like transformers with no inputs or outputs, extra whitespace, duplicate input names, and reserved keywords as input/output names to ensure comprehensive parser coverage.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
#[rstest]
#[expect(clippy::expect_used, reason = "Using expect for clearer test failures")]
fn transformer_single_parsed(transformer_single_io: &str) {
let parsed = parse(transformer_single_io);
assert!(parsed.errors().is_empty());
let transformers = parsed.root().transformers();
assert_eq!(transformers.len(), 1);
let t = transformers.first().expect("transformer missing");
assert_eq!(t.name(), Some("normalize".into()));
assert_eq!(
t.inputs(),
vec![("input".into(), "UnnormalizedData".into())]
);
assert_eq!(t.outputs(), vec![String::from("NormalizedData")]);
}
=======
#[rstest]
#[expect(clippy::expect_used, reason = "Using expect for clearer test failures")]
fn transformer_single_parsed(transformer_single_io: &str) {
let parsed = parse(transformer_single_io);
assert!(parsed.errors().is_empty());
let transformers = parsed.root().transformers();
assert_eq!(transformers.len(), 1);
let t = transformers.first().expect("transformer missing");
assert_eq!(t.name(), Some("normalize".into()));
assert_eq!(
t.inputs(),
vec![("input".into(), "UnnormalizedData".into())]
);
assert_eq!(t.outputs(), vec![String::from("NormalizedData")]);
}
#[rstest]
#[expect(clippy::expect_used, reason = "Using expect for clearer test failures")]
fn transformer_no_inputs() {
let src = "extern transformer no_inputs(): OutputType";
let parsed = parse(src);
assert!(parsed.errors().is_empty());
let transformers = parsed.root().transformers();
assert_eq!(transformers.len(), 1);
let t = transformers.first().expect("transformer missing");
assert_eq!(t.name(), Some("no_inputs".into()));
assert!(t.inputs().is_empty());
assert_eq!(t.outputs(), vec![String::from("OutputType")]);
}
#[rstest]
#[expect(clippy::expect_used, reason = "Using expect for clearer test failures")]
fn transformer_no_outputs() {
let src = "extern transformer no_outputs(input: InputType):";
let parsed = parse(src);
// Should either error or produce a transformer with empty outputs
let transformers = parsed.root().transformers();
if parsed.errors().is_empty() {
assert_eq!(transformers.len(), 1);
let t = transformers.first().expect("transformer missing");
assert_eq!(t.name(), Some("no_outputs".into()));
assert_eq!(t.inputs(), vec![("input".into(), "InputType".into())]);
assert!(t.outputs().is_empty());
} else {
// Acceptable if parser errors on missing outputs
assert!(!parsed.errors().is_empty());
}
}
#[rstest]
#[expect(clippy::expect_used, reason = "Using expect for clearer test failures")]
fn transformer_extra_whitespace() {
let src = " extern transformer spaced ( foo : Bar , baz : Qux ) : Out1 , Out2 ";
let parsed = parse(src);
assert!(parsed.errors().is_empty());
let transformers = parsed.root().transformers();
assert_eq!(transformers.len(), 1);
let t = transformers.first().expect("transformer missing");
assert_eq!(t.name(), Some("spaced".into()));
assert_eq!(
t.inputs(),
vec![
("foo".into(), "Bar".into()),
("baz".into(), "Qux".into())
]
);
assert_eq!(t.outputs(), vec!["Out1".to_string(), "Out2".to_string()]);
}
#[rstest]
#[expect(clippy::expect_used, reason = "Using expect for clearer test failures")]
fn transformer_duplicate_input_names() {
let src = "extern transformer dup_inputs(foo: Bar, foo: Baz): Out";
let parsed = parse(src);
// Should error or handle duplicate input names
if parsed.errors().is_empty() {
let transformers = parsed.root().transformers();
assert_eq!(transformers.len(), 1);
let t = transformers.first().expect("transformer missing");
// Should not allow duplicate input names
let inputs = t.inputs();
let foo_count = inputs.iter().filter(|(name, _)| name == "foo").count();
assert!(foo_count <= 1, "Duplicate input names should not be allowed");
} else {
// Acceptable if parser errors on duplicate names
assert!(!parsed.errors().is_empty());
}
}
#[rstest]
#[expect(clippy::expect_used, reason = "Using expect for clearer test failures")]
fn transformer_reserved_keyword_names() {
let src = "extern transformer reserved(transformer: Type, extern: Type): out";
let parsed = parse(src);
// Should error or handle reserved keywords as names
if parsed.errors().is_empty() {
let transformers = parsed.root().transformers();
assert_eq!(transformers.len(), 1);
let t = transformers.first().expect("transformer missing");
let inputs = t.inputs();
// Check if reserved keywords are present as input names
assert!(!inputs.iter().any(|(name, _)| name == "transformer" || name == "extern"),
"Reserved keywords should not be allowed as input names");
} else {
// Acceptable if parser errors on reserved keywords
assert!(!parsed.errors().is_empty());
}
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@sourcery-ai resolve |
Owner
Author
|
@sourcery-ai review |
SourceryAI
reviewed
Jul 14, 2025
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- The skip_to_transformer_keyword helper duplicates token-skipping logic; consider abstracting a generic skip_to_kind utility to use across different AST wrappers.
- The parse_ident_list function manually tracks parentheses depth to locate the colon; you could leverage the existing balanced_block or a token-based parser combinator to simplify and remove manual depth-tracking.
- The inline_ws().repeated() padding logic is repeated in transformer_decl; extracting a shared padded combinator or alias for common whitespace patterns would DRY up the parser definitions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The skip_to_transformer_keyword helper duplicates token-skipping logic; consider abstracting a generic skip_to_kind utility to use across different AST wrappers.
- The parse_ident_list function manually tracks parentheses depth to locate the colon; you could leverage the existing balanced_block or a token-based parser combinator to simplify and remove manual depth-tracking.
- The inline_ws().repeated() padding logic is repeated in transformer_decl; extracting a shared padded combinator or alias for common whitespace patterns would DRY up the parser definitions.
## Individual Comments
### Comment 1
<location> `src/parser/mod.rs:486` </location>
<code_context>
+ let mut st = State::new(tokens, src, Vec::new());
+
+ let handler = move |st: &mut State<'_>, span: Span| {
+ if st.stream.peek_after_ws_inline().map(|t| t.0) != Some(decl_kind) {
+ st.skip_line();
+ return;
</code_context>
<issue_to_address>
The check for decl_kind after K_EXTERN may skip valid declarations if whitespace/comments intervene.
This logic may fail if comments or other tokens appear between K_EXTERN and the declaration kind. Please ensure the check handles all valid input scenarios.
</issue_to_address>Hi @leynos! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- The parse_ident_list helper is currently specialized for transformer outputs; consider renaming it or documenting its exact contract to clarify its purpose and avoid confusion when reused elsewhere.
- The build_green_tree function now has duplicate maybe_start/maybe_finish calls for transformers similar to other node kinds—consider abstracting this pattern into a helper to reduce boilerplate and simplify future maintenance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The parse_ident_list helper is currently specialized for transformer outputs; consider renaming it or documenting its exact contract to clarify its purpose and avoid confusion when reused elsewhere.
- The build_green_tree function now has duplicate maybe_start/maybe_finish calls for transformers similar to other node kinds—consider abstracting this pattern into a helper to reduce boilerplate and simplify future maintenance.
## Individual Comments
### Comment 1
<location> `docs/documentation-style-guide.md:109` </location>
<code_context>
```
-[^markdownlint]: A linter that enforces consistent Markdown formatting.
+\[^markdownlint\]: A linter that enforces consistent Markdown formatting.
</code_context>
<issue_to_address>
Escaping in footnote reference is unnecessary and may break Markdown rendering.
The '[^markdownlint]:' syntax is correct in Markdown. Please revert the escaping unless there's a specific need for it.
</issue_to_address>
### Comment 2
<location> `src/parser/ast/parse_utils.rs:432` </location>
<code_context>
+/// Parse an identifier list following a colon.
+#[must_use]
+pub(super) fn parse_ident_list<I>(iter: I) -> Vec<String>
+where
+ I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
</code_context>
<issue_to_address>
Consider reusing an existing separated-by combinator and extracting depth counting into a helper to simplify parse_ident_list.
Consider reusing your existing “separated‐by” combinator instead of rolling manual loops/peekables. For example, if you have something like:
```rust
fn parse_separated<I, F, T>(
iter: &mut Peekable<I>,
sep: SyntaxKind,
mut f: F,
) -> Vec<T>
where
I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
F: FnMut(&mut Peekable<I>) -> T,
{ /* … */ }
```
you could rewrite `parse_ident_list` as:
```rust
use rowan::NodeOrToken;
fn skip_to_top_level_colon<I>(iter: &mut Peekable<I>)
where I: Iterator<Item = SyntaxElement<DdlogLanguage>> {
let mut depth = 0;
while let Some(elem) = iter.next() {
match elem.kind() {
SyntaxKind::T_LPAREN => depth += 1,
SyntaxKind::T_RPAREN => depth = depth.saturating_sub(1),
SyntaxKind::T_COLON if depth == 0 => break,
_ => {}
}
}
}
pub(super) fn parse_ident_list<I>(iter: I) -> Vec<String>
where
I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
{
let mut iter = iter.peekable();
skip_to_top_level_colon(&mut iter);
// consume the colon itself
iter.next();
parse_separated(&mut iter, SyntaxKind::T_COMMA, |it| {
skip_whitespace_and_comments(it);
match it.next() {
Some(NodeOrToken::Token(t)) if t.kind() == SyntaxKind::T_IDENT => {
t.text().to_string()
}
_ => unreachable!("expected identifier after comma"),
}
})
}
```
This:
- Moves your “depth counting” into a small helper (`skip_to_top_level_colon`)
- Delegates comma-separated iteration to `parse_separated`
- Keeps whitespace/comments skipping inside the short closure
Result: identical behavior with far less procedural boilerplate.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@sourcery-ai resolve |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
transformerspan tracking and CST nodeTransformerAST wrapper with accessor methodsTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_687194d20f708322999ef4e1be9e7b8e
Summary by Sourcery
Implement transformer parsing end-to-end by tracking transformer spans in the parser, enriching the AST with a Transformer node and accessors, and adding helper utilities and tests to validate transformer declaration parsing.
New Features:
Enhancements:
Tests: