Merge pull request #70255 from ahoppen/ahoppen/rename-refactoring

[Rename] Collection of improvements to make syntactic rename easier to read

Author: ahoppen

include/swift/IDE/IDEBridging.h

@@ -23,19 +23,38 @@
 #endif
 
 enum class LabelRangeType {
+  /// The matched location did not have any arguments.
   None,
 
+  /// The argument of a function/initializer/macro/... call
+  ///
+  /// ### Example
   /// `foo([a: ]2) or .foo([a: ]String)`
   CallArg,
 
-  /// `func([a b]: Int)`
+  /// The parameter of a function/initializer/macro/... declaration
+  ///
+  /// ### Example
+  /// `func foo([a b]: Int)`
   Param,
 
+  /// Parameters of a function that can't be collapsed if they are the same.
+  ///
+  /// This is the case for parameters of subscripts since subscripts have
+  /// unnamed
+  /// parameters by default.
+  ///
+  /// ### Example
   /// `subscript([a a]: Int)`
   NoncollapsibleParam,
 
-  /// `#selector(foo.func([a]:))`
-  Selector,
+  /// A reference to a function that also specifies argument labels to
+  /// disambiguate.
+  ///
+  /// ### Examples
+  /// - `#selector(foo.func([a]:))`
+  /// - `foo.func([a]:)`
+  CompoundName,
 };
 
 enum class ResolvedLocContext { Default, Selector, Comment, StringLiteral };

include/swift/Refactoring/Refactoring.h

@@ -10,14 +10,15 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef SWIFT_IDE_REFACTORING_H
-#define SWIFT_IDE_REFACTORING_H
+#ifndef SWIFT_REFACTORING_REFACTORING_H
+#define SWIFT_REFACTORING_REFACTORING_H
 
 #include "swift/AST/DiagnosticConsumer.h"
 #include "swift/Basic/LLVM.h"
 #include "swift/Basic/StringExtras.h"
 #include "swift/IDE/CancellableResult.h"
 #include "swift/IDE/Utils.h"
+#include "swift/Refactoring/RenameLoc.h"
 #include "llvm/ADT/StringRef.h"
 
 namespace swift {
@@ -61,21 +62,6 @@ struct RenameInfo {
 
 llvm::Optional<RenameInfo> getRenameInfo(ResolvedCursorInfoPtr cursorInfo);
 
-enum class NameUsage {
-  Unknown,
-  Reference,
-  Definition,
-  Call
-};
-
-struct RenameLoc {
-  unsigned Line;
-  unsigned Column;
-  NameUsage Usage;
-  StringRef OldName;
-  const bool IsFunctionLike;
-};
-
 /// An array of \c RenameLoc that also keeps the underlying string storage of
 /// the \c StringRef inside the \c RenameLoc alive.
 class RenameLocs {
@@ -184,4 +170,4 @@ collectRefactorings(ResolvedCursorInfoPtr CursorInfo, bool ExcludeRename);
 } // namespace ide
 } // namespace swift
 
-#endif // SWIFT_IDE_REFACTORING_H
+#endif // SWIFT_REFACTORING_REFACTORING_H

include/swift/Refactoring/RenameLoc.h

@@ -0,0 +1,86 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef SWIFT_REFACTORING_RENAMELOC_H
+#define SWIFT_REFACTORING_RENAMELOC_H
+
+#include "swift/Basic/LLVM.h"
+
+namespace swift {
+namespace ide {
+
+/// Describes how a `ResolvedLoc` is being used
+enum class RenameLocUsage {
+  /// The definition of a function/subscript/variable/...
+  Definition,
+
+  /// The symbol is being referenced.
+  ///
+  /// This includes
+  ///  - References to variables
+  ///  - Unapplied references to functions (`myStruct.memberFunc`)
+  ///  - Calls to subscripts (`myArray[1]`, location is `[` here, length 1)
+  Reference,
+
+  /// A function that is being called.
+  Call,
+
+  /// Unknown name usage occurs if we don't have an entry in the index that
+  /// tells us whether the location is a call, reference or a definition. The
+  /// most common reasons why this happens is if the editor is adding syntactic
+  /// results (eg. from comments or string literals).
+  Unknown
+};
+
+/// The input to `findSyntacticRenameRanges`.
+///
+/// Specifies the location of a base name for which `findSyntacticRenameRanges`
+/// should find the argument labels as well as some semantic information needed
+/// to resolve the rename ranges.
+struct RenameLoc {
+  /// The line at which the base name starts (1-based).
+  unsigned Line;
+
+  /// The column at which the base name (excluding trivia) starts (1-based).
+  unsigned Column;
+
+  /// The offset at which the related name starts.
+  unsigned Offset;
+
+  /// The length of the base name in the related identifier. For functions,
+  /// this does not include the parameters/arguments.
+  unsigned Length;
+
+  /// How the identifier is being used (call, reference, definition, unknown).
+  ///
+  /// Used to decide whether a given occurance should be renamed and/or if its
+  /// argument labels should be renamed.
+  RenameLocUsage Usage;
+
+  /// The old decl name being renamed.
+  ///
+  /// ### Examples
+  /// - `foo(a:b:)` for a function with two parameters.
+  /// - `foo` for a variable.
+  /// - `foo(_:)` for a function with a single unnamed parameter
+  /// - `foo()` for a function without parameters.
+  StringRef OldName;
+
+  RenameLoc(unsigned Line, unsigned Column, RenameLocUsage Usage,
+            StringRef OldName)
+      : Line(Line), Column(Column), Usage(Usage), OldName(OldName) {}
+};
+
+} // namespace ide
+} // namespace swift
+
+#endif // SWIFT_REFACTORING_RENAMELOC_H

lib/ASTGen/Sources/SwiftIDEUtilsBridging/NameMatcherBridging.swift

@@ -51,7 +51,7 @@ fileprivate extension IDEBridging.LabelRangeType {
     case .call: self = .CallArg
     case .parameters: self = .Param
     case .noncollapsibleParameters: self = .NoncollapsibleParam
-    case .selector: self = .Selector
+    case .selector: self = .CompoundName
     }
   }
 }

