Improve GN public header file checking

This makes the header checker and include iterator work from InputFiles (which basically just hold the file buffer) rather than just raw strings. This allows us to reference these files from Err.

Some extra line/char tracking is now in the include iterator so we can actually quote from and annotate the place in the source file where the bad include is, which looks much nicer than "the file blah included blah". It also makes it possible to write much shorter error messages that still make sense.

This adds visibility checking as previously documented in the help for "public" and a unit test for that.

This updates the documentation for "public" which was wrong (it referred to patterns which was not used).

R=scottmg@chromium.org

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

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

13 files changed, 234 insertions, 92 deletions

diff --git a/BUILD.gn b/BUILD.gn
index 224dedb..e8dbc1d 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -32,7 +32,6 @@ group("root") {
     #"//sdch",
     #"//skia",
     #"//third_party/WebKit/Source/platform",
-    "//third_party/freetype2",
     #"//third_party/icu:icudata",
     #"//third_party/leveldatabase",
     "//third_party/libpng",
@@ -47,4 +46,10 @@ group("root") {
     "//ui/gfx/geometry",
     "//url",
   ]
+
+  if (is_linux) {
+    deps += [
+      "//third_party/freetype2",
+    ]
+  }
 }
diff --git a/third_party/freetype2/BUILD.gn b/third_party/freetype2/BUILD.gn
index 1f43030..636772c 100644
--- a/third_party/freetype2/BUILD.gn
+++ b/third_party/freetype2/BUILD.gn
@@ -2,6 +2,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+assert(is_linux, "This file should only be depended on from Linux.")
+
 config("freetype2_config") {
   include_dirs = [ "include", "src/include" ]
 }
diff --git a/tools/gn/c_include_iterator.cc b/tools/gn/c_include_iterator.cc
index 3690ded..b8cc59c 100644
--- a/tools/gn/c_include_iterator.cc
+++ b/tools/gn/c_include_iterator.cc
@@ -5,6 +5,9 @@
 #include "tools/gn/c_include_iterator.h"
 
 #include "base/logging.h"
