Merge pull request #2401 from ahoppen/ahoppen/fixme-namematcher

Address remaining `FIXME`s for the new `NameMatcher`

Author: ahoppen

Sources/SwiftIDEUtils/CMakeLists.txt

@@ -13,6 +13,7 @@ add_swift_syntax_library(SwiftIDEUtils
   Syntax+Classifications.swift
   SyntaxClassification.swift
   SyntaxClassifier.swift
+  Utils.swift
 )
 
 target_link_swift_syntax_libraries(SwiftIDEUtils PUBLIC

Sources/SwiftIDEUtils/DeclNameLocation.swift

@@ -26,11 +26,17 @@ public struct DeclNameLocation: Equatable {
     /// ## Examples
     /// - `a` in `func foo(a: Int) {}`
     /// - `a` and `b` in `func foo(a b: Int) {}`
+    /// - `additionalTrailingClosure:` in `foo {} additionalTrailingClosure: {}`
+    ///   - We cannot use `labeledCall` here because when changing the parameter label to `_`, we need to change the
+    ///     call to `foo {} _: {}`. If we used `labeledCall`, the label would be removed, leaving us with `foo {} {}`.
     case labeled(firstName: Range<AbsolutePosition>, secondName: Range<AbsolutePosition>?)
 
     /// The argument of a call.
     ///
-    /// The range of the label does not include trivia. The range of the colon *does* include trivia.
+    /// The range of the label does not include trivia.
+    ///
+    /// The range of the colon *does* include trivia. This is so we can remove the colon including the space after it
+    /// when changing a labeled parameter to an unlabeled parameter.
     ///
     /// ## Examples
     /// - `a` and `:` in `foo(a: 1)`
@@ -46,10 +52,6 @@ public struct DeclNameLocation: Equatable {
     }
 
     static func labeledCall(label: TokenSyntax, colon: TokenSyntax) -> Argument {
-      // FIXME: (NameMatcher) The `labeledCall` case is problematic for two reasons
-      // 1. The fact that `colon` includes trivia is inconsistent with the associated values in `label` and `labeledCall`
-      // 2. If `colon` didn't need to contain trivia, we wouldn't need the `labeledCall` case at all.
-      // See if we can unify `labeledCall` and `labeled`.
       return .labeledCall(label: label.rangeWithoutTrivia, colon: colon.position..<colon.endPosition)
     }
 
@@ -68,6 +70,18 @@ public struct DeclNameLocation: Equatable {
         return argumentPosition..<argumentPosition
       }
     }
+
+    /// Shift the ranges `utf8Offset` bytes to the right, ie. add `utf8Offset` to the upper and lower bound.
+    func advanced(by utf8Offset: Int) -> DeclNameLocation.Argument {
+      switch self {
+      case .labeled(firstName: let firstName, secondName: let secondName):
+        return .labeled(firstName: firstName.advanced(by: utf8Offset), secondName: secondName?.advanced(by: utf8Offset))
+      case .labeledCall(label: let label, colon: let colon):
+        return .labeledCall(label: label.advanced(by: utf8Offset), colon: colon.advanced(by: utf8Offset))
+      case .unlabeled(argumentPosition: let argumentPosition):
+        return .unlabeled(argumentPosition: argumentPosition.advanced(by: utf8Offset))
+      }
+    }
   }
 
   /// The arguments of a ``DeclNameLocation``.
