Skip to content

Commit

Permalink
Fix(engine) Save parse positions when finding unresolved macros
Browse files Browse the repository at this point in the history 
Now that ASTs contain parse positions, use them when reporting errors
about unknown macros.

When doing the first pass to find all macro references, save macros as
a map<macro name,parse position> instead of a set<macro name>.

In the second pass, when reporting any unresolved macro references,
also report the parse position.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
  • Loading branch information
@mstemm
mstemm committed Sep 20, 2022
1 parent a757c24 commit 6b52071
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 19 deletions.
17 changes: 9 additions & 8 deletions tests/engine/test_filter_macro_resolver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ TEST_CASE("Should resolve macros on a filter AST", "[rule_loader]")
// first run
REQUIRE(resolver.run(filter) == true);
REQUIRE(resolver.get_resolved_macros().size() == 1);
REQUIRE(*resolver.get_resolved_macros().begin() == macro_name);
REQUIRE(resolver.get_resolved_macros().begin()->first == macro_name);
REQUIRE(resolver.get_unknown_macros().empty());
REQUIRE(filter->is_equal(expected.get()));

Expand All @@ -71,7 +71,7 @@ TEST_CASE("Should resolve macros on a filter AST", "[rule_loader]")
REQUIRE(resolver.run(filter) == true);
REQUIRE(filter.get() != old_filter_ptr);
REQUIRE(resolver.get_resolved_macros().size() == 1);
REQUIRE(*resolver.get_resolved_macros().begin() == macro_name);
REQUIRE(resolver.get_resolved_macros().begin()->first == macro_name);
REQUIRE(resolver.get_unknown_macros().empty());
REQUIRE(filter->is_equal(macro.get()));

Expand Down Expand Up @@ -181,7 +181,7 @@ TEST_CASE("Should find unknown macros", "[rule_loader]")
filter_macro_resolver resolver;
REQUIRE(resolver.run(filter) == false);
REQUIRE(resolver.get_unknown_macros().size() == 1);
REQUIRE(*resolver.get_unknown_macros().begin() == macro_name);
REQUIRE(resolver.get_unknown_macros().begin()->first == macro_name);
REQUIRE(resolver.get_resolved_macros().empty());
}

Expand All @@ -204,9 +204,9 @@ TEST_CASE("Should find unknown macros", "[rule_loader]")
// first run
REQUIRE(resolver.run(filter) == true);
REQUIRE(resolver.get_resolved_macros().size() == 1);
REQUIRE(*resolver.get_resolved_macros().begin() == a_macro_name);
REQUIRE(resolver.get_resolved_macros().begin()->first == a_macro_name);
REQUIRE(resolver.get_unknown_macros().size() == 1);
REQUIRE(*resolver.get_unknown_macros().begin() == b_macro_name);
REQUIRE(resolver.get_unknown_macros().begin()->first == b_macro_name);
REQUIRE(filter->is_equal(expected_filter.get()));
}
}
Expand All @@ -222,7 +222,7 @@ TEST_CASE("Should undefine macro", "[rule_loader]")
resolver.set_macro(macro_name, macro);
REQUIRE(resolver.run(a_filter) == true);
REQUIRE(resolver.get_resolved_macros().size() == 1);
REQUIRE(*resolver.get_resolved_macros().begin() == macro_name);
REQUIRE(resolver.get_resolved_macros().begin()->first == macro_name);
REQUIRE(resolver.get_unknown_macros().empty());
REQUIRE(a_filter->is_equal(macro.get()));

Expand All @@ -231,6 +231,7 @@ TEST_CASE("Should undefine macro", "[rule_loader]")
REQUIRE(resolver.get_resolved_macros().empty());
REQUIRE(resolver.get_unknown_macros().size() == 1);
REQUIRE(*resolver.get_unknown_macros().begin() == macro_name);
REQUIRE(resolver.get_unknown_macros().begin()->first == macro_name);
}

