Fix pragma: private in relative includes

When a header was included via a ""-style include relative to the
including header, and that header was marked private, the privateness
wasn't noticed, because the conversion to quoted include was variable
(it could be used as-written, or converted from a path with or without
the context of an including file).

Add a new visibility map for paths rather than quoted includes so these
can be tracked thereby.

7 files changed, 158 insertions, 27 deletions

diff --git a/iwyu_include_picker.cc b/iwyu_include_picker.cc
index 4aa2bd9..5129066 100644
--- a/iwyu_include_picker.cc
+++ b/iwyu_include_picker.cc
@@ -1045,6 +1045,14 @@ MappedInclude::MappedInclude(const string& q, const string& p)
     "Must be quoted include, was: " << quoted_include;
 }
 
+bool MappedInclude::HasAbsoluteQuotedInclude() const {
+  if (!StartsWith(quoted_include, "\"") || quoted_include.size() < 2) {
+    return false;
+  }
+  string path(quoted_include.begin() + 1, quoted_include.end() - 1);
+  return IsAbsolutePath(path);
+}
+
 IncludePicker::IncludePicker(bool no_default_mappings)
     : has_called_finalize_added_include_lines_(false) {
   if (!no_default_mappings) {
@@ -1192,6 +1200,11 @@ void IncludePicker::MarkIncludeAsPrivate(
   MarkVisibility(&include_visibility_map_, quoted_filepath_pattern, kPrivate);
 }
 
+void IncludePicker::MarkPathAsPrivate(const string& path) {
+  CHECK_(!has_called_finalize_added_include_lines_ && "Can't mutate anymore");
+  MarkVisibility(&path_visibility_map_, path, kPrivate);
+}
+
 void IncludePicker::AddFriendRegex(const string& includee_filepath,
                                    const string& quoted_friend_regex) {
   friend_to_headers_map_["@" + quoted_friend_regex].insert(includee_filepath);
@@ -1301,8 +1314,7 @@ vector<MappedInclude> IncludePicker::GetPublicValues(
 
   for (const MappedInclude& value : *values) {
     CHECK_(!StartsWith(value.quoted_include, "@"));
-    if (GetOrDefault(include_visibility_map_, value.quoted_include, kPublic)
-        == kPublic)
+    if (GetVisibility(value, kPublic) == kPublic)
       retval.push_back(value);
   }
   return retval;
@@ -1332,14 +1344,20 @@ vector<string> IncludePicker::GetCandidateHeadersForSymbol(
 vector<MappedInclude> IncludePicker::GetCandidateHeadersForFilepath(
     const string& filepath, const string& including_filepath) const {
   CHECK_(has_called_finalize_added_include_lines_ && "Must finalize includes");
-  const string quoted_header = ConvertToQuotedInclude(
-      filepath, MakeAbsolutePath(GetParentPath(including_filepath)));
+  string quoted_header;
+  if (including_filepath.empty()) {
+    quoted_header = ConvertToQuotedInclude(filepath);
+  } else {
+    quoted_header = ConvertToQuotedInclude(
+        filepath, MakeAbsolutePath(GetParentPath(including_filepath)));
+  }
   vector<MappedInclude> retval =
       GetPublicValues(filepath_include_map_, quoted_header);
+
   // We also need to consider the header itself.  Make that an option if it's
   // public or there's no other option.
   MappedInclude default_header(quoted_header, filepath);
-  if (retval.empty() || GetVisibility(quoted_header, kPublic) == kPublic) {
+  if (retval.empty() || GetVisibility(default_header, kPublic) == kPublic) {
     // Insert at front so it's the preferred option
     retval.insert(retval.begin(), default_header);
   }
@@ -1355,10 +1373,12 @@ vector<string> IncludePicker::GetCandidateHeadersForFilepathIncludedFrom(
   // We pass the own files path to ConvertToQuotedInclude so the quoted include
   // for the case that there is no matching `-I` option is just the filename
   // (e.g. "foo.cpp") instead of the absolute file path.
+  const string including_path =
+      MakeAbsolutePath(GetParentPath(including_filepath));
   const string quoted_includer = ConvertToQuotedInclude(
-      including_filepath, MakeAbsolutePath(GetParentPath(including_filepath)));
+      including_filepath, including_path);
   const string quoted_includee = ConvertToQuotedInclude(
-      included_filepath, MakeAbsolutePath(GetParentPath(including_filepath)));
+      included_filepath, including_path);
   const set<string>* headers_with_includer_as_friend =
       FindInMap(&friend_to_headers_map_, quoted_includer);
   if (headers_with_includer_as_friend != nullptr &&
@@ -1369,11 +1389,10 @@ vector<string> IncludePicker::GetCandidateHeadersForFilepathIncludedFrom(
     mapped_includes =
         GetCandidateHeadersForFilepath(included_filepath, including_filepath);
     if (mapped_includes.size() == 1) {
-      const string& quoted_header = mapped_includes[0].quoted_include;
-      if (GetVisibility(quoted_header) == kPrivate) {
+      if (GetVisibility(mapped_includes[0]) == kPrivate) {
         VERRS(0) << "Warning: "
                  << "No public header found to replace the private header "
-                 << quoted_header << "\n";
+                 << included_filepath << "\n";
       }
     }
   }
@@ -1385,14 +1404,21 @@ vector<string> IncludePicker::GetCandidateHeadersForFilepathIncludedFrom(
   // ConvertToQuotedInclude because it avoids trouble when the same
   // file is accessible via different include search-paths, or is
   // accessed via a symlink.
+  // We also try to recalculate any quoted includes using absolute paths,
+  // because they can likely be converted to an include relative to the
+  // current file.
   vector<string> retval;
   for (MappedInclude& mapped_include : mapped_includes) {
     const string& quoted_include_as_written =
       MaybeGetIncludeNameAsWritten(including_filepath, mapped_include.path);
-    if (quoted_include_as_written.empty()) {
-      retval.push_back(mapped_include.quoted_include);
-    } else {
+    if (!quoted_include_as_written.empty()) {
       retval.push_back(quoted_include_as_written);
+    } else if (mapped_include.HasAbsoluteQuotedInclude() &&
+               !mapped_include.path.empty()) {
+      retval.push_back(ConvertToQuotedInclude(mapped_include.path,
+                                              including_path));
+    } else {
+      retval.push_back(mapped_include.quoted_include);
     }
   }
   return retval;
@@ -1416,8 +1442,10 @@ bool IncludePicker::HasMapping(const string& map_from_filepath,
 
 bool IncludePicker::IsPublic(const clang::FileEntry* file) const {
   CHECK_(file && "Need existing FileEntry");
-  const string quoted_file = ConvertToQuotedInclude(GetFilePath(file));
-  return (GetVisibility(quoted_file) == kPublic);
+  const string path = GetFilePath(file);
+  const string quoted_file = ConvertToQuotedInclude(path);
+  const MappedInclude include(quoted_file, path);
+  return (GetVisibility(include) == kPublic);
 }
 
 // Parses a YAML/JSON file containing mapping directives of various types.
@@ -1587,9 +1615,13 @@ IncludeVisibility IncludePicker::ParseVisibility(
 }
 
 IncludeVisibility IncludePicker::GetVisibility(
-    const string& quoted_include, IncludeVisibility default_value) const {
-  return GetOrDefault(
-      include_visibility_map_, quoted_include, default_value);
+    const MappedInclude& include, IncludeVisibility default_value) const {
+  const IncludeVisibility* include_visibility =
+    FindInMap(&include_visibility_map_, include.quoted_include);
+  if (include_visibility) {
+    return *include_visibility;
+  }
+  return GetOrDefault(path_visibility_map_, include.path, default_value);
 }
 
 }  // namespace include_what_you_use
diff --git a/iwyu_include_picker.h b/iwyu_include_picker.h
index 905a559..75b135e 100644
--- a/iwyu_include_picker.h
+++ b/iwyu_include_picker.h
@@ -76,6 +76,8 @@ struct MappedInclude {
 
   string quoted_include;
   string path;
+
+  bool HasAbsoluteQuotedInclude() const;
 };
 
 class IncludePicker {
@@ -85,7 +87,7 @@ class IncludePicker {
   typedef map<string, vector<MappedInclude>> IncludeMap;
 
   // Used to track visibility as specified either in mapping files or via
-  // pragmas.  The keys are quoted includes.  The values are the
+  // pragmas.  The keys are quoted includes or paths.  The values are the
   // visibility of the respective files.
   typedef map<string, IncludeVisibility> VisibilityMap;
 
@@ -109,6 +111,11 @@ class IncludePicker {
   // mappings to map such includes to public (not-private) includes.
   void MarkIncludeAsPrivate(const string& quoted_include);
 
+  // Indicate that the given path should be considered
+  // a "private" include.  If possible, we use the include-picker
+  // mappings to map such includes to public (not-private) includes.
+  void MarkPathAsPrivate(const string& path);
+
   // Add this to say that "any file whose name matches the
   // friend_regex is allowed to include includee_filepath".  The regex
   // uses the POSIX Entended Regular Expression syntax and should
@@ -209,10 +216,10 @@ class IncludePicker {
   // string is not recognized.
   IncludeVisibility ParseVisibility(const string& visibility) const;
 
-  // Return the visibility of a given quoted_include if known, else
+  // Return the visibility of a given mapped include if known, else
   // kUnusedVisibility.
   IncludeVisibility GetVisibility(
-      const string& quoted_include,
+      const MappedInclude&,
       IncludeVisibility default_value = kUnusedVisibility) const;
 
   // For the given key, return the vector of values associated with
@@ -239,9 +246,16 @@ class IncludePicker {
   IncludeMap filepath_include_map_;
 
   // A map of all quoted-includes to whether they're public or private.
-  // Quoted-includes that are not present in this map are assumed public.
+  // Files whose visibility cannot be determined by this map nor the one
+  // below are assumed public.
   VisibilityMap include_visibility_map_;
 
+  // A map of paths to whether they're public or private.
+  // Files whose visibility cannot be determined by this map nor the one
+  // above are assumed public.
+  // The include_visibility_map_ takes priority over this one.
+  VisibilityMap path_visibility_map_;
+
   // All the includes we've seen so far, to help with globbing and
   // other dynamic mapping.  For each file, we list who #includes it.
   map<string, set<string>> quoted_includes_to_quoted_includers_;
diff --git a/iwyu_preprocessor.cc b/iwyu_preprocessor.cc
index a70e420..ff93d39 100644
--- a/iwyu_preprocessor.cc
+++ b/iwyu_preprocessor.cc
@@ -239,11 +239,10 @@ void IwyuPreprocessorInfo::HandlePragmaComment(SourceRange comment_range) {
   }
 
   if (MatchOneToken(tokens, "private", 1, begin_loc)) {
-    const string quoted_this_file
-        = ConvertToQuotedInclude(GetFilePath(begin_loc));
-    MutableGlobalIncludePicker()->MarkIncludeAsPrivate(quoted_this_file);
-    ERRSYM(this_file_entry) << "Adding private include: "
-                                    << quoted_this_file << "\n";
+    const string path_this_file = GetFilePath(begin_loc);
+    MutableGlobalIncludePicker()->MarkPathAsPrivate(path_this_file);
+    ERRSYM(this_file_entry) << "Adding private path: "
+                            << path_this_file << "\n";
     return;
   }
 
diff --git a/tests/cxx/export_private_near.h b/tests/cxx/export_private_near.h
new file mode 100644
index 0000000..eab0c8b
--- /dev/null
+++ b/tests/cxx/export_private_near.h
@@ -0,0 +1,20 @@
+//===--- export_private_near.h - test input file for iwyu -----------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// This file is meant to be a generic exporting header.  Similarly to
+// export_near.h, it exports another header.  However, unlike that file, this
+// one exports private.h, which is marked private, and therefore IWYU should
+// not allow you to directly include private.h.
+
+#ifndef INCLUDE_WHAT_YOU_USE_TESTS_CXX_EXPORT_PRIVATE_NEAR_H_
+#define INCLUDE_WHAT_YOU_USE_TESTS_CXX_EXPORT_PRIVATE_NEAR_H_
+
+#include "private.h" // IWYU pragma: export
+
+#endif  // INCLUDE_WHAT_YOU_USE_TESTS_CXX_EXPORT_PRIVATE_NEAR_H_
diff --git a/tests/cxx/private.h b/tests/cxx/private.h
new file mode 100644
index 0000000..802d42d
--- /dev/null
+++ b/tests/cxx/private.h
@@ -0,0 +1,22 @@
+//===--- private.h - test input file for iwyu -----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// This file is meant to be #included by "export_private_near.h".  The pragmas
+// in these two headers mean that export_private.h is the normal header to
+// include to access PrivateClass, rather than including this file directly.
+
+#ifndef INCLUDE_WHAT_YOU_USE_TESTS_CXX_PRIVATE_H_
+#define INCLUDE_WHAT_YOU_USE_TESTS_CXX_PRIVATE_H_
+
+// IWYU pragma: private
+
+class PrivateClass {
+};
+
+#endif  // INCLUDE_WHAT_YOU_USE_TESTS_CXX_PRIVATE_H_
diff --git a/tests/cxx/relative_include_of_export_added-d1.h b/tests/cxx/relative_include_of_export_added-d1.h
new file mode 100644
index 0000000..8093ef5
--- /dev/null
+++ b/tests/cxx/relative_include_of_export_added-d1.h
@@ -0,0 +1,15 @@
+//===--- relative_include_of_export_added-d1.h - test input file for iwyu -===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef INCLUDE_WHAT_YOU_USE_TESTS_CXX_RELATIVE_INCLUDE_OF_EXPORT_ADDED_D1_H_
+#define INCLUDE_WHAT_YOU_USE_TESTS_CXX_RELATIVE_INCLUDE_OF_EXPORT_ADDED_D1_H_
+
+#include "export_private_near.h"
+
+#endif // INCLUDE_WHAT_YOU_USE_TESTS_CXX_RELATIVE_INCLUDE_OF_EXPORT_ADDED_D1_H_
diff --git a/tests/cxx/relative_include_of_export_added.cc b/tests/cxx/relative_include_of_export_added.cc
new file mode 100644
index 0000000..8cec51e
--- /dev/null
+++ b/tests/cxx/relative_include_of_export_added.cc
@@ -0,0 +1,29 @@
+//===--- relative_include_of_export_added.cc - test input file for iwyu ---===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// The purpose of this test is to ensure that when an include is added which is
+// the public counterpart to a private header, that header can be added as a
+// relative include rather than using a full path.
+#include "relative_include_of_export_added-d1.h"
+
+// IWYU: PrivateClass is...*"export_private_near.h"
+PrivateClass x;
+
+/**** IWYU_SUMMARY
+
+tests/cxx/relative_include_of_export_added.cc should add these lines:
+#include "export_private_near.h"
+
+tests/cxx/relative_include_of_export_added.cc should remove these lines:
+- #include "relative_include_of_export_added-d1.h"  // lines XX-XX
+
+The full include-list for tests/cxx/relative_include_of_export_added.cc:
+#include "export_private_near.h"  // for PrivateClass
+
+***** IWYU_SUMMARY */