@@ -92,6 +106,22 @@ public struct DeclNameLocation: Equatable {
 
     /// The argument label to disambiguate multiple functions with the same base name, like `foo(a:)`.
     case selector([Argument])
+
+    /// Shift the ranges `utf8Offset` bytes to the right, ie. add `utf8Offset` to the upper and lower bound.
+    func advanced(by utf8Offset: Int) -> DeclNameLocation.Arguments {
+      switch self {
+      case .noArguments:
+        return .noArguments
+      case .call(let arguments, firstTrailingClosureIndex: let firstTrailingClosureIndex):
+        return .call(arguments.map { $0.advanced(by: utf8Offset) }, firstTrailingClosureIndex: firstTrailingClosureIndex)
+      case .parameters(let parameters):
+        return .parameters(parameters.map { $0.advanced(by: utf8Offset) })
+      case .noncollapsibleParameters(let parameters):
+        return .noncollapsibleParameters(parameters.map { $0.advanced(by: utf8Offset) })
+      case .selector(let arguments):
+        return .selector(arguments.map { $0.advanced(by: utf8Offset) })
+      }
+    }
   }
 
   public enum Context {

Sources/SwiftIDEUtils/NameMatcher.swift

@@ -55,9 +55,9 @@ public class NameMatcher: SyntaxAnyVisitor {
 
   // MARK: - Public entry
 
-  public static func resolve(baseNamePositions: some Sequence<AbsolutePosition>, in sourceFile: SourceFileSyntax) -> [DeclNameLocation] {
+  public static func resolve(baseNamePositions: some Sequence<AbsolutePosition>, in tree: some SyntaxProtocol) -> [DeclNameLocation] {
     let matcher = NameMatcher(baseNamePositions: baseNamePositions)
-    matcher.walk(sourceFile)
+    matcher.walk(tree)
     return matcher.resolvedLocs
   }
 
@@ -161,7 +161,9 @@ public class NameMatcher: SyntaxAnyVisitor {
     }
     if let additionalTrailingClosures {
       argumentLabels += additionalTrailingClosures.map { (additionalTrailingClosure) -> DeclNameLocation.Argument in
-        // FIXME: (NameMatcher) This really should be callLabel but the old NameMatcher also reported this as labeled
+        // We need to report additional trailing closure labels in the same way that we report function parameters
+        // because changing the argument label to `_` should result in an additional trailing closure label `_:` instead
+        // of removing the label, which is what `labeledCall` does
         return .labeled(firstName: additionalTrailingClosure.label, secondName: nil)
       }
     }
@@ -193,16 +195,29 @@ public class NameMatcher: SyntaxAnyVisitor {
 
   public override func visit(_ token: TokenSyntax) -> SyntaxVisitorContinueKind {
     while let baseNamePosition = firstPositionToResolve(in: token.leadingTriviaRange) ?? firstPositionToResolve(in: token.trailingTriviaRange) {
+      // Parse the comment from the position that we want to resolve. This should parse any function calls or compound decl names, the rest of
+      // the comment will probably be parsed as garbage but that's OK because we don't actually care about it.
       let positionOffsetInToken = baseNamePosition.utf8Offset - token.position.utf8Offset
-      guard let tokenLength = getFirstTokenLength(in: token.syntaxTextBytes[positionOffsetInToken...]) else {
-        continue
+      let commentTree = token.syntaxTextBytes[positionOffsetInToken...].withUnsafeBufferPointer { (buffer) -> ExprSyntax in
+        var parser = Parser(buffer)
+        return ExprSyntax.parse(from: &parser)
+      }
+      // Run a new `NameMatcher`. Since the input of that name matcher is the text after the position to resolve, we
+      // want to resolve the position at offset 0.
+      let resolvedInComment = NameMatcher.resolve(baseNamePositions: [AbsolutePosition(utf8Offset: 0)], in: commentTree)
+
+      let positionRemoved = removePositionToResolveIfExists(at: baseNamePosition)
+      precondition(positionRemoved, "Found a position with `firstPositionToResolve but didn't find it again to remove it?")
+
+      // Adjust the positions to point back to the original tree, set the context as `comment` and record them.
+      resolvedLocs += resolvedInComment.map { locationInComment in
+        DeclNameLocation(
+          baseNameRange: locationInComment.baseNameRange.advanced(by: baseNamePosition.utf8Offset),
+          arguments: locationInComment.arguments.advanced(by: baseNamePosition.utf8Offset),
+          context: .comment,
+          isActive: isActiveStack.last!
+        )
       }
-      // FIXME: (NameMatcher) We could also resolve argument labels inside comments if we are parsing anyway.
-      addResolvedLocIfRequested(
-        baseNameRange: baseNamePosition..<baseNamePosition.advanced(by: tokenLength.utf8Length),
-        argumentLabels: .noArguments,
-        context: .comment
-      )
     }
 
     if case .stringSegment = token.tokenKind {

Sources/SwiftIDEUtils/Utils.swift

@@ -0,0 +1,20 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2023 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 SwiftSyntax
+
+extension Range<AbsolutePosition> {
+  /// Shift the range `utf8Offset` bytes to the right, ie. add `utf8Offset` to the upper and lower bound.
+  func advanced(by utf8Offset: Int) -> Range<AbsolutePosition> {
+    return self.lowerBound.advanced(by: utf8Offset)..<self.upperBound.advanced(by: utf8Offset)
+  }
+}

Tests/SwiftIDEUtilsTest/NameMatcherTests.swift

@@ -171,8 +171,32 @@ public class NameMatcherTests: XCTestCase {
       */
       """,
       expected: [
-        DeclNameLocationSpec(baseName: "foo", context: .comment),
-        DeclNameLocationSpec(baseName: "foo", context: .comment),
+        DeclNameLocationSpec(baseName: "foo", argumentLabels: [], type: .call, context: .comment),
+        DeclNameLocationSpec(baseName: "foo", argumentLabels: ["first"], type: .selector, context: .comment),
+      ]
+    )
+  }
+
+  func testResolveArgumentLabelsInComments() {
+    assertNameMatcherResult(
+      "// 1️⃣fn(x:)",
+      expected: [
+        DeclNameLocationSpec(baseName: "fn", argumentLabels: ["x"], type: .selector, context: .comment)
+      ]
+    )
+
+    assertNameMatcherResult(
+      "// 1️⃣fn(x:) 2️⃣foo(other:)",
+      expected: [
+        DeclNameLocationSpec(baseName: "fn", argumentLabels: ["x"], type: .selector, context: .comment),
+        DeclNameLocationSpec(baseName: "foo", argumentLabels: ["other"], type: .selector, context: .comment),
+      ]
+    )
+
+    assertNameMatcherResult(
+      "// 1️⃣fn(x: 1)",
+      expected: [
+        DeclNameLocationSpec(baseName: "fn", argumentLabels: ["x: "], type: .call, context: .comment)
       ]
     )
   }