// checks that the macro AST is cloned and not shared across resolved filters
Expand All @@ -240,11 +241,11 @@ TEST_CASE("Should clone macro AST", "[rule_loader]")
std::shared_ptr<unary_check_expr> macro = std::move(unary_check_expr::create("test.field", "", "exists"));
std::shared_ptr<expr> filter = std::move(value_expr::create(macro_name));
filter_macro_resolver resolver;

resolver.set_macro(macro_name, macro);
REQUIRE(resolver.run(filter) == true);
REQUIRE(resolver.get_resolved_macros().size() == 1);
REQUIRE(*resolver.get_resolved_macros().begin() == macro_name);
REQUIRE(resolver.get_resolved_macros().begin()->first == macro_name);
REQUIRE(resolver.get_unknown_macros().empty());
REQUIRE(filter->is_equal(macro.get()));

Expand Down
8 changes: 4 additions & 4 deletions userspace/engine/filter_macro_resolver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,12 @@ void filter_macro_resolver::set_macro(
m_macros[name] = macro;
}

const unordered_set<string>& filter_macro_resolver::get_unknown_macros() const
const filter_macro_resolver::macro_info_map& filter_macro_resolver::get_unknown_macros() const
{
return m_unknown_macros;
}

const unordered_set<string>& filter_macro_resolver::get_resolved_macros() const
const filter_macro_resolver::macro_info_map& filter_macro_resolver::get_resolved_macros() const
{
return m_resolved_macros;
}
Expand Down Expand Up @@ -141,11 +141,11 @@ void filter_macro_resolver::visitor::visit(ast::value_expr* e)
{
m_node_substitute = std::move(new_node);
}
m_resolved_macros->insert(e->value);
(*m_resolved_macros)[e->value] = e->get_pos();
}
else
{
m_node_substitute = nullptr;
m_unknown_macros->insert(e->value);
(*m_unknown_macros)[e->value] = e->get_pos();
}
}
20 changes: 13 additions & 7 deletions userspace/engine/filter_macro_resolver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,21 +58,26 @@ class filter_macro_resolver
std::string name,
std::shared_ptr<libsinsp::filter::ast::expr> macro);

/*!
\brief used in get_{resolved,unknown}_macros
*/
typedef std::unordered_map<std::string,libsinsp::filter::ast::pos_info> macro_info_map;

/*!
\brief Returns a set containing the names of all the macros
substituted during the last invocation of run(). Should be
non-empty if the last invocation of run() returned true.
*/
const std::unordered_set<std::string>& get_resolved_macros() const;
const macro_info_map& get_resolved_macros() const;

/*!
\brief Returns a set containing the names of all the macros
that remained unresolved during the last invocation of run().
A macro remains unresolved if it is found inside the processed
filter but it was not defined with set_macro();
*/
const std::unordered_set<std::string>& get_unknown_macros() const;
const macro_info_map& get_unknown_macros() const;

private:
typedef std::unordered_map<
std::string,
Expand All @@ -82,8 +87,9 @@ class filter_macro_resolver
struct visitor : public libsinsp::filter::ast::expr_visitor
{
std::unique_ptr<libsinsp::filter::ast::expr> m_node_substitute;
std::unordered_set<std::string>* m_unknown_macros;
std::unordered_set<std::string>* m_resolved_macros;
macro_info_map* m_unknown_macros;
macro_info_map* m_resolved_macros;

macro_defs* m_macros;

void visit(libsinsp::filter::ast::and_expr* e) override;
Expand All @@ -95,7 +101,7 @@ class filter_macro_resolver
void visit(libsinsp::filter::ast::binary_check_expr* e) override;
};

std::unordered_set<std::string> m_unknown_macros;
std::unordered_set<std::string> m_resolved_macros;
macro_info_map m_unknown_macros;
macro_info_map m_resolved_macros;
macro_defs m_macros;
};

0 comments on commit 6b52071

Please sign in to comment.