fix(tdl): Add source location information when serializing an AST node. - #211
Conversation
WalkthroughAdds SourceLocation::serialize_to_str and updates many AST/type serializer implementations to embed source-location text in their outputs. Several nodes switch name rendering from direct getters to serializing the first child node. Tests are updated to reflect the new location-aware format and expanded input coverage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant N as AST Node (e.g., Function)
participant SL as SourceLocation
participant C as Child Node (Name)
participant F as Formatter
N->>SL: serialize_to_str()
SL-->>N: "(line:column)"
N->>C: serialize_to_str(indentation+2)
C-->>N: "child-serialized"
N->>F: fmt::format(header with location, blocks)
F-->>N: "final-serialized"
note over N: Output includes [Tag](line:column) and nested Name block
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 0
🧹 Nitpick comments (6)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.cpp (1)
15-19: Reduce duplication with a tiny formatter helperMultiple node serializers now repeat the same pattern: indentation + node tag + SourceLocation + optional colon. Consider a small helper (e.g., format_node_header(tag, indentation_level, loc)) in ast/utils to keep this consistent and make future tweaks trivial.
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.cpp (1)
14-18: Possible formatting inconsistency: missing trailing colon vs other type serializersInt (and several others) render “…]]](line:col):…”. Bool currently stops at the location with no colon. If downstream consumers or tests expect a colon after the type tag uniformly, this will be a surprise. If Bool truly has no spec, you can still emit a terminal “:”.
Proposed tweak:
- return fmt::format( - "{}[Type[Primitive[Bool]]]{})", - create_indentation(indentation_level), - get_source_location().serialize_to_str() - ); + return fmt::format( + "{}[Type[Primitive[Bool]]]{}:", + create_indentation(indentation_level), + get_source_location().serialize_to_str() + );If tests intentionally assert no colon for Bool, keep as-is and consider documenting the exception.
src/spider/tdl/parser/SourceLocation.hpp (1)
5-8: Move formatting out of the header to cut compile-time and couplingHaving fmt/format.h in a widely included header increases rebuild cost and couples all TUs to fmt. Make serialize_to_str out-of-line and drop the fmt include from the header.
Header changes:
@@ -#include <string> - -#include <fmt/format.h> +#include <string> @@ - [[nodiscard]] auto serialize_to_str() const -> std::string { - return fmt::format("({}:{})", m_line, m_column); - } + [[nodiscard]] auto serialize_to_str() const -> std::string;Add a new implementation file:
// src/spider/tdl/parser/SourceLocation.cpp #include "SourceLocation.hpp" #include <fmt/format.h> namespace spider::tdl::parser { auto SourceLocation::serialize_to_str() const -> std::string { return fmt::format("({}:{})", m_line, m_column); } } // namespace spider::tdl::parserOptional nit: get_line()/get_column() can be constexpr without behaviour change.
Also applies to: 20-23
src/spider/tdl/parser/ast/node_impl/Identifier.cpp (1)
14-19: Guard against ambiguous names in pretty-printingIf identifiers can contain ':' or newline, consider quoting or escaping to keep the output trivially machine-parsable (e.g., wrap m_name in quotes and escape embedded quotes/newlines).
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (1)
93-99: Consider centralizing node header formattingMultiple serializers now repeat the pattern "{}{Tag}{}:". A tiny helper (e.g., format_node_header(indent, tag, loc)) would remove duplication and keep future tweaks to one place.
src/spider/tdl/parser/ast/node_impl/TranslationUnit.cpp (1)
74-81: Add explicit include for SourceLocation to avoid transitive dependencyThis file uses get_source_location().serialize_to_str() but doesn’t include SourceLocation.hpp, unlike the other updated serializers. Add the include for consistency and build hygiene.
Apply this diff near the other includes:
#include <spider/tdl/parser/ast/node_impl/StructSpec.hpp> #include <spider/tdl/parser/ast/utils.hpp> +#include <spider/tdl/parser/SourceLocation.hpp>
📜 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 (16)
src/spider/tdl/parser/SourceLocation.hpp(2 hunks)src/spider/tdl/parser/ast/node_impl/Function.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/Identifier.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/NamedVar.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/Namespace.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/StructSpec.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/TranslationUnit.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp(2 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.cpp(1 hunks)tests/tdl/test-parser-ast.cpp(17 hunks)tests/tdl/test-parser.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
src/spider/tdl/parser/ast/node_impl/Identifier.cpp (4)
src/spider/tdl/parser/ast/utils.cpp (2)
create_indentation(14-19)create_indentation(14-14)src/spider/tdl/parser/ast/Node.hpp (1)
indentation_level(91-92)src/spider/tdl/parser/ast/node_impl/Identifier.hpp (1)
indentation_level(29-30)src/spider/tdl/parser/ast/node_impl/NamedVar.hpp (1)
indentation_level(35-36)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.cpp (4)
src/spider/tdl/parser/ast/utils.hpp (1)
create_indentation(22-22)src/spider/tdl/parser/ast/utils.cpp (2)
create_indentation(14-19)create_indentation(14-14)src/spider/tdl/parser/ast/Node.hpp (1)
indentation_level(91-92)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
indentation_level(27-28)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp (2)
src/spider/tdl/parser/ast/utils.hpp (1)
create_indentation(22-22)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.hpp (1)
indentation_level(31-32)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.cpp (4)
src/spider/tdl/parser/ast/utils.hpp (1)
create_indentation(22-22)src/spider/tdl/parser/ast/utils.cpp (2)
create_indentation(14-19)create_indentation(14-14)src/spider/tdl/parser/ast/Node.hpp (1)
indentation_level(91-92)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (1)
indentation_level(30-31)
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (3)
src/spider/tdl/parser/ast/utils.hpp (1)
create_indentation(22-22)src/spider/tdl/parser/ast/Node.hpp (1)
indentation_level(91-92)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (1)
indentation_level(43-44)
src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (3)
src/spider/tdl/parser/ast/utils.hpp (1)
create_indentation(22-22)src/spider/tdl/parser/ast/Node.hpp (1)
indentation_level(91-92)src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (1)
indentation_level(53-54)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (2)
src/spider/tdl/parser/ast/utils.hpp (1)
create_indentation(22-22)src/spider/tdl/parser/ast/utils.cpp (2)
create_indentation(14-19)create_indentation(14-14)
src/spider/tdl/parser/ast/node_impl/TranslationUnit.cpp (1)
src/spider/tdl/parser/ast/node_impl/TranslationUnit.hpp (1)
indentation_level(45-46)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (3)
src/spider/tdl/parser/ast/utils.cpp (2)
create_indentation(14-19)create_indentation(14-14)src/spider/tdl/parser/ast/Node.hpp (1)
indentation_level(91-92)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp (1)
indentation_level(31-32)
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/ast/node_impl/NamedVar.cpp (1)
src/spider/tdl/parser/ast/utils.hpp (1)
create_indentation(22-22)
src/spider/tdl/parser/ast/node_impl/Namespace.cpp (3)
src/spider/tdl/parser/ast/utils.hpp (1)
create_indentation(22-22)src/spider/tdl/parser/ast/Node.hpp (1)
indentation_level(91-92)src/spider/tdl/parser/ast/node_impl/Namespace.hpp (1)
indentation_level(53-54)
⏰ 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 (34)
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.cpp (1)
15-19: LGTM: location inserted in the right spot for primitives with specsThe colon-delimited placement between the type tag and the spec mirrors the project-wide convention. Error propagation via TRYX is correct.
src/spider/tdl/parser/ast/node_impl/Identifier.cpp (1)
14-19: LGTM: consistent header + location + colon + payloadGood alignment with the new convention and clean use of indentation. No error-unwrapping needed here.
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (2)
52-62: LGTM: header now carries location; child-identifier serialisation preservedNice, the location is added after “[Type[Struct]]” and before the colon; the “Name:” block continues to render via the first child Identifier with proper error propagation.
52-62: Struct nodes are only instantiated via the factory –get_child_unsafe(0)is safeVerified via a repo-wide search that the only occurrence of
std::make_unique<Struct>(Struct{…})is in
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cppat line 45 (inside thecreate()method), and there are no other direct calls tonew Structormake_unique<Struct>elsewhere. This guarantees the first child is always anIdentifier, so usingget_child_unsafe(0)here is justified.• src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp:45 – lone instantiation in factory method
(Optional) If you’d like to harden against future regressions, you could add a
debug_assert(children().size() > 0)before serializing, but it isn’t strictly necessary given the invariant held by the factory.src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (1)
93-99: LGTM: Location suffix integrated correctly in Map serializationPlaceholders and indentation are consistent; the added source-location segment sits exactly before the colon as per the new convention.
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp (2)
38-43: LGTM: Empty Tuple now carries source locationHeader formatting with cTypeTag and location is correct and matches the project-wide pattern.
63-67: LGTM: Non-empty Tuple header embeds location as intendedThe header line is consistent with other container types; child traversal remains untouched.
src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (1)
35-41: LGTM: NamedVar serialization updated to include source locationHeader placement of the location string is consistent; Id and Type sections keep existing indentation semantics.
src/spider/tdl/parser/ast/node_impl/TranslationUnit.cpp (2)
74-81: LGTM: TranslationUnit header now includes source locationThe insertion point and formatting align with the new standard across nodes.
74-81: Align location serialization to “(line:column)” throughoutScanning the repository confirms that every call to
serialize_to_str()(including inSourceLocation.hpp) emits coordinates in the “(line:column)” form, and all existing tests expect that colon‐separated format. No occurrences of the old “(line, col)” syntax remain intests/or in node implementations.Action items:
- Update the PR description (and any accompanying docs) to refer to the “(line:column)” format instead of “(line_num, column_num)”.
- Confirm that any external consumers of the serialized AST (e.g., downstream tools, scripts or documentation) have been updated to parse “(line:column)” tokens and do not rely on a comma‐separated form.
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (1)
29-33: LGTM: Location added to List header cleanlyConsistent placement and indentation. No functional changes beyond serialization.
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.cpp (1)
15-18: Consistent source location inclusion in serialization.The implementation correctly adds the source location information to the serialization output using the new
get_source_location().serialize_to_str()pattern. This aligns with the PR objective to embed source location information in all AST node serializations.src/spider/tdl/parser/ast/node_impl/Namespace.cpp (1)
94-102: Proper implementation of source location and name serialization changes.The changes correctly:
- Add source location information via
get_source_location().serialize_to_str()- Replace direct name access (
get_name()) with child node serialization (get_child_unsafe(0)->serialize_to_str())- Maintain proper indentation hierarchy for the nested Name block
This follows the established pattern across other node types in the PR and preserves source location information for the identifier token.
src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (1)
94-102: Consistent source location and child serialization implementation.The implementation properly:
- Integrates source location using
get_source_location().serialize_to_str()- Changes from direct name access to serializing the first child node with proper error handling via
YSTDLIB_ERROR_HANDLING_TRYX- Maintains correct indentation levels for the nested Name structure
This aligns with the pattern established across other AST nodes in the PR.
tests/tdl/test-parser-ast.cpp (17)
184-184: Test update reflects new serialization format.The test expectation correctly includes the
(0:0)source location suffix, matching the new serialization format where all nodes include their source location information.
200-212: Comprehensive test coverage for primitive type location serialization.All Int type variants now correctly expect the
(0:0)source location suffix in their serialized output, ensuring thorough test coverage for the new location-aware serialization format.
231-236: Float type test expectations updated correctly.The test expectations for Float types properly include the source location suffix, maintaining consistency with the updated serialization format.
258-258: Bool type serialization test updated appropriately.The Bool type test correctly expects the location suffix in the serialized output.
278-290: Container type nested serialization tests updated comprehensively.The test expectations for List of Map correctly include source location information at every level:
- The outer List container:
[Type[Container[List]]](0:0)- The inner Map container:
[Type[Container[Map]]](0:0)- The primitive key and value types:
[Type[Primitive[Int]]](0:0)and[Type[Primitive[Float]]](0:0)This ensures complete test coverage for nested container serialization with source locations.
314-328: Complex nested container serialization correctly validated.The Map of List test expectations properly include source location suffixes at all nesting levels, providing comprehensive validation of the new serialization format for complex nested structures.
405-419: NamedVar serialization test updated with proper location information.The test correctly expects source location suffixes for:
- The NamedVar node itself
- The nested Identifier
- The nested Map type and its key/value primitive types
This validates the complete serialization chain for named variables.
430-436: Empty Tuple serialization test updated appropriately.The empty Tuple test correctly expects the source location suffix while maintaining the "Empty" content designation.
458-474: Complex Tuple serialization thoroughly tested.The test expectations for Tuples with elements correctly include source location information at every level of nesting, ensuring comprehensive validation of the new format.
524-554: StructSpec serialization comprehensively validated.The test properly validates the new StructSpec serialization format with:
- Source location for the StructSpec itself
- Nested Name block with Identifier serialization
- Source locations for all field components (NamedVar nodes, Identifiers, and types)
This ensures complete test coverage for structured type definitions.
624-632: Struct type serialization test updated correctly.The Struct type test expectations include proper source location information and the nested Name structure, maintaining consistency with the new serialization pattern.
706-738: Function serialization thoroughly tested with all components.The comprehensive Function test correctly validates:
- Source location for the Function node
- Nested Name block with Identifier serialization
- Return type with full nested structure and locations
- Parameter serialization with complete type information and locations
This provides extensive validation of function declaration serialization.
762-786: Function without return type properly tested.The test correctly handles functions with void return types while maintaining proper source location information throughout the serialization.
809-828: Function with empty parameters correctly validated.The test for functions with no parameters properly includes source location information and maintains the "No Params" designation.
847-858: Minimal function case thoroughly tested.The test for functions with no parameters and no return type correctly validates the simplest function serialization case with proper source location inclusion.
903-925: Namespace serialization comprehensively validated.The Namespace test expectations correctly include:
- Source location for the Namespace itself
- Nested Name block structure
- Complete Function serialization with all components and their source locations
This ensures thorough validation of namespace declarations.
966-1021: TranslationUnit serialization thoroughly tested.The comprehensive TranslationUnit test validates the complete hierarchy with source locations at every level:
- TranslationUnit root node
- All nested StructSpecs with their components
- All nested Namespaces with their Function contents
This provides complete end-to-end validation of the serialization system.
src/spider/tdl/parser/ast/node_impl/Function.cpp (2)
105-116: Function serialization properly updated with source location and name changes.The implementation correctly:
- Includes source location via
get_source_location().serialize_to_str()- Changes from direct name access to child node serialization with proper error handling
- Maintains consistent indentation structure for the nested Name block
- Handles both parameter and no-parameter cases appropriately
118-130: No-parameter case properly handled.The no-parameter branch of the function serialization maintains the same source location and name serialization changes while correctly handling the "No Params" case.
tests/tdl/test-parser.cpp (1)
271-273: No further assertions required – full-string comparison already enforces coordinate accuracyThe end-to-end equality check against
cExpectedSerializedAstwill fail if any location is off by even one character or line, so all source-location coordinates (including namespace, struct, functions, etc.) are already fully verified. Adding individualfind-style checks would be purely redundant:• Full literal match covers every position resolve_review_comment
2 participants
Description
Before this PR, the serialization method of an AST node would not serialize any source location info. This PR fixes this by properly serializing the source location:
(line_num, column_num).StructSpec,Function, andNamespace, the identifier (name) was serialized directly as a string before this PR. This PR fixes it by properly serializing the name as a child identifier node, so that the source location of the ID token can be tracked properly.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Tests