feat(tdl): Add full TDL grammar and a boilerplate parser. - #204
Conversation
WalkthroughAdds a full TDL translation-unit grammar and regenerated ANTLR artifacts, exposes a parse driver API compiled into spider_tdl, extends SourceLocation to include column/constexpr and equality, updates CMake and formatting for ANTLR, and adds unit tests covering success and error cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test/Client
participant IS as std::istream
participant ParseAPI as parse_translation_unit_from_istream
participant AIS as ANTLRInputStream
participant LX as TaskDefLangLexer
participant TS as CommonTokenStream
participant PX as TaskDefLangParser
participant EL as ErrorListener (lexer)
participant EP as ErrorListener (parser)
Test->>ParseAPI: call(IS)
activate ParseAPI
ParseAPI->>AIS: construct from IS
ParseAPI->>LX: construct lexer(AIS)
ParseAPI->>EL: replace lexer error listener
ParseAPI->>TS: build token stream(LX)
ParseAPI->>PX: construct parser(TS)
ParseAPI->>EP: replace parser error listener
ParseAPI->>PX: translationUnit()
alt lexer/parser reported errors
PX-->>ParseAPI: listeners set Error
ParseAPI-->>Test: return Error (std_checked)
else success
ParseAPI-->>Test: return success (void)
end
deactivate ParseAPI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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: 2
♻️ Duplicate comments (1)
src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.h (1)
16-20: Generated file: changes look consistent with grammar update.Token expansion to T__0..T__21, ID, SPACE, COMMENT aligns with the richer grammar. No manual edits suggested.
Note: Per our past learning, we avoid formatting suggestions for ANTLR-generated code.
🧹 Nitpick comments (16)
tests/.clang-format (1)
10-11: Add explicit support for antlr4-runtime includes in regex.Nice catch adding antlr4. Some platforms include headers as <antlr4-runtime.h> or <antlr4-runtime/...>. While your current regex will match these because it starts with "antlr4", being explicit improves readability and reduces surprises.
Apply this diff:
- - Regex: "^<(antlr4|absl|boost|catch2|fmt|mariadb|msgpack|spdlog|ystdlib)" + - Regex: "^<(antlr4|antlr4-runtime|absl|boost|catch2|fmt|mariadb|msgpack|spdlog|ystdlib)"src/spider/tdl/parser/TaskDefLang.g4 (5)
27-30: Rewrite left-recursive list to iterative form to simplify parse trees and reduce recursion depth.ANTLR4 handles direct left recursion, but for simple comma-separated lists the iterative pattern is clearer and generates a tighter tree.
Apply this diff:
-namedVarList -: namedVar -| namedVarList ',' namedVar -; +namedVarList +: namedVar (',' namedVar)* +;
45-49: Clarify whether empty tuples are allowed; if not, remove the empty alternative.As written, Tuple<> is valid because typeList has an empty alternative. If that’s not intended, make the list non-empty. If it is intended, consider making it explicit at the Tuple site for readability.
Two options:
- Disallow empty tuples:
-typeList -: type -| typeList ',' type -| -; +typeList +: type (',' type)* +;
- Keep empty tuples but localize the optionality:
-typeList -: type -| typeList ',' type -| -; +builtinType + // ... other alts ... +| 'Tuple' '<' (type (',' type)*)? '>' +;If you choose the second option, also adjust builtinType as in the related comment below.
13-16: Use explicit optional for return type for readability.Equivalent semantics, fewer empty alternatives.
Apply this diff:
-ret -: '->' type -| -; +ret +: ('->' type)? +;
18-21: Use explicit optional for params to reduce ambiguity in error messages.Same semantics, cleaner diagnostics in many cases.
Apply this diff:
-params -: namedVarList -| -; +params +: namedVarList? +;
66-66: Optional: consider adding block comments.Single-line comments are supported; if you want /* ... */ too, add a rule. Make it non-greedy and skip.
For example:
BLOCK_COMMENT: '/*' .*? '*/' -> skip;src/spider/tdl/parser/SourceLocation.hpp (2)
10-19: Make trivial accessors and operator== constexpr to enable compile-time comparisons in tests.You already made the ctor constexpr. Marking getters and equality likewise costs nothing and improves usability (e.g., static_assert with constant expressions).
Apply this diff:
- [[nodiscard]] auto get_line() const noexcept -> size_t { return m_line; } + [[nodiscard]] constexpr auto get_line() const noexcept -> size_t { return m_line; } - [[nodiscard]] auto get_column() const noexcept -> size_t { return m_column; } + [[nodiscard]] constexpr auto get_column() const noexcept -> size_t { return m_column; } - [[nodiscard]] auto operator==(SourceLocation const& other) const noexcept -> bool { + [[nodiscard]] constexpr auto operator==(SourceLocation const& other) const noexcept -> bool { return m_line == other.m_line && m_column == other.m_column; }Optionally also add:
[[nodiscard]] constexpr auto operator!=(SourceLocation const& other) const noexcept -> bool = default;
21-25: Clarify indexing contract in a comment (1-based vs 0-based).Downstream error reporters and tests should agree on whether lines/columns start at 1. A brief comment here helps avoid subtle off-by-one bugs later.
Would you like me to add a short doc comment stating the convention?
src/spider/tdl/parser/parse.hpp (2)
19-21: Double‑check the availability of boost::outcome_v2::std_checked in included headers.You include
<boost/outcome/std_result.hpp>but returnboost::outcome_v2::std_checked<void, Error>. Depending on Boost.Outcome configuration,std_checkedmay not be declared by that header. If your toolchain doesn’t providestd_checkedhere, either:
- include the header that defines
std_checkedin your environment, or- switch to
std_result<void, Error>(if compatible with your policy needs), or- add a local alias for clarity:
+#include <boost/outcome/std_result.hpp> +namespace spider::tdl::parser { +using ParseResult = boost::outcome_v2::std_checked<void, Error>; +// ... -[[nodiscard]] auto parse_translation_unit_from_istream(std::istream& input) - -> boost::outcome_v2::std_checked<void, Error>; +[[nodiscard]] auto parse_translation_unit_from_istream(std::istream& input) -> ParseResult;This also makes future signature changes (e.g., returning an AST root) easier.
10-18: Tighten contract: explicitly state stream consumption and exception guarantees.Consider documenting whether the function:
- consumes the entire stream (and leaves
eofbitset), and- is
noexceptor may throw (ANTLR and allocations may throw).If you intend to keep it non-throwing and report errors via
Error, addingnoexceptwould make the contract explicit.src/spider/tdl/parser/parse.cpp (2)
24-33: Avoid building a parse tree when you don’t use it.You discard the parse tree with
std::ignore, but ANTLR will still build it by default. Disable it to save CPU and memory:antlr4::CommonTokenStream token_stream{&lexer}; antlr_generated::TaskDefLangParser parser{&token_stream}; ErrorListener parser_error_listener{"Parser"}; parser.removeErrorListeners(); parser.addErrorListener(&parser_error_listener); + // We only validate syntax at this stage; skip building the parse tree for performance. + parser.setBuildParseTree(false); + // Optional: use faster SLL mode (fallback to LL on error if needed later). + // parser.getInterpreter<antlr4::atn::ParserATNSimulator>()->setPredictionMode( + // antlr4::atn::PredictionMode::SLL);
34-43: Error selection policy is first-come-first-served — good; consider merging multiple diagnostics later.Returning the first lexer/parser error is fine for v1. If you later want richer diagnostics (multiple errors, recovery locations), consider extending
ErrorListenerto accumulate and return a compositeError. No action required now.tests/tdl/test-parser.cpp (2)
64-67: Reduce brittleness of error-message assertions.Exact string match on ANTLR diagnostics is fragile across runtime versions. Prefer substring match and keep the precise location check:
- constexpr std::string_view cExpectedErrorMessage{"Parser: missing ID at '{'"}; - constexpr SourceLocation cExpectedErrorLocation{1, 10}; - REQUIRE(error.get_message() == cExpectedErrorMessage); - REQUIRE(error.get_source_location() == cExpectedErrorLocation); + // Match stable fragments rather than the entire diagnostic. + REQUIRE_THAT(std::string(error.get_message()), + Catch::Matchers::ContainsSubstring("missing ID")); + REQUIRE_THAT(std::string(error.get_message()), + Catch::Matchers::ContainsSubstring("{")); + constexpr SourceLocation cExpectedErrorLocation{1, 10}; + REQUIRE(error.get_source_location() == cExpectedErrorLocation);You’ll need the matcher include:
#include <catch2/catch_test_macros.hpp> +#include <catch2/matchers/catch_matchers_string.hpp>
51-54: Minor: exercise both success and idempotency paths.The “basic” test is good. Consider an additional SECTION that re-parses the same input (fresh stream) to guard against hidden global state or cached static singletons in the ANTLR machinery.
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (2)
295-314: Namespace body currently forbids struct definitions; validate intent
namespacecontains zero or morefuncDefitems only. If TDL should allowstructDefinside namespaces, add it to the loop and regenerate. Otherwise, leave as-is.Example grammar change (in TaskDefLang.g4, not here):
- namespace: 'namespace' id '{' funcDef* '}' ; + namespace: 'namespace' id '{' (funcDef | structDef)* '}' ;(Note: Regenerate ANTLR artifacts after updating the grammar.)
921-962: Tuple type lists permit empty lists; confirm ifTuple<>is valid TDL
typeListcan be empty due to the epsilon branch; thusTuple<>parses. If at least one type should be required, remove the epsilon alternative.Example grammar tweak (in TaskDefLang.g4):
- typeList: type (',' type)* | /* empty */ ; + typeList: type (',' type)* ;(Then regenerate generated sources.)
📜 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(2 hunks)src/spider/tdl/parser/SourceLocation.hpp(1 hunks)src/spider/tdl/parser/TaskDefLang.g4(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.cpp(2 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.h(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp(4 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h(2 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.h(1 hunks)src/spider/tdl/parser/parse.cpp(1 hunks)src/spider/tdl/parser/parse.hpp(1 hunks)tests/.clang-format(1 hunks)tests/CMakeLists.txt(1 hunks)tests/tdl/test-parser.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-21T00:14:42.758Z
Learnt from: sitaowang1998
PR: y-scope/spider#203
File: src/spider/.clang-format:10-10
Timestamp: 2025-08-21T00:14:42.758Z
Learning: ANTLR-generated C++ code is not processed by clang-format, so formatting suggestions for generated ANTLR files are not applicable.
Applied to files:
tests/.clang-format
🧬 Code Graph Analysis (6)
tests/tdl/test-parser.cpp (3)
src/spider/tdl/parser/parse.hpp (1)
parse_translation_unit_from_istream(19-20)src/spider/tdl/parser/parse.cpp (2)
parse_translation_unit_from_istream(15-43)parse_translation_unit_from_istream(15-16)src/spider/tdl/parser/SourceLocation.hpp (2)
SourceLocation(10-10)SourceLocation(10-10)
src/spider/tdl/parser/SourceLocation.hpp (2)
src/spider/tdl/Error.hpp (3)
nodiscard(27-27)nodiscard(29-31)nodiscard(33-35)src/spider/tdl/parser/ast/Node.hpp (5)
nodiscard(48-48)nodiscard(53-53)nodiscard(76-82)nodiscard(95-97)nodiscard(119-121)
src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.h (1)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (2)
ID(748-750)ID(748-748)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h (2)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (106)
ID(748-750)ID(748-748)TranslationUnitContext(159-161)getRuleIndex(184-186)getRuleIndex(184-184)getRuleIndex(270-272)getRuleIndex(270-270)getRuleIndex(344-346)getRuleIndex(344-344)getRuleIndex(405-407)getRuleIndex(405-405)getRuleIndex(472-474)getRuleIndex(472-472)getRuleIndex(541-543)getRuleIndex(541-541)getRuleIndex(598-600)getRuleIndex(598-598)getRuleIndex(685-687)getRuleIndex(685-685)EOF(163-165)EOF(163-163)namespace_(167-169)namespace_(167-167)namespace_(171-173)namespace_(171-171)namespace_(282-282)structDef(175-177)structDef(175-175)structDef(179-181)structDef(179-179)structDef(697-697)accept(189-194)accept(189-189)accept(275-280)accept(275-275)accept(349-354)accept(349-349)accept(410-415)accept(410-410)accept(477-482)accept(477-477)accept(546-551)accept(546-546)accept(603-608)accept(603-603)accept(690-695)accept(690-690)id(257-259)id(257-257)id(331-333)id(331-331)id(532-534)id(532-532)id(676-678)id(676-676)id(765-765)id(801-803)id(801-801)funcDef(261-263)funcDef(261-261)funcDef(265-267)funcDef(265-265)funcDef(356-356)params(335-337)params(335-335)params(484-484)ret(339-341)ret(339-339)ret(417-417)type(400-402)type(400-400)type(536-538)type(536-536)type(818-818)type(876-878)type(876-876)type(977-979)type(977-977)type(981-983)type(981-981)namedVarList(467-469)namedVarList(467-467)namedVarList(593-595)namedVarList(593-593)namedVarList(611-613)namedVarList(611-611)namedVarList(615-615)namedVarList(680-682)namedVarList(680-680)namedVar(553-553)namedVar(589-591)namedVar(589-589)builtinType(797-799)builtinType(797-797)builtinType(1002-1002)typeList(880-882)typeList(880-880)typeList(898-900)typeList(898-898)typeList(902-902)typeList(985-987)typeList(985-985)namedVarListSempred(1128-1136)namedVarListSempred(1128-1128)typeListSempred(1138-1146)typeListSempred(1138-1138)src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (11)
std(24-26)std(28-30)std(32-34)std(36-38)std(40-42)std(44-46)std(48-50)std(52-54)std(56-58)std(60-62)std(64-66)
src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (1)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (1)
TranslationUnitContext(159-161)
src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.h (2)
src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (11)
std(24-26)std(28-30)std(32-34)std(36-38)std(40-42)std(44-46)std(48-50)std(52-54)std(56-58)std(60-62)std(64-66)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (1)
TranslationUnitContext(159-161)
⏰ 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-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
🔇 Additional comments (18)
src/spider/tdl/parser/TaskDefLang.g4 (1)
5-7: Confirm whether structs are allowed inside namespaces; grammar currently forbids it.Top-level allows namespace | structDef, but inside a namespace only funcDef is allowed. If you intend structs inside namespaces, update the rule to permit structDef as well.
If desired, change to:
-: 'namespace' id '{' (funcDef)* '}' +: 'namespace' id '{' (funcDef | structDef)* '}'tests/CMakeLists.txt (2)
5-5: LGTM: test added to build.Adding tdl/test-parser.cpp into SPIDER_TEST_SOURCES wires the new parser test into unitTest.
38-55: Confirm antlr4 runtime linkage is resolved transitively via spider_tdl.If spider_tdl doesn’t link to antlr4-runtime privately, unitTest may need it explicitly. Otherwise, you’ll see unresolved symbols at link time.
If needed, add:
target_link_libraries(unitTest PRIVATE antlr4_shared) # or antlr4_static, depending on your buildLet me know if you prefer a quick script to verify linkage in CI.
src/spider/CMakeLists.txt (2)
228-231: Good call wiring the parser driver into spider_tdl.
tdl/parser/parse.cppbeing part ofSPIDER_TDL_SHARED_SOURCESensures the driver is compiled into the library. Looks consistent with the public API exposure below.
256-257: Confirm public exposure/installation of parse.hpp.
tdl/parser/parse.hppis added to the public headers ofspider_tdl. If you ship/install headers, ensure your install rules include this path so downstreams can include<spider/tdl/parser/parse.hpp>. If this project is internal-only, feel free to ignore.src/spider/tdl/parser/parse.cpp (1)
18-21: Verify ANTLRInputStream(istream&) constructor support in your runtime version.Some ANTLR4 C++ runtime versions prefer loading from a string. If your CI toolchain doesn’t support the
std::istream&constructor, fallback to:- antlr4::ANTLRInputStream input_stream{input}; + std::ostringstream oss; + oss << input.rdbuf(); + antlr4::ANTLRInputStream input_stream{oss.str()};This keeps behaviour identical while avoiding runtime version pitfalls.
tests/tdl/test-parser.cpp (1)
65-66: Column off-by-one risk — validate convention (0-based vs 1-based).
"namespace { ... }"places{after 10 characters (“namespace ”), so depending on whether columns are 0- or 1-based,{could be at column 10 or 11. EnsureSourceLocationandErrorListeneragree on a single convention. Adjust the expected column accordingly if CI fails.src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.cpp (2)
61-79: Generated lexer aligns with the declared grammar tokens.Token tables include expected keywords and punctuation (namespace, fn, struct, primitives, List</Map</Tuple<, separators). No manual changes requested.
82-136: Whitespace and comment handling look standard.
SPACEandCOMMENTare present with hidden-channel handling, which should keep them out of the parser’s way. Good.src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.h (1)
23-45: No legacy visitor methods detected — no downstream overrides to updateI ran ripgrep searches across the codebase for any occurrences of
StartContextorvisitStartand for custom overrides of visitor methods outside of the ANTLR-generated directory. Nothing was found, so there are no stale references or overrides that need updating.src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (1)
20-66: LGTM: Base visitor provides sensible defaults for all new rulesEach new rule has a default
visitChildren(ctx)implementation and the rootvisitTranslationUnitoverride is present. No issues spotted.src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (4)
210-239: Top-level only allows namespaces and structs; confirm that top-level functions are intentionally disallowed
translationUnit()loops onnamespace_ | structDefuntil EOF. If TDL intends to permit free functions at the top level (outside namespaces), the grammar will need a third branch here. If they must be in namespaces, this is correct.Would you confirm the intended language rule? If free functions should be allowed, I can propose a grammar tweak and regenerate the parser.
710-731: Trailing comma accepted for struct fields; ensure the asymmetry with params is intentional
structDefallows an optional trailing comma before}whileparams/namedVarListdo not. If this asymmetry is by design (common for data declarations), great; if not, consider either:
- Allow trailing commas in params as well, or
- Disallow trailing commas in struct fields.
I can adjust the grammar either way and regenerate, if you confirm the desired behaviour.
832-858: Starter set fortypecorrectly recognises built-ins and generic formsThe
type()lookahead covers primitive built-ins and generic starts (List<,Map<,Tuple<via T__18, T__20, T__21), and falls back toid(). This aligns withbuiltinType()branches. No issues here.
1117-1146: Sempred indices appear unusual but are likely generator-determined; keep generated code source-of-truth
namedVarListSempredusescase 0andtypeListSempredusescase 1. While a single predicate often maps tocase 0, ANTLR can assign indices based on internal transforms. Since this file is generated, avoid hand edits. If you see unexpected associativity or list parsing issues in tests, regenerate from the grammar to ensure indices are correct.Add a regression test covering:
- Multiple parameter list items
- Multiple tuple type items
- Nested generics with commas (e.g.,
Tuple<Map<int32, List<double>>, bool>)
I can propose unit test additions if helpful.src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h (3)
16-26: Token and rule enums match parser.cpp initialisationThe token set (T__0..T__21, ID, SPACE, COMMENT) and rule indices match the
.cppinitialiser lists. No discrepancies spotted.
200-214: Both recursive list rules expose precedence overloads; good
namedVarList(int precedence)andtypeList(int precedence)overloads are present alongside convenience wrappers. Matches the.cppdefinitions.
58-74: No client calls to the ANTLRstart()rule detected—ignore this migration warningThe grep output only shows usages of
Driver::start(...)andTaskContext::start(...)in tests, examples, and client‐side code (unrelated to the ANTLR parser’sstart()rule). There are no references to the removedstart()parser method, so no client code needs to switch totranslationUnit().Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/spider/tdl/parser/TaskDefLang.g4 (1)
56-59: Whitespace-tolerant generics: the shift to'List' '<' ... '>'/'Map' '<' ... '>'looks great.This addresses the prior ergonomics concern around
'List<'literals and allowsList <T>with spacing/newlines. Consistent with how other punctuation is tokenized.Also applies to: 60-63, 68-78
tests/tdl/test-parser.cpp (1)
87-87: Thanks for fixing the extra closing brace in the anonymous namespace.The prior over-close would have broken compilation; current closing is correct.
🧹 Nitpick comments (15)
src/spider/tdl/parser/TaskDefLang.g4 (6)
5-7: Consider allowing struct definitions inside namespaces (or confirm they’re intentionally top-level-only).Right now, a namespace body admits only
funcDef. If TDL intends to allow user-defined types to be scoped within namespaces (common in many languages), the grammar should permitstructDefthere as well.If that’s your intent, apply:
-namespace -: 'namespace' id '{' (funcDef)* '}' -; +namespace +: 'namespace' id '{' (funcDef | structDef)* '}' +;If top-level-only structs are a deliberate design choice, please document it in the grammar file to avoid confusion.
13-16: Replace empty-alternative optionals with EBNF for clarity and fewer ambiguities.Using an explicit empty alternative (
|) is harder to read and can introduce unnecessary ambiguity. Prefer?.-ret -: '->' retType -| -; +ret +: ('->' retType)? +; -params -: namedVarList -| -; +params +: (namedVarList)? +;Also applies to: 18-21
27-30: Simplify left‑recursive list into a straightforward EBNF list.ANTLR v4 handles direct left recursion, but the EBNF form is simpler and removes the need for a semantic predicate.
-namedVarList -: namedVar -| namedVarList ',' namedVar -; +namedVarList +: namedVar (',' namedVar)* +;Note: your trailing-comma support at the struct site remains via
(',' )?instructDef.
50-55: Make tuple element lists explicit EBNF and drop the extra precedence machinery.Today
varTypeListis left‑recursive with an empty alternative. Prefer an optional EBNF list. Two options:
- Minimal change (keep
varTypeListbut EBNF):-varTypeList -: varType -| varTypeList ',' varType -| -; +varTypeList +: /* empty */ +| varType (',' varType)* +;
- Cleaner (inline the optional list and remove
varTypeListentirely):-tupleType -: 'Tuple' '<' varTypeList '>' -; +tupleType +: 'Tuple' '<' (varType (',' varType)*)? '>' +;The second option eliminates left recursion and removes the need for
varTypeListSempred.Also applies to: 64-66
65-66: Do you want to allow empty tuples?
Tuple<>is accepted via the emptyvarTypeList; the test explicitly exercises this. If zero‑arity tuples are not desired, require at least one element:-tupleType -: 'Tuple' '<' varTypeList '>' -; +tupleType +: 'Tuple' '<' varType (',' varType)* '>' +;Otherwise, consider a short comment in the grammar noting that empty tuples are intentionally supported.
82-82: Optional: add block comment support.Only
//line comments are recognised. If multi-line comments are desired, add a block comment rule (ensure DOT matches newlines in your lexer settings):COMMENT: '//' (~[\r\n])* -> skip; +BLOCK_COMMENT: '/*' .*? '*/' -> skip;If DOTALL is not enabled for the lexer, switch
.*?to a newline-safe pattern (e.g.,(.|\r|\n)*?).src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h (2)
288-292: Left-recursion sempreds are a byproduct of the current list rules — these can disappear with EBNF.If you adopt the EBNF refactors suggested in TaskDefLang.g4, the
namedVarListSempred/varTypeListSempredscaffolding becomes unnecessary, reducing generated complexity.
2-2: Stabilise ANTLR version in build to avoid accidental churn.Header indicates ANTLR 4.13.2. Please pin this version in your toolchain/CMake to keep generated headers stable across environments.
tests/tdl/test-parser.cpp (3)
78-85: Make the error string assertions less brittle.Comparing the entire “expecting { … }” set is fragile because token-set ordering can change with minor grammar tweaks or ANTLR versions. Prefer substring/starts-with checks.
Apply:
- constexpr std::string_view cExpectedErrorMessage{ - "Parser: mismatched input 'Tuple' expecting {'List', 'Map', 'int8', 'int16', " - "'int32', 'int64', 'float', 'double', 'bool', ID}" - }; - constexpr SourceLocation cExpectedErrorLocation{1, 40}; - REQUIRE(error.get_message() == cExpectedErrorMessage); + using Catch::Matchers::ContainsSubstring; + using Catch::Matchers::StartsWith; + constexpr SourceLocation cExpectedErrorLocation{1, 40}; + REQUIRE_THAT(std::string{error.get_message()}, ContainsSubstring("mismatched input 'Tuple'")); + REQUIRE_THAT(std::string{error.get_message()}, ContainsSubstring("expecting")); REQUIRE(error.get_source_location() == cExpectedErrorLocation);And add the matcher header near the top:
#include <catch2/catch_test_macros.hpp> +#include <catch2/matchers/catch_matchers_string.hpp>
63-66: Keep the strict equality here or switch to StartsWith; either is fine.This message is unlikely to change, but if you prefer symmetry with the other case:
- REQUIRE(error.get_message() == cExpectedErrorMessage); + using Catch::Matchers::StartsWith; + REQUIRE_THAT(std::string{error.get_message()}, StartsWith("Parser: missing ID at '{'"));
15-48: Great, this sample exercises many constructs. Consider two quick additions.
- Add a case with whitespace in generics (e.g.,
List < int8 >) to lock in the whitespace-friendly tokenisation.- If you decide to allow
structinsidenamespace, include one such struct to cover that path.Happy to draft these if you want them in this PR.
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (4)
289-330: Namespace allows only functions; should structs be allowed inside namespaces?Current rule body accepts fn definitions only. If TDL intends to nest structs within namespaces, we’ll need to expand this rule. If top-level structs only are intended, ignore this.
Proposed grammar change (do not edit generated .cpp; update TaskDefLang.g4 and regenerate):
-namespace - : 'namespace' id '{' funcDef* '}' +namespace + : 'namespace' id '{' (funcDef | structDef)* '}' ;I can follow up with tests that prove both nested and top-level structs parse as expected. Want me to do that?
618-675: Params don’t allow a trailing comma; struct fields do. Consider consistent trailing-comma policy.namedVarList is used for params and disallows a trailing comma, whereas structDef permits one before the closing brace. Consider allowing a trailing comma in params for consistency and easier diffs.
Proposed grammar tweak:
-params - : namedVarList? - ; +// Allow an optional trailing comma like in structs +params + : namedVar (',' namedVar)* (',')? + ;This would require removing the left-recursive namedVarList from params usage or introducing a second rule such as namedVarListWithOptTrailing. I can prep the concrete grammar diffs and regenerate if you confirm the desired policy.
983-1054: Tuple<> emptiness: grammar currently accepts an empty tuple type. Is that intended?varTypeList can be empty, so Tuple<> parses successfully. If zero-arity tuples aren’t part of TDL, tighten the rule to require at least one type.
Proposed grammar change:
-// 0 or more -varTypeList - : (varType (',' varType)*)? - ; +// 1 or more +varTypeList + : varType (',' varType)* + ;Follow-up tests I can add:
- Positive: Tuple
- Positive: Tuple<int32, double>
- Negative: Tuple<>
903-953: Tuple types restricted to return positions; confirm that’s a deliberate design choice.retType supports tupleType, but varType doesn’t. This forbids tuples in params and struct fields. If that’s intentional, all good. If not, consider allowing tupleType in varType as well.
Possible grammar adjustment:
-varType - : builtinType - | id - ; +varType + : builtinType + | tupleType + | id + ;I can update tests to cover tuple params/fields if you confirm tuples should be generally usable.
📜 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 (8)
src/spider/tdl/parser/TaskDefLang.g4(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.cpp(2 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.h(1 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp(4 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h(2 hunks)src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.h(1 hunks)tests/tdl/test-parser.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.cpp
- src/spider/tdl/parser/antlr_generated/TaskDefLangLexer.h
🧰 Additional context used
🧬 Code graph analysis (4)
tests/tdl/test-parser.cpp (3)
src/spider/tdl/parser/parse.hpp (1)
parse_translation_unit_from_istream(19-20)src/spider/tdl/parser/parse.cpp (2)
parse_translation_unit_from_istream(15-43)parse_translation_unit_from_istream(15-16)src/spider/tdl/parser/SourceLocation.hpp (2)
SourceLocation(10-10)SourceLocation(10-10)
src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (1)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (1)
TranslationUnitContext(166-168)
src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.h (2)
src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (15)
std(24-26)std(28-30)std(32-34)std(36-38)std(40-42)std(44-46)std(48-50)std(52-54)std(56-58)std(60-62)std(64-66)std(68-70)std(72-74)std(76-78)std(80-82)src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (1)
TranslationUnitContext(166-168)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h (2)
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (120)
ID(755-757)ID(755-755)TranslationUnitContext(166-168)getRuleIndex(191-193)getRuleIndex(191-191)getRuleIndex(277-279)getRuleIndex(277-277)getRuleIndex(351-353)getRuleIndex(351-351)getRuleIndex(412-414)getRuleIndex(412-412)getRuleIndex(479-481)getRuleIndex(479-479)getRuleIndex(548-550)getRuleIndex(548-548)getRuleIndex(605-607)getRuleIndex(605-605)getRuleIndex(692-694)getRuleIndex(692-692)EOF(170-172)EOF(170-170)namespace_(174-176)namespace_(174-174)namespace_(178-180)namespace_(178-178)namespace_(289-289)structDef(182-184)structDef(182-182)structDef(186-188)structDef(186-186)structDef(704-704)accept(196-201)accept(196-196)accept(282-287)accept(282-282)accept(356-361)accept(356-356)accept(417-422)accept(417-417)accept(484-489)accept(484-484)accept(553-558)accept(553-553)accept(610-615)accept(610-610)accept(697-702)accept(697-697)id(264-266)id(264-264)id(338-340)id(338-338)id(539-541)id(539-539)id(683-685)id(683-683)id(772-772)id(808-810)id(808-808)funcDef(268-270)funcDef(268-268)funcDef(272-274)funcDef(272-272)funcDef(363-363)params(342-344)params(342-342)params(491-491)ret(346-348)ret(346-346)ret(424-424)retType(407-409)retType(407-407)retType(903-903)namedVarList(474-476)namedVarList(474-474)namedVarList(600-602)namedVarList(600-600)namedVarList(618-620)namedVarList(618-618)namedVarList(622-622)namedVarList(687-689)namedVarList(687-687)varType(543-545)varType(543-543)varType(825-825)varType(882-884)varType(882-882)varType(961-963)varType(961-961)varType(1062-1064)varType(1062-1062)varType(1117-1119)varType(1117-1117)varType(1121-1123)varType(1121-1121)namedVar(560-560)namedVar(596-598)namedVar(596-596)builtinType(804-806)builtinType(804-804)builtinType(1256-1256)tupleType(886-888)tupleType(886-886)tupleType(1197-1197)varTypeList(965-967)varTypeList(965-965)varTypeList(983-985)varTypeList(983-983)varTypeList(987-987)varTypeList(1180-1182)varTypeList(1180-1180)listType(1079-1079)listType(1235-1237)listType(1235-1235)mapType(1138-1138)mapType(1239-1241)mapType(1239-1239)namedVarListSempred(1359-1367)namedVarListSempred(1359-1359)varTypeListSempred(1369-1377)varTypeListSempred(1369-1369)src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (15)
std(24-26)std(28-30)std(32-34)std(36-38)std(40-42)std(44-46)std(48-50)std(52-54)std(56-58)std(60-62)std(64-66)std(68-70)std(72-74)std(76-78)std(80-82)
⏰ 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-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
🔇 Additional comments (11)
src/spider/tdl/parser/TaskDefLang.g4 (1)
45-48: Tuple types restricted to return positions — good alignment with product decision.
retType: varType | tupleTypeensures tuples are only valid as return types (not as variable types), matching prior discussion. Tests also cover rejecting tuple as a parameter type.src/spider/tdl/parser/antlr_generated/TaskDefLangParser.h (1)
23-27: Generated API matches the grammar changes (translation unit root, per‑rule contexts).The token set and rule contexts align with the updated grammar, and the public entry points (e.g.,
translationUnit()) look correct. No manual fixes needed here.Also applies to: 46-62, 78-80
src/spider/tdl/parser/antlr_generated/TaskDefLangVisitor.h (1)
23-53: Visitor surface is consistent and complete.Per‑rule visitor methods match the new contexts; default base visitor covers them. LGTM.
tests/tdl/test-parser.cpp (1)
50-53: No stray brace inaddErrorListenercallsI verified both invocations in
src/spider/tdl/parser/parse.cppand they are correctly written aslexer.addErrorListener(&lexer_error_listener); parser.addErrorListener(&parser_error_listener);with no extra
}inside the parentheses. You can safely disregard the build‐break concern.Likely an incorrect or invalid review comment.
src/spider/tdl/parser/antlr_generated/TaskDefLangBaseVisitor.h (1)
20-82: Base visitor overrides are exhaustive and correctly default to visitChildren.Matches the new parser contexts; nothing to change.
src/spider/tdl/parser/antlr_generated/TaskDefLangParser.cpp (6)
2-8: Heads-up: Generated source — treat as artefact, not hand-edited.This file is generated by ANTLR 4.13.2. Make sure we pin the toolchain (grammar + codegen + runtime) in build docs/CMake to avoid runtime/codegen drift. No changes requested here; just a reminder to regenerate via the grammar rather than editing this file directly.
Would you like me to add a short README note or a CMake comment that documents the exact ANTLR version and regeneration command?
40-54: Initialisation and thread-safety flow LGTM.The TLS vs call_once paths are standard for ANTLR and look correct. No concerns.
Also applies to: 1379-1385
203-256: Root rule transition to translationUnit() looks correct.The loop over top-level constructs until EOF with lookahead on 'namespace' or 'struct' is clean and matches the stated grammar.
704-747: Struct rule looks solid, including optional trailing comma and mandatory terminating semicolon.Behaviour matches the stated intent for field lists and top-level declarations.
1256-1346: Builtin types set seems minimal; confirm coverage (e.g., string/bytes).Builtins include numeric types and bool, plus List/Map. If TDL needs textual or binary blobs (string/bytes), they’ll have to be added in the lexer and grammar.
I can draft the additions (tokens + grammar + tests) if this gap is unintentional.
164-202: No stale ANTLR entrypoint or visitor references foundThe ripgrep results only surfaced calls to the application’s
start()API in tests and driver/context classes, not to the old grammar’sstart()orStartContext, and the parser is correctly invokingparser.translationUnit()inparse.cpp. There are no lingeringStartContextorvisitStartreferences outside of the generated ANTLR code.
2 participants
Description
This PR adds the full TDL grammar. The grammar is written in a way that compiler actions can be easily added in a future PR to construct an AST.
This PR further introduces a boilerplate parser based on this grammar. The parser can successfully parse a valid TDL file, and it can also raise an error if the file contains any syntax errors.
Two unit test cases are added in this PR to ensure:
NOTE:
For the parser method, we have to use
boost::outcome_v2::std_checked. The default result from ystdlib can't be used since the error typeErrorisn't compatible with any of the policies list here.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Improved Error Reporting
Tests
Chores