Address Hamish’s review comments

Author: ahoppen

include/swift/IDE/IDEBridging.h

@@ -112,20 +112,15 @@ class BridgedResolvedLocVector {
   /// Delete the heap-allocated memory owned by this object. Accessing
   /// `unbridged` is illegal after calling `destroy`.
   void destroy();
+
+  SWIFT_IMPORT_UNSAFE
+  void *getOpaqueValue() const;
 };
 
 
 SWIFT_NAME("BridgedResolvedLocVector.empty()")
 BridgedResolvedLocVector BridgedResolvedLocVector_createEmpty();
 
-/// Get an opaque pointer value that describes this
-/// `BridgedResolvedLocVector`. This opaque value can be returned by a
-/// `@_cdecl` function in Swift.
-/// 
-/// - Note: Cannot be defined as a member on BridgedResolvecLocVector because 
-///   Swift 5.8 does not import methods that return pointers. 
-void *BridgedResolvedLocVector_getOpaqueValue(const BridgedResolvedLocVector &vector);
-
 typedef std::vector<BridgedSourceLoc> SourceLocVector;
 
 /// Needed so that we can manually conform `SourceLocVectorIterator` to

include/swift/Refactoring/Refactoring.h

@@ -92,13 +92,13 @@ class RenameLocs {
   RenameLocs() {}
 };
 
-/// Return the location to rename when renaming the identifier at \p startLoc
+/// Return the locations to rename when renaming the \p valueDecl
 /// in \p sourceFile.
 ///
 /// - Parameters:
 ///   - sourceFile: The source file in which to perform local rename
-///   - renameInfo: Information about the symbol to rename. See `getRenameInfo`
-RenameLocs localRenameLocs(SourceFile *sourceFile, RenameInfo renameInfo);
+///   - valueDecl: The declaration that should be renamed
+RenameLocs localRenameLocs(SourceFile *sourceFile, const ValueDecl *valueDecl);
 
 /// Given a list of `RenameLoc`s, get the corresponding `ResolveLoc`s.
 ///

lib/ASTGen/Sources/SwiftIDEUtilsBridging/NameMatcherBridging.swift

@@ -107,5 +107,5 @@ public func runNameMatcher(
   let positions: [AbsolutePosition] =  locations.compactMap { sourceFile.position(of: $0) }
   let resolvedLocs = NameMatcher.resolve(baseNamePositions: positions, in: sourceFile.syntax)
   let bridged = BridgedResolvedLocVector(from: resolvedLocs, in: sourceFile)
-  return BridgedResolvedLocVector_getOpaqueValue(bridged)
+  return bridged.getOpaqueValue()
 }

lib/IDE/IDEBridging.cpp

