Fix #127: Improve macro location logic

- Rename GetUseLocationForMacroExpansion -> GetCanonicalUseLocation
- Let macro authors forward-declare symbols to push responsibility
  to expansion
- Symbols passed as arguments to macros are now attributed to expansion

Second commit attempt, with explicit use of spelling-location for
uses attributed to macro definition.

5 files changed, 93 insertions, 46 deletions

diff --git a/iwyu.cc b/iwyu.cc
index 24d0d00..4c859b0 100644
--- a/iwyu.cc
+++ b/iwyu.cc
@@ -431,17 +431,7 @@ class BaseAstVisitor : public RecursiveASTVisitor<Derived> {
 
   SourceLocation CurrentLoc() const {
     CHECK_(current_ast_node_ && "Call CurrentLoc within Visit* or Traverse*");
-    const SourceLocation loc = current_ast_node_->GetLocation();
-    if (!loc.isValid())
-      return loc;
-    // If the token is formed via macro concatenation, the spelling
-    // location will be in <scratch space>.  Use the instantiation
-    // location instead.
-    const SourceLocation spelling_loc = GetSpellingLoc(loc);
-    if (IsInMacro(loc) && StartsWith(PrintableLoc(spelling_loc), "<scratch "))
-      return GetInstantiationLoc(loc);
-    else
-      return spelling_loc;
+    return current_ast_node_->GetLocation();
   }
 
   string CurrentFilePath() const {
@@ -1380,30 +1370,72 @@ class IwyuBaseAstVisitor : public BaseAstVisitor<Derived> {
     return type;
   }
 
-  // Re-assign uses from macro authors to macro callers when possible.
-  // With macros, it can be very difficult to tell what types are the
-  // responsibility of the macro-caller, and which the macro-author.  e.g.
-  //    #define SIZE_IN_FOOS(ptr)  ( sizeof(*ptr) / sizeof(Foo) )
-  // Here the type of *ptr is the responsibility of the caller, while the
-  // type of Foo is the responsibility of the author, even though the
-  // Exprs for *ptr and Foo are both located inside the macro (the Expr
-  // for ptr is located at the macro-calling site, but not for *ptr).
-  // To help us guess the owner, we use this rule: if the macro-file
-  // intends-to-provide the type, then we keep ownership of the type
-  // at the macro.  Otherwise, we assume it's with the caller.  This
-  // works well as long as the file defining the macro is well-behaved.
-  //   This function should be called with a use-loc that is within
-  // an expanded macro (so the use-loc will point to either inside a
-  // macro definition, or to an argument to a macro call).  If it
-  // points within a macro definition, and that macro-definition file
-  // does not mean to re-export the symbol being used, then we reassign
-  // use of the decl to the macro-caller.
-  SourceLocation GetUseLocationForMacroExpansion(SourceLocation use_loc,
-                                                 const Decl* used_decl) {
-    CHECK_(IsInMacro(use_loc) && "Unexpected non-macro-expansion call");
-    if (!preprocessor_info().PublicHeaderIntendsToProvide(
-            GetFileEntry(use_loc), GetFileEntry(used_decl)))
-      return GetInstantiationLoc(use_loc);
+  // Return any forward-declare of decl earlier in the same file as loc.
+  const NamedDecl* GetForwardDeclareInSameFile(const Decl* decl,
+                                               SourceLocation loc) {
+    set<const NamedDecl*> redecls = GetClassRedecls(DynCastFrom(decl));
+    for (const NamedDecl* redecl : redecls) {
+      if (IsBeforeInSameFile(redecl, loc)) {
+        return redecl;
+      }
+    }
+
+    return nullptr;
+  }
+
+  // Get the canonical use location for a (location, decl) pair.
+  // Decide whether the file expanding the macro or the file defining the macro
+  // should be held responsible for a use.
+  SourceLocation GetCanonicalUseLocation(SourceLocation use_loc,
+                                         const NamedDecl* decl) {
+    // If we're not in a macro, just echo the use location.
+    if (!IsInMacro(use_loc))
+      return use_loc;
+
+    // If we're in a macro, and the macro file forward-declares decl, make sure
+    // we keep that forward-declaration.
+    const NamedDecl* fwd_decl = GetForwardDeclareInSameFile(decl, use_loc);
+    if (fwd_decl) {
+      const FileEntry* used_in = GetFileEntry(use_loc);
+      preprocessor_info().FileInfoFor(used_in)->ReportForwardDeclareUse(
+          use_loc, decl, IsNodeInsideCXXMethodBody(current_ast_node()),
+          nullptr);
+    }
+
+    // Resolve the best use location based on our current knowledge.
+    //
+    // 1) If the macro definition file forward-declares the used decl, that's a
+    //    hint that it wants the expansion location to take responsibility.
+    // 2) If the symbol being used is passed as an argument to the macro, we
+    //    want the expansion location to take responsibility (that thing they
+    //    passed in is something they know about, and the macro doesn't.)
+    // 3) If the use_loc is in <scratch space>, we assume it's formed by macro
+    //    argument concatenation, and attribute responsibility to the expansion
+    //    location.
+    //
+    // Otherwise, the macro file is responsible for including the symbol.
+    SourceLocation expansion_loc = GetInstantiationLoc(use_loc);
+    const char* responsible = "expansion";
+
+    if (fwd_decl != nullptr) {
+      VERRS(5) << "Macro file has a forward-decl attribution hint.\n";
+      use_loc = expansion_loc;
+    } else if (IsInScratchSpace(use_loc)) {
+      VERRS(5) << "Decl was used in <scratch space>, presumably as a result of "
+                  "macro arg concatenation.\n";
+      use_loc = expansion_loc;
+    } else if (GlobalSourceManager()->isMacroArgExpansion(use_loc)) {
+      VERRS(5) << "Use location is a macro arg expansion.\n";
+      use_loc = expansion_loc;
+    } else {
+      use_loc = GetSpellingLoc(use_loc);
+      responsible = "definition";
+    }
+
+    VERRS(5) << "Attributing " << decl->getNameAsString() << " use to macro "
+             << responsible << " location at " << PrintableLoc(use_loc)
+             << ".\n";
+
     return use_loc;
   }
 
@@ -1621,11 +1653,9 @@ class IwyuBaseAstVisitor : public BaseAstVisitor<Derived> {
     if (CanIgnoreDecl(target_decl))
       return;
 
-    // Figure out the best location to attribute uses inside macros.
-    if (IsInMacro(used_loc))
-      used_loc = GetUseLocationForMacroExpansion(used_loc, target_decl);
+    // Canonicalize the use location and report the use.
+    used_loc = GetCanonicalUseLocation(used_loc, target_decl);
     const FileEntry* used_in = GetFileEntry(used_loc);
-
     preprocessor_info().FileInfoFor(used_in)->ReportFullSymbolUse(
         used_loc, target_decl, IsNodeInsideCXXMethodBody(current_ast_node()),
         comment);
@@ -1694,11 +1724,9 @@ class IwyuBaseAstVisitor : public BaseAstVisitor<Derived> {
     if (CanIgnoreDecl(target_decl))
       return;
 
-    // Figure out the best location to attribute uses inside macros.
-    if (IsInMacro(used_loc))
-      used_loc = GetUseLocationForMacroExpansion(used_loc, target_decl);
+    // Canonicalize the use location and report the use.
+    used_loc = GetCanonicalUseLocation(used_loc, target_decl);
     const FileEntry* used_in = GetFileEntry(used_loc);
-
     preprocessor_info().FileInfoFor(used_in)->ReportForwardDeclareUse(
         used_loc, target_decl, IsNodeInsideCXXMethodBody(current_ast_node()),
         comment);
diff --git a/iwyu_location_util.cc b/iwyu_location_util.cc
index ccd6e3e..7a5fcc7 100644
--- a/iwyu_location_util.cc
+++ b/iwyu_location_util.cc
@@ -167,4 +167,8 @@ SourceLocation GetLocation(const clang::TemplateArgumentLoc* argloc) {
   return argloc->getLocation();
 }
 
+bool IsInScratchSpace(SourceLocation loc) {
+  return PrintableLoc(GetSpellingLoc(loc)) == "<scratch space>";
+}
+
 }  // namespace include_what_you_use
diff --git a/iwyu_location_util.h b/iwyu_location_util.h
index d65ddb2..af2b772 100644
--- a/iwyu_location_util.h
+++ b/iwyu_location_util.h
@@ -82,6 +82,12 @@ inline bool IsBuiltinOrCommandLineFile(const clang::FileEntry* file) {
   return IsBuiltinFile(file) || (!strcmp(file->getName(), "<command line>"));
 }
 
+// When macro args are concatenated e.g. '#define CAT(A, B) A##B', their
+// location ends up outside the source text, in what the compiler calls
+// "<scratch space>".
+// This function returns true if the provided loc is in scratch space.
+bool IsInScratchSpace(clang::SourceLocation loc);
+
 inline string GetFilePath(const clang::FileEntry* file) {
   return (IsBuiltinFile(file) ? "<built-in>" :
           NormalizeFilePath(file->getName()));
diff --git a/tests/cxx/macro_location-d2.h b/tests/cxx/macro_location-d2.h
index 7b57259..5d64cd7 100644
--- a/tests/cxx/macro_location-d2.h
+++ b/tests/cxx/macro_location-d2.h
@@ -9,6 +9,12 @@
 
 #include "tests/cxx/macro_location-d1.h"
 
+// The forward-declare is a hint that the use should be attributed
+// to users of the DECLARE_INDIRECT macro, not this file.
+class IndirectClass;
+
+#define DECLARE_INDIRECT(name) IndirectClass name;
+
 #define ARRAYSIZE(x)  ( sizeof(x) / sizeof(*(x)) )
 
 #define NEW_CLASS(name)                         \
diff --git a/tests/cxx/macro_location.h b/tests/cxx/macro_location.h
index 61914c1..007b2f6 100644
--- a/tests/cxx/macro_location.h
+++ b/tests/cxx/macro_location.h
@@ -6,7 +6,7 @@
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
-
+#include "tests/cxx/indirect.h"
 #include "tests/cxx/macro_location-d2.h"
 #include "tests/cxx/macro_location-d3.h"
 
@@ -23,7 +23,9 @@ NEW_CLASS(Bar);
 USE_CLASS(HClass);
 // This shouldn't cause macro_location-d1.h to need to include us for HClass.
 CREATE_VAR(HClass);
-
+// This should force us to #include a definition of IndirectClass, because it's
+// forward-declared by DECLARE_INDIRECT's file.
+DECLARE_INDIRECT(global_indirect);
 
 /**** IWYU_SUMMARY
 
@@ -35,7 +37,8 @@ tests/cxx/macro_location.h should remove these lines:
 - class Foo;  // lines XX-XX
 
 The full include-list for tests/cxx/macro_location.h:
-#include "tests/cxx/macro_location-d2.h"  // for ARRAYSIZE, CREATE_VAR, NEW_CLASS, USE_CLASS
+#include "tests/cxx/indirect.h"  // for IndirectClass
+#include "tests/cxx/macro_location-d2.h"  // for ARRAYSIZE, CREATE_VAR, DECLARE_INDIRECT, NEW_CLASS, USE_CLASS
 #include "tests/cxx/macro_location-i3.h"  // for Foo