Fix pretty printing bug

Summary:
Identifiers would be printed correctly if they were printed as part of a larger
`Expression` (so the visitor did the printing). However, if `PrintObject` was
called with an `Identifier *` then it would delegate to the template overload
and just print the pointer.

Reviewers: mtomic, teon.banek

Reviewed By: mtomic

Subscribers: mferencevic, pullbot

Differential Revision: https://phabricator.memgraph.io/D1802

src/query/frontend/ast/pretty_print.cpp

@@ -1,5 +1,7 @@
 #include "query/frontend/ast/pretty_print.hpp"
 
+#include <type_traits>
+
 #include "query/frontend/ast/ast.hpp"
 #include "utils/algorithm.hpp"
 #include "utils/string.hpp"
@@ -10,8 +12,7 @@ namespace {
 
 class ExpressionPrettyPrinter : public ExpressionVisitor<void> {
  public:
-  ExpressionPrettyPrinter(const AstStorage *storage, std::ostream *out)
-      : storage_(storage), out_(out) {}
+  explicit ExpressionPrettyPrinter(std::ostream *out);
 
   // Unary operators
   void Visit(NotOperator &op) override;
@@ -57,78 +58,65 @@ class ExpressionPrettyPrinter : public ExpressionVisitor<void> {
   void Visit(NamedExpression &op) override;
 
  private:
-  const AstStorage *storage_;
   std::ostream *out_;
-
-  // Declare all of the different `PrintObject` overloads upfront since they're
-  // mutually recursive. Without this, overload resolution depends on the
-  // ordering of the overloads within the source, which is quite fragile.
-
-  template <typename T>
-  void PrintObject(std::ostream *out, const T &arg);
-
-  void PrintObject(std::ostream *out, const std::string &str);
-
-  void PrintObject(std::ostream *out, Aggregation::Op op);
-
-  void PrintObject(std::ostream *out, Expression *expr);
-
-  void PrintObject(std::ostream *out, const PropertyValue &value);
-
-  template <typename T>
-  void PrintObject(std::ostream *out, const std::vector<T> &vec);
-
-  template <typename K, typename V>
-  void PrintObject(std::ostream *out, const std::map<K, V> &map);
-
-  template <typename T>
-  void PrintOperatorArgs(std::ostream *out, const T &arg) {
-    *out << " ";
-    PrintObject(out, arg);
-    *out << ")";
-  }
-
-  template <typename T, typename... Ts>
-  void PrintOperatorArgs(std::ostream *out, const T &arg, const Ts &... args) {
-    *out << " ";
-    PrintObject(out, arg);
-    PrintOperatorArgs(out, args...);
-  }
-
-  template <typename... Ts>
-  void PrintOperator(std::ostream *out, const std::string &name,
-                     const Ts &... args) {
-    *out << "(" << name;
-    PrintOperatorArgs(out, args...);
-  }
 };
 
+// Declare all of the different `PrintObject` overloads upfront since they're
+// mutually recursive. Without this, overload resolution depends on the ordering
+// of the overloads within the source, which is quite fragile.
+
 template <typename T>
-void ExpressionPrettyPrinter::PrintObject(std::ostream *out, const T &arg) {
+void PrintObject(std::ostream *out, const T &arg);
+
+void PrintObject(std::ostream *out, const std::string &str);
+
+void PrintObject(std::ostream *out, Aggregation::Op op);
+
+void PrintObject(std::ostream *out, Expression *expr);
+
+void PrintObject(std::ostream *out, Identifier *expr);
+
+void PrintObject(std::ostream *out, const PropertyValue &value);
+
+template <typename T>
+void PrintObject(std::ostream *out, const std::vector<T> &vec);
+
+template <typename K, typename V>
+void PrintObject(std::ostream *out, const std::map<K, V> &map);
+
+template <typename T>
+void PrintObject(std::ostream *out, const T &arg) {
+  static_assert(
+      !std::is_convertible<T, Expression *>::value,
+      "This overload shouldn't be called with pointers convertible "
+      "to Expression *. This means your other PrintObject overloads aren't "
+      "being called for certain AST nodes when they should (or perhaps such "
+      "overloads don't exist yet).");
   *out << arg;
 }
 
-void ExpressionPrettyPrinter::PrintObject(std::ostream *out,
-                                          const std::string &str) {
+void PrintObject(std::ostream *out, const std::string &str) {
   *out << utils::Escape(str);
 }
 
-void ExpressionPrettyPrinter::PrintObject(std::ostream *out,
-                                          Aggregation::Op op) {
+void PrintObject(std::ostream *out, Aggregation::Op op) {
   *out << Aggregation::OpToString(op);
 }
 
-void ExpressionPrettyPrinter::PrintObject(std::ostream *out, Expression *expr) {
+void PrintObject(std::ostream *out, Expression *expr) {
   if (expr) {
-    ExpressionPrettyPrinter printer{storage_, out};
+    ExpressionPrettyPrinter printer{out};
     expr->Accept(printer);
   } else {
     *out << "<null>";
   }
 }
 
-void ExpressionPrettyPrinter::PrintObject(std::ostream *out,
-                                          const PropertyValue &value) {
+void PrintObject(std::ostream *out, Identifier *expr) {
+  PrintObject(out, static_cast<Expression *>(expr));
+}
+
+void PrintObject(std::ostream *out, const PropertyValue &value) {
   switch (value.type()) {
     case PropertyValue::Type::Null:
       *out << "null";
@@ -161,27 +149,49 @@ void ExpressionPrettyPrinter::PrintObject(std::ostream *out,
 }
 
 template <typename T>
-void ExpressionPrettyPrinter::PrintObject(std::ostream *out,
-                                          const std::vector<T> &vec) {
+void PrintObject(std::ostream *out, const std::vector<T> &vec) {
   *out << "[";
-  utils::PrintIterable(*out, vec, ", ", [this](auto &stream, const auto &item) {
-    this->PrintObject(&stream, item);
+  utils::PrintIterable(*out, vec, ", ", [](auto &stream, const auto &item) {
+    PrintObject(&stream, item);
   });
   *out << "]";
 }
 
 template <typename K, typename V>
-void ExpressionPrettyPrinter::PrintObject(std::ostream *out,
-                                          const std::map<K, V> &map) {
+void PrintObject(std::ostream *out, const std::map<K, V> &map) {
   *out << "{";
-  utils::PrintIterable(*out, map, ", ", [this](auto &stream, const auto &item) {
-    this->PrintObject(&stream, item.first);
+  utils::PrintIterable(*out, map, ", ", [](auto &stream, const auto &item) {
+    PrintObject(&stream, item.first);
     stream << ": ";
-    this->PrintObject(&stream, item.second);
+    PrintObject(&stream, item.second);
   });
   *out << "}";
 }
 
+template <typename T>
+void PrintOperatorArgs(std::ostream *out, const T &arg) {
+  *out << " ";
+  PrintObject(out, arg);
+  *out << ")";
+}
+
+template <typename T, typename... Ts>
+void PrintOperatorArgs(std::ostream *out, const T &arg, const Ts &... args) {
+  *out << " ";
+  PrintObject(out, arg);
+  PrintOperatorArgs(out, args...);
+}
+
+template <typename... Ts>
+void PrintOperator(std::ostream *out, const std::string &name,
+                   const Ts &... args) {
+  *out << "(" << name;
+  PrintOperatorArgs(out, args...);
+}
+
+ExpressionPrettyPrinter::ExpressionPrettyPrinter(std::ostream *out)
+    : out_(out) {}
+
 #define UNARY_OPERATOR_VISIT(OP_NODE, OP_STR)        \
   void ExpressionPrettyPrinter::Visit(OP_NODE &op) { \
     PrintOperator(out_, OP_STR, op.expression_);     \
@@ -235,7 +245,7 @@ void ExpressionPrettyPrinter::Visit(ListLiteral &op) {
 void ExpressionPrettyPrinter::Visit(MapLiteral &op) {
   std::map<std::string, Expression *> map;
   for (const auto &kv : op.elements_) {
-    map[storage_->properties_[kv.first.ix]] = kv.second;
+    map[kv.first.name] = kv.second;
   }
   PrintObject(out_, map);
 }
@@ -284,8 +294,7 @@ void ExpressionPrettyPrinter::Visit(PrimitiveLiteral &op) {
 }
 
 void ExpressionPrettyPrinter::Visit(PropertyLookup &op) {
-  const auto &prop_name = storage_->properties_[op.property_.ix];
-  PrintOperator(out_, "PropertyLookup", op.expression_, prop_name);
+  PrintOperator(out_, "PropertyLookup", op.expression_, op.property_.name);
 }
 
 void ExpressionPrettyPrinter::Visit(ParameterLookup &op) {
@@ -298,15 +307,13 @@ void ExpressionPrettyPrinter::Visit(NamedExpression &op) {
 
 }  // namespace
 
-void PrintExpression(const AstStorage &storage, Expression *expr,
-                     std::ostream *out) {
-  ExpressionPrettyPrinter printer{&storage, out};
+void PrintExpression(Expression *expr, std::ostream *out) {
+  ExpressionPrettyPrinter printer{out};
   expr->Accept(printer);
 }
 
-void PrintExpression(const AstStorage &storage, NamedExpression *expr,
-                     std::ostream *out) {
-  ExpressionPrettyPrinter printer{&storage, out};
+void PrintExpression(NamedExpression *expr, std::ostream *out) {
+  ExpressionPrettyPrinter printer{out};
   expr->Accept(printer);
 }
 

src/query/frontend/ast/pretty_print.hpp

@@ -6,9 +6,7 @@
 
 namespace query {
 
-void PrintExpression(const AstStorage &storage, Expression *expr,
-                     std::ostream *out);
-void PrintExpression(const AstStorage &storage, NamedExpression *expr,
-                     std::ostream *out);
+void PrintExpression(Expression *expr, std::ostream *out);
+void PrintExpression(NamedExpression *expr, std::ostream *out);
 
 }  // namespace query

tests/manual/CMakeLists.txt

@@ -63,6 +63,9 @@ target_link_libraries(${test_prefix}query_hash mg-single-node kvstore_dummy_lib)
 add_manual_test(query_planner.cpp interactive_planning.cpp)
 target_link_libraries(${test_prefix}query_planner mg-single-node kvstore_dummy_lib)
 
+add_manual_test(expression_pretty_printer.cpp)
+target_link_libraries(${test_prefix}expression_pretty_printer mg-single-node kvstore_dummy_lib)
+
 add_manual_test(repl.cpp)
 target_link_libraries(${test_prefix}repl mg-single-node kvstore_dummy_lib)
 

tests/manual/expression_pretty_printer.cpp

@@ -0,0 +1,46 @@
+#include <iostream>
+#include <sstream>
+#include <string>
+
+#include "query/frontend/ast/ast.hpp"
+#include "query/frontend/ast/cypher_main_visitor.hpp"
+#include "query/frontend/ast/pretty_print.hpp"
+#include "query/frontend/opencypher/parser.hpp"
+
+std::string AssembleQueryString(const std::string &expression_string) {
+  return "return " + expression_string + " as expr";
+}
+
+query::Query *ParseQuery(const std::string &query_string,
+                         query::AstStorage *ast_storage) {
+  query::ParsingContext context;
+  query::frontend::opencypher::Parser parser(query_string);
+  query::frontend::CypherMainVisitor visitor(context, ast_storage);
+
+  visitor.visit(parser.tree());
+  return visitor.query();
+}
+
+query::Expression *GetExpression(query::Query *query) {
+  auto cypher_query = dynamic_cast<query::CypherQuery *>(query);
+  auto ret =
+      dynamic_cast<query::Return *>(cypher_query->single_query_->clauses_[0]);
+  auto expr = ret->body_.named_expressions[0]->expression_;
+
+  return expr;
+}
+
+int main() {
+  query::AstStorage ast_storage;
+  std::ostringstream ss;
+  ss << std::cin.rdbuf();
+
+  auto query_string = AssembleQueryString(ss.str());
+  auto query = ParseQuery(query_string, &ast_storage);
+  auto expr = GetExpression(query);
+
+  query::PrintExpression(expr, &std::cout);
+  std::cout << std::endl;
+
+  return 0;
+}

tests/unit/query_common.hpp

@@ -57,15 +57,15 @@ auto ToMap(const TypedValue &t) {
   return map;
 };
 
-std::string ToString(const AstStorage &storage, Expression *expr) {
+std::string ToString(Expression *expr) {
   std::ostringstream ss;
-  PrintExpression(storage, expr, &ss);
+  PrintExpression(expr, &ss);
   return ss.str();
 }
 
-std::string ToString(const AstStorage &storage, NamedExpression *expr) {
+std::string ToString(NamedExpression *expr) {
   std::ostringstream ss;
-  PrintExpression(storage, expr, &ss);
+  PrintExpression(expr, &ss);
   return ss.str();
 }
 

tests/unit/query_pretty_print.cpp

@@ -28,54 +28,77 @@ struct ExpressionPrettyPrinterTest : public ::testing::Test {
 
 TEST_F(ExpressionPrettyPrinterTest, Literals) {
   // 1
-  EXPECT_EQ(ToString(storage, LITERAL(1)), "1");
+  EXPECT_EQ(ToString(LITERAL(1)), "1");
 
   // "hello"
-  EXPECT_EQ(ToString(storage, LITERAL("hello")), "\"hello\"");
+  EXPECT_EQ(ToString(LITERAL("hello")), "\"hello\"");
 
   // null
-  EXPECT_EQ(ToString(storage, LITERAL(TypedValue::Null)), "null");
+  EXPECT_EQ(ToString(LITERAL(TypedValue::Null)), "null");
 
   // true
-  EXPECT_EQ(ToString(storage, LITERAL(true)), "true");
+  EXPECT_EQ(ToString(LITERAL(true)), "true");
 
   // false
-  EXPECT_EQ(ToString(storage, LITERAL(false)), "false");
+  EXPECT_EQ(ToString(LITERAL(false)), "false");
 
   // [1 null "hello"]
-  EXPECT_EQ(ToString(storage, LITERAL((std::vector<PropertyValue>{
-                                  1, PropertyValue::Null, "hello"}))),
+  EXPECT_EQ(ToString(LITERAL(
+                (std::vector<PropertyValue>{1, PropertyValue::Null, "hello"}))),
             "[1, null, \"hello\"]");
 
   // {hello: 1, there: 2}
-  EXPECT_EQ(ToString(storage, LITERAL((std::map<std::string, PropertyValue>{
-                                  {"hello", 1}, {"there", 2}}))),
+  EXPECT_EQ(ToString(LITERAL((std::map<std::string, PropertyValue>{
+                {"hello", 1}, {"there", 2}}))),
             "{\"hello\": 1, \"there\": 2}");
 }
 
+TEST_F(ExpressionPrettyPrinterTest, Identifiers) {
+  // x
+  EXPECT_EQ(ToString(IDENT("x")), "(Identifier \"x\")");
+
+  // hello_there
+  EXPECT_EQ(ToString(IDENT("hello_there")), "(Identifier \"hello_there\")");
+}
+
+TEST_F(ExpressionPrettyPrinterTest, Reducing) {
+  // all(x in list where x.prop = 42)
+  auto prop = dba.Property("prop");
+  EXPECT_EQ(ToString(ALL("x", LITERAL(std::vector<PropertyValue>{}),
+                         WHERE(EQ(PROPERTY_LOOKUP("x", prop), LITERAL(42))))),
+            "(All (Identifier \"x\") [] (== (PropertyLookup "
+            "(Identifier \"x\") \"prop\") 42))");
+
+  // reduce(accumulator = initial_value, variable IN list | expression)
+  EXPECT_EQ(
+      ToString(REDUCE("accumulator", IDENT("initial_value"), "variable",
+                      IDENT("list"), IDENT("expression"))),
+      "(Reduce (Identifier \"accumulator\") (Identifier \"initial_value\") "
+      "(Identifier \"variable\") (Identifier \"list\") (Identifier "
+      "\"expression\"))");
+}
+
 TEST_F(ExpressionPrettyPrinterTest, UnaryOperators) {
   // not(false)
-  EXPECT_EQ(ToString(storage, NOT(LITERAL(false))), "(Not false)");
+  EXPECT_EQ(ToString(NOT(LITERAL(false))), "(Not false)");
 
   // +1
-  EXPECT_EQ(ToString(storage, UPLUS(LITERAL(1))), "(+ 1)");
+  EXPECT_EQ(ToString(UPLUS(LITERAL(1))), "(+ 1)");
 
   // -1
-  EXPECT_EQ(ToString(storage, UMINUS(LITERAL(1))), "(- 1)");
+  EXPECT_EQ(ToString(UMINUS(LITERAL(1))), "(- 1)");
 
   // null IS NULL
-  EXPECT_EQ(ToString(storage, IS_NULL(LITERAL(TypedValue::Null))),
-            "(IsNull null)");
+  EXPECT_EQ(ToString(IS_NULL(LITERAL(TypedValue::Null))), "(IsNull null)");
 }
 
 TEST_F(ExpressionPrettyPrinterTest, BinaryOperators) {
   // and(null, 5)
-  EXPECT_EQ(ToString(storage, AND(LITERAL(TypedValue::Null), LITERAL(5))),
+  EXPECT_EQ(ToString(AND(LITERAL(TypedValue::Null), LITERAL(5))),
             "(And null 5)");
 
   // or(5, {hello: "there"}["hello"])
-  EXPECT_EQ(ToString(storage,
-                     OR(LITERAL(5),
+  EXPECT_EQ(ToString(OR(LITERAL(5),
                         PROPERTY_LOOKUP(
                             MAP(std::make_pair(storage.GetPropertyIx("hello"),
                                                LITERAL("there"))),
@@ -83,65 +106,60 @@ TEST_F(ExpressionPrettyPrinterTest, BinaryOperators) {
             "(Or 5 (PropertyLookup {\"hello\": \"there\"} \"hello\"))");
 
   // and(coalesce(null, 1), {hello: "there"})
-  EXPECT_EQ(
-      ToString(storage, AND(COALESCE(LITERAL(TypedValue::Null), LITERAL(1)),
-                            MAP(std::make_pair(storage.GetPropertyIx("hello"),
-                                               LITERAL("there"))))),
-      "(And (Coalesce [null, 1]) {\"hello\": \"there\"})");
+  EXPECT_EQ(ToString(AND(COALESCE(LITERAL(TypedValue::Null), LITERAL(1)),
+                         MAP(std::make_pair(storage.GetPropertyIx("hello"),
+                                            LITERAL("there"))))),
+            "(And (Coalesce [null, 1]) {\"hello\": \"there\"})");
 }
 
 TEST_F(ExpressionPrettyPrinterTest, Coalesce) {
   // coalesce()
-  EXPECT_EQ(ToString(storage, COALESCE()), "(Coalesce [])");
+  EXPECT_EQ(ToString(COALESCE()), "(Coalesce [])");
 
   // coalesce(null, null)
-  EXPECT_EQ(ToString(storage, COALESCE(LITERAL(TypedValue::Null),
-                                       LITERAL(TypedValue::Null))),
-            "(Coalesce [null, null])");
+  EXPECT_EQ(
+      ToString(COALESCE(LITERAL(TypedValue::Null), LITERAL(TypedValue::Null))),
+      "(Coalesce [null, null])");
 
   // coalesce(null, 2, 3)
-  EXPECT_EQ(ToString(storage, COALESCE(LITERAL(TypedValue::Null), LITERAL(2),
-                                       LITERAL(3))),
-            "(Coalesce [null, 2, 3])");
+  EXPECT_EQ(
+      ToString(COALESCE(LITERAL(TypedValue::Null), LITERAL(2), LITERAL(3))),
+      "(Coalesce [null, 2, 3])");
 
   // coalesce(null, 2, assert(false), 3)
-  EXPECT_EQ(
-      ToString(storage, COALESCE(LITERAL(TypedValue::Null), LITERAL(2),
-                                 FN("ASSERT", LITERAL(false)), LITERAL(3))),
-      "(Coalesce [null, 2, (Function \"ASSERT\" [false]), 3])");
+  EXPECT_EQ(ToString(COALESCE(LITERAL(TypedValue::Null), LITERAL(2),
+                              FN("ASSERT", LITERAL(false)), LITERAL(3))),
+            "(Coalesce [null, 2, (Function \"ASSERT\" [false]), 3])");
 
   // coalesce(null, assert(false))
-  EXPECT_EQ(ToString(storage, COALESCE(LITERAL(TypedValue::Null),
-                                       FN("ASSERT", LITERAL(false)))),
+  EXPECT_EQ(ToString(COALESCE(LITERAL(TypedValue::Null),
+                              FN("ASSERT", LITERAL(false)))),
             "(Coalesce [null, (Function \"ASSERT\" [false])])");
 
   // coalesce([null, null])
-  EXPECT_EQ(
-      ToString(storage, COALESCE(LITERAL(TypedValue(std::vector<TypedValue>{
-                            TypedValue::Null, TypedValue::Null})))),
-      "(Coalesce [[null, null]])");
+  EXPECT_EQ(ToString(COALESCE(LITERAL(TypedValue(
+                std::vector<TypedValue>{TypedValue::Null, TypedValue::Null})))),
+            "(Coalesce [[null, null]])");
 }
 
 TEST_F(ExpressionPrettyPrinterTest, ParameterLookup) {
   // and($hello, $there)
-  EXPECT_EQ(ToString(storage, AND(PARAMETER_LOOKUP(1), PARAMETER_LOOKUP(2))),
+  EXPECT_EQ(ToString(AND(PARAMETER_LOOKUP(1), PARAMETER_LOOKUP(2))),
             "(And (ParameterLookup 1) (ParameterLookup 2))");
 }
 
 TEST_F(ExpressionPrettyPrinterTest, PropertyLookup) {
   // {hello: "there"}["hello"]
   EXPECT_EQ(
-      ToString(storage, PROPERTY_LOOKUP(
-                            MAP(std::make_pair(storage.GetPropertyIx("hello"),
-                                               LITERAL("there"))),
-                            "hello")),
+      ToString(PROPERTY_LOOKUP(
+          MAP(std::make_pair(storage.GetPropertyIx("hello"), LITERAL("there"))),
+          "hello")),
       "(PropertyLookup {\"hello\": \"there\"} \"hello\")");
 }
 
 TEST_F(ExpressionPrettyPrinterTest, NamedExpression) {
   // n AS 1
-  EXPECT_EQ(ToString(storage, NEXPR("n", LITERAL(1))),
-            "(NamedExpression \"n\" 1)");
+  EXPECT_EQ(ToString(NEXPR("n", LITERAL(1))), "(NamedExpression \"n\" 1)");
 }
 
 }  // namespace