feat(tdl): Add helper classes for parser error handling. - #203
Conversation
WalkthroughIntroduces a new Error type and an ANTLR ErrorListener, moves SourceLocation from parser::ast to parser namespace, updates includes accordingly, adjusts tests, extends CMake public headers, and classifies antlr4 as an external header in .clang-format. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Parser as ANTLR Parser
participant EL as ErrorListener
participant TDL as spider::tdl::Error
User->>Parser: parse(input)
activate Parser
Parser->>EL: syntaxError(line, col, msg) [on error]
activate EL
note over EL: Format "<tag>: msg"<br/>Capture SourceLocation(line, col)
EL->>TDL: create Error(message, SourceLocation, optional error_code)
deactivate EL
Parser-->>User: result or error accessible via EL
deactivate Parser
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (1)
70-75: create(...) still uses unqualified SourceLocation; add alias or qualifyAfter moving
SourceLocationtospider::tdl::parser, the unqualified parameter inMap::createis risky unless a bridging alias exists. Recommend importing the symbol into this namespace or fully qualifying.Apply either:
Option A — import into the namespace (preferred for readability in this TU):
namespace spider::tdl::parser::ast::node_impl::type_impl::container_impl { +using ::spider::tdl::parser::SourceLocation; namespace {Option B — fully qualify the parameter:
- SourceLocation source_location + ::spider::tdl::parser::SourceLocation source_locationsrc/spider/tdl/parser/ast/node_impl/type_impl/Primitive.hpp (1)
11-13: Unqualified SourceLocation in constructor after namespace moveSame concern as in Container.hpp:
SourceLocationis no longer in the enclosingastnamespace. Import or fully qualify to avoid lookup failures.Suggested patch (alias approach):
namespace spider::tdl::parser::ast::node_impl::type_impl { +using ::spider::tdl::parser::SourceLocation; // Abstract base class for all primitive type nodes in the AST. class Primitive : public Type { protected: // Constructor explicit Primitive(SourceLocation source_location) : Type{source_location} {}src/spider/tdl/parser/ast/node_impl/Namespace.hpp (1)
46-50: Factory and ctor use unqualified SourceLocation; qualify or importBoth the
create(...)factory signature and the private constructor takeSourceLocationby value without qualification. After the type’s move tospider::tdl::parser, this is likely to break unless you have a compatibility alias.Minimal change:
namespace spider::tdl::parser::ast::node_impl { +using ::spider::tdl::parser::SourceLocation; ... [[nodiscard]] static auto create( std::unique_ptr<Node> name, std::vector<std::unique_ptr<Node>> functions, SourceLocation source_location ) -> ystdlib::error_handling::Result<std::unique_ptr<Node>>; ... private: // Constructor - explicit Namespace(SourceLocation source_location) : Node{source_location} {} + explicit Namespace(SourceLocation source_location) : Node{source_location} {}Alternatively, fully qualify both occurrences with
::spider::tdl::parser::SourceLocation.Also applies to: 91-93
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (1)
41-44: create(...) uses unqualified SourceLocation post-move
Struct::createtakesSourceLocationby value without qualification. Bring the moved type into scope or fully qualify.Apply one of:
Option A — alias in this namespace:
namespace spider::tdl::parser::ast::node_impl::type_impl { +using ::spider::tdl::parser::SourceLocation; auto Struct::create(std::unique_ptr<Node> name, SourceLocation source_location)Option B — fully qualify the parameter:
-auto Struct::create(std::unique_ptr<Node> name, SourceLocation source_location) +auto Struct::create(std::unique_ptr<Node> name, ::spider::tdl::parser::SourceLocation source_location)
🧹 Nitpick comments (11)
src/spider/tdl/parser/SourceLocation.hpp (3)
1-2: Header guard rename is correct; consider adding pragma once.Guard matches the new path/name. Optional: add
#pragma oncefor faster, simpler include protection on mainstream compilers.Apply:
#ifndef SPIDER_TDL_PARSER_SOURCELOCATION_HPP #define SPIDER_TDL_PARSER_SOURCELOCATION_HPP +// Optional, complementary to include guards. +#pragma once
6-6: Namespace relocation is a breaking change — provide a deprecation shim.Moving
SourceLocationfromspider::tdl::parser::asttospider::tdl::parserwill break external users that:
- include
<spider/tdl/parser/ast/SourceLocation.hpp>, or- reference
spider::tdl::parser::ast::SourceLocation.To smooth migration, consider adding a forwarding header at
src/spider/tdl/parser/ast/SourceLocation.hppthat includes this file and provides a deprecated alias:// src/spider/tdl/parser/ast/SourceLocation.hpp (shim) #pragma once #include <spider/tdl/parser/SourceLocation.hpp> namespace spider::tdl::parser::ast { using SourceLocation [[deprecated("Use spider::tdl::parser::SourceLocation")]] = spider::tdl::parser::SourceLocation; }If the project policy explicitly allows the break, document it prominently in the release notes and migration guide.
6-6: Minor API polish (optional): make trivial members constexpr.
SourceLocationis a tiny value type. Marking the ctor and accessorsconstexprenables compile-time use without runtime cost.Example change (inside this header):
// Optional improvements (C++20): constexpr SourceLocation(size_t line, size_t column) : m_line{line}, m_column{column} {} [[nodiscard]] constexpr auto get_line() const noexcept -> size_t { return m_line; } [[nodiscard]] constexpr auto get_column() const noexcept -> size_t { return m_column; }Also consider
= defaulting the special members and adding= defaultedoperator==if C++20 is guaranteed:constexpr SourceLocation(SourceLocation const&) = default; constexpr SourceLocation& operator=(SourceLocation const&) = default; constexpr ~SourceLocation() = default; constexpr bool operator==(SourceLocation const&) const = default;src/spider/tdl/parser/ast/node_impl/Identifier.hpp (1)
13-13: Consider a temporary compatibility shim to ease downstream breakageEven though the breaking change is acknowledged, you can smooth migration by adding a forwarding header at the old path that aliases the new type (and deprecates it). This lets external codebases compile during a transition window.
Example new file: src/spider/tdl/parser/ast/SourceLocation.hpp
#pragma once #include <spider/tdl/parser/SourceLocation.hpp> namespace spider::tdl::parser::ast { // Transitional alias; remove after one release. using SourceLocation [[deprecated( "Use spider::tdl::parser::SourceLocation and include <spider/tdl/parser/SourceLocation.hpp>" )]] = spider::tdl::parser::SourceLocation; } // namespace spider::tdl::parser::astOptionally install this header for one release cycle and mark it for removal in the changelog.
src/spider/tdl/parser/ast/node_impl/StructSpec.hpp (1)
18-18: Public header now includes parser/SourceLocation.hpp — consider explicit qualification for API clarity.Small style nit: since SourceLocation now lives in spider::tdl::parser, explicitly qualifying it in this public header avoids relying on unqualified lookup from a nested namespace and makes the API self-evident to readers.
Example:
// In declarations static auto create( std::unique_ptr<Node> name, std::vector<std::unique_ptr<Node>> fields, spider::tdl::parser::SourceLocation source_location ) -> ystdlib::error_handling::Result<std::shared_ptr<StructSpec>>; // In the inline ctor explicit StructSpec(spider::tdl::parser::SourceLocation source_location) : Node{source_location} {}Alternatively, a local using declaration inside this namespace is also acceptable:
using spider::tdl::parser::SourceLocation;src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.hpp (1)
15-15: Include updated — consider explicitly qualifying SourceLocation in the public API.Same rationale as in StructSpec.hpp: either fully qualify spider::tdl::parser::SourceLocation in the factory/ctor signatures or add a local using to make the dependency explicit.
src/spider/tdl/parser/ast/node_impl/Function.hpp (1)
19-19: Include updated — optional: fully qualify SourceLocation in declarations.For readability and to make the namespace move obvious to consumers of this header, consider using spider::tdl::parser::SourceLocation (or a local using) in create(...) and the constructor.
src/spider/tdl/Error.hpp (2)
27-27: Prefer returning const std::string& over std::string_view to avoid dangling view misuseReturning a view invites accidental long-lived references after the Error is moved. A const ref is safer with the same performance in typical usage.
Apply this diff:
- [[nodiscard]] auto get_message() const noexcept -> std::string_view { return m_message; } + [[nodiscard]] auto get_message() const noexcept -> std::string const& { return m_message; }
29-31: Optional: return SourceLocation by const reference to avoid copiesSourceLocation is small, so either is fine. Returning a const& avoids copies if this accessor is called frequently.
Apply this diff:
- [[nodiscard]] auto get_source_location() const noexcept -> parser::SourceLocation { - return m_source_location; - } + [[nodiscard]] auto get_source_location() const noexcept -> parser::SourceLocation const& { + return m_source_location; + }src/spider/tdl/parser/ErrorListener.hpp (2)
31-36: Preserve the first syntax error rather than overwriting on subsequent errorsStoring only one Error is fine, but current emplace overwrites earlier errors. Typically we keep the first to aid root-cause analysis.
Apply this diff:
- m_error.emplace( - fmt::format("{}: {}", m_tag, msg), - SourceLocation{line, char_position_in_line}, - std::nullopt - ); + if (!m_error) { + m_error.emplace( + fmt::format("{}: {}", m_tag, msg), + SourceLocation{line, char_position_in_line}, + std::nullopt + ); + }
38-50: Consider adding a reset() to reuse the listener across parsesIf the same listener instance is reused, exposing a clear/reset improves ergonomics.
Apply this diff:
public: @@ [[nodiscard]] auto error() const -> Error const& { @@ return m_error.value(); } + + // Reset captured error (for listener reuse) + void reset() noexcept { m_error.reset(); }
📜 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 (30)
src/spider/.clang-format(1 hunks)src/spider/CMakeLists.txt(2 hunks)src/spider/tdl/Error.hpp(1 hunks)src/spider/tdl/parser/ErrorListener.hpp(1 hunks)src/spider/tdl/parser/SourceLocation.hpp(2 hunks)src/spider/tdl/parser/ast/Node.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/Function.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/Function.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/Identifier.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/NamedVar.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/NamedVar.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/Namespace.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/Namespace.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/StructSpec.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/StructSpec.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/Type.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/Container.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/Primitive.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp(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/List.hpp(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/Map.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp(1 hunks)src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp(1 hunks)tests/tdl/test-parser-ast.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T21:38:24.972Z
Learnt from: LinZhihao-723
PR: y-scope/spider#196
File: src/spider/tdl/parser/ast/node_impl/Namespace.cpp:22-40
Timestamp: 2025-08-14T21:38:24.972Z
Learning: The spider codebase uses template specializations via type aliases for ErrorCategory implementations. The pattern `using SomeErrorCodeCategory = ystdlib::error_handling::ErrorCategory<SomeClass::ErrorCodeEnum>; template <> auto SomeErrorCodeCategory::name() const noexcept -> char const*` is valid and compiles correctly with both clang and gcc in this codebase.
Applied to files:
src/spider/tdl/parser/ast/node_impl/Function.cppsrc/spider/tdl/parser/ast/node_impl/Namespace.cpp
🧬 Code Graph Analysis (3)
src/spider/tdl/Error.hpp (3)
src/spider/tdl/parser/ErrorListener.hpp (2)
nodiscard(39-39)nodiscard(45-50)src/spider/tdl/parser/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)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/ErrorListener.hpp (2)
src/spider/tdl/Error.hpp (3)
nodiscard(27-27)nodiscard(29-31)nodiscard(33-35)src/spider/tdl/parser/SourceLocation.hpp (2)
nodiscard(13-13)nodiscard(15-15)
tests/tdl/test-parser-ast.cpp (1)
src/spider/tdl/parser/ast/Node.hpp (5)
nodiscard(48-48)nodiscard(53-53)nodiscard(76-82)nodiscard(95-97)nodiscard(119-121)
⏰ 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 (30)
src/spider/tdl/parser/ast/node_impl/NamedVar.cpp (1)
15-15: Include path update aligns with namespace move — LGTM.Switching to <spider/tdl/parser/SourceLocation.hpp> is consistent with the SourceLocation relocation. No behavioural changes here.
src/spider/tdl/parser/ast/node_impl/NamedVar.hpp (1)
13-13: Include path switch — LGTM; unqualified name lookup will resolve correctly.Referencing
SourceLocationfrom withinspider::tdl::parser::ast::...remains valid due to lookup into the enclosingspider::tdl::parsernamespace. No further changes needed here.src/spider/tdl/parser/ast/Node.hpp (1)
14-14: All outdated references removed — migration complete.Ran a repo-wide search for the old include path and qualified symbol; no occurrences of:
#include <spider/tdl/parser/ast/SourceLocation.hpp>spider::tdl::parser::ast::SourceLocationwere found. Migration is complete and no further action is required.
src/spider/tdl/parser/ast/node_impl/Identifier.hpp (2)
13-13: Include path update aligns with SourceLocation namespace move — OKSwitching to <spider/tdl/parser/SourceLocation.hpp> matches the PR’s relocation and unqualified use of SourceLocation continues to resolve via the enclosing spider::tdl::parser namespace. No behavioural change in this header.
13-13: No lingering references to the old SourceLocation header foundAll ripgrep checks returned zero occurrences of the old
spider/tdl/parser/ast/SourceLocation.hppinclude orparser::ast::SourceLocationtype, and the public headers list inCMakeLists.txtcorrectly includes the newtdl/parser/SourceLocation.hpp. No further action is required.src/spider/tdl/parser/ast/node_impl/type_impl/Struct.hpp (1)
17-17: Include path switch looks correct; API surface unchangedMoving to <spider/tdl/parser/SourceLocation.hpp> is consistent with the namespace relocation. Factory and methods keep the same signatures (modulo namespace), so no internal behavioural change.
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.hpp (1)
13-13: Header include updated correctlyThe List factory/ctor still take SourceLocation; unqualified lookup will resolve to spider::tdl::parser post-move. No additional changes required here.
src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/List.cpp (1)
14-14: Consistent include update in implementation — OKMatches the header change and the broader relocation. No functional impact in List::create or serialize_to_str.
src/spider/tdl/parser/ast/node_impl/Function.cpp (1)
21-21: Include path update is correct; error category specializations remain validThe switch to <spider/tdl/parser/SourceLocation.hpp> is consistent. Based on earlier learnings in this codebase, the ErrorCategory specializations pattern used below remains correct and needs no adjustment after this include change.
src/spider/tdl/parser/ast/node_impl/type_impl/Container.hpp (2)
5-5: Include path update to public parser header looks correctThe include now points at the relocated public header. No concerns here.
11-13: No alias required: unqualifiedSourceLocationwill resolve to the parentparsernamespaceUnqualified lookup in C++ searches from the innermost namespace outward, so inside
namespace spider::tdl::parser::ast…it will findspider::tdl::parser::SourceLocationwithout a using-alias or full qualification. All headers already#include <spider/tdl/parser/SourceLocation.hpp>, and CMake exports only the new public header (tdl/parser/SourceLocation.hpp) with no stale paths. No changes are needed here.src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Map.cpp (1)
18-18: Include path update matches the relocationHeader path switched to the new public location. Looks good.
src/spider/tdl/parser/ast/node_impl/type_impl/Primitive.hpp (1)
5-5: Public header include updated correctlyThe include reflects the new location of
SourceLocation. All good.src/spider/tdl/parser/ast/node_impl/Namespace.hpp (1)
18-18: Include updated to new SourceLocation path — OKMatches the PR’s header relocation. No issues.
src/spider/tdl/parser/ast/node_impl/type_impl/Struct.cpp (1)
16-16: Include path switched to public parser header — LGTMConsistent with the relocation.
src/spider/tdl/parser/ast/node_impl/Namespace.cpp (2)
21-21: Include path switch to parser/SourceLocation.hpp is correct and consistent with the namespace move.Unqualified
SourceLocationstill resolves via enclosing namespace lookup (spider::tdl::parseris an ancestor of...::ast::node_impl), so no code changes are required here. Also confirmed the existingErrorCategoryspecializations pattern remains consistent with prior learnings for this codebase.Using retrieved learnings: Verified the specialization pattern aligns with the approach used previously in this file.
21-21: No staleast::SourceLocationreferences detectedThe verification script confirms there are no lingering fully-qualified references to
spider::tdl::parser::ast::SourceLocationnor any includes of the old header path<spider/tdl/parser/ast/SourceLocation.hpp>. All occurrences now uniformly use the newparser::SourceLocationtype and its header<spider/tdl/parser/SourceLocation.hpp>. Great work!src/spider/tdl/parser/ast/node_impl/Type.hpp (1)
5-5: Header include updated to new parser/SourceLocation.hpp — good.
Type’s constructor continues to useSourceLocationunqualified withinspider::tdl::parser::ast::..., which remains valid after the move. No ABI/API changes in this header.src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Float.hpp (1)
13-13: Include path migration looks correct.Factory, constructor, and methods remain unchanged; unqualified
SourceLocationresolves via the enclosingparsernamespace.src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Int.hpp (1)
13-13: Include path migration looks correct.No API/behaviour changes; consistent with the project-wide
SourceLocationrelocation.src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.hpp (1)
13-13: Include path migration looks correct.
Tuple::createand the constructor continue to useSourceLocationunqualified safely within nested namespaces.src/spider/tdl/parser/ast/node_impl/type_impl/container_impl/Tuple.cpp (1)
17-17: All oldSourceLocationreferences removed—consistency verifiedThe ripgrep search confirmed no lingering includes or qualified type names pointing to the old location:
- No
spider/tdl/parser/ast/SourceLocation.hppincludes found.- No usages of
spider::tdl::parser::ast::SourceLocationdetected.This rename is consistent across the repo. Approving the code changes.
src/spider/tdl/parser/ast/node_impl/StructSpec.cpp (1)
20-20: Include path update looks correct.Matches the namespace relocation of SourceLocation; no behavioural impact in this TU.
src/spider/tdl/parser/ast/node_impl/type_impl/primitive_impl/Bool.hpp (1)
12-12: Include path update matches SourceLocation move — LGTMThe switch to <spider/tdl/parser/SourceLocation.hpp> is consistent with the namespace relocation.
src/spider/CMakeLists.txt (1)
232-256: No stale includes or type references detectedAll checks confirm the relocation of
SourceLocationis properly handled:
- No occurrences of the old include path
<spider/tdl/parser/ast/SourceLocation.hpp>.- No remaining fully qualified or unqualified references to
ast::SourceLocation.- All headers now include
<spider/tdl/parser/SourceLocation.hpp>, and unqualified lookup withinspider::tdl::parser::astcorrectly resolves to the parent namespace.No further action needed.
tests/tdl/test-parser-ast.cpp (3)
28-28: Include path switched to parser-level SourceLocation — LGTM
58-58: Forward declaration updated to parser::SourceLocation — LGTM
104-106: Aggregate init of SourceLocation — LGTMAssuming SourceLocation remains an aggregate with (line, column), this is fine. If constructors are added later, consider switching to an explicit SourceLocation{0, 0}.
src/spider/tdl/Error.hpp (1)
16-25: Error class design is clean and minimal — LGTMStraightforward, self-contained error value with message, location, and optional error_code.
src/spider/tdl/parser/ErrorListener.hpp (1)
31-36: Verify line/column indexing matches SourceLocation semanticsANTLR reports line as 1-based and charPositionInLine as 0-based. Confirm SourceLocation consumers expect the same to avoid off-by-one issues in diagnostics.
2 participants
Description
This class adds two helper classes for parser error handling:
Erroris a general class designed to represent an error. This class shall be reused in the semantic analysis stage as well.ErrorListeneris an implementation of Antlr's error listener. This class holds anErrorof the parser error.Since
Erroralso contains the source location,SourceLocationis moved from theparser::astnamespace toparser, as it should be information extracted by the parser, but not particular to the AST.Checklist
breaking change.
Validation performed
Summary by CodeRabbit