GN: (HeaderChecker) Allow any chain to forward direct dependent configs.

When finding a dependency chain, the header check will now prefer chains which
forward direct-dependent configs if the target which provides a header has
one. Previously an error would be produced because the chain which was found
did not forward configs, even though another chain did.

A breadth-first search is used instead of a depth-first search because short
chains are more likely to be intended by the author, and make for both less
work and clearer diagnostic messages.

This reduces the size of 'gn check' output on the chromium tree and the time
to execute. (Output directed to 'wc -l' and /dev/null, respectively.)

  Before: 104685 lines, 542.36s user, 35.573s total
  After: 36977 lines, 7.10s user, 1.526s total

Manual inspection reveals that the line reduction appears to be from the
removal of spurious errors when IsDependencyOf selected an indirect chain
which does not forward direct-dependent configs, even though a direct
dependency existed. (About 7 cases were examined manually; sheer volume makes
full manual verification infeasible.)

BUG=391505
TEST=HeaderCheckerTest, manual

Review URL: https://codereview.chromium.org/424133002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@288110 0039d316-1c4b-4281-b951-d872f2087c98

tools/gn/header_checker.cc

@@ -275,6 +275,7 @@ bool HeaderChecker::CheckInclude(const Target* from_target,
 
   const TargetVector& targets = found->second;
   std::vector<const Target*> chain;  // Prevent reallocating in the loop.
+  bool direct_dependent_configs_apply = false;
 
   // For all targets containing this file, we require that at least one be
   // a dependency of the current target, and all targets that are dependencies
@@ -287,7 +288,13 @@ bool HeaderChecker::CheckInclude(const Target* from_target,
     if (to_target == from_target)
       return true;
 
-    if (IsDependencyOf(to_target, from_target, &chain)) {
+    bool has_direct_dependent_compiler_settings =
+        HasDirectDependentCompilerSettings(to_target);
+    if (IsDependencyOf(to_target,
+                       from_target,
+                       has_direct_dependent_compiler_settings,
+                       &chain,
+                       &direct_dependent_configs_apply)) {
       DCHECK(chain.size() >= 2);
       DCHECK(chain[0] == to_target);
       DCHECK(chain[chain.size() - 1] == from_target);
@@ -319,14 +326,13 @@ bool HeaderChecker::CheckInclude(const Target* from_target,
 
       // If the to_target has direct_dependent_configs, they must apply to the
       // from_target.
-      if (HasDirectDependentCompilerSettings(to_target)) {
-        size_t problematic_index;
-        if (!DoDirectDependentConfigsApply(chain, &problematic_index)) {
-          *err = Err(CreatePersistentRange(source_file, range),
-                     "Can't include this header from here.",
-                     GetDependentConfigChainError(chain, problematic_index));
-          return false;
-        }
+      if (has_direct_dependent_compiler_settings &&
+          !direct_dependent_configs_apply) {
+        size_t problematic_index = GetDependentConfigChainProblemIndex(chain);
+        *err = Err(CreatePersistentRange(source_file, range),
+                   "Can't include this header from here.",
+                   GetDependentConfigChainError(chain, problematic_index));
+        return false;
       }
 
       found_dependency = true;
@@ -374,33 +380,81 @@ bool HeaderChecker::CheckInclude(const Target* from_target,
 
 bool HeaderChecker::IsDependencyOf(const Target* search_for,
                                    const Target* search_from,
-                                   std::vector<const Target*>* chain) const {
-  std::set<const Target*> checked;
-  return IsDependencyOf(search_for, search_from, chain, &checked);
+                                   bool prefer_direct_dependent_configs,
+                                   std::vector<const Target*>* chain,
+                                   bool* direct_dependent_configs_apply) const {
+  if (search_for == search_from)
+    return false;
+
+  // Find the shortest chain that forwards dependent configs, if one exists.
+  if (prefer_direct_dependent_configs &&
+      IsDependencyOf(search_for, search_from, true, chain)) {
+    if (direct_dependent_configs_apply)
+      *direct_dependent_configs_apply = true;
+    return true;
+  }
+
+  // If not, try to find any dependency chain at all.
+  if (IsDependencyOf(search_for, search_from, false, chain)) {
+    if (direct_dependent_configs_apply)
+      *direct_dependent_configs_apply = false;
+    return true;
+  }
+
+  return false;
 }
 
 bool HeaderChecker::IsDependencyOf(const Target* search_for,
                                    const Target* search_from,
-                                   std::vector<const Target*>* chain,
-                                   std::set<const Target*>* checked) const {
-  if (checked->find(search_for) != checked->end())
-    return false;  // Already checked this subtree.
-
-  const LabelTargetVector& deps = search_from->deps();
-  for (size_t i = 0; i < deps.size(); i++) {
-    if (deps[i].ptr == search_for) {
-      // Found it.
+                                   bool requires_dependent_configs,
+                                   std::vector<const Target*>* chain) const {
+  // This method conducts a breadth-first search through the dependency graph
+  // to find a shortest chain from search_from to search_for.
+  //
+  // work_queue maintains a queue of targets which need to be considered as
+  // part of this chain, in the order they were first traversed.
+  //
+  // Each time a new transitive dependency of search_from is discovered for
+  // the first time, it is added to work_queue and a "breadcrumb" is added,
+  // indicating which target it was reached from when first discovered.
+  //
+  // Once this search finds search_for, the breadcrumbs are used to reconstruct
+  // a shortest dependency chain (in reverse order) from search_from to
+  // search_for.
+
+  std::map<const Target*, const Target*> breadcrumbs;
+  std::queue<const Target*> work_queue;
+  work_queue.push(search_from);
+
+  while (!work_queue.empty()) {
+    const Target* target = work_queue.front();
+    work_queue.pop();
+
+    if (target == search_for) {
+      // Found it! Reconstruct the chain.
       chain->clear();
-      chain->push_back(deps[i].ptr);
+      while (target != search_from) {
+        chain->push_back(target);
+        target = breadcrumbs[target];
+      }
       chain->push_back(search_from);
       return true;
     }
 
-    // Recursive search.
-    checked->insert(deps[i].ptr);
-    if (IsDependencyOf(search_for, deps[i].ptr, chain, checked)) {
-      chain->push_back(search_from);
-      return true;
+    // If the callee requires direct-dependent configs be forwarded, then
+    // only targets for which they will be forwarded should be explored.
+    // Groups implicitly forward direct-dependent configs of all of their deps.
+    bool uses_all_deps = !requires_dependent_configs || target == search_from ||
+                         target->output_type() == Target::GROUP;
+
+    const LabelTargetVector& deps =
+        uses_all_deps ? target->deps()
+                      : target->forward_dependent_configs().vector();
+    for (size_t i = 0; i < deps.size(); i++) {
+      bool seeing_for_first_time =
+          breadcrumbs.insert(std::make_pair(deps[i].ptr, target)).second;
+      if (seeing_for_first_time)
+        work_queue.push(deps[i].ptr);
     }
   }
 
@@ -408,24 +462,17 @@ bool HeaderChecker::IsDependencyOf(const Target* search_for,
 }
 
 // static
-bool HeaderChecker::DoDirectDependentConfigsApply(
-    const std::vector<const Target*>& chain,
-    size_t* problematic_index) {
+size_t HeaderChecker::GetDependentConfigChainProblemIndex(
+    const std::vector<const Target*>& chain) {
   // Direct dependent configs go up the chain one level with the following
   // exceptions:
   // - Skip over groups
   // - Skip anything that explicitly forwards it
 
-  // All chains should be at least two (or it wouldn't be a chain).
-  DCHECK(chain.size() >= 2);
-
-  // A chain of length 2 is always OK as far as direct dependent configs are
-  // concerned since the targets are direct dependents.
-  if (chain.size() == 2)
-    return true;
+  // Chains of length less than three have no problems.
+  // These should have been filtered out earlier.
+  DCHECK(chain.size() >= 3);
 
-  // Check the middle configs to make sure they're either groups or configs
-  // are forwarded.
   for (size_t i = 1; i < chain.size() - 1; i++) {
     if (chain[i]->output_type() == Target::GROUP)
       continue;  // This one is OK, skip to next one.
@@ -436,11 +483,10 @@ bool HeaderChecker::DoDirectDependentConfigsApply(
         chain[i]->forward_dependent_configs();
     if (std::find_if(forwarded.begin(), forwarded.end(),
                      LabelPtrPtrEquals<Target>(chain[i - 1])) ==
-        forwarded.end()) {
-      *problematic_index = i;
-      return false;
-    }
+        forwarded.end())
+      return i;
   }
 
-  return true;
+  CHECK(false) << "Unable to diagnose dependent config chain problem.";
+  return 0;
 }

tools/gn/header_checker.h

@@ -41,8 +41,11 @@ class HeaderChecker : public base::RefCountedThreadSafe<HeaderChecker> {
  private:
   friend class base::RefCountedThreadSafe<HeaderChecker>;
   FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, IsDependencyOf);
+  FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest,
+                           IsDependencyOf_ForwardsDirectDependentConfigs);
   FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, CheckInclude);
-  FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, DoDirectDependentConfigsApply);
+  FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest,
+                           GetDependentConfigChainProblemIndex);
   ~HeaderChecker();
 
   struct TargetInfo {
@@ -84,32 +87,40 @@ class HeaderChecker : public base::RefCountedThreadSafe<HeaderChecker> {
                     Err* err) const;
 
   // Returns true if the given search_for target is a dependency of
-  // search_from. Many subtrees are duplicated so this function avoids
-  // duplicate checking across recursive calls by keeping track of checked
-  // targets in the given set. It should point to an empty set for the first
-  // call. A target is not considered to be a dependency of itself.
+  // search_from.
   //
   // If found, the vector given in "chain" will be filled with the reverse
   // dependency chain from the dest target (chain[0] = search_for) to the src
   // target (chain[chain.size() - 1] = search_from).
+  //
+  // If prefer_direct_dependent_configs is true, chains which forward direct
+  // dependent configs will be considered first, and a chain which does not
+  // will be returned only if no such chain exists.
+  //
+  // If direct_dependent_configs_apply is non-null, it will be set to true
+  // if the chain was found during a search that requires forwarding direct
+  // dependent configs, and false if it was found during a search of the
+  // entire dependency graph.
   bool IsDependencyOf(const Target* search_for,
                       const Target* search_from,
-                      std::vector<const Target*>* chain) const;
+                      bool prefer_direct_dependent_configs,
+                      std::vector<const Target*>* chain,
+                      bool* direct_dependent_configs_apply) const;
+
+  // For internal use by the previous override of IsDependencyOf.
+  // If requires_dependent_configs is true, only chains which forward
+  // direct dependent configs are considered.
   bool IsDependencyOf(const Target* search_for,
                       const Target* search_from,
-                      std::vector<const Target*>* chain,
-                      std::set<const Target*>* checked) const;
+                      bool requires_dependent_configs,
+                      std::vector<const Target*>* chain) const;
 
   // Given a reverse dependency chain (chain[0] is the lower-level target,
-  // chain[end] is the higher-level target), determines if all direct dependent
-  // configs on the lower-level target would apply to the higher-level one.
-  //
-  // If configs do not apply, this function returns false and indicates the
-  // index of the target that caused the config to not apply by putting it in
-  // problematic_index.
-  static bool DoDirectDependentConfigsApply(
-      const std::vector<const Target*>& chain,
-      size_t* problematic_index);
+  // chain[end] is the higher-level target) which does not forward direct
+  // dependent configs, determines the index of the target that caused the
+  // config to not apply.
+  static size_t GetDependentConfigChainProblemIndex(
+      const std::vector<const Target*>& chain);
 
   // Non-locked variables ------------------------------------------------------
   //

tools/gn/header_checker_unittest.cc

@@ -5,6 +5,7 @@
 #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"
 #include "tools/gn/target.h"
@@ -17,7 +18,8 @@ class HeaderCheckerTest : public testing::Test {
   HeaderCheckerTest()
       : a_(setup_.settings(), Label(SourceDir("//a/"), "a")),
         b_(setup_.settings(), Label(SourceDir("//b/"), "a")),
-        c_(setup_.settings(), Label(SourceDir("//c/"), "c")) {
+        c_(setup_.settings(), Label(SourceDir("//c/"), "c")),
+        d_(setup_.settings(), Label(SourceDir("//d/"), "d")) {
     a_.deps().push_back(LabelTargetPair(&b_));
     b_.deps().push_back(LabelTargetPair(&c_));
 
@@ -25,10 +27,12 @@ class HeaderCheckerTest : public testing::Test {
     a_.visibility().SetPublic();
     b_.visibility().SetPublic();
     c_.visibility().SetPublic();
+    d_.visibility().SetPublic();
 
     targets_.push_back(&a_);
     targets_.push_back(&b_);
     targets_.push_back(&c_);
+    targets_.push_back(&d_);
   }
 
  protected:
@@ -41,6 +45,7 @@ class HeaderCheckerTest : public testing::Test {
   Target a_;
   Target b_;
   Target c_;
+  Target d_;
 
   std::vector<const Target*> targets_;
 };
@@ -52,24 +57,88 @@ TEST_F(HeaderCheckerTest, IsDependencyOf) {
       new HeaderChecker(setup_.build_settings(), targets_));
 
   std::vector<const Target*> chain;
-  EXPECT_FALSE(checker->IsDependencyOf(&a_, &a_, &chain));
+  EXPECT_FALSE(checker->IsDependencyOf(&a_, &a_, false, &chain, NULL));
 
   chain.clear();
-  EXPECT_TRUE(checker->IsDependencyOf(&b_, &a_, &chain));
+  EXPECT_TRUE(checker->IsDependencyOf(&b_, &a_, false, &chain, NULL));
   ASSERT_EQ(2u, chain.size());
   EXPECT_EQ(&b_, chain[0]);
   EXPECT_EQ(&a_, chain[1]);
 
   chain.clear();
-  EXPECT_TRUE(checker->IsDependencyOf(&c_, &a_, &chain));
+  EXPECT_TRUE(checker->IsDependencyOf(&c_, &a_, false, &chain, NULL));
   ASSERT_EQ(3u, chain.size());
   EXPECT_EQ(&c_, chain[0]);
   EXPECT_EQ(&b_, chain[1]);
   EXPECT_EQ(&a_, chain[2]);
 
   chain.clear();
-  EXPECT_FALSE(checker->IsDependencyOf(&a_, &c_, &chain));
+  EXPECT_FALSE(checker->IsDependencyOf(&a_, &c_, false, &chain, NULL));
   EXPECT_TRUE(chain.empty());
+
+  // If an a -> c dependency exists, this should be chosen for the chain.
+  chain.clear();
+  a_.deps().push_back(LabelTargetPair(&c_));
+  EXPECT_TRUE(checker->IsDependencyOf(&c_, &a_, false, &chain, NULL));
+  EXPECT_EQ(&c_, chain[0]);
+  EXPECT_EQ(&a_, chain[1]);
+}
+
+TEST_F(HeaderCheckerTest, IsDependencyOf_ForwardsDirectDependentConfigs) {
+  scoped_refptr<HeaderChecker> checker(
+      new HeaderChecker(setup_.build_settings(), targets_));
+
+  // The a -> b -> c chain is found, since no chains that forward direct-
+  // dependent configs exist.
+  std::vector<const Target*> chain;
+  bool direct_dependent_configs_apply = false;
+  EXPECT_TRUE(checker->IsDependencyOf(
+      &c_, &a_, true, &chain, &direct_dependent_configs_apply));
+  EXPECT_FALSE(direct_dependent_configs_apply);
+  EXPECT_EQ(3u, chain.size());
+  EXPECT_EQ(&c_, chain[0]);
+  EXPECT_EQ(&b_, chain[1]);
+  EXPECT_EQ(&a_, chain[2]);
+
+  // Create a chain a -> d -> c where d forwards direct-dependent configs.
+  // This path should be preferred when dependency chains which forward
+  // direct-dependent configs are preferred.
+  chain.clear();
+  direct_dependent_configs_apply = false;
+  d_.deps().push_back(LabelTargetPair(&c_));
+  d_.forward_dependent_configs().push_back(LabelTargetPair(&c_));
+  a_.deps().push_back(LabelTargetPair(&d_));
+  EXPECT_TRUE(checker->IsDependencyOf(
+      &c_, &a_, true, &chain, &direct_dependent_configs_apply));
+  EXPECT_TRUE(direct_dependent_configs_apply);
+  EXPECT_EQ(3u, chain.size());
+  EXPECT_EQ(&c_, chain[0]);
+  EXPECT_EQ(&d_, chain[1]);
+  EXPECT_EQ(&a_, chain[2]);
+
+  // d also forwards direct-dependent configs if it is a group.
+  chain.clear();
+  direct_dependent_configs_apply = false;
+  d_.set_output_type(Target::GROUP);
+  d_.forward_dependent_configs().clear();
+  EXPECT_TRUE(checker->IsDependencyOf(
+      &c_, &a_, true, &chain, &direct_dependent_configs_apply));
+  EXPECT_TRUE(direct_dependent_configs_apply);
+  EXPECT_EQ(3u, chain.size());
+  EXPECT_EQ(&c_, chain[0]);
+  EXPECT_EQ(&d_, chain[1]);
+  EXPECT_EQ(&a_, chain[2]);
+
+  // A direct dependency a -> c carries direct-dependent configs.
+  chain.clear();
+  direct_dependent_configs_apply = false;
+  a_.deps().push_back(LabelTargetPair(&c_));
+  EXPECT_TRUE(checker->IsDependencyOf(
+      &c_, &a_, true, &chain, &direct_dependent_configs_apply));
+  EXPECT_TRUE(direct_dependent_configs_apply);
+  EXPECT_EQ(2u, chain.size());
+  EXPECT_EQ(&c_, chain[0]);
+  EXPECT_EQ(&a_, chain[1]);
 }
 
 TEST_F(HeaderCheckerTest, CheckInclude) {
@@ -79,9 +148,8 @@ TEST_F(HeaderCheckerTest, CheckInclude) {
 
   // Add a disconnected target d with a header to check that you have to have
   // to depend on a target listing a header.
-  Target d(setup_.settings(), Label(SourceDir("//"), "d"));
   SourceFile d_header("//d_header.h");
-  d.sources().push_back(SourceFile(d_header));
+  d_.sources().push_back(SourceFile(d_header));
 
   // Add a header on B and say everything in B is public.
   SourceFile b_public("//b_public.h");
@@ -95,7 +163,7 @@ TEST_F(HeaderCheckerTest, CheckInclude) {
   c_.public_headers().push_back(c_public);
   c_.set_all_headers_public(false);
 
-  targets_.push_back(&d);
+  targets_.push_back(&d_);
   scoped_refptr<HeaderChecker> checker(
       new HeaderChecker(setup_.build_settings(), targets_));
 
@@ -129,9 +197,36 @@ TEST_F(HeaderCheckerTest, CheckInclude) {
   err = Err();
   EXPECT_FALSE(checker->CheckInclude(&a_, input_file, c_public, range, &err));
   EXPECT_TRUE(err.has_error());
+  c_.visibility().SetPublic();
+
+  // If C has direct-dependent configs, then B must forward them to A.
+  // If B is a group, that suffices to forward direct-dependent configs.
+  {
+    Config direct(setup_.settings(), Label(SourceDir("//c/"), "config"));
+    direct.config_values().cflags().push_back("-DSOME_DEFINE");
+
+    c_.direct_dependent_configs().push_back(LabelConfigPair(&direct));
+    err = Err();
+    EXPECT_FALSE(checker->CheckInclude(&a_, input_file, c_public, range, &err));
+    EXPECT_TRUE(err.has_error());
+
+    b_.forward_dependent_configs().push_back(LabelTargetPair(&c_));
+    err = Err();
+    EXPECT_TRUE(checker->CheckInclude(&a_, input_file, c_public, range, &err));
+    EXPECT_FALSE(err.has_error());
+
+    b_.forward_dependent_configs().clear();
+    b_.set_output_type(Target::GROUP);
+    err = Err();
+    EXPECT_TRUE(checker->CheckInclude(&a_, input_file, c_public, range, &err));
+    EXPECT_FALSE(err.has_error());
+
+    b_.set_output_type(Target::UNKNOWN);
+    c_.direct_dependent_configs().clear();
+  }
 }
 
-TEST_F(HeaderCheckerTest, DoDirectDependentConfigsApply) {
+TEST_F(HeaderCheckerTest, GetDependentConfigChainProblemIndex) {
   // Assume we have a chain A -> B -> C -> D.
   Target target_a(setup_.settings(), Label(SourceDir("//a/"), "a"));
   Target target_b(setup_.settings(), Label(SourceDir("//b/"), "b"));
@@ -153,19 +248,12 @@ TEST_F(HeaderCheckerTest, DoDirectDependentConfigsApply) {
   chain.push_back(&target_b);
   chain.push_back(&target_a);
 
-  // This chain should be valid.
-  size_t badone = 0;
-  EXPECT_TRUE(HeaderChecker::DoDirectDependentConfigsApply(chain, &badone));
-
   // If C is not a group, it shouldn't work anymore.
   target_c.set_output_type(Target::SOURCE_SET);
-  EXPECT_FALSE(HeaderChecker::DoDirectDependentConfigsApply(chain, &badone));
-  EXPECT_EQ(1u, badone);
+  EXPECT_EQ(1u, HeaderChecker::GetDependentConfigChainProblemIndex(chain));
 
   // Or if B stops forwarding from C, it shouldn't work anymore.
   target_c.set_output_type(Target::GROUP);
-  badone = 0;
   target_b.forward_dependent_configs().clear();
-  EXPECT_FALSE(HeaderChecker::DoDirectDependentConfigsApply(chain, &badone));
-  EXPECT_EQ(2u, badone);
+  EXPECT_EQ(2u, HeaderChecker::GetDependentConfigChainProblemIndex(chain));
 }