Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/spider/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,8 @@ set(SPIDER_TDL_SHARED_SOURCES
tdl/parser/ast/node_impl/type_impl/Struct.cpp
tdl/parser/ast/utils.cpp
tdl/parser/parse.cpp
tdl/pass/analysis/DetectStructCircularDependency.cpp
tdl/pass/analysis/DetectUndefinedStruct.cpp
tdl/pass/analysis/StructSpecDependencyGraph.cpp
CACHE INTERNAL
"spider task definition language shared source files"
Expand Down Expand Up @@ -259,7 +261,11 @@ set(SPIDER_TDL_SHARED_HEADERS
tdl/parser/Exception.hpp
tdl/parser/parse.hpp
tdl/parser/SourceLocation.hpp
tdl/pass/analysis/DetectStructCircularDependency.hpp
tdl/pass/analysis/DetectUndefinedStruct.hpp
tdl/pass/analysis/StructSpecDependencyGraph.hpp
tdl/pass/Pass.hpp
tdl/pass/utils.hpp
CACHE INTERNAL
"spider task definition language shared header files"
)
Expand Down
11 changes: 11 additions & 0 deletions src/spider/tdl/parser/SourceLocation.hpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef SPIDER_TDL_PARSER_SOURCELOCATION_HPP
#define SPIDER_TDL_PARSER_SOURCELOCATION_HPP

#include <compare>
#include <cstddef>
#include <string>

Expand All @@ -25,6 +26,16 @@ class SourceLocation {
return m_line == other.m_line && m_column == other.m_column;
}

[[nodiscard]] auto operator<=>(SourceLocation const& other) const noexcept
-> std::strong_ordering {
if (auto const lint_comparison{m_line <=> other.m_line};
std::strong_ordering::equal != lint_comparison)
{
return lint_comparison;
}
return m_column <=> other.m_column;
}

private:
// Variables
size_t m_line;
Expand Down
31 changes: 27 additions & 4 deletions src/spider/tdl/parser/ast/node_impl/TranslationUnit.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <memory>
#include <string>
#include <string_view>
#include <type_traits>

#include <absl/container/flat_hash_map.h>
#include <ystdlib/error_handling/ErrorCode.hpp>
Expand Down Expand Up @@ -47,6 +48,28 @@ class TranslationUnit : public Node {
-> ystdlib::error_handling::Result<std::string> override;

// Methods
/**
* Visits all struct specs in the struct spec table in an unspecified order, invoking the given
* `visitor` for each struct spec.
* @tparam StructSpecVisitor
* @param visitor
* @return A void result on success, or an error code indicating the failure:
* - Forwards `visitor`'s return values.
*/
template <typename StructSpecVisitor>
requires(std::is_invocable_r_v<
ystdlib::error_handling::Result<void>,
StructSpecVisitor,
StructSpec const*
>)
[[nodiscard]] auto visit_struct_specs(StructSpecVisitor visitor) const
-> ystdlib::error_handling::Result<void> {
for (auto const& [_, struct_spec] : m_struct_spec_table) {
YSTDLIB_ERROR_HANDLING_TRYV(visitor(struct_spec.get()));
}
return ystdlib::error_handling::success();
}

/**
* @param name
* @return A shared pointer to the `StructSpec` with the given name if it exists in the struct
Expand Down Expand Up @@ -82,12 +105,12 @@ class TranslationUnit : public Node {
-> ystdlib::error_handling::Result<void>;

/**
* @return A newly constructed dependency graph of struct specs defined in this translation
* unit.
* @return A shared pointer pointing to a newly constructed dependency graph of struct specs
* defined in this translation unit.
*/
[[nodiscard]] auto create_struct_spec_dependency_graph() const
-> pass::analysis::StructSpecDependencyGraph {
return pass::analysis::StructSpecDependencyGraph{m_struct_spec_table};
-> std::shared_ptr<pass::analysis::StructSpecDependencyGraph> {
return std::make_shared<pass::analysis::StructSpecDependencyGraph>(m_struct_spec_table);
}

private:
Expand Down
63 changes: 63 additions & 0 deletions src/spider/tdl/pass/Pass.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#ifndef SPIDER_TDL_PASS_PASS_HPP
#define SPIDER_TDL_PASS_PASS_HPP

#include <memory>
#include <string>

#include <boost/outcome/std_result.hpp>

namespace spider::tdl::pass {
/**
* Represents an abstract pass over a TDL AST.
*/
class Pass {
public:
// Types
/**
* Represents an abstract error that can occur during the execution of a pass.
*/
class Error {
public:
// Constructor
Error() = default;

// Delete copy constructor and assignment operator
Error(Error const&) = delete;
auto operator=(Error const&) -> Error& = delete;

// Default move constructor and assignment operator
Error(Error&&) = default;
auto operator=(Error&&) -> Error& = default;

// Destructor
virtual ~Error() = default;

// Methods
[[nodiscard]] virtual auto to_string() const -> std::string = 0;
};

// Constructors
Pass() = default;

// Delete copy constructor and assignment operator
Pass(Pass const&) = delete;
auto operator=(Pass const&) -> Pass& = delete;

// Default move constructor and assignment operator
Pass(Pass&&) = default;
auto operator=(Pass&&) -> Pass& = default;

// Destructor
virtual ~Pass() = default;

// Methods
/**
* Executes the pass.
* @return A void result on success, or a pointer to the error on failure.
*/
[[nodiscard]] virtual auto run() -> boost::outcome_v2::std_checked<void, std::unique_ptr<Error>>
= 0;
};
} // namespace spider::tdl::pass

#endif // SPIDER_TDL_PASS_PASS_HPP
85 changes: 85 additions & 0 deletions src/spider/tdl/pass/analysis/DetectStructCircularDependency.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
#include "DetectStructCircularDependency.hpp"

#include <algorithm>
#include <memory>
#include <string>
#include <utility>
#include <vector>

#include <boost/outcome/std_result.hpp>
#include <boost/outcome/success_failure.hpp>
#include <fmt/format.h>
#include <fmt/ranges.h>

#include <spider/tdl/parser/ast/nodes.hpp>
#include <spider/tdl/pass/Pass.hpp>

namespace spider::tdl::pass::analysis {
auto DetectStructCircularDependency::Error::to_string() const -> std::string {
std::vector<std::string> circular_dependency_group_error_messages;
circular_dependency_group_error_messages.reserve(m_strongly_connected_components.size());
for (auto const& group : m_strongly_connected_components) {
std::vector<std::string> struct_descriptions;
struct_descriptions.reserve(group.size());
for (auto const& struct_spec : group) {
struct_descriptions.emplace_back(
fmt::format(
" `{}` at {}",
struct_spec->get_name(),
struct_spec->get_source_location().serialize_to_str()
)
);
}
circular_dependency_group_error_messages.emplace_back(
fmt::format(
"Found a circular dependency group of {} struct spec(s):\n{}",
group.size(),
fmt::join(struct_descriptions, "\n")
)
);
}
return fmt::format(
"Found {} circular dependency group(s):\n{}",
m_strongly_connected_components.size(),
fmt::join(circular_dependency_group_error_messages, "\n")
);
}

auto DetectStructCircularDependency::run()
-> boost::outcome_v2::std_checked<void, std::unique_ptr<Pass::Error>> {
auto const& strongly_connected_components{
m_struct_spec_dependency_graph->get_strongly_connected_components()
};
if (strongly_connected_components.empty()) {
return boost::outcome_v2::success();
}

std::vector<std::vector<std::shared_ptr<parser::ast::StructSpec const>>>
circular_dependency_groups;
circular_dependency_groups.reserve(strongly_connected_components.size());
for (auto const& scc : strongly_connected_components) {
std::vector<std::shared_ptr<parser::ast::StructSpec const>> group;
group.reserve(scc.size());
for (auto const id : scc) {
group.emplace_back(m_struct_spec_dependency_graph->get_struct_spec_from_id(id));
}
std::ranges::sort(group, [](auto const& lhs, auto const& rhs) -> bool {
return lhs->get_source_location() < rhs->get_source_location();
});
circular_dependency_groups.emplace_back(std::move(group));
}

std::ranges::sort(circular_dependency_groups, [](auto const& lhs, auto const& rhs) -> bool {
// Compare by the source location of the first struct spec in each group. This is safe
// because:
// - Each group is guaranteed to be non-empty.
// - Each struct spec should only appear in one SCC, which guarantees the source locations
// are unique.
Comment on lines +73 to +77

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment here is a little confusing. How does uniqueness have to do with the safety of the sort?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sorry my bad. I think instead of "is safe", I should say "is deterministic and non-ambiguous"

return lhs.front()->get_source_location() < rhs.front()->get_source_location();
});

return boost::outcome_v2::failure(
std::make_unique<Error>(std::move(circular_dependency_groups))
);
}
} // namespace spider::tdl::pass::analysis
63 changes: 63 additions & 0 deletions src/spider/tdl/pass/analysis/DetectStructCircularDependency.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#ifndef SPIDER_TDL_PASS_ANALYSIS_DETECTSTRUCTCIRCULARDEPENDENCY_HPP
#define SPIDER_TDL_PASS_ANALYSIS_DETECTSTRUCTCIRCULARDEPENDENCY_HPP

#include <memory>
#include <string>
#include <utility>
#include <vector>

#include <boost/outcome/std_result.hpp>

#include <spider/tdl/parser/ast/nodes.hpp>
#include <spider/tdl/pass/analysis/StructSpecDependencyGraph.hpp>
#include <spider/tdl/pass/Pass.hpp>

namespace spider::tdl::pass::analysis {
/**
* Wrapper of `StructSpecDependencyGraph` to detect circular dependencies among struct specs.
*/
class DetectStructCircularDependency : public Pass {
public:
// Types
/**
* Represents an error including all circular dependency groups (reported as strongly connected
* components).
*/
class Error : public Pass::Error {
public:
// Constructor
explicit Error(
std::vector<std::vector<std::shared_ptr<parser::ast::StructSpec const>>>
strongly_connected_components
)
: m_strongly_connected_components{std::move(strongly_connected_components)} {}

// Methods implementing `Pass::Error`
[[nodiscard]] auto to_string() const -> std::string override;

private:
// Variables
std::vector<std::vector<std::shared_ptr<parser::ast::StructSpec const>>>
m_strongly_connected_components;
};

// Constructor
explicit DetectStructCircularDependency(
std::shared_ptr<StructSpecDependencyGraph> struct_spec_dependency_graph
)
: m_struct_spec_dependency_graph{std::move(struct_spec_dependency_graph)} {}

// Methods implementing `Pass`
/**
* @return A void result on success, or a pointer to `DetectStructCircularDependency::Error`
* on failure.
*/
[[nodiscard]] auto run()
-> boost::outcome_v2::std_checked<void, std::unique_ptr<Pass::Error>> override;

private:
std::shared_ptr<StructSpecDependencyGraph> m_struct_spec_dependency_graph;
};
} // namespace spider::tdl::pass::analysis

#endif // SPIDER_TDL_PASS_ANALYSIS_DETECTSTRUCTCIRCULARDEPENDENCY_HPP
74 changes: 74 additions & 0 deletions src/spider/tdl/pass/analysis/DetectUndefinedStruct.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
#include "DetectUndefinedStruct.hpp"

#include <algorithm>
#include <memory>
#include <string>
#include <tuple>
#include <utility>
#include <vector>

#include <boost/outcome/std_result.hpp>
#include <boost/outcome/success_failure.hpp>
#include <fmt/format.h>
#include <fmt/ranges.h>
#include <ystdlib/error_handling/Result.hpp>

#include <spider/tdl/parser/ast/nodes.hpp>
#include <spider/tdl/pass/Pass.hpp>
#include <spider/tdl/pass/utils.hpp>

namespace spider::tdl::pass::analysis {
auto DetectUndefinedStruct::Error::to_string() const -> std::string {
std::vector<std::string> undefined_struct_error_messages;
undefined_struct_error_messages.reserve(m_undefined_struct.size());
for (auto const* undefined_struct : m_undefined_struct) {
undefined_struct_error_messages.emplace_back(
fmt::format(
"Referencing to an undefined struct `{}` at {}",
undefined_struct->get_name(),
undefined_struct->get_source_location().serialize_to_str()
)
);
}
return fmt::format(
"Found {} undefined struct reference(s):\n{}",
m_undefined_struct.size(),
fmt::join(undefined_struct_error_messages, "\n")
);
}

auto DetectUndefinedStruct::run()
-> boost::outcome_v2::std_checked<void, std::unique_ptr<Pass::Error>> {
std::vector<parser::ast::Struct const*> undefined_structs;

auto struct_visitor
= [&](parser::ast::Struct const* struct_node) -> ystdlib::error_handling::Result<void> {
if (nullptr == m_translation_unit->get_struct_spec(struct_node->get_name())) {
undefined_structs.emplace_back(struct_node);
}
return ystdlib::error_handling::success();
};

std::ignore = visit_struct_node_using_dfs(m_translation_unit, struct_visitor);
std::ignore = m_translation_unit->visit_struct_specs(
[&](
parser::ast::StructSpec const* struct_spec
) -> ystdlib::error_handling::Result<void> {
return visit_struct_node_using_dfs(struct_spec, struct_visitor);
}
);
Comment on lines +52 to +59

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Ensure traversal failures surface.

Both DFS traversals return Result<void>, but we drop their status with std::ignore, so any visitor failure is silently discarded. Please propagate the error with the standard helper so a failing visitor stops the pass instead of reporting success.

-    std::ignore = visit_struct_node_using_dfs(m_translation_unit, struct_visitor);
-    std::ignore = m_translation_unit->visit_struct_specs(
-            [&](parser::ast::StructSpec const* struct_spec) -> ystdlib::error_handling::Result<void> {
-                return visit_struct_node_using_dfs(struct_spec, struct_visitor);
-            }
-    );
+    YSTDLIB_ERROR_HANDLING_TRYV(
+            visit_struct_node_using_dfs(m_translation_unit, struct_visitor)
+    );
+    YSTDLIB_ERROR_HANDLING_TRYV(
+            m_translation_unit->visit_struct_specs(
+                    [&](parser::ast::StructSpec const* struct_spec)
+                            -> ystdlib::error_handling::Result<void> {
+                        return visit_struct_node_using_dfs(struct_spec, struct_visitor);
+                    }
+            )
+    );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
std::ignore = visit_struct_node_using_dfs(m_translation_unit, struct_visitor);
std::ignore = m_translation_unit->visit_struct_specs(
[&](
parser::ast::StructSpec const* struct_spec
) -> ystdlib::error_handling::Result<void> {
return visit_struct_node_using_dfs(struct_spec, struct_visitor);
}
);
YSTDLIB_ERROR_HANDLING_TRYV(
visit_struct_node_using_dfs(m_translation_unit, struct_visitor)
);
YSTDLIB_ERROR_HANDLING_TRYV(
m_translation_unit->visit_struct_specs(
[&](parser::ast::StructSpec const* struct_spec)
-> ystdlib::error_handling::Result<void> {
return visit_struct_node_using_dfs(struct_spec, struct_visitor);
}
)
);
🤖 Prompt for AI Agents
In src/spider/tdl/pass/analysis/DetectUndefinedStruct.cpp around lines 52-59,
both DFS calls currently discard their Result<void> via std::ignore which hides
visitor failures; instead capture each call's Result, check it, and immediately
return it on error so the pass fails fast. Concretely: replace the std::ignore
assignments with storing the Result from
visit_struct_node_using_dfs(m_translation_unit, struct_visitor) and if that
result is an error return it; do the same for the call to
m_translation_unit->visit_struct_specs (i.e., call it, capture its Result, and
return the error if any). Ensure the lambda passed to visit_struct_specs still
returns the Result from visit_struct_node_using_dfs(struct_spec, struct_visitor)
so failures propagate up.


if (undefined_structs.empty()) {
return boost::outcome_v2::success();
}
std::ranges::sort(
undefined_structs,
[](parser::ast::Struct const* lhs, parser::ast::Struct const* rhs) -> bool {
return lhs->get_source_location() < rhs->get_source_location();
}
);
return boost::outcome_v2::failure(
std::make_unique<DetectUndefinedStruct::Error>(std::move(undefined_structs))
);
}
} // namespace spider::tdl::pass::analysis
Loading