[rename] Use isPointWithin when looking for a declaration at location

[arphaman]

Summary

This patch is a followup to https://reviews.llvm.org/D50740 .

This patch fixes the issue where clang-refactor local-rename was unable to find a declaration in a header file if that header file was included more than once, and the location of the point of interest was in a different included instance of a file than then declaration itself. The use of isPointWithin ensures that we're checking if the point is in the source range regardless of the included file.

[jkorous]

LGTM Thanks for fixing this!

lib/Tooling/Refactoring/Rename/USRFinder.cpp

public:
   bool visitSymbolOccurrence(const NamedDecl *ND,
                              ArrayRef<SourceRange> NameRanges) {
     if (!ND)
       return true;
     for (const auto &Range : NameRanges) {
       SourceLocation Start = Range.getBegin();
       SourceLocation End = Range.getEnd();
       if (!Start.isValid() || !Start.isFileID() || !End.isValid() ||
-          !End.isFileID() || !isPointWithin(Start, End))
+          !End.isFileID() ||
+          !Context.getSourceManager().isPointWithin(Point, Start, End))
         return true;
     }
     Result = ND;
     return false;
   }
 
   const NamedDecl *getNamedDecl() const { return Result; }
 
 private:
-  // Determines if the Point is within Start and End.
-  bool isPointWithin(const SourceLocation Start, const SourceLocation End) {
-    // FIXME: Add tests for Point == End.
-    return Point == Start || Point == End ||
-           (Context.getSourceManager().isBeforeInTranslationUnit(Start,
-                                                                 Point) &&
-            Context.getSourceManager().isBeforeInTranslationUnit(Point, End));
-  }
 
   const NamedDecl *Result = nullptr;
   const SourceLocation Point; // The location to find the NamedDecl.
   const ASTContext &Context;
 };
 
 } // end anonymous namespace
 
 const NamedDecl *getNamedDeclAt(const ASTContext &Context,
                                 const SourceLocation Point) {
   const SourceManager &SM = Context.getSourceManager();
   NamedDeclOccurrenceFindingVisitor Visitor(Point, Context);
 
   // Try to be clever about pruning down the number of top-level declarations we
-  // see. If both start and end is either before or after the point we're
-  // looking for the point cannot be inside of this decl. Don't even look at it.
+  // see. If the range of the declaration doesn't contain the source location,
+  // then don't even look at it.
   for (auto *CurrDecl : Context.getTranslationUnitDecl()->decls()) {
-    SourceLocation StartLoc = CurrDecl->getBeginLoc();
-    SourceLocation EndLoc = CurrDecl->getEndLoc();
-    if (StartLoc.isValid() && EndLoc.isValid() &&
-        SM.isBeforeInTranslationUnit(StartLoc, Point) !=
-            SM.isBeforeInTranslationUnit(EndLoc, Point))
+    CharSourceRange DeclRange = Lexer::makeFileCharRange(
+        CharSourceRange::getTokenRange(CurrDecl->getSourceRange()),
+        Context.getSourceManager(), Context.getLangOpts());
+    // FIXME: Use early break if the decl is found.
+    if (DeclRange.isValid() &&
+        SM.isPointWithin(Point, DeclRange.getBegin(), DeclRange.getEnd()))
       Visitor.TraverseDecl(CurrDecl);
   }
 
   return Visitor.getNamedDecl();
 }
 
 namespace {
 

test/Refactor/LocalRename/Inputs/MultiHeader.h

+#ifdef HAVE_DECLS
+
+struct MyStruct {
+  struct Foo {
+    int field;
+  } bar;
+};
+
+#endif

test/Refactor/LocalRename/IsPointWithinDifferentInclude.c

+// RUN: clang-refactor local-rename -selection=%S/Inputs/MultiHeader.h:5:9 -new-name=renamedField %s -- -I %S/Inputs 2>&1 | FileCheck %s
+
+// The MultiHeader file included twice.
+// The first inclusion contains no declarations. However, the selection
+// "MultiHeader:5:9" is mapped to a location in this inclusion of the file.
+#include "MultiHeader.h"
+
+// The second inclusion contains the declarations. The range of the
+// declaration we would like is located in this inclusion.
+#define HAVE_DECLS
+#include "MultiHeader.h"
+
+// Ensure that we still find the declaration even though the location and the
+// declaration's range are located in two different inclusions.
+// CHECK:      struct Foo {
+// CHECK-NEXT:   int renamedField;