@@ -56,6 +56,6 @@ const std::vector<ResolvedLoc> &BridgedResolvedLocVector::unbridged() {
 
 void BridgedResolvedLocVector::destroy() { delete vector; }
 
-void *BridgedResolvedLocVector_getOpaqueValue(const BridgedResolvedLocVector &vector) {
-  return static_cast<void *>(vector.vector);
+void *BridgedResolvedLocVector::getOpaqueValue() const {
+  return static_cast<void *>(this->vector);
 }

lib/Refactoring/LocalRename.cpp

@@ -193,7 +193,7 @@ class RenameRangeCollector : public IndexDataConsumer {
 
   /// Take the resuls from the collector.
   /// This invalidates the collector and must only be called once.
-  RenameLocs takeResults() {
+  RenameLocs takeResults() && {
     return RenameLocs(locations, std::move(stringStorage));
   }
 
@@ -278,7 +278,7 @@ RenameRangeCollector::indexSymbolToRenameLoc(const index::IndexSymbol &symbol) {
 ///
 /// This \c DeclContext contains all possible references to \c VD within the
 /// file.
-DeclContext *getRenameScope(ValueDecl *VD) {
+DeclContext *getRenameScope(const ValueDecl *VD) {
   auto *Scope = VD->getDeclContext();
   // There may be sibling decls that the renamed symbol is visible from.
   switch (Scope->getContextKind()) {
@@ -303,15 +303,15 @@ DeclContext *getRenameScope(ValueDecl *VD) {
   return Scope;
 }
 
-/// Get the `RenameInfo` at `startLoc` and validate that we can perform local
+/// Get the declaration at `startLoc` and validate that we can perform local
 /// rename on it (e.g. checking that the original definition isn't a system
 /// symbol).
 ///
-/// If the validation succeeds, return the `RenameInfo`, otherwise add an error
-/// to `diags` and return `None`.
-static llvm::Optional<RenameInfo>
-getRenameInfoForLocalRename(SourceFile *sourceFile, SourceLoc startLoc,
-                            DiagnosticEngine &diags) {
+/// If the validation succeeds, return the `ValueDecl`, otherwise add an error
+/// to `diags` and return `nullptr`.
+static ValueDecl *getDeclForLocalRename(SourceFile *sourceFile,
+                                        SourceLoc startLoc,
+                                        DiagnosticEngine &diags) {
   auto cursorInfo = evaluateOrDefault(
       sourceFile->getASTContext().evaluator,
       CursorInfoRequest{CursorInfoOwner(sourceFile, startLoc)},
@@ -320,55 +320,56 @@ getRenameInfoForLocalRename(SourceFile *sourceFile, SourceLoc startLoc,
   llvm::Optional<RenameInfo> info = getRenameInfo(cursorInfo);
   if (!info) {
     diags.diagnose(startLoc, diag::unresolved_location);
-    return llvm::None;
+    return nullptr;
   }
 
   switch (info->Availability.AvailableKind) {
   case RefactorAvailableKind::Available:
     break;
   case RefactorAvailableKind::Unavailable_system_symbol:
     diags.diagnose(startLoc, diag::decl_is_system_symbol, info->VD->getName());
-    return llvm::None;
+    return nullptr;
   case RefactorAvailableKind::Unavailable_has_no_location:
     diags.diagnose(startLoc, diag::value_decl_no_loc, info->VD->getName());
-    return llvm::None;
+    return nullptr;
   case RefactorAvailableKind::Unavailable_has_no_name:
     diags.diagnose(startLoc, diag::decl_has_no_name);
-    return llvm::None;
+    return nullptr;
   case RefactorAvailableKind::Unavailable_has_no_accessibility:
     diags.diagnose(startLoc, diag::decl_no_accessibility);
-    return llvm::None;
+    return nullptr;
   case RefactorAvailableKind::Unavailable_decl_from_clang:
     diags.diagnose(startLoc, diag::decl_from_clang);
-    return llvm::None;
+    return nullptr;
   case RefactorAvailableKind::Unavailable_decl_in_macro:
     diags.diagnose(startLoc, diag::decl_in_macro);
-    return llvm::None;
+    return nullptr;
   }
 
-  return info;
+  return info->VD;
 }
 
-RenameLocs swift::ide::localRenameLocs(SourceFile *SF, RenameInfo renameInfo) {
+RenameLocs swift::ide::localRenameLocs(SourceFile *SF,
+                                       const ValueDecl *valueDecl) {
   DeclContext *RenameScope = SF;
-  if (!RenameScope) {
+  if (RenameScope) {
     // If the value is declared in a DeclContext that's a child of the file in
     // which we are performing the rename, we can limit our analysis to this
     // decl context.
     //
     // Cases where the rename scope is not a child of the source file include
     // if we are getting related identifiers of a type A that is defined in
     // another file. In this case, we need to analyze the entire file.
-    auto DeclarationScope = getRenameScope(renameInfo.VD);
+    auto DeclarationScope = getRenameScope(valueDecl);
     if (DeclarationScope->isChildContextOf(SF)) {
       RenameScope = DeclarationScope;
     }
   }
 
-  RenameRangeCollector rangeCollector(renameInfo.VD);
+  RenameRangeCollector rangeCollector(valueDecl);
   indexDeclContext(RenameScope, rangeCollector);
 
-  return rangeCollector.takeResults();
+  return std::move(rangeCollector).takeResults();
 }
 
 CancellableResult<std::vector<SyntacticRenameRangeDetails>>
@@ -385,13 +386,12 @@ swift::ide::findLocalRenameRanges(SourceFile *SF, RangeConfig Range) {
   Diags.addConsumer(DiagConsumer);
 
   auto StartLoc = Lexer::getLocForStartOfToken(SM, Range.getStart(SM));
-  llvm::Optional<RenameInfo> info =
-      getRenameInfoForLocalRename(SF, StartLoc, Diags);
-  if (!info || DiagConsumer.didErrorOccur()) {
+  ValueDecl *declToRename = getDeclForLocalRename(SF, StartLoc, Diags);
+  if (!declToRename || DiagConsumer.didErrorOccur()) {
     return ResultType::failure(ErrBuffer);
   }
 
-  RenameLocs RenameRanges = localRenameLocs(SF, *info);
+  RenameLocs RenameRanges = localRenameLocs(SF, declToRename);
 
   return findSyntacticRenameRanges(SF, RenameRanges.getLocations(),
                                    /*NewName=*/StringRef());

lib/Refactoring/SyntacticRenameRangeDetails.cpp

@@ -36,7 +36,7 @@ class RenameRangeDetailCollector {
   RegionType addSyntacticRenameRanges(const ResolvedLoc &Resolved,
                                       const RenameLoc &Config);
 
-  std::vector<RenameRangeDetail> getResult() const { return Ranges; }
+  std::vector<RenameRangeDetail> &&takeResult() { return std::move(Ranges); }
 
 private:
   void addRenameRange(CharSourceRange Label, RefactoringRangeKind RangeKind,
@@ -458,5 +458,5 @@ SyntacticRenameRangeDetails swift::ide::getSyntacticRenameRangeDetails(
     const RenameLoc &Config) {
   RenameRangeDetailCollector RangeCollector(SM, OldName);
   RegionType Type = RangeCollector.addSyntacticRenameRanges(Resolved, Config);
-  return {Type, RangeCollector.getResult()};
+  return {Type, RangeCollector.takeResult()};
 }

test/SourceKit/RelatedIdents/pound_if.swift

@@ -2,7 +2,7 @@
 func foo(x: Int) {
 #if true
   print(x)
-#else {
+#else
   print(x)
 #endif
 }

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

@@ -2569,7 +2569,7 @@ void SwiftLangSupport::findRelatedIdentifiersInFile(
           return;
         }
 
-        RenameLocs Locs = localRenameLocs(SrcFile, *Info);
+        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