Merge pull request #70287 from ahoppen/ahoppen/related-identifiers-for-rename

[SourceKit] Update related identifiers request to serve local rename

Author: ahoppen

lib/Refactoring/SyntacticRenameRangeDetails.cpp

@@ -470,8 +470,13 @@ RegionType RenameRangeDetailCollector::addSyntacticRenameRanges(
       isCallSite = true;
       break;
     case RenameLocUsage::Definition:
-      // All function definitions have argument labels that should be renamed.
-      handleLabels = true;
+      // Don't rename labels of operators. There is a mismatch between the
+      // indexer reporting all labels as `_` even if they are spelled as e.g.
+      // `x: Int` in the function declaration. Since the labels only appear
+      // on the operator declaration and in no calls, just don't rename them for
+      // now.
+      // For all other (normal) function declarations, always rename the labels.
+      handleLabels = !Lexer::isOperator(Old.base());
       isCallSite = false;
       break;
     case RenameLocUsage::Reference:

test/SourceKit/RelatedIdents/operator.swift

@@ -0,0 +1,17 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file --leading-lines %s %t
+
+//--- a.swift
+
+struct Foo {}
+// RUN: %sourcekitd-test -req=related-idents -pos=%(line + 1):6 %t/a.swift -- %t/a.swift | %FileCheck %s
+func +(x: Foo, y: Foo) {}
+Foo() + Foo()
+
+//--- dummy.swift
+
+// CHECK: START RANGES
+// CHECK: 8:6 - 1 - source.syntacticrename.definition
+// CHECK: 9:7 - 1 - source.syntacticrename.call
+// CHECK: END RANGES
+// CHECK: NAME: +(_:_:)

test/SourceKit/RelatedIdents/related_idents.swift

@@ -109,11 +109,12 @@ func ifLet(test: Int?) {
 
 // RUN: %sourcekitd-test -req=related-idents -pos=6:17 %s -- -module-name related_idents %s | %FileCheck -check-prefix=CHECK1 %s
 // CHECK1: START RANGES
-// CHECK1-NEXT: 1:7 - 2
-// CHECK1-NEXT: 6:11 - 2
-// CHECK1-NEXT: 6:16 - 2
-// CHECK1-NEXT: 9:11 - 2
+// CHECK1-NEXT: 1:7 - 2 - source.syntacticrename.definition
+// CHECK1-NEXT: 6:11 - 2 - source.syntacticrename.reference
+// CHECK1-NEXT: 6:16 - 2 - source.syntacticrename.reference
+// CHECK1-NEXT: 9:11 - 2 - source.syntacticrename.reference
 // CHECK1-NEXT: END RANGES
+// CHECK1: NAME: C1
 
 // RUN: %sourcekitd-test -req=related-idents -pos=5:9 %s -- -module-name related_idents %s | %FileCheck -check-prefix=CHECK2 %s
 // CHECK2: START RANGES
@@ -196,6 +197,7 @@ func ifLet(test: Int?) {
 // CHECK11-NEXT: 74:1 - 13
 // CHECK11-NEXT: 75:1 - 11
 // CHECK11-NEXT: 76:1 - 11
+// CHECK11: NAME: escapedName(x:)
 
 
 // RUN: %sourcekitd-test -req=related-idents -pos=79:7 %s -- -module-name related_idents %s | %FileCheck -check-prefix=CHECK12 %s

test/refactoring/SyntacticRename/Outputs/functions/infix-operator.swift.expected

@@ -28,7 +28,7 @@ class AStruct {
         return {a in a};
     }
 
-    static func /*infix-operator:def*/<base>+</base> (<arglabel index=0>left</arglabel><param index=0></param>: AStruct, <arglabel index=1>right</arglabel><param index=1></param>: AStruct) -> AStruct {
+    static func /*infix-operator:def*/<base>+</base> (left: AStruct, right: AStruct) -> AStruct {
         return AStruct()
     }
 

test/refactoring/SyntacticRename/Outputs/functions/prefix-operator.swift.expected

@@ -32,7 +32,7 @@ class AStruct {
         return AStruct()
     }
 
-    static prefix func /*prefix-operator:def*/<base>-</base> (<arglabel index=0>struct</arglabel><param index=0></param>: AStruct) -> AStruct {
+    static prefix func /*prefix-operator:def*/<base>-</base> (struct: AStruct) -> AStruct {
         return AStruct()
     }
 }

test/refactoring/SyntacticRename/operator.swift

@@ -0,0 +1,17 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+// RUN: %refactor -find-rename-ranges -source-filename %t/input.swift -pos="test" -old-name "+(x:y:)" -new-name "-(x:y:)" > %t/output.txt
+// RUN: diff -u %t/expected.swift %t/output.txt 
+
+//--- input.swift
+
+struct Foo {}
+func /*test:def*/+(x: Foo, y: Foo) {}
+Foo() /*test:ref*/+ Foo()
+
+//--- expected.swift
+
+struct Foo {}
+func /*test:def*/<base>+</base>(x: Foo, y: Foo) {}
+Foo() /*test:ref*/<base>+</base> Foo()
+

tools/SourceKit/include/SourceKit/Core/LangSupport.h

@@ -786,6 +786,19 @@ struct SemanticRefactoringInfo {
 struct RelatedIdentInfo {
   unsigned Offset;
   unsigned Length;
+  swift::ide::RenameLocUsage Usage;
+};
+
+/// Result of `findRelatedIdentifiersInFile`.
+struct RelatedIdentsResult {
+  SmallVector<RelatedIdentInfo, 8> RelatedIdents;
+  std::string OldName;
+
+  RelatedIdentsResult(SmallVector<RelatedIdentInfo, 8> RelatedIdents,
+                      std::string OldName)
+      : RelatedIdents(RelatedIdents), OldName(OldName) {}
+
+  static RelatedIdentsResult empty() { return RelatedIdentsResult({}, ""); }
 };
 
 /// Represent one branch of an if config.
@@ -1149,11 +1162,20 @@ class LangSupport {
       SourceKitCancellationToken CancellationToken,
       std::function<void(const RequestResult<CursorInfoData> &)> Receiver) = 0;
 
+  /// - Parameters:
+  ///   - IncludeNonEditableBaseNames: If `true` also return results if the
+  ///     referenced declaration is an initializer or subscript. This is
+  ///     intended if the related identifiers response is used for rename, which
+  ///     allows renaming parameter labels of these declaration.
+  ///     If the function's base name should be highlighted, this should be
+  ///     `false` because e.g. highlighting a subscript with
+  ///     `IncludeNonEditableBaseNames = true` would return the locations of all
+  ///     `[` that call that subscript.
   virtual void findRelatedIdentifiersInFile(
       StringRef PrimaryFilePath, StringRef InputBufferName, unsigned Offset,
-      bool CancelOnSubsequentRequest, ArrayRef<const char *> Args,
-      SourceKitCancellationToken CancellationToken,
-      std::function<void(const RequestResult<ArrayRef<RelatedIdentInfo>> &)>
+      bool IncludeNonEditableBaseNames, bool CancelOnSubsequentRequest,
+      ArrayRef<const char *> Args, SourceKitCancellationToken CancellationToken,
+      std::function<void(const RequestResult<RelatedIdentsResult> &)>
           Receiver) = 0;
 
   virtual void findActiveRegionsInFile(

tools/SourceKit/lib/SwiftLang/SwiftLangSupport.h

@@ -682,10 +682,10 @@ class SwiftLangSupport : public LangSupport {
 
   void findRelatedIdentifiersInFile(
       StringRef PrimaryFilePath, StringRef InputBufferName, unsigned Offset,
-      bool CancelOnSubsequentRequest, ArrayRef<const char *> Args,
-      SourceKitCancellationToken CancellationToken,
-      std::function<void(const RequestResult<ArrayRef<RelatedIdentInfo>> &)>
-          Receiver) override;
+      bool IncludeNonEditableBaseNames, bool CancelOnSubsequentRequest,
+      ArrayRef<const char *> Args, SourceKitCancellationToken CancellationToken,
+      std::function<void(const RequestResult<RelatedIdentsResult> &)> Receiver)
+      override;
 
   void findActiveRegionsInFile(
       StringRef PrimaryFilePath, StringRef InputBufferName,

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

@@ -2491,126 +2491,139 @@ SwiftLangSupport::findUSRRange(StringRef DocumentName, StringRef USR) {
 
 void SwiftLangSupport::findRelatedIdentifiersInFile(
     StringRef PrimaryFilePath, StringRef InputBufferName, unsigned Offset,
-    bool CancelOnSubsequentRequest, ArrayRef<const char *> Args,
-    SourceKitCancellationToken CancellationToken,
-    std::function<void(const RequestResult<ArrayRef<RelatedIdentInfo>> &)>
-        Receiver) {
+    bool IncludeNonEditableBaseNames, bool CancelOnSubsequentRequest,
+    ArrayRef<const char *> Args, SourceKitCancellationToken CancellationToken,
+    std::function<void(const RequestResult<RelatedIdentsResult> &)> Receiver) {
 
   std::string Error;
   SwiftInvocationRef Invok =
       ASTMgr->getTypecheckInvocation(Args, PrimaryFilePath, Error);
   if (!Invok) {
     LOG_WARN_FUNC("failed to create an ASTInvocation: " << Error);
-    Receiver(RequestResult<ArrayRef<RelatedIdentInfo>>::fromError(Error));
+    Receiver(RequestResult<RelatedIdentsResult>::fromError(Error));
     return;
   }
 
   class RelatedIdConsumer : public SwiftASTConsumer {
     std::string InputFile;
     unsigned Offset;
-    std::function<void(const RequestResult<ArrayRef<RelatedIdentInfo>> &)>
-        Receiver;
+    bool IncludeNonEditableBaseNames;
+    std::function<void(const RequestResult<RelatedIdentsResult> &)> Receiver;
     SwiftInvocationRef Invok;
 
+#if SWIFT_BUILD_SWIFT_SYNTAX
+    // FIXME: Don't silently eat errors here.
+    RelatedIdentsResult getRelatedIdents(SourceFile *SrcFile,
+                                         CompilerInstance &CompInst) {
+      unsigned BufferID = SrcFile->getBufferID().value();
+      SourceLoc Loc = Lexer::getLocForStartOfToken(CompInst.getSourceMgr(),
+                                                   BufferID, Offset);
+      if (Loc.isInvalid())
+        return RelatedIdentsResult::empty();
+
+      SourceManager &SrcMgr = CompInst.getASTContext().SourceMgr;
+
+      ResolvedCursorInfoPtr CursorInfo =
+          evaluateOrDefault(CompInst.getASTContext().evaluator,
+                            CursorInfoRequest{CursorInfoOwner(SrcFile, Loc)},
+                            new ResolvedCursorInfo());
+      auto ValueRefCursorInfo =
+          dyn_cast<ResolvedValueRefCursorInfo>(CursorInfo);
+      if (!ValueRefCursorInfo)
+        return RelatedIdentsResult::empty();
+      if (ValueRefCursorInfo->isKeywordArgument())
+        return RelatedIdentsResult::empty();
+
+      ValueDecl *VD = ValueRefCursorInfo->typeOrValue();
+      if (!VD)
+        return RelatedIdentsResult::empty(); // This was a module reference.
+
+      // Only accept pointing to an identifier.
+      if (!IncludeNonEditableBaseNames && !ValueRefCursorInfo->isRef() &&
+          (isa<ConstructorDecl>(VD) || isa<DestructorDecl>(VD) ||
+           isa<SubscriptDecl>(VD)))
+        return RelatedIdentsResult::empty();
+
+      llvm::Optional<RenameInfo> Info = getRenameInfo(CursorInfo);
+
+      if (!Info) {
+        return RelatedIdentsResult::empty();
+      }
+
+      RenameLocs Locs = localRenameLocs(SrcFile, Info->VD);
+
+      std::string OldName = Locs.getLocations().front().OldName.str();
+#ifndef NDEBUG
+      for (auto loc : Locs.getLocations()) {
+        assert(loc.OldName == OldName &&
+               "Found related identfiers with different names?");
+      }
+#endif
+
+      // Ignore any errors produced by `resolveRenameLocations` since, if some
+      // symbol failed to resolve, we still want to return all the other
+      // symbols. This makes related idents more fault-tolerant.
+      DiagnosticEngine Diags(SrcMgr);
+
+      std::vector<ResolvedLoc> ResolvedLocs = 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()) {
+          continue;
+        }
+        unsigned Offset =
+            SrcMgr.getLocOffsetInBuffer(ResolvedLoc.range.getStart(), BufferID);
+        Ranges.push_back(
+            {Offset, ResolvedLoc.range.getByteLength(), RenameLoc.Usage});
+      }
+
+      return RelatedIdentsResult(Ranges, OldName);
+    }
+#endif
+
   public:
     RelatedIdConsumer(
-        StringRef InputFile, unsigned Offset,
-        std::function<void(const RequestResult<ArrayRef<RelatedIdentInfo>> &)>
+        StringRef InputFile, unsigned Offset, bool IncludeNonEditableBaseNames,
+        std::function<void(const RequestResult<RelatedIdentsResult> &)>
             Receiver,
         SwiftInvocationRef Invok)
         : InputFile(InputFile.str()), Offset(Offset),
+          IncludeNonEditableBaseNames(IncludeNonEditableBaseNames),
           Receiver(std::move(Receiver)), Invok(Invok) {}
 
-    // FIXME: Don't silently eat errors here.
     void handlePrimaryAST(ASTUnitRef AstUnit) override {
-      using ResultType = RequestResult<ArrayRef<RelatedIdentInfo>>;
+      using ResultType = RequestResult<RelatedIdentsResult>;
 #if !SWIFT_BUILD_SWIFT_SYNTAX
-      Receiver(
-          ResultType::fromError("relatedidents is not supported because "
-                                "sourcekitd was built without swift-syntax"));
+      ResultType::fromError(
+          "relatedidents is not supported because sourcekitd was built without "
+          "swift-syntax");
       return;
 #else
       auto &CompInst = AstUnit->getCompilerInstance();
 
       auto *SrcFile = retrieveInputFile(InputFile, CompInst);
       if (!SrcFile) {
-        Receiver(RequestResult<ArrayRef<RelatedIdentInfo>>::fromError(
+        Receiver(RequestResult<RelatedIdentsResult>::fromError(
             "Unable to find input file"));
         return;
       }
 
-      SmallVector<RelatedIdentInfo, 8> Ranges;
-
-      auto Action = [&]() {
-        unsigned BufferID = SrcFile->getBufferID().value();
-        SourceLoc Loc =
-          Lexer::getLocForStartOfToken(CompInst.getSourceMgr(), BufferID, Offset);
-        if (Loc.isInvalid())
-          return;
-
-        SourceManager &SrcMgr = CompInst.getASTContext().SourceMgr;
-
-        ResolvedCursorInfoPtr CursorInfo =
-            evaluateOrDefault(CompInst.getASTContext().evaluator,
-                              CursorInfoRequest{CursorInfoOwner(SrcFile, Loc)},
-                              new ResolvedCursorInfo());
-        auto ValueRefCursorInfo =
-            dyn_cast<ResolvedValueRefCursorInfo>(CursorInfo);
-        if (!ValueRefCursorInfo)
-          return;
-        if (ValueRefCursorInfo->isKeywordArgument())
-          return;
-
-        ValueDecl *VD = ValueRefCursorInfo->typeOrValue();
-        if (!VD)
-          return; // This was a module reference.
-
-        // Only accept pointing to an identifier.
-        if (!ValueRefCursorInfo->isRef() &&
-            (isa<ConstructorDecl>(VD) || isa<DestructorDecl>(VD) ||
-             isa<SubscriptDecl>(VD)))
-          return;
-        if (VD->isOperator())
-          return;
-
-        llvm::Optional<RenameInfo> Info = getRenameInfo(CursorInfo);
-
-        if (!Info) {
-          return;
-        }
-
-        RenameLocs Locs = localRenameLocs(SrcFile, Info->VD);
-
-        // Ignore any errors produced by `resolveRenameLocations` since, if some
-        // symbol failed to resolve, we still want to return all the other
-        // symbols. This makes related idents more fault-tolerant.
-        DiagnosticEngine Diags(SrcMgr);
-
-        std::vector<ResolvedLoc> ResolvedLocs = resolveRenameLocations(
-            Locs.getLocations(), /*NewName=*/StringRef(), *SrcFile, Diags);
-
-        assert(ResolvedLocs.size() == Locs.getLocations().size());
-        for (auto ResolvedLoc : ResolvedLocs) {
-          if (ResolvedLoc.range.isInvalid()) {
-            continue;
-          }
-          unsigned Offset = SrcMgr.getLocOffsetInBuffer(
-              ResolvedLoc.range.getStart(), BufferID);
-          Ranges.push_back({Offset, ResolvedLoc.range.getByteLength()});
-        }
-      };
-      Action();
-      Receiver(ResultType::fromResult(Ranges));
+      RelatedIdentsResult Result = getRelatedIdents(SrcFile, CompInst);
+      Receiver(ResultType::fromResult(Result));
 #endif
     }
 
     void cancelled() override {
-      Receiver(RequestResult<ArrayRef<RelatedIdentInfo>>::cancelled());
+      Receiver(RequestResult<RelatedIdentsResult>::cancelled());
     }
 
     void failed(StringRef Error) override {
       LOG_WARN_FUNC("related idents failed: " << Error);
-      Receiver(RequestResult<ArrayRef<RelatedIdentInfo>>::fromError(Error));
+      Receiver(RequestResult<RelatedIdentsResult>::fromError(Error));
     }
 
     static CaseStmt *getCaseStmtOfCanonicalVar(Decl *D) {
@@ -2624,8 +2637,8 @@ void SwiftLangSupport::findRelatedIdentifiersInFile(
     }
   };
 
-  auto Consumer = std::make_shared<RelatedIdConsumer>(InputBufferName, Offset,
-                                                      Receiver, Invok);
+  auto Consumer = std::make_shared<RelatedIdConsumer>(
+      InputBufferName, Offset, IncludeNonEditableBaseNames, Receiver, Invok);
   /// FIXME: When request cancellation is implemented and Xcode adopts it,
   /// don't use 'OncePerASTToken'.
   static const char OncePerASTToken = 0;

tools/SourceKit/tools/sourcekitd-test/sourcekitd-test.cpp

@@ -2456,10 +2456,13 @@ static void printRelatedIdents(sourcekitd_variant_t Info, StringRef Filename,
     sourcekitd_variant_t Range = sourcekitd_variant_array_get_value(Res, i);
     int64_t Offset = sourcekitd_variant_dictionary_get_int64(Range, KeyOffset);
     int64_t Length = sourcekitd_variant_dictionary_get_int64(Range, KeyLength);
+    auto Usage = sourcekitd_variant_dictionary_get_uid(Range, KeyNameType);
     auto LineCol = resolveToLineCol(Offset, Filename, VFSFiles);
-    OS << LineCol.first << ':' << LineCol.second << " - " << Length << '\n';
+    OS << LineCol.first << ':' << LineCol.second << " - " << Length << " - "
+       << sourcekitd_uid_get_string_ptr(Usage) << '\n';
   }
   OS << "END RANGES\n";
+  OS << "NAME: " << sourcekitd_variant_dictionary_get_string(Info, KeyName);
 }
 
 static void printActiveRegions(sourcekitd_variant_t Info, StringRef Filename,