feat(tdl): Add ANTLR actions to generate the AST during the parsing. - #210
Conversation
WalkthroughExports a new parser Exception header, rewrites the TDL grammar to construct AST nodes during parsing, updates generated ANTLR parser/lexer/visitor files to carry AST retvals and a new FuncDefs rule, changes the parse API to return a TranslationUnit AST (with exception-to-Error conversion), and tightens tests to assert AST serialization and specific error cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Lexer
participant Parser
participant Listeners as Error listeners
participant AST as TranslationUnit
participant Ex as Exception
Client->>Parser: parse_translation_unit_from_istream(input)
Parser->>Lexer: tokenize(input)
Lexer-->>Parser: tokens
Note over Parser: Parser builds AST during rule actions\n(each rule produces retval nodes with SourceLocation)
Parser->>Parser: translationUnit()
alt AST built without throwing
Parser-->>AST: context->tu (unique_ptr)
Parser->>Listeners: check lexer/parser listeners
alt listeners report error
Parser-->>Client: return Error(from listeners)
else
Parser-->>Client: return moved AST (success)
end
else AST construction throws
Parser-->>Ex: throw Exception(error_code, location)
Parser->>Listeners: check lexer/parser listeners
alt listeners report error
Parser-->>Client: return Error(from listeners)
else
Ex-->>Parser: to_error()
Parser-->>Client: return Error(from Exception)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.cpp (1)
2-11: Generated file — headers appear due to grammar @Header; no manual editsGiven our team’s policy to avoid linting generated sources, this looks fine. If you adopt @parser::header as suggested in the grammar, these includes will disappear from this file on the next generation.
🧹 Nitpick comments (19)
src/spider/tdl/parser/Exception.hpp (1)
67-69: Improve end-user message content carried into Errorwhat() returns a fixed string, so to_error() currently loses useful context (e.g., the std::error_code’s message). Recommend threading the error_code.message() into the Error message to simplify downstream reporting.
Apply this diff:
- [[nodiscard]] auto to_error() const -> Error { - return Error{std::string{what()}, m_source_location, m_error_code}; - } + [[nodiscard]] auto to_error() const -> Error { + std::string msg = std::string{what()}; + if (m_error_code) { + msg.append(": ").append(m_error_code.message()); + } + return Error{std::move(msg), m_source_location, m_error_code}; + }src/spider/tdl/parser/TaskDefLang.g4 (4)
3-13: Scope the injected headers to the parser only to avoid polluting generated lexer/visitor unitsUsing @Header injects these includes into all generated artifacts (lexer, parser, visitors). The lexer doesn’t need AST/Exception headers, and this increases rebuild surface. Prefer @parser::header (and only add a @lexer::header if you truly need something in the lexer).
Apply this diff:
-@header { +@parser::header { #include <memory> #include <utility> #include <vector> #include <spider/tdl/parser/ast/FloatSpec.hpp> #include <spider/tdl/parser/ast/IntSpec.hpp> #include <spider/tdl/parser/ast/nodes.hpp> #include <spider/tdl/parser/Exception.hpp> #include <spider/tdl/parser/SourceLocation.hpp> }If any lexer-specific include is required later, add a separate, minimal:
@lexer::header { // (keep empty for now) }
15-35: Avoid C++ keyword collisions: rename rule ‘namespace’ to ‘namespaceDecl’ANTLR usually suffixed C++ keywords in generated method names (e.g., namespace_()), but relying on keyword workarounds hurts readability and IDE navigation. Renaming the rule and references improves clarity.
Apply these diffs:
-translationUnit returns [std::unique_ptr<spider::tdl::parser::ast::TranslationUnit> tu] +translationUnit returns [std::unique_ptr<spider::tdl::parser::ast::TranslationUnit> tu] @init { $tu = spider::tdl::parser::ast::TranslationUnit::create({ $ctx->start->getLine(), $ctx->start->getCharPositionInLine() }); } -: (namespace { - auto const ns_loc{$namespace.retval->get_source_location()}; +: (namespaceDecl { + auto const ns_loc{$namespaceDecl.retval->get_source_location()}; spider::tdl::parser::Exception::throw_tryv( - $tu->add_namespace(std::move($namespace.retval)), + $tu->add_namespace(std::move($namespaceDecl.retval)), ns_loc ); } | structDef { auto const struct_loc{$structDef.retval->get_source_location()}; spider::tdl::parser::Exception::throw_tryv( $tu->add_struct_spec(std::move($structDef.retval)), struct_loc ); })* EOF-namespace returns [std::unique_ptr<spider::tdl::parser::ast::Node> retval] +namespaceDecl returns [std::unique_ptr<spider::tdl::parser::ast::Node> retval] : 'namespace' id '{' funcDefs '}' { SourceLocation const loc{ $ctx->start->getLine(), $ctx->start->getCharPositionInLine() }; $retval = spider::tdl::parser::Exception::throw_tryx( spider::tdl::parser::ast::Namespace::create( std::move($id.retval), std::move($funcDefs.retval), loc ), loc ); }Also applies to: 37-52
92-99: params: explicit @init for clarity (nit)You’re clearing $retval in the empty alt; adding an @init { $retval.clear(); } conveys intent, avoids accidental reuse if ANTLR rewrites alts.
Proposed small change:
-params returns [std::vector<std::unique_ptr<spider::tdl::parser::ast::Node>> retval] -: namedVarList { +params returns [std::vector<std::unique_ptr<spider::tdl::parser::ast::Node>> retval] +@init { $retval.clear(); } +: namedVarList { $retval = std::move($namedVarList.retval); } | { - $retval.clear(); + /* empty params already handled by @init */ } ;
54-63: Function definitions aggregation: also consider ordering guarantees (nit)Your approach preserves source order. After the EBNF refactor, you’ll still preserve it via push order. Just a heads-up to keep tests asserting order where relevant.
src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.h (1)
2-10: Move AST and Error Headers into the Parser-Only Header BlockThe generated
TaskDefLangLexer.h(and related lexer files) still pull in:
<spider/tdl/parser/ast/FloatSpec.hpp><spider/tdl/parser/ast/IntSpec.hpp><spider/tdl/parser/ast/nodes.hpp><spider/tdl/parser/Exception.hpp><spider/tdl/parser/SourceLocation.hpp>Yet none of these types are ever referenced by the lexer (grep found no usages in lexer files), so they simply bloat rebuilds and couple the lexer to AST/error headers.
Action items:
- In your
.g4(e.g.TaskDefLang.g4), remove AST/error includes from the global@headerblock.- Add a
@parser::headerblock containing only those AST and error headers.- Keep only minimal shared includes (e.g.
<vector>) in the global@header.- Regenerate all ANTLR artifacts—do not hand-edit the generated files.
Example grammar adjustment:
--- TaskDefLang.g4 - @header { - #include <vector> - #include <spider/tdl/parser/ast/FloatSpec.hpp> - #include <spider/tdl/parser/ast/IntSpec.hpp> - #include <spider/tdl/parser/ast/nodes.hpp> - #include <spider/tdl/parser/Exception.hpp> - #include <spider/tdl/parser/SourceLocation.hpp> - } + @header { + #include <vector> + } + + @parser::header { + #include <spider/tdl/parser/ast/FloatSpec.hpp> + #include <spider/tdl/parser/ast/IntSpec.hpp> + #include <spider/tdl/parser/ast/nodes.hpp> + #include <spider/tdl/parser/Exception.hpp> + #include <spider/tdl/parser/SourceLocation.hpp> + }This decouples the lexer from AST/error types and avoids unnecessary rebuilds.
src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.h (1)
2-10: Trim heavy includes from the public visitor interface.This header declares only an interface; it doesn’t need AST node definitions or Exception/SourceLocation. Pulling them in here increases transitive dependencies for every includer.
Action: relocate these includes to the parser implementation or a dedicated builder, or switch the grammar’s header block to the parser-only section so they don’t land in the visitor interface.
src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.cpp (1)
2-10: Non-essential includes in TU—keep, but consider narrowing.In a .cpp, extra includes are less harmful, but you likely don’t need to pull in all AST headers and Exception/SourceLocation if this translation unit only wires up the visitor base. If these landed via a global @Header block, consider scoping them to @parser::header to minimise churn.
src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (1)
2-10: Reduce header bloat in BaseVisitor.Similar to TaskDefLangVisitor.h, BaseVisitor does not require full AST/Exception headers. Relocate or narrow includes via the grammar’s header sections to keep the public surface lean.
src/spider/tdl/parser/parse.hpp (2)
10-10: Prefer forward declaration over including all AST nodes.Including
ast/nodes.hppin a public header forces every includer to parse the full AST set. Since you return a pointer, you can forward-declareast::TranslationUnitand move the heavy include to the .cpp.Apply this change (within this header, remove the heavy include):
-#include <spider/tdl/parser/ast/nodes.hpp>And add a forward declaration (outside the changed range; example placement shown):
// Place after the includes, before the function declaration: namespace spider::tdl::parser::ast { struct TranslationUnit; }
17-18: Docstring aligns with new semantics—consider minor clarity.Optionally clarify ownership and immutability, e.g., “Returns a unique_ptr owning the AST root (caller owns).” If the AST shouldn’t be mutated by clients, consider returning
std::unique_ptr<const ast::TranslationUnit>.tests/tdl/test-parser.cpp (3)
54-59: Guard against accidental nullptr translation_unit before dereference.Outcome success doesn’t guarantee non-null unique_ptr in perpetuity. Add a sanity check to prevent UB if future changes ever allow a null AST on success.
TEST_CASE("Parsing `cTestInput1`", "[tdl][parser]") { std::istringstream input_stream{std::string{cTestInput1}}; auto const parse_result{parse_translation_unit_from_istream(input_stream)}; REQUIRE_FALSE(parse_result.has_error()); - auto const& translation_unit{parse_result.value()}; + auto const& translation_unit{parse_result.value()}; + REQUIRE(translation_unit != nullptr);
60-246: The golden-string equality makes the test brittle; consider stabilizing the assertion.Exact textual equality across hundreds of lines is fragile to benign formatting changes (spacing, newlines, serialization wording). Options:
- Normalise newlines/whitespace before comparing.
- Print the actual string on failure for easier diffing.
- Move expected output to a snapshot file to simplify maintenance.
Minimal, low-noise tweak (adds better failure diagnostics) while keeping equality:
- REQUIRE(serialize_result.value() == cExpectedSerializedAst); + INFO("\n--- Serialized AST (actual) ---\n" << serialize_result.value()); + REQUIRE(serialize_result.value() == cExpectedSerializedAst);If cross-platform is a concern, consider normalising CRLF to LF before comparison:
+#include <algorithm> +#include <string> ... - INFO("\n--- Serialized AST (actual) ---\n" << serialize_result.value()); - REQUIRE(serialize_result.value() == cExpectedSerializedAst); + auto actual = serialize_result.value(); + actual.erase(std::remove(actual.begin(), actual.end(), '\r'), actual.end()); + INFO("\n--- Serialized AST (actual) ---\n" << actual); + REQUIRE(actual == cExpectedSerializedAst);
281-302: Add a test to verify ANTLR lexer/parser errors take precedence over propagated Exceptions.You already cover the pure-propagated Exception path. It’d be valuable to assert the documented precedence behaviour when both could arise.
Example SECTION to append:
+ SECTION("ANTLR error takes precedence over propagated exception") { + // Tuple<> as a param triggers a parser mismatch; duplicate function name would also + // trigger a propagated exception if reached. Expect the ANTLR error instead. + constexpr std::string_view cBothErrors{ + "namespace test { fn empty(Tuple<int8>); fn empty(); }" + }; + std::istringstream input_stream{std::string{cBothErrors}}; + auto const parse_result{parse_translation_unit_from_istream(input_stream)}; + REQUIRE(parse_result.has_error()); + auto const& error{parse_result.error()}; + REQUIRE(error.get_message().rfind("Parser:", 0) == 0); // starts with "Parser:" + }If you want, I can push an update with this new test.
src/spider/tdl/parser/parse.cpp (2)
38-45: Deduplicate error-listener checks to reduce repetition.Same two-branch check is performed twice. A small helper improves readability and reduces risk of inconsistent future edits.
// Parse the translation unit - try { + auto first_error = [&]() -> std::optional<Error> { + if (lexer_error_listener.has_error()) return lexer_error_listener.error(); + if (parser_error_listener.has_error()) return parser_error_listener.error(); + return std::nullopt; + }; + try { auto* context{parser.translationUnit()}; - if (lexer_error_listener.has_error()) { - return lexer_error_listener.error(); - } - - if (parser_error_listener.has_error()) { - return parser_error_listener.error(); - } + if (auto e = first_error()) return *e; return std::move(context->tu); } catch (Exception const& e) { // When an exception is caught, we still prioritize the parser and lexer errors since they // are the root cause of the exceptions. - if (lexer_error_listener.has_error()) { - return lexer_error_listener.error(); - } - - if (parser_error_listener.has_error()) { - return parser_error_listener.error(); - } + if (auto le = first_error()) return *le; return e.to_error(); }Also applies to: 51-57
46-47: Defensive check: ensure context->tu is non-null before returning.If grammar actions ever fail to create the TU yet no listener reported an error, this could return a null AST on “success”. Consider guarding and converting to an Error.
I can wire a small guard to return a synthetic Error if desired.
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (2)
382-472: Left-recursive FuncDefs rule is implemented correctly; consider minor allocation hint.The precedence-based left recursion plus accumulating retval via moves is solid. As a micro-optimisation, you could reserve capacity when re-building the vector to reduce reallocations, though impact is minimal here.
If you want this, it’s best added via grammar action so it persists across regeneration.
665-686: Minor: clearing an already-empty vector is redundant for empty parameter lists.In the empty-params branch, _localctx->retval.clear() is unnecessary as the vector is default-constructed empty.
Note: If you choose to address this, please change it in the grammar actions (not by editing generated sources).
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h (1)
2-11: Consider reducing header surface by forward declaring AST types.Including heavy AST headers (nodes, IntSpec, FloatSpec, Exception) in the public parser header increases rebuild costs. If feasible via grammar header actions, forward-declare the minimal AST types used in member declarations and move heavy includes to the .cpp.
Trade-off: generated code simplicity vs. compile-time. If generation tooling makes this awkward, feel free to skip.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
src/spider/CMakeLists.txt(1 hunks)src/spider/tdl/parser/Exception.hpp(1 hunks)src/spider/tdl/parser/TaskDefLang.g4(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.cpp(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h(2 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.cpp(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.h(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp(43 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h(20 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.cpp(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.h(2 hunks)src/spider/tdl/parser/parse.cpp(2 hunks)src/spider/tdl/parser/parse.hpp(1 hunks)tests/tdl/test-parser.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-17T19:44:06.132Z
Learnt from: sitaowang1998
PR: y-scope/spider#168
File: lint-tasks.yaml:62-65
Timestamp: 2025-07-17T19:44:06.132Z
Learning: ANTLR-generated C++ code in the G_SRC_DSL_DIR (src/stdl) should not be included in linting tasks because it's auto-generated code that doesn't follow manual coding standards.
Applied to files:
src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.cppsrc/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.cpp
🧬 Code graph analysis (3)
tests/tdl/test-parser.cpp (2)
src/spider/tdl/parser/parse.cpp (2)
parse_translation_unit_from_istream(18-61)parse_translation_unit_from_istream(18-19)src/spider/tdl/parser/parse.hpp (1)
parse_translation_unit_from_istream(20-21)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h (3)
src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (17)
spider(22-100)std(35-37)std(39-41)std(43-45)std(47-49)std(51-53)std(55-57)std(59-61)std(63-65)std(67-69)std(71-73)std(75-77)std(79-81)std(83-85)std(87-89)std(91-93)std(95-97)src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.h (1)
spider(22-71)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (55)
getRuleIndex(218-220)getRuleIndex(218-218)getRuleIndex(320-322)getRuleIndex(320-320)getRuleIndex(395-397)getRuleIndex(395-395)getRuleIndex(493-495)getRuleIndex(493-493)getRuleIndex(569-571)getRuleIndex(569-569)getRuleIndex(641-643)getRuleIndex(641-641)getRuleIndex(715-717)getRuleIndex(715-715)getRuleIndex(786-788)getRuleIndex(786-786)id(311-313)id(311-311)id(480-482)id(480-480)id(706-708)id(706-706)id(873-875)id(873-873)id(976-976)id(1021-1023)id(1021-1021)funcDefs(315-317)funcDefs(315-315)funcDefs(390-392)funcDefs(390-390)funcDefs(408-410)funcDefs(408-408)funcDefs(412-412)funcDef(386-388)funcDef(386-386)funcDef(505-505)accept(223-228)accept(223-223)accept(325-330)accept(325-325)accept(400-405)accept(400-400)accept(498-503)accept(498-498)accept(574-579)accept(574-574)accept(646-651)accept(646-646)accept(720-725)accept(720-720)accept(791-796)accept(791-791)funcDefsSempred(1674-1682)funcDefsSempred(1674-1674)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (1)
src/spider/tdl/parser/Exception.hpp (1)
throw_tryx(26-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (27)
src/spider/tdl/parser/Exception.hpp (1)
13-76: Solid, minimal exception type for Result-to-exception bridging — LGTMThe design is clear, header-only, and fits the C++ target’s needs for throwing with location context. Accessors are nodiscard, and the helpers constrain T to be move-constructible, which aligns with returning unique_ptr and vectors throughout the grammar actions. No blockers from me here.
src/spider/tdl/parser/TaskDefLang.g4 (3)
158-172: Type rules look consistent; confirm tupleType is intentionally return-only
- varType handles builtins + user-defined structs (via id), with containers (List/Map) routed through builtinType: OK.
- retType additionally allows tupleType, meaning tuples are return-only today.
If that’s by design (multi-value returns only) we’re good. If tuples should also be allowed in parameters or fields, consider allowing tupleType in varType as well.
Also applies to: 174-181, 195-206, 208-223, 225-236, 238-278
146-156: Identifier creation: good location capture — LGTMUsing $ctx->start for SourceLocation makes sense here, and Identifier::create returning a unique_ptr aligns with the rest of the AST factory usage.
15-35: Top-level TU assembly: error propagation and location plumbing look correctCapturing child node locations and passing them into throw_tryv on add_namespace/add_struct_spec is a good pattern to preserve precise error origin.
src/spider/CMakeLists.txt (1)
258-259: Expose Exception.hpp publicly — LGTMAdding tdl/parser/Exception.hpp to SPIDER_TDL_SHARED_HEADERS is correct and matches its use across parser clients and tests.
src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.h (1)
1-65: ANTLR-generated code is already excluded from lintingVerified that all ANTLR‐generated sources under
tdl/parser/antlr_generated/are omitted from lint and format jobs:
- In CMakeLists.txt (around lines 200–210), the
SPIDER_TDL_ANTLR_GENERATED_SOURCESvariable lists only generated files, keeping them out of manual lint targets.- In lint‐tasks.yaml (around lines 64–66), the
EXCLUDE_PATTERNSentry explicitly includes"tdl/parser/antlr_generated/*", preventing these files from being processed by the lint pipeline.No further changes required.
src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.h (1)
38-39: Review Approved:visitFuncDefsAddition VerifiedAll cross-file references for the new
visitFuncDefsmethod have been checked and are consistent:
TaskDefLangVisitor.h declares
virtual std::any visitFuncDefs(TaskDefLangParser::FuncDefsContext *context) = 0;(line 38).TaskDefLangParser.h defines
class FuncDefsContext : public antlr4::ParserRuleContext(lines 59, 112).TaskDefLangBaseVisitor.h overrides
virtual std::any visitFuncDefs(TaskDefLangParser::FuncDefsContext *ctx) override(line 39).TaskDefLangParser.cpp implements the
acceptmethod to dispatch to
parserVisitor->visitFuncDefs(this);(line 402).No further changes are required—this addition correctly aligns with the grammar evolution.
src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (1)
39-41: BaseVisitor override for visitFuncDefs: LGTM.The default
visitChildren(ctx)behaviour is consistent with other nodes and keeps backward-compatible traversal semantics.src/spider/tdl/parser/parse.hpp (2)
5-5: Header hygiene: include is appropriate.Good call adding for the unique_ptr return type.
21-21: Confirmstd_checkedalias availabilityThe alias
boost::outcome_v2::std_checked<T, E>is indeed declared in<boost/outcome/std_result.hpp>and brought intoboost::outcome_v2viaBOOST_OUTCOME_V2_NAMESPACE(live.boost.org). No additional headers or aliases are required to usestd_checked.If you instead intend to use the
result<T, E>alias, switch to:
- Header:
<boost/outcome/result.hpp>- Type:
boost::outcome_v2::result<std::unique_ptr<ast::TranslationUnit>, Error>Otherwise, your current include and use of
std_checkedacross the project is correct.tests/tdl/test-parser.cpp (1)
8-8: Include of AST nodes header in tests is appropriate.Needed for accessing ast types (e.g., Namespace::ErrorCodeEnum) and for serialization. No issues.
src/spider/tdl/parser/parse.cpp (2)
47-60: Exception prioritization logic is correct.Catching parser::Exception and still preferring ANTLR listener errors matches the intended precedence semantics.
18-20: Signature alignment confirmed across declaration, definition, and call sites
- The function declaration in src/spider/tdl/parser/parse.hpp (lines 20–21) and the definition in src/spider/tdl/parser/parse.cpp (lines 18–19) both now return
boost::outcome_v2::std_checked<std::unique_ptr<ast::TranslationUnit>, Error>.- All call sites (e.g. in tests/tdl/test-parser.cpp) invoke
parse_translation_unit_from_istreamviaautoand correctly handle the outcome.No further updates are required.
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (9)
2-11: Including AST and Exception headers in the generated parser is warranted.Required to construct AST nodes and surface location-aware exceptions during parse-time AST building.
234-238: Good: TranslationUnit root is created early with a well-defined SourceLocation.Location uses _localctx->start (0-based column), consistent with tests expecting positions like {1, 1} and {1, 10}.
If the project decides to use 1-based columns later, this is the central place to adjust.
256-281: Safe insertion into TU with location-propagated error reporting.Using throw_tryv(...) to add namespaces/structs ensures semantic errors are surfaced at the declaration’s location. Nicely done.
345-368: Namespace AST construction looks correct and exception-safe.
- Id and funcDefs are moved into Namespace::create.
- Location captured from rule start is consistent with other rules.
989-999: Identifier node creation and location capture are correct.Constructing Identifier with token text and start location matches the serialization expectations.
1050-1091: VarType rule: struct references vs builtins are cleanly separated.ID → Struct type with location; Builtins forwarded from builtinType. The error handling via throw_tryx on Struct creation is appropriate.
1225-1295: Tuple element list supports empty and multi-element cases (Tuple<>).This aligns with tests (Empty tuple case). The accumulation logic mirrors NamedVarList/FuncDefs.
1532-1660: Builtin types: location computed once per alternative; correct forwarding for list/map.
- Primitive Int/Float/Bool nodes get a consistent loc.
- Container alternatives forward the retval from nested rules.
1662-1702: Semantic predicate wiring is consistent with new rules.funcDefsSempred, namedVarListSempred, varTypeListSempred indices align with rule indices introduced earlier.
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h (5)
75-87: Making TranslationUnitContext expose the AST root (unique_ptr) is aligned with the new API.This cleanly transfers ownership to parse.cpp via std::move(context->tu).
97-104: NamespaceContext additions (idContext + funcDefsContext + retval) look coherent.No concerns; this mirrors the codegen in the .cpp.
112-129: FuncDefsContext public surface is sensible for a left-recursive list.Exposes child accessors and the accumulated retval vector. Matches the visitor additions.
360-365: New funcDefs semantic predicate hook is correctly declared.Keeps parity with the .cpp and the new rule.
33-38: Rule index renumbering is internal to generated code—no external references found
- A ripgrep search for
getRuleIndex()andTaskDefLangParser::Rule…references only returned hits in the ANTLR-generated parser files (TaskDefLangParser.h/.cpp).- There are no integer comparisons or hard-coded rule-ID usages in any other part of the codebase.
The shift of
RuleFuncDefsand subsequent enum values is confined to the generated parser. No client code needs updating.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/spider/tdl/parser/TaskDefLang.g4 (2)
54-63: Refactor left-recursive funcDefs to iterative EBNF to simplify and avoid sempred complexity.ANTLR 4 supports left recursion, but combining it with attribute actions adds complexity and increases the chance of subtle bugs. An iterative form is simpler and avoids deep recursion on long lists. This also removes the need for a dedicated sempred for this rule.
Apply this diff at the grammar level:
-funcDefs returns [std::vector<std::unique_ptr<spider::tdl::parser::ast::Node>> retval] -: funcDef { - $retval.clear(); - $retval.emplace_back(std::move($funcDef.retval)); -} -| parsed_funcs=funcDefs funcDef { - $retval = std::move($parsed_funcs.retval); - $retval.emplace_back(std::move($funcDef.retval)); -} -; +funcDefs returns [std::vector<std::unique_ptr<spider::tdl::parser::ast::Node>> retval] +@init { $retval.clear(); } +: f=funcDef { $retval.emplace_back(std::move($f.retval)); } + ( f2=funcDef { $retval.emplace_back(std::move($f2.retval)); } )* +;
118-127: Refactor left-recursive namedVarList to iterative EBNF.Same rationale as funcDefs: reduces complexity, avoids recursion depth on large param lists, and removes sempreds.
Suggested replacement:
-namedVarList returns [std::vector<std::unique_ptr<spider::tdl::parser::ast::Node>> retval] -: namedVar { - $retval.clear(); - $retval.emplace_back(std::move($namedVar.retval)); -} -| parsed_named_vars=namedVarList ',' namedVar { - $retval = std::move($parsed_named_vars.retval); - $retval.emplace_back(std::move($namedVar.retval)); -} -; +namedVarList returns [std::vector<std::unique_ptr<spider::tdl::parser::ast::Node>> retval] +@init { $retval.clear(); } +: nv=namedVar { $retval.emplace_back(std::move($nv.retval)); } + ( ',' nv2=namedVar { $retval.emplace_back(std::move($nv2.retval)); } )* +;src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (2)
380-473: funcDefs left recursion introduces sempred and recursion depth for long lists. Prefer iterative form.This complexity is a direct result of the grammar shape. Switching the grammar to iterative EBNF will regenerate this section into a simpler loop without sempreds and with less stack use on large inputs.
If you adopt the grammar diff suggested in TaskDefLang.g4, re-generate to pick up the improvement here.
799-865: namedVarList left recursion has the same maintainability and perf drawbacks.Regenerate after the grammar refactor; this will become a simple loop and remove namedVarListSempred.
🧹 Nitpick comments (4)
src/spider/tdl/parser/TaskDefLang.g4 (2)
92-99: Minor grammar tidy-up: prefer EBNF instead of explicit empty alternative.You can avoid an explicit empty alternative and imperative clear() by using a single optional group with @init. This reduces actions surface area.
Proposed change:
-params returns [std::vector<std::unique_ptr<spider::tdl::parser::ast::Node>> retval] -: namedVarList { - $retval = std::move($namedVarList.retval); -} -| { - $retval.clear(); -} -; +params returns [std::vector<std::unique_ptr<spider::tdl::parser::ast::Node>> retval] +@init { $retval.clear(); } +: ( namedVarList { $retval = std::move($namedVarList.retval); } )? +;
183-195: Empty alternative in varTypeList enables Tuple<>; reconfirm whether zero-arity tuples are desired.The explicit empty alternative permits Tuple<>. If that’s not desired, switch to a one-or-more EBNF. If it is desired, we can still simplify the rule to an optional plus-form without left recursion.
Option A (keep allowing empty, remove left recursion):
-varTypeList returns [std::vector<std::unique_ptr<spider::tdl::parser::ast::Node>> retval] -: varType { - $retval.clear(); - $retval.emplace_back(std::move($varType.retval)); -} -| parsed_var_types=varTypeList ',' varType { - $retval = std::move($parsed_var_types.retval); - $retval.emplace_back(std::move($varType.retval)); -} -| { - $retval.clear(); -} -; +varTypeList returns [std::vector<std::unique_ptr<spider::tdl::parser::ast::Node>> retval] +@init { $retval.clear(); } +: ( vt=varType { $retval.emplace_back(std::move($vt.retval)); } + ( ',' vt2=varType { $retval.emplace_back(std::move($vt2.retval)); } )* + )? +;Option B (disallow empty tuples):
+varTypeList returns [std::vector<std::unique_ptr<spider::tdl::parser::ast::Node>> retval] +@init { $retval.clear(); } +: vt=varType { $retval.emplace_back(std::move($vt.retval)); } + ( ',' vt2=varType { $retval.emplace_back(std::move($vt2.retval)); } )* +;src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (2)
1215-1298: VarTypeList permits empty; Tuple<> is therefore accepted. Confirm semantics.The base-case branch that clears retval is the generated counterpart of the empty alternative. If zero-arity tuples are not part of the type system, switch to a one-or-more rule and regenerate.
1665-1705: Sempreds exist only due to left-recursive list rules.They’ll disappear after the EBNF refactor, simplifying the parser.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/spider/tdl/parser/TaskDefLang.g4(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp(42 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (1)
src/spider/tdl/parser/Exception.hpp (1)
throw_tryx(26-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (22)
src/spider/tdl/parser/TaskDefLang.g4 (13)
3-13: Header injection looks correct and minimal.Includes for AST specs, Exception, and SourceLocation are necessary for the in-grammar actions. No issues spotted.
15-35: Solid TU construction and error propagation.Creating the TranslationUnit in @init and attaching namespaces/structs via throw_tryv with precise SourceLocation is clean and exception-safe.
37-52: Namespace node assembly is clear and location-aware.Good use of throw_tryx and moving retvals. This also keeps parse-time semantic failures localized to the rule’s start location.
65-81: Function assembly is correct; SourceLocation captured at rule start.No correctness concerns. Move semantics and exception translation look good.
83-90: Optional return is well-encoded.Returning nullptr when '->' is absent is unambiguous and keeps the AST concise.
101-116: NamedVar node creation is consistent with the rest of the AST actions.Location and exception wrapping are correct.
129-144: Trailing comma permitted in structDef but not in params — intentional?Struct fields allow an optional trailing ',' before '}', whereas function params do not. If this difference is not intentional, consider normalising the experience (either allow trailing commas in params or disallow here).
Would you like a follow-up diff to add an optional trailing comma to params for consistency?
158-172: VarType excludes tupleType; confirm type system intent.Currently, tupleType appears only in retType, so tuples cannot be used as parameter or field types, nor nested in List/Map. If that’s by design, consider documenting it in the grammar comments; if not, we can extend varType accordingly.
I can send a focused diff to allow tupleType in varType (and thereby in List/Map if desired).
197-208: List type construction is correct.Good move semantics and location handling.
210-225: Map type construction is correct; key/value types are varType.Note: with the current grammar, keys/values cannot be tuples — only builtin/list/map/struct. Confirm that this restriction is intentional.
227-238: Tuple type creation is clean; semantics depend on varTypeList choice.Once you confirm whether empty tuples are allowed, we can lock this down with either Option A or B above.
240-280: Builtin types are clear; tuple intentionally excluded here.Loc is captured in @init, which is appropriate. Inclusion of List/Map as “builtin” aligns with current type model.
282-285: Lexical rules (ID/SPACE/COMMENT) are fine.Skipping whitespace/comments in the lexer keeps parser rules tidy.
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (9)
2-11: Generated includes from @Header are correctly injected.Matches the grammar’s @Header; no issues.
66-70: Rule name table correctly reflects new/renamed rules.Addition of funcDefs and other AST-bearing rules is reflected; OK.
230-237: TranslationUnit assembly matches grammar actions and uses throw_tryv.Namespace/StructSpec attachments are exception-wrapped and location-aware. Looks correct.
Also applies to: 256-281
345-369: Namespace context wires id + funcDefs into Namespace AST cleanly.Everything aligns with the grammar; no issues.
894-951: Trailing comma acceptance is implemented; confirm it’s intentionally limited to structDef.Params do not accept a trailing comma; struct fields do. If that asymmetry isn’t desired, adjust the grammar and regenerate.
1538-1663: BuiltinType covers primitives/List/Map; tuple types are excluded here by design.No correctness concerns. If you later allow tuple as a general varType, this section will need coordinated changes.
653-697: Return/params/funcDef behaviours align with the grammar and AST actions.Good balance of optionality (ret), empty params, and location-aware function construction.
Also applies to: 581-619, 505-547
1038-1100: Type selection paths are accurate; confirm tuple restrictions outside returns.varType disallows tuple; retType allows it (including via Tuple<...>). If this is intentional, consider a brief comment in the grammar to guide future contributors.
Also applies to: 1129-1185
976-1009: Identifier AST creation correctly uses token text and SourceLocation.No issues.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (1)
231-235: Generated code won’t compile as-is due to unqualified SourceLocation and TU create brace-init
- Lines 231–235:
TranslationUnit::create({ ... })should receive aspider::tdl::parser::SourceLocation{...}.- Multiple blocks construct
SourceLocation const loc{...}without namespace qualification (e.g., Lines 353–366, 504–517, 718–730, 881–893, 1286–1294, 1359–1371, 1428–1436, 1479–1483). These will be undefined.Do not patch this file directly. Fix the grammar actions (TaskDefLang.g4) as suggested; regenerate to produce compilable code.
Use this quick check:
#!/bin/bash set -euo pipefail # TU create with ambiguous brace-init rg -n 'TranslationUnit::create\(\s*\{' src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp # Unqualified SourceLocation rg -nP '\b(?<!spider::tdl::parser::)SourceLocation\b' src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp -SAlso applies to: 353-366, 504-517, 718-730, 881-893, 1286-1294, 1359-1371, 1428-1436, 1493-1595
🧹 Nitpick comments (6)
src/spider/tdl/parser/TaskDefLang.g4 (4)
54-61: Confirm intent: namespaces now require ≥1 function (funcDefs uses “+”)
funcDefsis+-quantified, makingnamespace Foo {}invalid. If empty namespaces should be allowed, switch to*.-: (funcDef { +: (funcDef { $retval.emplace_back(std::move($funcDef.retval)); -})+ +})* ;
181-194: Tuple<> emptiness semantics: do you want to allow Tuple<> (empty)?
varTypeList’s second alternative returns an empty vector when the next token is>; this makesTuple<>legal. If a non-empty tuple is required, drop the empty alternative. If a 2+ element tuple is required, enforce it here.
- Non-empty (≥1):
-: first_var_type=varType { ... } (',' subsequent_var_type=varType { ... })* -| { - $retval.clear(); -} +: first_var_type=varType { + $retval.emplace_back(std::move($first_var_type.retval)); +} (',' subsequent_var_type=varType { + $retval.emplace_back(std::move($subsequent_var_type.retval)); +})* ;
- At least 2 elements:
-: first_var_type=varType { ... } (',' subsequent_var_type=varType { ... })* +: first_var_type=varType { + $retval.emplace_back(std::move($first_var_type.retval)); +} ',' second_var_type=varType { + $retval.emplace_back(std::move($second_var_type.retval)); +} (',' subsequent_var_type=varType { + $retval.emplace_back(std::move($subsequent_var_type.retval)); +})* ;
156-170: Design choice: tuples only allowed in return types — verify or support tuples as parameter/member typesCurrently,
varTypeexcludestupleType, so tuples can appear only in returns (retType). If that limitation isn’t intentional, extendvarTypeto includetupleTypeto enable nested types likeList<Tuple<...>>and parameter/struct fields with tuple types.Minimal change:
varType returns [std::unique_ptr<spider::tdl::parser::ast::Node> retval] -: builtinType { +: builtinType { $retval = std::move($builtinType.retval); } -| id { +| id { ... } +| tupleType { + $retval = std::move($tupleType.retval); +} ;If you adopt this, you can keep
tupleTypeinretTypeor simplifyretType: varType;.Also applies to: 172-179
127-142: Struct trailing comma handling is good — consider mirroring for function params for consistency
structDefaccepts an optional trailing comma before};params/namedVarListdo not. Consider allowing a trailing comma inparamsfor consistency and nicer diffs.Example:
-namedVarList ... : first=namedVar { ... } (',' next=namedVar { ... })* ; +namedVarList ... : first=namedVar { ... } (',' next=namedVar { ... })* (',')? ;src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (1)
96-112: Behavioural confirmation: empty varTypeList alternative maps to lookahead ‘>’The generated parser selects the empty
varTypeListalt when lookahead isT__13(‘>’). If you decide to disallowTuple<>, adjust the grammar as noted; the generator will update these switch cases accordingly.Also applies to: 420-434, 1204-1216, 1405-1424
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h (1)
2-11: Consider forward declarations to reduce header coupling and build timesIncluding heavy AST headers in the generated header increases compile times for all dependents. Since this header only needs pointer types, forward-declare the AST types here and include the full headers in the generated .cpp (which already happens via @Header). With ANTLR C++ target, you can move full includes to
@parser::header(if template supports) or keep them in the .g4@headerbut wrap forward declarations specifically for the .h via a preprocessor guard.Example forward decls (placed before the class):
- namespace spider::tdl::parser::ast { struct Node; struct TranslationUnit; struct Bool; struct Int; struct Float; struct List; struct Map; struct Tuple; struct StructSpec; }
This is optional and depends on your generation strategy, but it can materially speed up builds on larger codebases.
Also applies to: 75-88, 93-107, 112-127, 162-175, 194-210, 226-241, 244-259, 260-276, 277-293, 294-309, 310-325, 326-356
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/spider/tdl/parser/TaskDefLang.g4(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp(37 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h(17 hunks)tests/tdl/test-parser.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/tdl/test-parser.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T21:42:57.051Z
Learnt from: LinZhihao-723
PR: y-scope/spider#210
File: src/spider/tdl/parser/parse.hpp:21-21
Timestamp: 2025-08-25T21:42:57.051Z
Learning: The spider TDL parser API (parse_translation_unit_from_istream) is in early development with no external users yet, so breaking changes don't require full version bumping and changelog documentation.
Applied to files:
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cppsrc/spider/tdl/parser/TaskDefLang.g4
🧬 Code graph analysis (2)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (1)
src/spider/tdl/parser/Exception.hpp (1)
throw_tryx(26-29)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h (3)
src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (17)
spider(22-100)std(35-37)std(39-41)std(43-45)std(47-49)std(51-53)std(55-57)std(59-61)std(63-65)std(67-69)std(71-73)std(75-77)std(79-81)std(83-85)std(87-89)std(91-93)std(95-97)src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.h (1)
spider(22-71)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (56)
getRuleIndex(215-217)getRuleIndex(215-215)getRuleIndex(317-319)getRuleIndex(317-317)getRuleIndex(392-394)getRuleIndex(392-392)getRuleIndex(464-466)getRuleIndex(464-464)getRuleIndex(540-542)getRuleIndex(540-540)getRuleIndex(612-614)getRuleIndex(612-612)getRuleIndex(686-688)getRuleIndex(686-686)getRuleIndex(757-759)getRuleIndex(757-757)id(308-310)id(308-308)id(451-453)id(451-451)id(677-679)id(677-677)id(825-827)id(825-825)id(928-928)id(973-975)id(973-973)funcDefs(312-314)funcDefs(312-312)funcDefs(404-404)accept(220-225)accept(220-220)accept(322-327)accept(322-322)accept(397-402)accept(397-397)accept(469-474)accept(469-469)accept(545-550)accept(545-545)accept(617-622)accept(617-617)accept(691-696)accept(691-691)accept(762-767)accept(762-762)namespace_(198-200)namespace_(198-198)namespace_(202-204)namespace_(202-202)namespace_(329-329)namedVar(698-698)namedVar(748-750)namedVar(748-748)namedVar(752-754)namedVar(752-752)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
🔇 Additional comments (1)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h (1)
33-38: Rule indices renumbered — verify downstream consumers and testsRules shifted due to the new
FuncDefsnon-terminal. If any tooling or tests reference rule indices or names, ensure they don’t rely on previous numeric values.
2 participants
Description
This PR uses ANTLR actions to add AST generation in the parser, directly embedded in the grammar file. The parsed AST will be returned as a
TranslationUnitas introduced in #209.Since we also execute a part of the semantic checks during the parsing (when an AST node is created), we need to propagate these errors through exceptions. This PR also adds a parser
Exceptionfor us to propagate these errors. Need to notice that these errors are second-level citizens, as the native ANTLR parser and lexer errors are prioritized when forwarding errors.We've added unit tests to ensure the AST can be serialized into an expected form. However, the source location is not properly tested yet since the current serialization method doesn't support that. We will add this in a future PR.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Refactor
Tests