From 4d98903d4c986f720ddb3f18d32c1125ef3e680e Mon Sep 17 00:00:00 2001 From: Jan Niklas Hasse Date: Wed, 6 Dec 2023 20:49:28 +0100 Subject: Improve misleading error message when an output is defined multiple times --- src/manifest_parser.cc | 4 ++-- src/manifest_parser_test.cc | 4 ++-- src/missing_deps_test.cc | 6 +++--- src/state.cc | 11 +++++++++-- src/state.h | 2 +- src/state_test.cc | 2 +- 6 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index aa52989..c4b2980 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -336,8 +336,8 @@ bool ManifestParser::ParseEdge(string* err) { return lexer_.Error("empty path", err); uint64_t slash_bits; CanonicalizePath(&path, &slash_bits); - if (!state_->AddOut(edge, path, slash_bits)) { - lexer_.Error("multiple rules generate " + path, err); + if (!state_->AddOut(edge, path, slash_bits, err)) { + lexer_.Error(std::string(*err), err); return false; } } diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc index 8a7b135..c5a1fe8 100644 --- a/src/manifest_parser_test.cc +++ b/src/manifest_parser_test.cc @@ -978,7 +978,7 @@ TEST_F(ParserTest, ImplicitOutputDupeError) { ManifestParser parser(&state, &fs_); string err; EXPECT_FALSE(parser.ParseTest(kInput, &err)); - EXPECT_EQ("input:4: multiple rules generate foo\n", err); + EXPECT_EQ("input:4: foo is defined as an output multiple times\n", err); } TEST_F(ParserTest, ImplicitOutputDupesError) { @@ -989,7 +989,7 @@ TEST_F(ParserTest, ImplicitOutputDupesError) { ManifestParser parser(&state, &fs_); string err; EXPECT_FALSE(parser.ParseTest(kInput, &err)); - EXPECT_EQ("input:4: multiple rules generate foo\n", err); + EXPECT_EQ("input:4: foo is defined as an output multiple times\n", err); } TEST_F(ParserTest, NoExplicitOutput) { diff --git a/src/missing_deps_test.cc b/src/missing_deps_test.cc index 12ae8ed..dae377b 100644 --- a/src/missing_deps_test.cc +++ b/src/missing_deps_test.cc @@ -64,9 +64,9 @@ struct MissingDependencyScannerTest : public testing::Test { compile_rule_.AddBinding("deps", deps_type); generator_rule_.AddBinding("deps", deps_type); Edge* header_edge = state_.AddEdge(&generator_rule_); - state_.AddOut(header_edge, "generated_header", 0); + state_.AddOut(header_edge, "generated_header", 0, nullptr); Edge* compile_edge = state_.AddEdge(&compile_rule_); - state_.AddOut(compile_edge, "compiled_object", 0); + state_.AddOut(compile_edge, "compiled_object", 0, nullptr); } void CreateGraphDependencyBetween(const char* from, const char* to) { @@ -130,7 +130,7 @@ TEST_F(MissingDependencyScannerTest, MissingDepFixedIndirect) { CreateInitialState(); // Adding an indirect dependency also fixes the issue Edge* intermediate_edge = state_.AddEdge(&generator_rule_); - state_.AddOut(intermediate_edge, "intermediate", 0); + state_.AddOut(intermediate_edge, "intermediate", 0, nullptr); CreateGraphDependencyBetween("compiled_object", "intermediate"); CreateGraphDependencyBetween("intermediate", "generated_header"); RecordDepsLogDep("compiled_object", "generated_header"); diff --git a/src/state.cc b/src/state.cc index 0a68f21..d4b9a71 100644 --- a/src/state.cc +++ b/src/state.cc @@ -133,10 +133,17 @@ void State::AddIn(Edge* edge, StringPiece path, uint64_t slash_bits) { node->AddOutEdge(edge); } -bool State::AddOut(Edge* edge, StringPiece path, uint64_t slash_bits) { +bool State::AddOut(Edge* edge, StringPiece path, uint64_t slash_bits, + std::string* err) { Node* node = GetNode(path, slash_bits); - if (node->in_edge()) + if (Edge* other = node->in_edge()) { + if (other == edge) { + *err = path.AsString() + " is defined as an output multiple times"; + } else { + *err = "multiple rules generate " + path.AsString(); + } return false; + } edge->outputs_.push_back(node); node->set_in_edge(edge); node->set_generated_by_dep_loader(false); diff --git a/src/state.h b/src/state.h index 886b78f..29bed56 100644 --- a/src/state.h +++ b/src/state.h @@ -109,7 +109,7 @@ struct State { /// ensures that the generated_by_dep_loader() flag for all these nodes /// is set to false, to indicate that they come from the input manifest. void AddIn(Edge* edge, StringPiece path, uint64_t slash_bits); - bool AddOut(Edge* edge, StringPiece path, uint64_t slash_bits); + bool AddOut(Edge* edge, StringPiece path, uint64_t slash_bits, std::string* err); void AddValidation(Edge* edge, StringPiece path, uint64_t slash_bits); bool AddDefault(StringPiece path, std::string* error); diff --git a/src/state_test.cc b/src/state_test.cc index 96469f9..e0e3060 100644 --- a/src/state_test.cc +++ b/src/state_test.cc @@ -36,7 +36,7 @@ TEST(State, Basic) { Edge* edge = state.AddEdge(rule); state.AddIn(edge, "in1", 0); state.AddIn(edge, "in2", 0); - state.AddOut(edge, "out", 0); + state.AddOut(edge, "out", 0, nullptr); EXPECT_EQ("cat in1 in2 > out", edge->EvaluateCommand()); -- cgit v0.12