Merge pull request #1237 from ahoppen/refactoring-review-comments

Address my own review comments to #1179

Author: ahoppen

Sources/SourceKitLSP/CMakeLists.txt

@@ -14,6 +14,7 @@ add_library(SourceKitLSP STATIC
   SourceKitLSPServer+Options.swift
   SymbolLocation+DocumentURI.swift
   TestDiscovery.swift
+  TextEdit+IsNoop.swift
   WorkDoneProgressManager.swift
   Workspace.swift
 )

Sources/SourceKitLSP/Rename.swift

@@ -1490,13 +1490,3 @@ fileprivate extension RelatedIdentifiersResponse {
     }
   }
 }
-
-fileprivate extension TextEdit {
-  /// Returns `true` the replaced text is the same as the new text
-  func isNoOp(in snapshot: DocumentSnapshot) -> Bool {
-    if snapshot.text[snapshot.indexRange(of: range)] == newText {
-      return true
-    }
-    return false
-  }
-}

Sources/SourceKitLSP/Swift/CodeActions/AddDocumentation.swift

@@ -37,22 +37,25 @@ import SwiftSyntax
 public struct AddDocumentation: EditRefactoringProvider {
   @_spi(Testing)
   public static func textRefactor(syntax: DeclSyntax, in context: Void) -> [SourceEdit] {
-    let hasDocumentation = syntax.leadingTrivia.contains(where: { trivia in
+    let hasDocumentation = syntax.leadingTrivia.contains { trivia in
       switch trivia {
-      case .blockComment(_), .docBlockComment(_), .lineComment(_), .docLineComment(_):
+      case .blockComment, .docBlockComment, .lineComment, .docLineComment:
         return true
       default:
         return false
       }
-    })
+    }
+
+    // We consider nodes at the start of the source file at being on a new line
+    let isOnNewLine =
+      syntax.leadingTrivia.contains(where: \.isNewline) || syntax.previousToken(viewMode: .sourceAccurate) == nil
 
-    guard !hasDocumentation else {
+    guard !hasDocumentation && isOnNewLine else {
       return []
     }
 
     let newlineAndIndentation = [.newlines(1)] + (syntax.firstToken(viewMode: .sourceAccurate)?.indentationOfLine ?? [])
     var content: [TriviaPiece] = []
-    content += newlineAndIndentation
     content.append(.docLineComment("/// A description"))
 
     if let parameters = syntax.parameters?.parameters {
@@ -82,8 +85,9 @@ public struct AddDocumentation: EditRefactoringProvider {
       content += newlineAndIndentation
       content.append(.docLineComment("/// - Returns:"))
     }
+    content += newlineAndIndentation
 
-    let insertPos = syntax.position
+    let insertPos = syntax.positionAfterSkippingLeadingTrivia
     return [
       SourceEdit(
         range: insertPos..<insertPos,
@@ -94,6 +98,13 @@ public struct AddDocumentation: EditRefactoringProvider {
 }
 
 extension AddDocumentation: SyntaxRefactoringCodeActionProvider {
+  static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
+    return scope.innermostNodeContainingRange?.findParentOfSelf(
+      ofType: DeclSyntax.self,
+      stoppingIf: { $0.is(CodeBlockItemSyntax.self) || $0.is(MemberBlockItemSyntax.self) || $0.is(ExprSyntax.self) }
+    )
+  }
+
   static var title: String { "Add documentation" }
 }
 

Sources/SourceKitLSP/Swift/CodeActions/ConvertIntegerLiteral.swift

@@ -14,9 +14,6 @@ import LanguageServerProtocol
 import SwiftRefactor
 import SwiftSyntax
 
-// TODO: Make the type IntegerLiteralExprSyntax.Radix conform to CaseEnumerable
-// in swift-syntax.
-
 extension IntegerLiteralExprSyntax.Radix {
   static var allCases: [Self] = [.binary, .octal, .decimal, .hex]
 }
@@ -26,7 +23,7 @@ extension IntegerLiteralExprSyntax.Radix {
 struct ConvertIntegerLiteral: SyntaxCodeActionProvider {
   static func codeActions(in scope: SyntaxCodeActionScope) -> [CodeAction] {
     guard
-      let token = scope.firstToken,
+      let token = scope.innermostNodeContainingRange,
       let integerExpr = token.parent?.as(IntegerLiteralExprSyntax.self),
       let integerValue = Int(
         integerExpr.split().value.filter { $0 != "_" },

Sources/SourceKitLSP/Swift/CodeActions/ConvertJSONToCodableStruct.swift

@@ -192,6 +192,17 @@ public struct ConvertJSONToCodableStruct: EditRefactoringProvider {
 }
 
 extension ConvertJSONToCodableStruct: SyntaxRefactoringCodeActionProvider {
+  static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Syntax? {
+    var node: Syntax? = scope.innermostNodeContainingRange
+    while let unwrappedNode = node, ![.codeBlockItem, .memberBlockItem].contains(unwrappedNode.kind) {
+      if preflightRefactoring(unwrappedNode) != nil {
+        return unwrappedNode
+      }
+      node = unwrappedNode.parent
+    }
+    return nil
+  }
+
   static var title = "Create Codable structs from JSON"
 }
 

Sources/SourceKitLSP/Swift/CodeActions/PackageManifestEdits.swift

@@ -20,9 +20,7 @@ import SwiftSyntax
 /// edit a package manifest.
 struct PackageManifestEdits: SyntaxCodeActionProvider {
   static func codeActions(in scope: SyntaxCodeActionScope) -> [CodeAction] {
-    guard let token = scope.firstToken,
-      let call = token.findEnclosingCall()
-    else {
+    guard let call = scope.innermostNodeContainingRange?.findEnclosingCall() else {
       return []
     }
 

Sources/SourceKitLSP/Swift/CodeActions/SyntaxCodeActionProvider.swift

@@ -40,26 +40,50 @@ struct SyntaxCodeActionScope {
   /// considered, i.e., where the cursor or selection is.
   var range: Range<AbsolutePosition>
 
-  init(
+  /// The innermost node that contains the entire selected source range
+  var innermostNodeContainingRange: Syntax?
+
+  init?(
     snapshot: DocumentSnapshot,
-    syntaxTree tree: SourceFileSyntax,
+    syntaxTree file: SourceFileSyntax,
     request: CodeActionRequest
-  ) throws {
+  ) {
     self.snapshot = snapshot
     self.request = request
-    self.file = tree
+    self.file = file
 
-    let start = snapshot.absolutePosition(of: request.range.lowerBound)
-    let end = snapshot.absolutePosition(of: request.range.upperBound)
-    let left = file.token(at: start)
-    let right = file.token(at: end)
-    let leftOff = left?.position ?? AbsolutePosition(utf8Offset: 0)
-    let rightOff = right?.endPosition ?? leftOff
-    self.range = leftOff..<rightOff
+    guard let left = tokenForRefactoring(at: request.range.lowerBound, snapshot: snapshot, syntaxTree: file),
+      let right = tokenForRefactoring(at: request.range.upperBound, snapshot: snapshot, syntaxTree: file)
+    else {
+      return nil
+    }
+    self.range = left.position..<right.endPosition
+    self.innermostNodeContainingRange = findCommonAncestorOrSelf(Syntax(left), Syntax(right))
   }
+}
 
-  /// The first token in the
-  var firstToken: TokenSyntax? {
-    file.token(at: range.lowerBound)
+private func tokenForRefactoring(
+  at position: Position,
+  snapshot: DocumentSnapshot,
+  syntaxTree: SourceFileSyntax
+) -> TokenSyntax? {
+  let absolutePosition = snapshot.absolutePosition(of: position)
+  if absolutePosition == syntaxTree.endPosition {
+    // token(at:) will not find the end of file token if the end of file token has length 0. Special case this and
+    // return the last proper token in this case.
+    return syntaxTree.endOfFileToken.previousToken(viewMode: .sourceAccurate)
+  }
+  guard let token = syntaxTree.token(at: absolutePosition) else {
+    return nil
+  }
+  // See `adjustPositionToStartOfIdentifier`. We need to be a little more aggressive for the refactorings and also
+  // adjust to the start of punctuation eg. if the end of the selected range is after a `}`, we want the end token for
+  // the refactoring to be the `}`, not the token after `}`.
+  if absolutePosition == token.position,
+    let previousToken = token.previousToken(viewMode: .sourceAccurate),
+    previousToken.endPositionBeforeTrailingTrivia == absolutePosition
+  {
+    return previousToken
   }
+  return token
 }

Sources/SourceKitLSP/Swift/CodeActions/SyntaxRefactoringCodeActionProvider.swift

@@ -18,29 +18,35 @@ import SwiftSyntax
 /// swift-syntax) into a SyntaxCodeActionProvider.
 protocol SyntaxRefactoringCodeActionProvider: SyntaxCodeActionProvider, EditRefactoringProvider {
   static var title: String { get }
+
+  /// Returns the node that the syntax refactoring should be performed on, if code actions are requested for the given
+  /// scope.
+  static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input?
 }
 
 /// SyntaxCodeActionProviders with a \c Void context can automatically be
 /// adapted provide a code action based on their refactoring operation.
 extension SyntaxRefactoringCodeActionProvider where Self.Context == Void {
   static func codeActions(in scope: SyntaxCodeActionScope) -> [CodeAction] {
-    guard
-      let token = scope.firstToken,
-      let node = token.parent?.as(Input.self)
-    else {
+    guard let node = nodeToRefactor(in: scope) else {
       return []
     }
 
     let sourceEdits = Self.textRefactor(syntax: node)
-    if sourceEdits.isEmpty {
-      return []
-    }
 
-    let textEdits = sourceEdits.map { edit in
-      TextEdit(
+    let textEdits = sourceEdits.compactMap { (edit) -> TextEdit? in
+      let edit = TextEdit(
         range: scope.snapshot.range(of: edit.range),
         newText: edit.replacement
       )
+      if edit.isNoOp(in: scope.snapshot) {
+        return nil
+      }
+      return edit
+    }
+
+    if textEdits.isEmpty {
+      return []
     }
 
     return [
@@ -57,22 +63,77 @@ extension SyntaxRefactoringCodeActionProvider where Self.Context == Void {
 
 extension AddSeparatorsToIntegerLiteral: SyntaxRefactoringCodeActionProvider {
   public static var title: String { "Add digit separators" }
+
+  static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
+    return scope.innermostNodeContainingRange?.findParentOfSelf(
+      ofType: IntegerLiteralExprSyntax.self,
+      stoppingIf: { $0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self) }
+    )
+  }
 }
 
 extension FormatRawStringLiteral: SyntaxRefactoringCodeActionProvider {
   public static var title: String {
     "Convert string literal to minimal number of '#'s"
   }
+
+  static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
+    return scope.innermostNodeContainingRange?.findParentOfSelf(
+      ofType: StringLiteralExprSyntax.self,
+      stoppingIf: {
+        $0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self)
+          || $0.keyPathInParent == \ExpressionSegmentSyntax.expressions
+      }
+    )
+  }
 }
 
 extension MigrateToNewIfLetSyntax: SyntaxRefactoringCodeActionProvider {
   public static var title: String { "Migrate to shorthand 'if let' syntax" }
+
+  static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
+    return scope.innermostNodeContainingRange?.findParentOfSelf(
+      ofType: IfExprSyntax.self,
+      stoppingIf: { $0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self) }
+    )
+  }
 }
 
 extension OpaqueParameterToGeneric: SyntaxRefactoringCodeActionProvider {
   public static var title: String { "Expand 'some' parameters to generic parameters" }
+
+  static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
+    return scope.innermostNodeContainingRange?.findParentOfSelf(
+      ofType: DeclSyntax.self,
+      stoppingIf: { $0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self) }
+    )
+  }
 }
 
 extension RemoveSeparatorsFromIntegerLiteral: SyntaxRefactoringCodeActionProvider {
   public static var title: String { "Remove digit separators" }
+
+  static func nodeToRefactor(in scope: SyntaxCodeActionScope) -> Input? {
+    return scope.innermostNodeContainingRange?.findParentOfSelf(
+      ofType: IntegerLiteralExprSyntax.self,
+      stoppingIf: { $0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self) }
+    )
+  }
+}
+
+extension Syntax {
+  /// Finds the innermost parent of the given type while not walking outside of nodes that satisfy `stoppingIf`.
+  func findParentOfSelf<ParentType: SyntaxProtocol>(
+    ofType: ParentType.Type,
+    stoppingIf: (Syntax) -> Bool
+  ) -> ParentType? {
+    var node: Syntax? = self
+    while let unwrappedNode = node, !stoppingIf(unwrappedNode) {
+      if let expectedType = unwrappedNode.as(ParentType.self) {
+        return expectedType
+      }
+      node = unwrappedNode.parent
+    }
+    return nil
+  }
 }

Sources/SourceKitLSP/Swift/SwiftLanguageService.swift

@@ -755,9 +755,10 @@ extension SwiftLanguageService {
     return response
   }
 
-  func retrieveCodeActions(_ req: CodeActionRequest, providers: [CodeActionProvider]) async throws
-    -> [CodeAction]
-  {
+  func retrieveCodeActions(
+    _ req: CodeActionRequest,
+    providers: [CodeActionProvider]
+  ) async throws -> [CodeAction] {
     guard providers.isEmpty == false else {
       return []
     }
@@ -776,7 +777,9 @@ extension SwiftLanguageService {
     let snapshot = try documentManager.latestSnapshot(uri)
 
     let syntaxTree = await syntaxTreeManager.syntaxTree(for: snapshot)
-    let scope = try SyntaxCodeActionScope(snapshot: snapshot, syntaxTree: syntaxTree, request: request)
+    guard let scope = SyntaxCodeActionScope(snapshot: snapshot, syntaxTree: syntaxTree, request: request) else {
+      return []
+    }
     return await allSyntaxCodeActions.concurrentMap { provider in
       return provider.codeActions(in: scope)
     }.flatMap { $0 }
@@ -1152,8 +1155,8 @@ extension DocumentSnapshot {
     callerFile: StaticString = #fileID,
     callerLine: UInt = #line
   ) -> Range<Position> {
-    let lowerBound = self.position(of: node.position)
-    let upperBound = self.position(of: node.endPosition)
+    let lowerBound = self.position(of: node.position, callerFile: callerFile, callerLine: callerLine)
+    let upperBound = self.position(of: node.endPosition, callerFile: callerFile, callerLine: callerLine)
     return lowerBound..<upperBound
   }
 

Sources/SourceKitLSP/TextEdit+IsNoop.swift

@@ -0,0 +1,23 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2024 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
+//
+//===----------------------------------------------------------------------===//
+
+import LanguageServerProtocol
+
+extension TextEdit {
+  /// Returns `true` the replaced text is the same as the new text
+  func isNoOp(in snapshot: DocumentSnapshot) -> Bool {
+    if snapshot.text[snapshot.indexRange(of: range)] == newText {
+      return true
+    }
+    return false
+  }
+}