[SourceKit] Address FIXMEs for the NameMatcher

rdar://118996561

Author: ahoppen

Diff for: include/swift/IDE/IDEBridging.h

@@ -64,8 +64,6 @@ struct ResolvedLoc {
   /// The range of the call's base name.
   swift::CharSourceRange range;
 
-  // FIXME: (NameMatcher) We should agree on whether `labelRanges` contains the
-  // colon or not
   /// The range of the labels.
   ///
   /// What the label range contains depends on the `labelRangeType`:
@@ -75,6 +73,9 @@ struct ResolvedLoc {
   ///   the trivia on their sides
   /// - For function arguments that don't have a label, this is an empty range
   ///   that points to the start of the argument (exculding trivia).
+  ///
+  /// See documentation on `DeclNameLocation.Argument` in swift-syntax for more
+  /// background.
   std::vector<swift::CharSourceRange> labelRanges;
 
   /// The in index in `labelRanges` that belongs to the first trailing closure

Diff for: include/swift/Refactoring/Refactoring.h

@@ -87,6 +87,12 @@ class RenameLocs {
 RenameLocs localRenameLocs(SourceFile *sourceFile, const ValueDecl *valueDecl);
 
 #if SWIFT_BUILD_SWIFT_SYNTAX
+/// A `RenameLoc` together with the `ResolvedLoc` that it resolved to.
+struct ResolvedAndRenameLoc {
+  RenameLoc renameLoc;
+  ResolvedLoc resolved;
+};
+
 /// Given a list of `RenameLoc`s, get the corresponding `ResolveLoc`s.
 ///
 /// These resolve locations contain more structured information, such as the
@@ -95,10 +101,9 @@ RenameLocs localRenameLocs(SourceFile *sourceFile, const ValueDecl *valueDecl);
 /// If a \p newName is passed, it is used to verify that all \p renameLocs can
 /// be renamed to this name. If any the names cannot be renamed, an empty vector
 /// is returned and the issue is diagnosed via \p diags.
-std::vector<ResolvedLoc> resolveRenameLocations(ArrayRef<RenameLoc> renameLocs,
-                                                StringRef newName,
-                                                SourceFile &sourceFile,
-                                                DiagnosticEngine &diags);
+std::vector<ResolvedAndRenameLoc>
+resolveRenameLocations(ArrayRef<RenameLoc> renameLocs, StringRef newName,
+                       SourceFile &sourceFile, DiagnosticEngine &diags);
 #endif
 
 struct RangeConfig {

Diff for: lib/Refactoring/SyntacticRename.cpp

@@ -22,7 +22,7 @@ using namespace swift;
 using namespace swift::ide;
 
 #if SWIFT_BUILD_SWIFT_SYNTAX
-std::vector<ResolvedLoc>
+std::vector<ResolvedAndRenameLoc>
 swift::ide::resolveRenameLocations(ArrayRef<RenameLoc> RenameLocs,
                                    StringRef NewName, SourceFile &SF,
                                    DiagnosticEngine &Diags) {
@@ -72,6 +72,8 @@ swift::ide::resolveRenameLocations(ArrayRef<RenameLoc> RenameLocs,
     UnresolvedLocs.push_back({Location});
   }
 
+  assert(UnresolvedLocs.size() == RenameLocs.size());
+
   std::vector<BridgedSourceLoc> BridgedUnresolvedLocs;
   BridgedUnresolvedLocs.reserve(UnresolvedLocs.size());
   for (SourceLoc Loc : UnresolvedLocs) {
@@ -85,26 +87,28 @@ swift::ide::resolveRenameLocations(ArrayRef<RenameLoc> RenameLocs,
   const std::vector<ResolvedLoc> &resolvedLocsInSourceOrder =
       bridgedResolvedLocs.takeUnbridged();
 
-  // Callers expect the resolved locs in the same order as the unresolved locs.
-  // Sort them.
-  // FIXME: (NameMatcher) Can we change the callers to not rely on this?
-  std::vector<ResolvedLoc> resolvedLocsInRequestedOrder;
-  for (SourceLoc unresolvedLoc : UnresolvedLocs) {
-    auto found =
-        llvm::find_if(resolvedLocsInSourceOrder,
-                      [unresolvedLoc](const ResolvedLoc &resolved) {
-                        return resolved.range.getStart() == unresolvedLoc;
-                      });
+  // Callers need to corrolate the `ResolvedLoc` with the `RenameLoc` that they
+  // originated from. Match them.
+  std::vector<ResolvedAndRenameLoc> resolvedAndRenameLocs;
+  for (auto [unresolvedLoc, renameLoc] :
+       llvm::zip_equal(UnresolvedLocs, RenameLocs)) {
+    auto found = llvm::find_if(
+        resolvedLocsInSourceOrder,
+        [unresolvedLoc = unresolvedLoc](const ResolvedLoc &resolved) {
+          return resolved.range.getStart() == unresolvedLoc;
+        });
+    ResolvedLoc resolvedLoc;
     if (found == resolvedLocsInSourceOrder.end()) {
-      resolvedLocsInRequestedOrder.push_back(
+      resolvedLoc =
           ResolvedLoc(CharSourceRange(),
                       /*LabelRanges=*/{}, llvm::None, LabelRangeType::None,
-                      /*IsActive=*/true, ResolvedLocContext::Comment));
+                      /*IsActive=*/true, ResolvedLocContext::Comment);
     } else {
-      resolvedLocsInRequestedOrder.push_back(*found);
+      resolvedLoc = *found;
     }
+    resolvedAndRenameLocs.push_back({renameLoc, resolvedLoc});
   }
-  return resolvedLocsInRequestedOrder;
+  return resolvedAndRenameLocs;
 }
 #endif
 
@@ -126,21 +130,19 @@ swift::ide::findSyntacticRenameRanges(SourceFile *SF,
   swift::PrintingDiagnosticConsumer DiagConsumer(DiagOS);
   DiagEngine.addConsumer(DiagConsumer);
 
-  auto ResolvedLocs =
+  auto ResolvedAndRenameLocs =
       resolveRenameLocations(RenameLocs, NewName, *SF, DiagEngine);
-  if (ResolvedLocs.size() != RenameLocs.size() || DiagConsumer.didErrorOccur())
+  if (ResolvedAndRenameLocs.size() != RenameLocs.size() ||
+      DiagConsumer.didErrorOccur())
     return ResultType::failure(ErrBuffer);
 
   std::vector<SyntacticRenameRangeDetails> Result;
-  size_t index = 0;
-  for (const RenameLoc &Rename : RenameLocs) {
-    ResolvedLoc &Resolved = ResolvedLocs[index++];
-
-    SyntacticRenameRangeDetails Details =
-        getSyntacticRenameRangeDetails(SM, Rename.OldName, Resolved, Rename);
+  for (const ResolvedAndRenameLoc &Loc : ResolvedAndRenameLocs) {
+    SyntacticRenameRangeDetails Details = getSyntacticRenameRangeDetails(
+        SM, Loc.renameLoc.OldName, Loc.resolved, Loc.renameLoc);
     if (Details.Type == RegionType::Mismatch) {
-      DiagEngine.diagnose(Resolved.range.getStart(), diag::mismatched_rename,
-                          NewName);
+      DiagEngine.diagnose(Loc.resolved.range.getStart(),
+                          diag::mismatched_rename, NewName);
       Result.emplace_back(SyntacticRenameRangeDetails{Details.Type, {}});
     } else {
       Result.push_back(Details);

Diff for: tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

@@ -2565,20 +2565,19 @@ void SwiftLangSupport::findRelatedIdentifiersInFile(
       // symbols. This makes related idents more fault-tolerant.
       DiagnosticEngine Diags(SrcMgr);
 
-      std::vector<ResolvedLoc> ResolvedLocs = resolveRenameLocations(
-          Locs.getLocations(), /*NewName=*/StringRef(), *SrcFile, Diags);
+      std::vector<ResolvedAndRenameLoc> ResolvedAndRenameLocs =
+          resolveRenameLocations(Locs.getLocations(), /*NewName=*/StringRef(),
+                                 *SrcFile, Diags);
 
       SmallVector<RelatedIdentInfo, 8> Ranges;
-      assert(ResolvedLocs.size() == Locs.getLocations().size());
-      for (auto [RenameLoc, ResolvedLoc] :
-           llvm::zip_equal(Locs.getLocations(), ResolvedLocs)) {
-        if (ResolvedLoc.range.isInvalid()) {
+      for (auto Loc : ResolvedAndRenameLocs) {
+        if (Loc.resolved.range.isInvalid()) {
           continue;
         }
-        unsigned Offset =
-            SrcMgr.getLocOffsetInBuffer(ResolvedLoc.range.getStart(), BufferID);
+        unsigned Offset = SrcMgr.getLocOffsetInBuffer(
+            Loc.resolved.range.getStart(), BufferID);
         Ranges.push_back(
-            {Offset, ResolvedLoc.range.getByteLength(), RenameLoc.Usage});
+            {Offset, Loc.resolved.range.getByteLength(), Loc.renameLoc.Usage});
       }
 
       return RelatedIdentsResult(Ranges, OldName);