From ea378842971c7a5ba1af0f04f07caa7d8cad9006 Mon Sep 17 00:00:00 2001
From: Nico Weber <thakis@chromium.org>
Date: Tue, 2 Jul 2013 15:43:40 -0700
Subject: Improve error message for duplicate rules and unknown pools.

Also add more tests for invalid manifests. According to gcov, these invalid
inputs weren't tested before.
---
 src/manifest_parser.cc      |   8 ++--
 src/manifest_parser_test.cc | 102 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 5 deletions(-)

diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc
index d742331..d4f0007 100644
--- a/src/manifest_parser.cc
+++ b/src/manifest_parser.cc
@@ -146,10 +146,8 @@ bool ManifestParser::ParseRule(string* err) {
   if (!ExpectToken(Lexer::NEWLINE, err))
     return false;
 
-  if (state_->LookupRule(name) != NULL) {
-    *err = "duplicate rule '" + name + "'";
-    return false;
-  }
+  if (state_->LookupRule(name) != NULL)
+    return lexer_.Error("duplicate rule '" + name + "'", err);
 
   Rule* rule = new Rule(name);  // XXX scoped_ptr
 
@@ -307,7 +305,7 @@ bool ManifestParser::ParseEdge(string* err) {
   if (!pool_name.empty()) {
     Pool* pool = state_->LookupPool(pool_name);
     if (pool == NULL)
-      return lexer_.Error("unknown pool name", err);
+      return lexer_.Error("unknown pool name '" + pool_name + "'", err);
     edge->pool_ = pool;
   }
 
diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc
index 2638edc..b333549 100644
--- a/src/manifest_parser_test.cc
+++ b/src/manifest_parser_test.cc
@@ -377,6 +377,17 @@ TEST_F(ParserTest, Errors) {
     State state;
     ManifestParser parser(&state, NULL);
     string err;
+    EXPECT_FALSE(parser.ParseTest("build\n", &err));
+    EXPECT_EQ("input:1: expected path\n"
+              "build\n"
+              "     ^ near here"
+              , err);
+  }
+
+  {
+    State state;
+    ManifestParser parser(&state, NULL);
+    string err;
     EXPECT_FALSE(parser.ParseTest("build x: y z\n", &err));
     EXPECT_EQ("input:1: unknown build rule 'y'\n"
               "build x: y z\n"
@@ -422,6 +433,32 @@ TEST_F(ParserTest, Errors) {
     ManifestParser parser(&state, NULL);
     string err;
     EXPECT_FALSE(parser.ParseTest("rule cat\n"
+                                  "  command = echo\n"
+                                  "rule cat\n"
+                                  "  command = echo\n", &err));
+    EXPECT_EQ("input:3: duplicate rule 'cat'\n"
+              "rule cat\n"
+              "        ^ near here"
+              , err);
+  }
+
+  {
+    State state;
+    ManifestParser parser(&state, NULL);
+    string err;
+    EXPECT_FALSE(parser.ParseTest("rule cat\n"
+                                  "  command = echo\n"
+                                  "  rspfile = cat.rsp\n", &err));
+    EXPECT_EQ(
+        "input:4: rspfile and rspfile_content need to be both specified\n",
+        err);
+  }
+
+  {
+    State state;
+    ManifestParser parser(&state, NULL);
+    string err;
+    EXPECT_FALSE(parser.ParseTest("rule cat\n"
                                   "  command = ${fafsd\n"
                                   "foo = bar\n",
                                   &err));
@@ -583,6 +620,71 @@ TEST_F(ParserTest, Errors) {
                                   "  generator = 1\n", &err));
     EXPECT_EQ("input:4: unexpected indent\n", err);
   }
+
+  {
+    State state;
+    ManifestParser parser(&state, NULL);
+    string err;
+    EXPECT_FALSE(parser.ParseTest("pool\n", &err));
+    EXPECT_EQ("input:1: expected pool name\n", err);
+  }
+
+  {
+    State state;
+    ManifestParser parser(&state, NULL);
+    string err;
+    EXPECT_FALSE(parser.ParseTest("pool foo\n", &err));
+    EXPECT_EQ("input:2: expected 'depth =' line\n", err);
+  }
+
+  {
+    State state;
+    ManifestParser parser(&state, NULL);
+    string err;
+    EXPECT_FALSE(parser.ParseTest("pool foo\n"
+                                  "  depth = 4\n"
+                                  "pool foo\n", &err));
+    EXPECT_EQ("input:3: duplicate pool 'foo'\n"
+              "pool foo\n"
+              "        ^ near here"
+              , err);
+  }
+
+  {
+    State state;
+    ManifestParser parser(&state, NULL);
+    string err;
+    EXPECT_FALSE(parser.ParseTest("pool foo\n"
+                                  "  depth = -1\n", &err));
+    EXPECT_EQ("input:2: invalid pool depth\n"
+              "  depth = -1\n"
+              "            ^ near here"
+              , err);
+  }
+
+  {
+    State state;
+    ManifestParser parser(&state, NULL);
+    string err;
+    EXPECT_FALSE(parser.ParseTest("pool foo\n"
+                                  "  bar = 1\n", &err));
+    EXPECT_EQ("input:2: unexpected variable 'bar'\n"
+              "  bar = 1\n"
+              "         ^ near here"
+              , err);
+  }
+
+  {
+    State state;
+    ManifestParser parser(&state, NULL);
+    string err;
+    // Pool names are dereferenced at edge parsing time.
+    EXPECT_FALSE(parser.ParseTest("rule run\n"
+                                  "  command = echo\n"
+                                  "  pool = unnamed_pool\n"
+                                  "build out: run in\n", &err));
+    EXPECT_EQ("input:5: unknown pool name 'unnamed_pool'\n", err);
+  }
 }
 
 TEST_F(ParserTest, MissingInput) {
-- 
cgit v0.12