lib/IDE/SwiftSourceDocInfo.cpp

@@ -612,8 +612,9 @@ bool NameMatcher::tryResolve(ASTWalker::ParentTy Node, DeclNameLoc NameLoc,
 
   if (NameLoc.isCompound()) {
     auto Labels = getSelectorLabelRanges(getSourceMgr(), NameLoc);
-    bool Resolved = tryResolve(Node, NameLoc.getBaseNameLoc(),
-                               LabelRangeType::Selector, Labels, llvm::None);
+    bool Resolved =
+        tryResolve(Node, NameLoc.getBaseNameLoc(), LabelRangeType::CompoundName,
+                   Labels, llvm::None);
     if (!isDone()) {
       for (auto Label: Labels) {
         if (tryResolve(Node, Label.getStart())) {

lib/Refactoring/ExtractFunction.cpp

@@ -77,10 +77,8 @@ static SourceLoc getNewFuncInsertLoc(DeclContext *DC,
   return SourceLoc();
 }
 
-static std::vector<NoteRegion> getNotableRegions(StringRef SourceText,
-                                                 unsigned NameOffset,
-                                                 StringRef Name,
-                                                 bool IsFunctionLike = false) {
+static std::vector<NoteRegion>
+getNotableRegions(StringRef SourceText, unsigned NameOffset, StringRef Name) {
   auto InputBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(SourceText, "<extract>");
 
@@ -106,8 +104,7 @@ static std::vector<NoteRegion> getNotableRegions(StringRef SourceText,
   assert(!Resolved.empty() && "Failed to resolve generated func name loc");
 
   RenameLoc RenameConfig = {LineAndCol.first, LineAndCol.second,
-                            NameUsage::Definition, /*OldName=*/Name,
-                            IsFunctionLike};
+                            RenameLocUsage::Definition, /*OldName=*/Name};
   std::vector<RenameRangeDetail> Ranges =
       getSyntacticRenameRangeDetails(SM, Name, Resolved.back(), RenameConfig)
           .Ranges;
@@ -290,13 +287,11 @@ bool RefactoringActionExtractFunction::performChange() {
 
   StringRef DeclStr(Buffer.begin() + FuncBegin, FuncEnd - FuncBegin);
   auto NotableFuncRegions =
-      getNotableRegions(DeclStr, FuncNameOffset, ExtractedFuncName,
-                        /*IsFunctionLike=*/true);
+      getNotableRegions(DeclStr, FuncNameOffset, ExtractedFuncName);
 
   StringRef CallStr(Buffer.begin() + ReplaceBegin, ReplaceEnd - ReplaceBegin);
   auto NotableCallRegions =
-      getNotableRegions(CallStr, CallNameOffset, ExtractedFuncName,
-                        /*IsFunctionLike=*/true);
+      getNotableRegions(CallStr, CallNameOffset, ExtractedFuncName);
 
   // Insert the new function's declaration.
   EditConsumer.accept(SM, InsertLoc, DeclStr, NotableFuncRegions);

lib/Refactoring/LocalRename.cpp

@@ -207,22 +207,25 @@ class RenameRangeCollector : public IndexDataConsumer {
   bool finishDependency(bool isClangModule) override { return true; }
 
   Action startSourceEntity(const IndexSymbol &symbol) override {
-    if (symbol.USR == usr) {
-      if (auto loc = indexSymbolToRenameLoc(symbol)) {
-        // Inside capture lists like `{ [test] in }`, 'test' refers to both the
-        // newly declared, captured variable and the referenced variable it is
-        // initialized from. Make sure to only rename it once.
-        auto existingLoc = llvm::find_if(locations, [&](RenameLoc searchLoc) {
-          return searchLoc.Line == loc->Line && searchLoc.Column == loc->Column;
-        });
-        if (existingLoc == locations.end()) {
-          locations.push_back(std::move(*loc));
-        } else {
-          assert(existingLoc->OldName == loc->OldName &&
-                 existingLoc->IsFunctionLike == loc->IsFunctionLike &&
-                 "Asked to do a different rename for the same location?");
-        }
-      }
+    if (symbol.USR != usr) {
+      return IndexDataConsumer::Continue;
+    }
+    auto loc = indexSymbolToRenameLoc(symbol);
+    if (!loc) {
+      return IndexDataConsumer::Continue;
+    }
+    
+    // Inside capture lists like `{ [test] in }`, 'test' refers to both the
+    // newly declared, captured variable and the referenced variable it is
+    // initialized from. Make sure to only rename it once.
+    auto existingLoc = llvm::find_if(locations, [&](RenameLoc searchLoc) {
+      return searchLoc.Line == loc->Line && searchLoc.Column == loc->Column;
+    });
+    if (existingLoc == locations.end()) {
+      locations.push_back(std::move(*loc));
+    } else {
+      assert(existingLoc->OldName == loc->OldName &&
+             "Asked to do a different rename for the same location?");
     }
     return IndexDataConsumer::Continue;
   }
@@ -241,37 +244,19 @@ RenameRangeCollector::indexSymbolToRenameLoc(const index::IndexSymbol &symbol) {
     return llvm::None;
   }
 
-  NameUsage usage = NameUsage::Unknown;
+  RenameLocUsage usage = RenameLocUsage::Unknown;
   if (symbol.roles & (unsigned)index::SymbolRole::Call) {
-    usage = NameUsage::Call;
+    usage = RenameLocUsage::Call;
   } else if (symbol.roles & (unsigned)index::SymbolRole::Definition) {
-    usage = NameUsage::Definition;
+    usage = RenameLocUsage::Definition;
   } else if (symbol.roles & (unsigned)index::SymbolRole::Reference) {
-    usage = NameUsage::Reference;
+    usage = RenameLocUsage::Reference;
   } else {
     llvm_unreachable("unexpected role");
   }
 
-  bool isFunctionLike = false;
-
-  switch (symbol.symInfo.Kind) {
-  case index::SymbolKind::EnumConstant:
-  case index::SymbolKind::Function:
-  case index::SymbolKind::Constructor:
-  case index::SymbolKind::ConversionFunction:
-  case index::SymbolKind::InstanceMethod:
-  case index::SymbolKind::ClassMethod:
-  case index::SymbolKind::StaticMethod:
-    isFunctionLike = true;
-    break;
-  case index::SymbolKind::Class:
-  case index::SymbolKind::Enum:
-  case index::SymbolKind::Struct:
-  default:
-    break;
-  }
   StringRef oldName = stringStorage->copyString(symbol.name);
-  return RenameLoc{symbol.line, symbol.column, usage, oldName, isFunctionLike};
+  return RenameLoc{symbol.line, symbol.column, usage, oldName};
 }
 
 /// Get the decl context that we need to walk when renaming \p VD.

lib/Refactoring/SyntacticRename.cpp

@@ -63,7 +63,7 @@ swift::ide::resolveRenameLocations(ArrayRef<RenameLoc> RenameLocs,
         return {};
       }
 
-      if (RenameLoc.Usage == NameUsage::Call && !RenameLoc.IsFunctionLike) {
+      if (RenameLoc.Usage == RenameLocUsage::Call && !OldName.isFunction()) {
         Diags.diagnose(Location, diag::name_not_functionlike, NewName);
         return {};
       }