Improve public header file checking

tools/gn/header_checker.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 "tools/gn/header_checker.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "tools/gn/build_settings.h"
 #include "tools/gn/builder.h"
 #include "tools/gn/c_include_iterator.h"
 #include "tools/gn/err.h"
 #include "tools/gn/filesystem_utils.h"
 #include "tools/gn/scheduler.h"
 #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()),
       build_settings_(build_settings) {
   for (size_t i = 0; i < targets.size(); i++)
     AddTargetToFileMap(targets[i]);
 }
 
 HeaderChecker::~HeaderChecker() {
 }
@@ skipping 98 matching lines @@
 
   base::FilePath path = build_settings_->GetFullPath(file);
   std::string contents;
   if (!base::ReadFileToString(path, &contents)) {
     *err = Err(from_target->defined_from(), "Source file not found.",
         "This target includes as a source:\n  " + file.value() +
         "\nwhich was not found.");
     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;
   }
 
   return true;
 }
 
 // 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
   // our include finder is too primitive and returns all includes, even if
   // they're in a #if not executed in the current build. In that case, it's
   // not unusual for the buildfiles to not specify that header at all.
   FileMap::const_iterator found = file_map_.find(include_file);
   if (found == file_map_.end())
     return true;
 
   const TargetVector& targets = found->second;
 
   // 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
   // must have the file listed in the public section.
   bool found_dependency = false;
   for (size_t i = 0; i < targets.size(); i++) {
     // We always allow source files in a target to include headers also in that
     // target.
     if (targets[i].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;
     }
   }
 
   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;
   }
 
   return true;
 }
 
 bool HeaderChecker::IsDependencyOf(const Target* search_for,
                                    const Target* search_from) const {
   std::set<const Target*> checked;
   return IsDependencyOf(search_for, search_from, &checked);
@@ skipping 11 matching lines @@
       return true;  // Found it.
 
     // Recursive search.
     checked->insert(deps[i].ptr);
     if (IsDependencyOf(search_for, deps[i].ptr, checked))
       return true;
   }
 
   return false;
 }