Improve GN header checker, make //ui pass.

tools/gn/header_checker_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <vector>
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "tools/gn/config.h"
 #include "tools/gn/header_checker.h"
 #include "tools/gn/scheduler.h"
@@ skipping 68 matching lines @@
   p.set_output_type(Target::SOURCE_SET);
   p.SetToolchain(setup_.toolchain(), &err);
   EXPECT_FALSE(err.has_error());
   p.private_deps().push_back(LabelTargetPair(&c_));
   p.visibility().SetPublic();
   p.OnResolved(&err);
 
   a_.public_deps().push_back(LabelTargetPair(&p));
 
   // A does not depend on itself.
-  bool is_public = false;
-  std::vector<const Target*> chain;
-  EXPECT_FALSE(checker->IsDependencyOf(&a_, &a_, &chain, &is_public));
+  bool is_permitted = false;
+  HeaderChecker::Chain chain;
+  EXPECT_FALSE(checker->IsDependencyOf(&a_, &a_, &chain, &is_permitted));
 
-  // A depends on B.
+  // A depends publicly on B.
   chain.clear();
-  is_public = false;
-  EXPECT_TRUE(checker->IsDependencyOf(&b_, &a_, &chain, &is_public));
+  is_permitted = false;
+  EXPECT_TRUE(checker->IsDependencyOf(&b_, &a_, &chain, &is_permitted));
   ASSERT_EQ(2u, chain.size());
-  EXPECT_EQ(&b_, chain[0]);
-  EXPECT_EQ(&a_, chain[1]);
-  EXPECT_TRUE(is_public);
+  EXPECT_EQ(HeaderChecker::ChainLink(&b_, true), chain[0]);
+  EXPECT_EQ(HeaderChecker::ChainLink(&a_, true), chain[1]);
+  EXPECT_TRUE(is_permitted);
 
   // A indirectly depends on C. The "public" dependency path through B should
   // be identified.
   chain.clear();
-  is_public = false;
-  EXPECT_TRUE(checker->IsDependencyOf(&c_, &a_, &chain, &is_public));
+  is_permitted = false;
+  EXPECT_TRUE(checker->IsDependencyOf(&c_, &a_, &chain, &is_permitted));
   ASSERT_EQ(3u, chain.size());
-  EXPECT_EQ(&c_, chain[0]);
-  EXPECT_EQ(&b_, chain[1]);
-  EXPECT_EQ(&a_, chain[2]);
-  EXPECT_TRUE(is_public);
+  EXPECT_EQ(HeaderChecker::ChainLink(&c_, true), chain[0]);
+  EXPECT_EQ(HeaderChecker::ChainLink(&b_, true), chain[1]);
+  EXPECT_EQ(HeaderChecker::ChainLink(&a_, true), chain[2]);
+  EXPECT_TRUE(is_permitted);
 
   // C does not depend on A.
   chain.clear();
-  is_public = false;
-  EXPECT_FALSE(checker->IsDependencyOf(&a_, &c_, &chain, &is_public));
+  is_permitted = false;
+  EXPECT_FALSE(checker->IsDependencyOf(&a_, &c_, &chain, &is_permitted));
   EXPECT_TRUE(chain.empty());
-  EXPECT_FALSE(is_public);
+  EXPECT_FALSE(is_permitted);
 
-  // Add a private A -> C dependency. This should not be detected since it's
-  // private, even though it's shorter than the A -> B -> C one.
+  // Remove the B -> C public dependency, leaving P's private dep on C the only
+  // path from A to C. This should now be found.
   chain.clear();
-  is_public = false;
-  a_.private_deps().push_back(LabelTargetPair(&c_));
-  EXPECT_TRUE(checker->IsDependencyOf(&c_, &a_, &chain, &is_public));
-  EXPECT_EQ(&c_, chain[0]);
-  EXPECT_EQ(&b_, chain[1]);
-  EXPECT_EQ(&a_, chain[2]);
-  EXPECT_TRUE(is_public);
+  EXPECT_EQ(&c_, b_.public_deps()[0].ptr);  // Validate it's the right one.
+  b_.public_deps().erase(b_.public_deps().begin());
+  EXPECT_TRUE(checker->IsDependencyOf(&c_, &a_, &chain, &is_permitted));
+  EXPECT_EQ(3u, chain.size());
+  EXPECT_EQ(HeaderChecker::ChainLink(&c_, false), chain[0]);
+  EXPECT_EQ(HeaderChecker::ChainLink(&p, true), chain[1]);

[scottmg, 2014/09/18 22:33:52]

hm, no i guess is_public for ChainLink makes sense.

+  EXPECT_EQ(HeaderChecker::ChainLink(&a_, true), chain[2]);
+  EXPECT_FALSE(is_permitted);
 

[scottmg, 2014/09/18 22:33:52]

since the iteration depends on discarding disallowed paths before finding an
allowed one, maybe adding a test that has the private before/after.

i.e.

A - B - C
  \   /
    P

vs.
    Q
  /   \
A - B - C
  \   /
    P

-  // Remove the B -> C public dependency, leaving A's private dep on C the only
-  // path. This should now be found.
+  // P privately depends on C. That dependency should be OK since it's only
+  // one hop.
   chain.clear();
-  EXPECT_EQ(&c_, b_.public_deps()[0].ptr); // Validate it's the right one.
-  b_.public_deps().erase(b_.public_deps().begin());
-  EXPECT_TRUE(checker->IsDependencyOf(&c_, &a_, &chain, &is_public));
-  EXPECT_EQ(&c_, chain[0]);
-  EXPECT_EQ(&a_, chain[1]);
-  EXPECT_FALSE(is_public);
+  is_permitted = false;
+  EXPECT_TRUE(checker->IsDependencyOf(&c_, &p, &chain, &is_permitted));
+  ASSERT_EQ(2u, chain.size());
+  EXPECT_EQ(HeaderChecker::ChainLink(&c_, false), chain[0]);
+  EXPECT_EQ(HeaderChecker::ChainLink(&p, true), chain[1]);
+  EXPECT_TRUE(is_permitted);

[scottmg, 2014/09/18 22:33:52]

Maybe a test that

A -> B -> C -> D

is right too, when B->C is private. That is, A -> D disallowed, but chain[0] is
public.

 }
 
 TEST_F(HeaderCheckerTest, CheckInclude) {
   InputFile input_file(SourceFile("//some_file.cc"));
   input_file.SetContents(std::string());
   LocationRange range;  // Dummy value.
 
   // Add a disconnected target d with a header to check that you have to have
   // to depend on a target listing a header.
   SourceFile d_header("//d_header.h");
@@ skipping 59 matching lines @@
   EXPECT_TRUE(err.has_error());
 
   // Add an allow_circular_includes_from on A that lists B.
   a_.allow_circular_includes_from().insert(b_.label());
 
   // Now the include from B to A should be allowed.
   err = Err();
   EXPECT_TRUE(checker->CheckInclude(&b_, input_file, a_public, range, &err));
   EXPECT_FALSE(err.has_error());
 }