From c429923b3155e0ae91c1ef2332596479e2d05b02 Mon Sep 17 00:00:00 2001
From: Teon Banek <theongugl@gmail.com>
Date: Tue, 18 Apr 2017 14:56:45 +0200
Subject: [PATCH] Prevent same names in named expressions

Reviewers: florijan, mislav.bradac

Reviewed By: florijan

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D291
---
 .../frontend/semantic/symbol_generator.cpp    | 34 ++++++++++++-------
 .../frontend/semantic/symbol_generator.hpp    |  3 ++
 tests/unit/query_semantic.cpp                 | 21 ++++++++++++
 3 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/src/query/frontend/semantic/symbol_generator.cpp b/src/query/frontend/semantic/symbol_generator.cpp
index 01e3ac3e2..3689309ac 100644
--- a/src/query/frontend/semantic/symbol_generator.cpp
+++ b/src/query/frontend/semantic/symbol_generator.cpp
@@ -4,6 +4,8 @@
 
 #include "query/frontend/semantic/symbol_generator.hpp"
 
+#include <unordered_set>
+
 namespace query {
 
 auto SymbolGenerator::CreateSymbol(const std::string &name, Symbol::Type type) {
@@ -28,21 +30,30 @@ auto SymbolGenerator::GetOrCreateSymbol(const std::string &name,
   return CreateSymbol(name, type);
 }
 
+void SymbolGenerator::BindNamedExpressionSymbols(
+    const std::vector<NamedExpression *> &named_expressions) {
+  std::unordered_set<std::string> seen_names;
+  for (auto &named_expr : named_expressions) {
+    // Improvement would be to infer the type of the expression.
+    const auto &name = named_expr->name_;
+    if (!seen_names.insert(name).second) {
+      throw SemanticException(
+          "Multiple results with the same name '{}' are not allowed.", name);
+    }
+    symbol_table_[*named_expr] = CreateSymbol(name);
+  }
+}
+
 // Clauses
 
 void SymbolGenerator::Visit(Create &create) { scope_.in_create = true; }
 void SymbolGenerator::PostVisit(Create &create) { scope_.in_create = false; }
 
-void SymbolGenerator::Visit(Return &ret) {
-  scope_.in_return = true;
-}
+void SymbolGenerator::Visit(Return &ret) { scope_.in_return = true; }
 void SymbolGenerator::PostVisit(Return &ret) {
-  for (auto &named_expr : ret.named_expressions_) {
-    // Named expressions establish bindings for expressions which come after
-    // return, but not for the expressions contained inside.
-    symbol_table_[*named_expr] = CreateSymbol(named_expr->name_);
-    // Improvement to type checking system would be to infer the type of the expression.
-  }
+  // Named expressions establish bindings for expressions which come after
+  // return, but not for the expressions contained inside.
+  BindNamedExpressionSymbols(ret.named_expressions_);
   scope_.in_return = false;
 }
 
@@ -56,10 +67,7 @@ bool SymbolGenerator::PreVisit(With &with) {
   // only those established through named expressions. New declarations must not
   // be visible inside named expressions themselves.
   scope_.symbols.clear();
-  for (auto &named_expr : with.named_expressions_) {
-    // Improvement would be to infer the type of the expression.
-    symbol_table_[*named_expr] = CreateSymbol(named_expr->name_);
-  }
+  BindNamedExpressionSymbols(with.named_expressions_);
   if (with.where_) with.where_->Accept(*this);
   return false;  // We handled the traversal ourselves.
 }
diff --git a/src/query/frontend/semantic/symbol_generator.hpp b/src/query/frontend/semantic/symbol_generator.hpp
index c983b7bf2..8e3df829a 100644
--- a/src/query/frontend/semantic/symbol_generator.hpp
+++ b/src/query/frontend/semantic/symbol_generator.hpp
@@ -76,6 +76,9 @@ class SymbolGenerator : public TreeVisitorBase {
   auto GetOrCreateSymbol(const std::string &name,
                          Symbol::Type type = Symbol::Type::Any);
 
+  void BindNamedExpressionSymbols(
+      const std::vector<NamedExpression *> &named_expressions);
+
   SymbolTable &symbol_table_;
   Scope scope_;
 };
diff --git a/tests/unit/query_semantic.cpp b/tests/unit/query_semantic.cpp
index 341456a6b..02e8fa4e4 100644
--- a/tests/unit/query_semantic.cpp
+++ b/tests/unit/query_semantic.cpp
@@ -534,4 +534,25 @@ TEST(TestSymbolGenerator, MatchWithCreate) {
   EXPECT_EQ(m, symbol_table.at(*node_3->identifier_));
 }
 
+TEST(TestSymbolGenerator, SameResults) {
+  // Test MATCH (n) WITH n AS m, n AS m
+  {
+    AstTreeStorage storage;
+    auto query = QUERY(MATCH(PATTERN(NODE("n"))),
+                       WITH(IDENT("n"), AS("m"), IDENT("n"), AS("m")));
+    SymbolTable symbol_table;
+    SymbolGenerator symbol_generator(symbol_table);
+    EXPECT_THROW(query->Accept(symbol_generator), SemanticException);
+  }
+  // Test MATCH (n) RETURN n, n
+  {
+    AstTreeStorage storage;
+    auto query = QUERY(MATCH(PATTERN(NODE("n"))),
+                       RETURN(IDENT("n"), AS("n"), IDENT("n"), AS("n")));
+    SymbolTable symbol_table;
+    SymbolGenerator symbol_generator(symbol_table);
+    EXPECT_THROW(query->Accept(symbol_generator), SemanticException);
+  }
+}
+
 }