feat(tdl): Add Function AST node. - #195
Conversation
WalkthroughAdds a new Function AST node to the TDL: introduces Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant FunctionFactory as Function::create
participant Validator
participant AST as Function node
Caller->>FunctionFactory: create(name, return_type, params)
FunctionFactory->>Validator: validate name kind, return type kind, param kinds
FunctionFactory->>Validator: check duplicate param names
alt validation OK
FunctionFactory->>AST: construct node and attach children
AST-->>FunctionFactory: node
FunctionFactory-->>Caller: Result(node)
else validation error
FunctionFactory-->>Caller: Result(error)
end
sequenceDiagram
actor Caller
participant FunctionNode as Function
participant Params as NamedVar[]
participant ReturnType as Type (opt)
Caller->>FunctionNode: serialize_to_str(indent)
FunctionNode->>Params: visit_params -> serialize each (indent+2)
alt has return
FunctionNode->>ReturnType: serialize_to_str(indent+2)
ReturnType-->>FunctionNode: return string
else no return
FunctionNode-->>FunctionNode: use "void"
end
FunctionNode-->>Caller: formatted string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewersTip 🔌 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 (
|
sitaowang1998
changed the title
Function AST node.Function AST node.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/tdl/test-parser-ast.cpp (1)
493-670: Consider adding a focused test for visit_params ordering and propagationOptionally add a small test to exercise Function::visit_params to ensure parameters are visited in-order and errors from the visitor propagate.
For example, within SECTION("Function"), add:
SECTION("visit_params enumerates in order") { auto func_result{Function::create( Identifier::create(std::string{cTestFuncName}), {}, std::vector<std::unique_ptr<Node>>{ create_named_var("a", Int::create(IntSpec::Int8)), create_named_var("b", Int::create(IntSpec::Int16)) } )}; REQUIRE_FALSE(func_result.has_error()); auto const* func_node{dynamic_cast<Function const*>(func_result.value().get())}; REQUIRE(nullptr != func_node); std::vector<std::string> names; auto res = func_node->visit_params([&](NamedVar const& nv) -> ystdlib::error_handling::Result<void> { names.emplace_back(std::string{nv.get_id()->get_name()}); return ystdlib::error_handling::success(); }); REQUIRE_FALSE(res.has_error()); REQUIRE(names == std::vector<std::string>{"a", "b"}); }src/spider/tdl/parser/ast/node_impl/Function.hpp (1)
111-113: Initialize m_has_return defensivelyAlthough the only constructor sets m_has_return, adding an in-class default prevents accidental uninitialized use if future constructors are introduced.
Apply this diff:
- bool m_has_return; + bool m_has_return{false};src/spider/tdl/parser/ast/node_impl/Function.cpp (2)
22-38: Define error-category specializations in the primary template’s namespace for claritySpecializing member functions of ErrorCategory is typically done within ystdlib::error_handling for readability and to avoid any confusion with alias-based specializations. Recommend moving these definitions into the namespace and dropping the alias for the specializations.
Apply this diff:
-using spider::tdl::parser::ast::node_impl::Function; -using FunctionErrorCodeCategory = ystdlib::error_handling::ErrorCategory<Function::ErrorCodeEnum>; - -template <> -auto FunctionErrorCodeCategory::name() const noexcept -> char const* { - return "spider::tdl::parser::ast::node_impl::Function"; -} - -template <> -auto FunctionErrorCodeCategory::message(Function::ErrorCodeEnum error_enum) const -> std::string { - switch (error_enum) { - case Function::ErrorCodeEnum::DuplicatedParamName: - return "The parameters have duplicated names."; - default: - return "Unknown error code enum"; - } -} +using spider::tdl::parser::ast::node_impl::Function; +namespace ystdlib::error_handling { +template <> +auto ErrorCategory<spider::tdl::parser::ast::node_impl::Function::ErrorCodeEnum>::name() const noexcept + -> char const* { + return "spider::tdl::parser::ast::node_impl::Function"; +} + +template <> +auto ErrorCategory<spider::tdl::parser::ast::node_impl::Function::ErrorCodeEnum>::message( + spider::tdl::parser::ast::node_impl::Function::ErrorCodeEnum error_enum) const -> std::string { + switch (error_enum) { + case spider::tdl::parser::ast::node_impl::Function::ErrorCodeEnum::DuplicatedParamName: + return "The parameters have duplicated names."; + default: + return "Unknown error code enum"; + } +} +} // namespace ystdlib::error_handling
75-101: Minor perf nit: pre-reserve serialized_params capacityWe know the param count; reserving avoids reallocations in the common case.
Apply this diff:
auto Function::serialize_to_str(size_t indentation_level) const -> ystdlib::error_handling::Result<std::string> { - std::vector<std::string> serialized_params; + std::vector<std::string> serialized_params; + serialized_params.reserve(get_num_params());
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/spider/CMakeLists.txt(2 hunks)src/spider/tdl/parser/ast/node_impl/Function.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/Function.hpp(1 hunks)tests/tdl/test-parser-ast.cpp(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/tdl/test-parser-ast.cpp (7)
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (4)
nodiscard(45-49)nodiscard(62-62)name(37-38)Struct(66-66)src/spider/tdl/parser/ast/node_impl/Function.hpp (9)
nodiscard(51-51)nodiscard(53-57)nodiscard(62-69)nodiscard(71-73)nodiscard(87-100)nodiscard(107-109)name(40-44)Function(104-104)Function(104-104)src/spider/tdl/parser/ast/Node.hpp (9)
nodiscard(46-46)nodiscard(51-51)nodiscard(74-80)nodiscard(113-115)Node(32-32)Node(36-36)Node(40-40)Node(40-40)Node(95-95)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.hpp (1)
nodiscard(32-32)src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (4)
nodiscard(53-57)nodiscard(59-59)nodiscard(73-84)name(45-46)src/spider/tdl/parser/ast/node_impl/Identifier.hpp (4)
name(22-24)name(22-22)Identifier(35-35)Identifier(35-35)src/spider/tdl/parser/ast/node_impl/NamedVar.hpp (1)
NamedVar(50-50)
src/spider/tdl/parser/ast/node_impl/Function.hpp (1)
src/spider/tdl/parser/ast/node_impl/Function.cpp (2)
name(26-28)name(26-26)
src/spider/tdl/parser/ast/node_impl/Function.cpp (1)
src/spider/tdl/parser/ast/node_impl/Function.hpp (5)
Function(104-104)Function(104-104)name(40-44)indentation_level(47-48)visit_params(87-88)
⏰ 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-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (11)
src/spider/CMakeLists.txt (1)
198-206: Build integration for Function node looks correctFunction.cpp and Function.hpp are properly wired into SPIDER_TDL_SHARED_SOURCES/HEADERS, and spider_tdl already links fmt and absl::flat_hash_set which Function.cpp uses. LGTM.
Also applies to: 216-224
tests/tdl/test-parser-ast.cpp (6)
28-44: Good test-only helpers; concise and safeThe helpers reduce duplication and correctly validate creation results before returning nodes. Clear and idiomatic.
Also applies to: 45-63
511-557: Function Basic case: comprehensive assertions and exact serialization matchCovers children count, param count, name, return presence, and full serialization. Solid.
559-596: Function without return type: checks are accurateValidates nullptr return type, child/param counts, and expected serialization with void. Looks good.
598-634: Function with empty param list: correct expectationsAsserts zero params and verifies serialized “No Params” block. Good coverage.
636-658: Function with no return and no params: edge case coveredVerifies minimal child set and serialization. Nicely done.
660-669: Duplicate parameter name detection test is preciseCorrectly constructs a duplicate and asserts error code mapping to DuplicatedParamName.
src/spider/tdl/parser/ast/node_impl/Function.hpp (2)
21-29: Error code scaffolding is minimal and clearThe enum and alias integrate cleanly with ystdlib error handling. LGTM.
71-74: Param count derivation is concise and correctDeriving param count from total children minus non-param children is robust to both return/no-return cases. Good.
src/spider/tdl/parser/ast/node_impl/Function.cpp (2)
41-73: Factory validation and child ordering look solid
- Validates Identifier name, optional Type return, and NamedVar params.
- Duplicate param detection using absl::flat_hash_set<string_view> is efficient and correct.
- Children added in the intended order: name, optional return, then params.
LGTM.
101-122: Serializer produces stable, exact outputWell-structured output with consistent indentation, void handling, and conditional Params vs No Params block. Matches tests’ expectations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/spider/tdl/parser/ast/node_impl/Function.hpp (5)
30-39: Clarify factory contract: document optional return type and child invariants.The docstring doesn’t currently state that
return_typeis optional (nullptr) nor the exact type expectations for inputs. Tightening this helps callers and future maintainers.Apply this diff to enrich the comment:
/** - * @param name - * @param return_type - * @param params + * @param name Must be an `Identifier`; cannot be null. + * @param return_type Optional; pass nullptr to represent “no return”. If non-null, must be a `Type`. + * @param params Zero or more parameters; each must be a `NamedVar`. Duplicate parameter names are rejected. * @return A result containing a unique pointer to a new `Function` instance with the given * name, return type, and parameters on success, or an error code indicating the failure: * - ErrorCodeEnum::DuplicatedParamName if `params` contains duplicated parameter names. - * - Forwards `validate_child_node_type`'s return values. + * - Forwards `validate_child_node_type`'s return values. + * @note Child nodes are never stored as null; when the function has no return type, no child is stored at index 1. */
40-45: Consider a typed factory overload for stronger compile-time guarantees.The current factory accepts generic
Nodepointers and validates at runtime. Offering an additional overload likecreate(std::unique_ptr<Identifier>, std::unique_ptr<Type>, std::vector<std::unique_ptr<NamedVar>>)would give users a type-safe path while keeping this generic one for flexibility.If you’re open to it, I can draft the overload and forward it to the existing implementation.
53-57: Minor: prefer reference static_cast for readability.Casting the child to a reference avoids an extra pointer hop and reads a bit clearer.
Apply this diff:
- return static_cast<Identifier const*>(get_child_unsafe(0))->get_name(); + return static_cast<Identifier const&>(*get_child_unsafe(0)).get_name();
63-69: Comment wording: align with the actual condition.The comment mentions “if not nullptr” but the code path is guarded by
m_has_return. Align the comment with the actual invariant the factory enforces.Apply this diff:
- // The factory function ensures that the second child is of type `Type`, if not nullptr. + // The factory function ensures that if `m_has_return` is true, the second child is of type `Type`.
82-100: Nice param visitor API; small flexibility enhancement possible.Great use of a constrained template to enforce the visitor signature. If you want to be more permissive, consider allowing visitors that return
void(and internally wrap them intoResult<void>). Not required, but could make call sites lighter for simple visitors.Happy to sketch a small adapter (SFINAE/if constexpr) that supports both
Result<void>andvoidvisitors.
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/spider/tdl/parser/ast/node_impl/Function.hpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/spider/tdl/parser/ast/node_impl/Function.hpp (1)
src/spider/tdl/parser/ast/node_impl/Function.cpp (2)
name(26-28)name(26-26)
⏰ 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 (3)
src/spider/tdl/parser/ast/node_impl/Function.hpp (3)
71-73: Confirm noexcept correctness.
get_num_params()is markednoexceptand callsget_num_children(). Please ensureNode::get_num_children()isnoexcept; otherwise, an exception here would invokestd::terminate().If needed, I can scan the codebase to confirm
get_num_children()isnoexcept.
116-118: ODR check: ensure the error-code enum registration is not defining non-inline symbols in the header.Placing
YSTDLIB_ERROR_HANDLING_MARK_AS_ERROR_CODE_ENUM(...)in a header can introduce multiple definitions at link time unless it’s header-only/inline by design. The related implementation for the category exists inFunction.cpp(per the snippets), so this is probably fine—but please confirm the macro only declares traits/specializations and doesn’t emit TU-local objects.If it does emit non-inline symbols, move the invocation to
Function.cpp. For example:-YSTDLIB_ERROR_HANDLING_MARK_AS_ERROR_CODE_ENUM( - spider::tdl::parser::ast::node_impl::Function::ErrorCodeEnum -); +// Moved to Function.cpp to avoid ODR issues.And in src/spider/tdl/parser/ast/node_impl/Function.cpp, add:
#include <spider/tdl/parser/ast/node_impl/Function.hpp> YSTDLIB_ERROR_HANDLING_MARK_AS_ERROR_CODE_ENUM( spider::tdl::parser::ast::node_impl::Function::ErrorCodeEnum );
21-114: Overall: solid design and clear invariants.The node layout and API make sense, invariants are well-stated, and the param-visit helper is convenient. Header-only pieces are minimal and justified by the template. Assuming the factory enforces the documented invariants, this looks good.
2 participants
Description
This PR adds
FunctionAST node. A function nodes contains the following (stored in sequence as child nodes):Since we don't allow the child node to be null, if the function doesn't have a return type, we don't store any node for the return.
Checklist
breaking change.
Validation performed
Functionnode.Summary by CodeRabbit
New Features
Tests