From 9219b0cd71f2b95f208acf5c0c4731236e9da8e4 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 11 Dec 2023 16:55:50 -0800 Subject: [PATCH] [SourceKit] Address FIXMEs for the `NameMatcher` rdar://118996561 --- include/swift/IDE/IDEBridging.h | 5 +- include/swift/Refactoring/Refactoring.h | 13 +++-- lib/Refactoring/SyntacticRename.cpp | 52 ++++++++++--------- .../lib/SwiftLang/SwiftSourceDocInfo.cpp | 17 +++--- 4 files changed, 47 insertions(+), 40 deletions(-) diff --git a/include/swift/IDE/IDEBridging.h b/include/swift/IDE/IDEBridging.h index f9692f21ade19..3e29eb8b3d8d5 100644 --- a/include/swift/IDE/IDEBridging.h +++ b/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 labelRanges; /// The in index in `labelRanges` that belongs to the first trailing closure diff --git a/include/swift/Refactoring/Refactoring.h b/include/swift/Refactoring/Refactoring.h index 6d34a34f2b7b2..48c2cf9d08fcc 100644 --- a/include/swift/Refactoring/Refactoring.h +++ b/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 resolveRenameLocations(ArrayRef renameLocs, - StringRef newName, - SourceFile &sourceFile, - DiagnosticEngine &diags); +std::vector +resolveRenameLocations(ArrayRef renameLocs, StringRef newName, + SourceFile &sourceFile, DiagnosticEngine &diags); #endif struct RangeConfig { diff --git a/lib/Refactoring/SyntacticRename.cpp b/lib/Refactoring/SyntacticRename.cpp index 66b2e47dea061..63c6040e630fc 100644 --- a/lib/Refactoring/SyntacticRename.cpp +++ b/lib/Refactoring/SyntacticRename.cpp @@ -22,7 +22,7 @@ using namespace swift; using namespace swift::ide; #if SWIFT_BUILD_SWIFT_SYNTAX -std::vector +std::vector swift::ide::resolveRenameLocations(ArrayRef RenameLocs, StringRef NewName, SourceFile &SF, DiagnosticEngine &Diags) { @@ -72,6 +72,8 @@ swift::ide::resolveRenameLocations(ArrayRef RenameLocs, UnresolvedLocs.push_back({Location}); } + assert(UnresolvedLocs.size() == RenameLocs.size()); + std::vector BridgedUnresolvedLocs; BridgedUnresolvedLocs.reserve(UnresolvedLocs.size()); for (SourceLoc Loc : UnresolvedLocs) { @@ -85,26 +87,28 @@ swift::ide::resolveRenameLocations(ArrayRef RenameLocs, const std::vector &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 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 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 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 --git a/tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp b/tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp index 75ef7e7fa21c2..e6d21fe70cea6 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp +++ b/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 ResolvedLocs = resolveRenameLocations( - Locs.getLocations(), /*NewName=*/StringRef(), *SrcFile, Diags); + std::vector ResolvedAndRenameLocs = + resolveRenameLocations(Locs.getLocations(), /*NewName=*/StringRef(), + *SrcFile, Diags); SmallVector 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);