+#include "base/strings/string_util.h"
+#include "tools/gn/input_file.h"
+#include "tools/gn/location.h"
 
 namespace {
 
@@ -47,24 +50,34 @@ bool ShouldCountTowardNonIncludeLines(const base::StringPiece& line) {
     return false;  // Don't count comments.
   if (StartsWith(line, "#"))
     return false;  // Don't count preprocessor.
+  if (base::ContainsOnlyChars(line, base::kWhitespaceASCII))
+    return false;  // Don't count whitespace lines.
   return true;  // Count everything else.
 }
 
 // Given a line, checks to see if it looks like an include or import and
 // extract the path. The type of include is returned. Returns INCLUDE_NONE on
 // error or if this is not an include line.
+//
+// The 1-based character number on the line that the include was found at
+// will be filled into *begin_char.
 IncludeType ExtractInclude(const base::StringPiece& line,
-                           base::StringPiece* path) {
+                           base::StringPiece* path,
+                           int* begin_char) {
   static const char kInclude[] = "#include";
   static const size_t kIncludeLen = arraysize(kInclude) - 1;  // No null.
   static const char kImport[] = "#import";
   static const size_t kImportLen = arraysize(kImport) - 1;  // No null.
 
+  base::StringPiece trimmed = TrimLeadingWhitespace(line);
+  if (trimmed.empty())
+    return INCLUDE_NONE;
+
   base::StringPiece contents;
-  if (StartsWith(line, base::StringPiece(kInclude, kIncludeLen)))
-    contents = TrimLeadingWhitespace(line.substr(kIncludeLen));
-  else if (StartsWith(line, base::StringPiece(kImport, kImportLen)))
-    contents = TrimLeadingWhitespace(line.substr(kImportLen));
+  if (StartsWith(trimmed, base::StringPiece(kInclude, kIncludeLen)))
+    contents = TrimLeadingWhitespace(trimmed.substr(kIncludeLen));
+  else if (StartsWith(trimmed, base::StringPiece(kImport, kImportLen)))
+    contents = TrimLeadingWhitespace(trimmed.substr(kImportLen));
 
   if (contents.empty())
     return INCLUDE_NONE;
@@ -87,6 +100,8 @@ IncludeType ExtractInclude(const base::StringPiece& line,
     return INCLUDE_NONE;
 
   *path = contents.substr(1, terminator_index - 1);
+  // Note: one based so we do "+ 1".
+  *begin_char = static_cast<int>(path->data() - line.data()) + 1;
   return type;
 }
 
@@ -94,47 +109,55 @@ IncludeType ExtractInclude(const base::StringPiece& line,
 
 const int CIncludeIterator::kMaxNonIncludeLines = 10;
 
-CIncludeIterator::CIncludeIterator(const base::StringPiece& file)
-    : file_(file),
+CIncludeIterator::CIncludeIterator(const InputFile* input)
+    : input_file_(input),
+      file_(input->contents()),
       offset_(0),
+      line_number_(0),
       lines_since_last_include_(0) {
 }
 
 CIncludeIterator::~CIncludeIterator() {
 }
 
-bool CIncludeIterator::GetNextIncludeString(base::StringPiece* out) {
+bool CIncludeIterator::GetNextIncludeString(base::StringPiece* out,
+                                            LocationRange* location) {
   base::StringPiece line;
+  int cur_line_number = 0;
   while (lines_since_last_include_ <= kMaxNonIncludeLines &&
-         GetNextLine(&line)) {
-    base::StringPiece trimmed = TrimLeadingWhitespace(line);
-    if (trimmed.empty())
-      continue;  // Just ignore all empty lines.
-
+         GetNextLine(&line, &cur_line_number)) {
     base::StringPiece include_contents;
-    IncludeType type = ExtractInclude(trimmed, &include_contents);
+    int begin_char;
+    IncludeType type = ExtractInclude(line, &include_contents, &begin_char);
     if (type == INCLUDE_USER) {
       // Only count user includes for now.
       *out = include_contents;
+      *location = LocationRange(
+          Location(input_file_, cur_line_number, begin_char),
+          Location(input_file_, cur_line_number,
+                   begin_char + include_contents.size()));
+
       lines_since_last_include_ = 0;
       return true;
     }
 
-    if (ShouldCountTowardNonIncludeLines(trimmed))
+    if (ShouldCountTowardNonIncludeLines(line))
       lines_since_last_include_++;
   }
   return false;
 }
 
-bool CIncludeIterator::GetNextLine(base::StringPiece* line) {
+bool CIncludeIterator::GetNextLine(base::StringPiece* line, int* line_number) {
   if (offset_ == file_.size())
     return false;
 
   size_t begin = offset_;
   while (offset_ < file_.size() && file_[offset_] != '\n')
     offset_++;
+  line_number_++;
 
   *line = file_.substr(begin, offset_ - begin);
+  *line_number = line_number_;
 
   // If we didn't hit EOF, skip past the newline for the next one.
   if (offset_ < file_.size())
diff --git a/tools/gn/c_include_iterator.h b/tools/gn/c_include_iterator.h
index 2a25ef6..27cc00b 100644
--- a/tools/gn/c_include_iterator.h
+++ b/tools/gn/c_include_iterator.h
@@ -8,32 +8,43 @@
 #include "base/basictypes.h"
 #include "base/strings/string_piece.h"
 
+class InputFile;
+class LocationRange;
+
 // Iterates through #includes in C source and header files.
 //
 // This only returns includes we want to check, which is user includes with
 // double-quotes: #include "..."
 class CIncludeIterator {
  public:
-  // The buffer pointed to must outlive this class.
-  CIncludeIterator(const base::StringPiece& file);
+  // The InputFile pointed to must outlive this class.
+  CIncludeIterator(const InputFile* input);
   ~CIncludeIterator();
 
-  // Fills in the string with the contents of the next include and returns
-  // true, or returns false if there are no more includes.
-  bool GetNextIncludeString(base::StringPiece* out);
+  // Fills in the string with the contents of the next include, and the
+  // location with where it came from, and returns true, or returns false if
+  // there are no more includes.
+  bool GetNextIncludeString(base::StringPiece* out, LocationRange* location);
 
   // Maximum numbef of non-includes we'll tolerate before giving up. This does
   // not count comments or preprocessor.
   static const int kMaxNonIncludeLines;
 
  private:
-  // Returns false on EOF, otherwise fills in the given line.
-  bool GetNextLine(base::StringPiece* line);
+  // Returns false on EOF, otherwise fills in the given line and the one-based
+  // line number into *line_number.
+  bool GetNextLine(base::StringPiece* line, int* line_number);
+
+  const InputFile* input_file_;
 
+  // This just points into input_file_.contents() for convenience.
   base::StringPiece file_;
 
+  // 0-based offset into the file.
   size_t offset_;
 
+  int line_number_;  // One-based. Indicates the last line we read.
+
   // Number of lines we've processed since seeing the last include (or the
   // beginning of the file) with some exceptions.
   int lines_since_last_include_;
diff --git a/tools/gn/c_include_iterator_unittest.cc b/tools/gn/c_include_iterator_unittest.cc
index 53d41ed..c5b9714 100644
--- a/tools/gn/c_include_iterator_unittest.cc
+++ b/tools/gn/c_include_iterator_unittest.cc
@@ -4,6 +4,20 @@
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "tools/gn/c_include_iterator.h"
+#include "tools/gn/input_file.h"
+#include "tools/gn/location.h"
+
+namespace {
+
+bool RangeIs(const LocationRange& range,
+             int line, int begin_char, int end_char) {
+  return range.begin().line_number() == line &&
+         range.end().line_number() == line &&
+         range.begin().char_offset() == begin_char &&
+         range.end().char_offset() == end_char;
+}
+
+}  // namespace
 
 TEST(CIncludeIterator, Basic) {
   std::string buffer;
@@ -19,18 +33,30 @@ TEST(CIncludeIterator, Basic) {
   buffer.append("\n");
   buffer.append("void SomeCode() {\n");
 
-  CIncludeIterator iter(buffer);
+  InputFile file(SourceFile("//foo.cc"));
+  file.SetContents(buffer);
+
+  CIncludeIterator iter(&file);
 
   base::StringPiece contents;
-  EXPECT_TRUE(iter.GetNextIncludeString(&contents));
+  LocationRange range;
+  EXPECT_TRUE(iter.GetNextIncludeString(&contents, &range));
   EXPECT_EQ("foo/bar.h", contents);
-  EXPECT_TRUE(iter.GetNextIncludeString(&contents));
+  EXPECT_TRUE(RangeIs(range, 3, 11, 20)) << range.begin().Describe(true);
+
+  EXPECT_TRUE(iter.GetNextIncludeString(&contents, &range));
   EXPECT_EQ("foo/baz.h", contents);
-  EXPECT_TRUE(iter.GetNextIncludeString(&contents));
+  EXPECT_TRUE(RangeIs(range, 7, 12, 21)) << range.begin().Describe(true);
+
+  EXPECT_TRUE(iter.GetNextIncludeString(&contents, &range));
   EXPECT_EQ("la/deda.h", contents);
-  EXPECT_TRUE(iter.GetNextIncludeString(&contents));
+  EXPECT_TRUE(RangeIs(range, 8, 11, 20)) << range.begin().Describe(true);
+
+  EXPECT_TRUE(iter.GetNextIncludeString(&contents, &range));
   EXPECT_EQ("weird_mac_import.h", contents);
-  EXPECT_FALSE(iter.GetNextIncludeString(&contents));
+  EXPECT_TRUE(RangeIs(range, 9, 10, 28)) << range.begin().Describe(true);
+
+  EXPECT_FALSE(iter.GetNextIncludeString(&contents, &range));
 }
 
 // Tests that we don't search for includes indefinitely.
@@ -40,10 +66,14 @@ TEST(CIncludeIterator, GiveUp) {
     buffer.append("x\n");
   buffer.append("#include \"foo/bar.h\"\n");
 
+  InputFile file(SourceFile("//foo.cc"));
+  file.SetContents(buffer);
+
   base::StringPiece contents;
+  LocationRange range;
 
-  CIncludeIterator iter(buffer);
-  EXPECT_FALSE(iter.GetNextIncludeString(&contents));
+  CIncludeIterator iter(&file);
+  EXPECT_FALSE(iter.GetNextIncludeString(&contents, &range));
   EXPECT_TRUE(contents.empty());
 }
 
@@ -58,10 +88,14 @@ TEST(CIncludeIterator, DontGiveUp) {
     buffer.append("#preproc\n");
   buffer.append("#include \"foo/bar.h\"\n");
 
+  InputFile file(SourceFile("//foo.cc"));
+  file.SetContents(buffer);
+
   base::StringPiece contents;
+  LocationRange range;
 
-  CIncludeIterator iter(buffer);
-  EXPECT_TRUE(iter.GetNextIncludeString(&contents));
+  CIncludeIterator iter(&file);
+  EXPECT_TRUE(iter.GetNextIncludeString(&contents, &range));
   EXPECT_EQ("foo/bar.h", contents);
 }
 
@@ -81,12 +115,16 @@ TEST(CIncludeIterator, TolerateNonIncludes) {
     buffer.append("#include \"" + include + "\"\n");
   }
 
+  InputFile file(SourceFile("//foo.cc"));
+  file.SetContents(buffer);
+
   base::StringPiece contents;
+  LocationRange range;
 
-  CIncludeIterator iter(buffer);
+  CIncludeIterator iter(&file);
   for (size_t group = 0; group < kGroupCount; group++) {
-    EXPECT_TRUE(iter.GetNextIncludeString(&contents));
+    EXPECT_TRUE(iter.GetNextIncludeString(&contents, &range));
     EXPECT_EQ(include, contents.as_string());
   }
-  EXPECT_FALSE(iter.GetNextIncludeString(&contents));
+  EXPECT_FALSE(iter.GetNextIncludeString(&contents, &range));
 }
diff --git a/tools/gn/header_checker.cc b/tools/gn/header_checker.cc
index 54dfb50..3b92c85 100644
--- a/tools/gn/header_checker.cc
+++ b/tools/gn/header_checker.cc
@@ -19,6 +19,35 @@
 #include "tools/gn/target.h"
 #include "tools/gn/trace.h"
 
+namespace {
+
+// This class makes InputFiles on the stack as it reads files to check. When
+// we throw an error, the Err indicates a locatin which has a pointer to
+// an InputFile that must persist as long as the Err does.
+//
+// To make this work, this function creates a clone of the InputFile managed
+// by the InputFileManager so the error can refer to something that
+// persists. This means that the current file contents will live as long as
+// the program, but this is OK since we're erroring out anyway.
+LocationRange CreatePersistentRange(const InputFile& input_file,
+                                    const LocationRange& range) {
+  InputFile* clone_input_file;
+  std::vector<Token>* tokens;  // Don't care about this.
+  scoped_ptr<ParseNode>* parse_root;  // Don't care about this.
+
+  g_scheduler->input_file_manager()->AddDynamicInput(
+      input_file.name(), &clone_input_file, &tokens, &parse_root);
+  clone_input_file->SetContents(input_file.contents());
+
+  return LocationRange(
+      Location(clone_input_file, range.begin().line_number(),
+               range.begin().char_offset()),
+      Location(clone_input_file, range.end().line_number(),
+               range.end().char_offset()));
+}
+
+}  // namespace
+
 HeaderChecker::HeaderChecker(const BuildSettings* build_settings,
                              const std::vector<const Target*>& targets)
     : main_loop_(base::MessageLoop::current()),
@@ -137,11 +166,15 @@ bool HeaderChecker::CheckFile(const Target* from_target,
     return false;
   }
 
-  CIncludeIterator iter(contents);
+  InputFile input_file(file);
+  input_file.SetContents(contents);
+
+  CIncludeIterator iter(&input_file);
   base::StringPiece current_include;
-  while (iter.GetNextIncludeString(&current_include)) {
+  LocationRange range;
+  while (iter.GetNextIncludeString(&current_include, &range)) {
     SourceFile include = SourceFileForInclude(current_include);
-    if (!CheckInclude(from_target, file, include, err))
+    if (!CheckInclude(from_target, input_file, include, range, err))
       return false;
   }
 
@@ -151,8 +184,9 @@ bool HeaderChecker::CheckFile(const Target* from_target,
 // If the file exists, it must be in a dependency of the given target, and it
 // must be public in that dependency.
 bool HeaderChecker::CheckInclude(const Target* from_target,
-                                 const SourceFile& source_file,
+                                 const InputFile& source_file,
                                  const SourceFile& include_file,
+                                 const LocationRange& range,
                                  Err* err) const {
   // Assume if the file isn't declared in our sources that we don't need to
   // check it. It would be nice if we could give an error if this happens, but
@@ -176,17 +210,28 @@ bool HeaderChecker::CheckInclude(const Target* from_target,
       return true;
 
     if (IsDependencyOf(targets[i].target, from_target)) {
-      // The include is in a target that's a proper dependency. Now verify
-      // that the include is a public file.
+      // The include is in a target that's a proper dependency. Verify that
+      // the including target has visibility.
+      if (!targets[i].target->visibility().CanSeeMe(from_target->label())) {
+        std::string msg = "The included file is in " +
+            targets[i].target->label().GetUserVisibleName(false) +
+            "\nwhich is not visible from " +
+            from_target->label().GetUserVisibleName(false) +
+            "\n(see \"gn help visibility\").";
+
+        // Danger: must call CreatePersistentRange to put in Err.
+        *err = Err(CreatePersistentRange(source_file, range),
+            "Including a header from non-visible target.", msg);
+        return false;
+      }
+
+      // The file must also be public in the target.
       if (!targets[i].is_public) {
-        // Depending on a private header.
-        std::string msg = "The file " + source_file.value() +
-            "\nincludes " + include_file.value() +
-            "\nwhich is private to the target " +
-            targets[i].target->label().GetUserVisibleName(false);
-
-        // TODO(brettw) blame the including file.
-        *err = Err(NULL, "Including a private header.", msg);
+        // Danger: must call CreatePersistentRange to put in Err.
+        *err = Err(CreatePersistentRange(source_file, range),
+                   "Including a private header.",
+                   "This file is private to the target " +
+                       targets[i].target->label().GetUserVisibleName(false));
         return false;
       }
       found_dependency = true;
@@ -194,21 +239,19 @@ bool HeaderChecker::CheckInclude(const Target* from_target,
   }
 
   if (!found_dependency) {
-    std::string msg =
-        source_file.value() + " includes the header\n" +
-        include_file.value() + " which is not in any dependency of\n" +
+    std::string msg = "It is not in any dependency of " +
         from_target->label().GetUserVisibleName(false);
-    msg += "\n\nThe include file is in the target(s):\n";
+    msg += "\nThe include file is in the target(s):\n";
     for (size_t i = 0; i < targets.size(); i++)
       msg += "  " + targets[i].target->label().GetUserVisibleName(false) + "\n";
+    if (targets.size() > 1)
+      msg += "at least one of ";
+    msg += "which should somehow be reachable from " +
+        from_target->label().GetUserVisibleName(false);
 
-    msg += "\nMake sure one of these is a direct or indirect dependency\n"
-        "of " + from_target->label().GetUserVisibleName(false);
-
-    // TODO(brettw) blame the including file.
-    // Probably this means making and leaking an input file for it, and also
-    // tracking the locations for each include.
-    *err = Err(NULL, "Include not allowed.", msg);
+    // Danger: must call CreatePersistentRange to put in Err.
+    *err = Err(CreatePersistentRange(source_file, range),
+               "Include not allowed.", msg);
     return false;
   }
 
diff --git a/tools/gn/header_checker.h b/tools/gn/header_checker.h
index f1c7aa3..f62ca2c 100644
--- a/tools/gn/header_checker.h
+++ b/tools/gn/header_checker.h
@@ -18,7 +18,9 @@
 #include "tools/gn/err.h"
 
 class BuildSettings;
+class InputFile;
 class Label;
+class LocationRange;
 class SourceFile;
 class Target;
 
@@ -71,10 +73,13 @@ class HeaderChecker : public base::RefCountedThreadSafe<HeaderChecker> {
                  Err* err) const;
 
   // Checks that the given file in the given target can include the given
-  // include file. If disallowed, returns false and sets the error.
+  // include file. If disallowed, returns false and sets the error. The
+  // range indicates the location of the include in the file for error
+  // reporting.
   bool CheckInclude(const Target* from_target,
-                    const SourceFile& source_file,
+                    const InputFile& source_file,
                     const SourceFile& include_file,
+                    const LocationRange& range,
                     Err* err) const;
 
   // Returns true if the given search_for target is a dependency of
diff --git a/tools/gn/header_checker_unittest.cc b/tools/gn/header_checker_unittest.cc
index 90e4165..05c5f64 100644
--- a/tools/gn/header_checker_unittest.cc
+++ b/tools/gn/header_checker_unittest.cc
@@ -15,12 +15,17 @@ namespace {
 class HeaderCheckerTest : public testing::Test {
  public:
   HeaderCheckerTest()
-      : a_(setup_.settings(), Label(SourceDir("//"), "a")),
-        b_(setup_.settings(), Label(SourceDir("//"), "a")),
-        c_(setup_.settings(), Label(SourceDir("//"), "c")) {
+      : a_(setup_.settings(), Label(SourceDir("//a/"), "a")),
+        b_(setup_.settings(), Label(SourceDir("//b/"), "a")),
+        c_(setup_.settings(), Label(SourceDir("//c/"), "c")) {
     a_.deps().push_back(LabelTargetPair(&b_));
     b_.deps().push_back(LabelTargetPair(&c_));
 
+    // Start with all public visibility.
+    a_.visibility().SetPublic();
+    b_.visibility().SetPublic();
+    c_.visibility().SetPublic();
+
     targets_.push_back(&a_);
     targets_.push_back(&b_);
     targets_.push_back(&c_);
@@ -53,6 +58,10 @@ TEST_F(HeaderCheckerTest, IsDependencyOf) {
 }
 
 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.
   Target d(setup_.settings(), Label(SourceDir("//"), "d"));
@@ -78,25 +87,31 @@ TEST_F(HeaderCheckerTest, CheckInclude) {
   // A file in target A can't include a header from D because A has no
   // dependency on D.
   Err err;
-  SourceFile source_file("//some_file.cc");
-  EXPECT_FALSE(checker->CheckInclude(&a_, source_file, d_header, &err));
+  EXPECT_FALSE(checker->CheckInclude(&a_, input_file, d_header, range, &err));
   EXPECT_TRUE(err.has_error());
 
   // A can include the public header in B.
   err = Err();
-  EXPECT_TRUE(checker->CheckInclude(&a_, source_file, b_public, &err));
+  EXPECT_TRUE(checker->CheckInclude(&a_, input_file, b_public, range, &err));
   EXPECT_FALSE(err.has_error());
 
   // Check A depending on the public and private headers in C.
   err = Err();
-  EXPECT_TRUE(checker->CheckInclude(&a_, source_file, c_public, &err));
+  EXPECT_TRUE(checker->CheckInclude(&a_, input_file, c_public, range, &err));
   EXPECT_FALSE(err.has_error());
-  EXPECT_FALSE(checker->CheckInclude(&a_, source_file, c_private, &err));
+  EXPECT_FALSE(checker->CheckInclude(&a_, input_file, c_private, range, &err));
   EXPECT_TRUE(err.has_error());
 
   // A can depend on a random file unknown to the build.
   err = Err();
-  EXPECT_TRUE(checker->CheckInclude(&a_, source_file, SourceFile("//random.h"),
-                                    &err));
+  EXPECT_TRUE(checker->CheckInclude(&a_, input_file, SourceFile("//random.h"),
+                                    range, &err));
   EXPECT_FALSE(err.has_error());
+
+  // If C is not visible from A, A can't include public headers even if there
+  // is a dependency path.
+  c_.visibility().SetPrivate(c_.label().dir());
+  err = Err();
+  EXPECT_FALSE(checker->CheckInclude(&a_, input_file, c_public, range, &err));
+  EXPECT_TRUE(err.has_error());
 }
diff --git a/tools/gn/input_conversion.cc b/tools/gn/input_conversion.cc
index d25ceff..2534f4d 100644
--- a/tools/gn/input_conversion.cc
+++ b/tools/gn/input_conversion.cc
@@ -37,7 +37,7 @@ Value ParseValueOrScope(const Settings* settings,
   std::vector<Token>* tokens;
   scoped_ptr<ParseNode>* parse_root_ptr;
   g_scheduler->input_file_manager()->AddDynamicInput(
-      &input_file, &tokens, &parse_root_ptr);
+      SourceFile(), &input_file, &tokens, &parse_root_ptr);
 
   input_file->SetContents(input);
   if (origin) {
diff --git a/tools/gn/input_file_manager.cc b/tools/gn/input_file_manager.cc
index ddd7959..04578b6 100644
--- a/tools/gn/input_file_manager.cc
+++ b/tools/gn/input_file_manager.cc
@@ -167,10 +167,11 @@ const ParseNode* InputFileManager::SyncLoadFile(
   return data->parsed_root.get();
 }
 
-void InputFileManager::AddDynamicInput(InputFile** file,
+void InputFileManager::AddDynamicInput(const SourceFile& name,
+                                       InputFile** file,
                                        std::vector<Token>** tokens,
                                        scoped_ptr<ParseNode>** parse_root) {
-  InputFileData* data = new InputFileData(SourceFile());
+  InputFileData* data = new InputFileData(name);
   {
     base::AutoLock lock(lock_);
     dynamic_inputs_.push_back(data);
diff --git a/tools/gn/input_file_manager.h b/tools/gn/input_file_manager.h
index 5c5b5b0..82b3cc5 100644
--- a/tools/gn/input_file_manager.h
+++ b/tools/gn/input_file_manager.h
@@ -79,7 +79,8 @@ class InputFileManager : public base::RefCountedThreadSafe<InputFileManager> {
   // the values and lose context for error reporting, or somehow keep the
   // associated parse nodes, tokens, and file data in memory. This function
   // allows the latter.
-  void AddDynamicInput(InputFile** file,
+  void AddDynamicInput(const SourceFile& name,
+                       InputFile** file,
                        std::vector<Token>** tokens,
                        scoped_ptr<ParseNode>** parse_root);
 
diff --git a/tools/gn/variables.cc b/tools/gn/variables.cc
index 8e41e62..4b49f60 100644
--- a/tools/gn/variables.cc
+++ b/tools/gn/variables.cc
@@ -684,36 +684,32 @@ const char kPublic_HelpShort[] =
 const char kPublic_Help[] =
     "public: Declare public header files for a target.\n"
     "\n"
-    "  A list of files and patterns that other targets can include. These\n"
-    "  permissions are checked via the \"check\" command\n"
-    "  (see \"gn help check\").\n"
+    "  A list of files that other targets can include. These permissions are\n"
+    "  checked via the \"check\" command (see \"gn help check\").\n"
     "\n"
     "  If no public files are declared, other targets (assuming they have\n"
-    "  visibility to depend on this target) can include any file. If this\n"
-    "  variable is defined on a target, dependent targets may only include\n"
-    "  files on this whitelist.\n"
-    "\n"
-    "  The entries in this list are patterns (see \"gn help patterns\") so\n"
-    "  you can use simple wildcard matching if you have a directory of public\n"
-    "  files.\n"
+    "  visibility to depend on this target can include any file in the\n"
+    "  sources list. If this variable is defined on a target, dependent\n"
+    "  targets may only include files on this whitelist.\n"
     "\n"
     "  Header file permissions are also subject to visibility. A target\n"
     "  must be visible to another target to include any files from it at all\n"
     "  and the public headers indicate which subset of those files are\n"
-    "  permitted.\n"
+    "  permitted. See \"gn help visibility\" for more.\n"
     "\n"
     "  Public files are inherited through the dependency tree. So if there is\n"
     "  a dependency A -> B -> C, then A can include C's public headers.\n"
     "  However, the same is NOT true of visibility, so unless A is in C's\n"
     "  visibility list, the include will be rejected.\n"
     "\n"
+    "  GN only knows about files declared in the \"sources\" and \"public\"\n"
+    "  sections of targets. If a file is included that is now known to the\n"
+    "  build, it will be allowed.\n"
+    "\n"
     "Examples:\n"
     "  These exact files are public:\n"
     "    public = [ \"foo.h\", \"bar.h\" ]\n"
     "\n"
-    "  All files in the \"public\" directory are public:\n"
-    "    public = [ \"public/*\" ]\n"
-    "\n"
     "  No files are public (no targets may include headers from this one):\n"
     "    public = []\n";
 
diff --git a/tools/gn/visibility.cc b/tools/gn/visibility.cc
index c03ef56..41bfc63 100644
--- a/tools/gn/visibility.cc
+++ b/tools/gn/visibility.cc
@@ -78,11 +78,13 @@ bool Visibility::Set(const SourceDir& current_dir,
 }
 
 void Visibility::SetPublic() {
+  patterns_.clear();
   patterns_.push_back(
       VisPattern(VisPattern::RECURSIVE_DIRECTORY, SourceDir(), std::string()));
 }
 
 void Visibility::SetPrivate(const SourceDir& current_dir) {
+  patterns_.clear();
   patterns_.push_back(
       VisPattern(VisPattern::DIRECTORY, current_dir, std::string()));
 }