From 4e1953dc0835e033812b66afc30dec9b5d26b08b Mon Sep 17 00:00:00 2001 From: Anthony Latsis Date: Tue, 1 Jul 2025 22:33:07 +0100 Subject: [PATCH 1/7] SwiftIDEUtilsTest: Add tests for `FixItApplier` (cherry picked from commit fdaf1be16322c76985385e4f1c627d158b6bac3b) --- .../SwiftIDEUtilsTest/FixItApplierTests.swift | 341 ++++++++++++++++++ 1 file changed, 341 insertions(+) create mode 100644 Tests/SwiftIDEUtilsTest/FixItApplierTests.swift diff --git a/Tests/SwiftIDEUtilsTest/FixItApplierTests.swift b/Tests/SwiftIDEUtilsTest/FixItApplierTests.swift new file mode 100644 index 00000000000..51540be17bf --- /dev/null +++ b/Tests/SwiftIDEUtilsTest/FixItApplierTests.swift @@ -0,0 +1,341 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2025 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 +// +//===----------------------------------------------------------------------===// + +@_spi(FixItApplier) import SwiftIDEUtils +import SwiftSyntax +import XCTest + +private extension SourceEdit { + init(range: Range, replacement: String) { + self.init( + range: AbsolutePosition(utf8Offset: range.lowerBound)..) { + if subrange.isEmpty { return } + var lower = subrange.lowerBound + var upper = subrange.upperBound + while lower < upper { + formIndex(before: &upper) + swapAt(lower, upper) + formIndex(after: &lower) + } + } +} + +private extension MutableCollection where Self: BidirectionalCollection, Element: Comparable { + /// Permutes this collection's elements through all the lexical orderings. + /// + /// Call `nextPermutation()` repeatedly starting with the collection in sorted + /// order. When the full cycle of all permutations has been completed, the + /// collection will be back in sorted order and this method will return + /// `false`. + /// + /// - Returns: A Boolean value indicating whether the collection still has + /// remaining permutations. When this method returns `false`, the collection + /// is in ascending order according to `areInIncreasingOrder`. + /// + /// - Complexity: O(*n*), where *n* is the length of the collection. + mutating func nextPermutation(upperBound: Index? = nil) -> Bool { + // Ensure we have > 1 element in the collection. + guard !isEmpty else { return false } + var i = index(before: endIndex) + if i == startIndex { return false } + + let upperBound = upperBound ?? endIndex + + while true { + let ip1 = i + formIndex(before: &i) + + // Find the last ascending pair (ie. ..., a, b, ... where a < b) + if self[i] < self[ip1] { + // Find the last element greater than self[i] + // swift-format-ignore: NeverForceUnwrap + // This is _always_ at most `ip1` due to if statement above + let j = lastIndex(where: { self[i] < $0 })! + + // At this point we have something like this: + // 0, 1, 4, 3, 2 + // ^ ^ + // i j + swapAt(i, j) + self.reverse(subrange: ip1.. Date: Wed, 2 Jul 2025 12:02:31 +0100 Subject: [PATCH 2/7] SwiftSyntax: Conform `AbsolutePosition` to `Strideable` (cherry picked from commit a6162a5ffa3a8f6831f3d4e808bcbbc01df68778) --- Sources/SwiftIDEUtils/FixItApplier.swift | 16 +++++----------- Sources/SwiftSyntax/AbsolutePosition.swift | 12 +++++++++--- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/Sources/SwiftIDEUtils/FixItApplier.swift b/Sources/SwiftIDEUtils/FixItApplier.swift index de5785ff941..3c242c702f3 100644 --- a/Sources/SwiftIDEUtils/FixItApplier.swift +++ b/Sources/SwiftIDEUtils/FixItApplier.swift @@ -68,7 +68,7 @@ public enum FixItApplier { source.replaceSubrange(startIndex.. SourceEdit? in - if remainingEdit.replacementRange.overlaps(edit.replacementRange) { + if remainingEdit.range.overlaps(edit.range) { // The edit overlaps with the previous edit. We can't apply both // without conflicts. Apply the one that's listed first and drop the // later edit. @@ -78,12 +78,10 @@ public enum FixItApplier { // If the remaining edit starts after or at the end of the edit that we just applied, // shift it by the current edit's difference in length. if edit.endUtf8Offset <= remainingEdit.startUtf8Offset { - let startPosition = AbsolutePosition( - utf8Offset: remainingEdit.startUtf8Offset - edit.replacementRange.count + edit.replacementLength.utf8Length - ) - let endPosition = AbsolutePosition( - utf8Offset: remainingEdit.endUtf8Offset - edit.replacementRange.count + edit.replacementLength.utf8Length - ) + let shift = edit.replacementLength.utf8Length - edit.range.count + let startPosition = AbsolutePosition(utf8Offset: remainingEdit.startUtf8Offset + shift) + let endPosition = AbsolutePosition(utf8Offset: remainingEdit.endUtf8Offset + shift) + return SourceEdit(range: startPosition.. { - return startUtf8Offset.. Bool { + return lhs.utf8Offset < rhs.utf8Offset + } +} + +extension AbsolutePosition: Strideable { public func advanced(by offset: Int) -> AbsolutePosition { - return AbsolutePosition(utf8Offset: self.utf8Offset + offset) + AbsolutePosition(utf8Offset: self.utf8Offset + offset) } - public static func < (lhs: AbsolutePosition, rhs: AbsolutePosition) -> Bool { - return lhs.utf8Offset < rhs.utf8Offset + public func distance(to other: AbsolutePosition) -> Int { + self.utf8Offset.distance(to: other.utf8Offset) } } From 6ea303787ca44ae6986d8a34449193d0719ccd0f Mon Sep 17 00:00:00 2001 From: Anthony Latsis Date: Wed, 2 Jul 2025 15:58:12 +0100 Subject: [PATCH 3/7] FixItApplier: Accept syntax tree generically (cherry picked from commit 9084149697fdd1727bf24e4f0a8dc2b421170473) --- Sources/SwiftIDEUtils/FixItApplier.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/SwiftIDEUtils/FixItApplier.swift b/Sources/SwiftIDEUtils/FixItApplier.swift index 3c242c702f3..b27641956b1 100644 --- a/Sources/SwiftIDEUtils/FixItApplier.swift +++ b/Sources/SwiftIDEUtils/FixItApplier.swift @@ -32,7 +32,7 @@ public enum FixItApplier { public static func applyFixes( from diagnostics: [Diagnostic], filterByMessages messages: [String]?, - to tree: any SyntaxProtocol + to tree: some SyntaxProtocol ) -> String { let messages = messages ?? diagnostics.compactMap { $0.fixIts.first?.message.message } @@ -54,7 +54,7 @@ public enum FixItApplier { /// - Returns: A `String` representation of the modified syntax tree after applying the edits. public static func apply( edits: [SourceEdit], - to tree: any SyntaxProtocol + to tree: some SyntaxProtocol ) -> String { var edits = edits var source = tree.description From 98f10ab6ee941c45cdb2f2b154813ec2872f4a32 Mon Sep 17 00:00:00 2001 From: Anthony Latsis Date: Tue, 1 Jul 2025 23:43:52 +0100 Subject: [PATCH 4/7] FixItApplier: Eliminate quadratic `compactMap` (cherry picked from commit b953b57af39630d26c7f54e375198a4bd7a4d0fb) --- Sources/SwiftIDEUtils/FixItApplier.swift | 44 +++++++++++++++++------- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/Sources/SwiftIDEUtils/FixItApplier.swift b/Sources/SwiftIDEUtils/FixItApplier.swift index b27641956b1..539e4a310b6 100644 --- a/Sources/SwiftIDEUtils/FixItApplier.swift +++ b/Sources/SwiftIDEUtils/FixItApplier.swift @@ -46,12 +46,12 @@ public enum FixItApplier { return self.apply(edits: edits, to: tree) } - /// Apply the given edits to the syntax tree. + /// Applies the given edits to the given syntax tree. /// /// - Parameters: - /// - edits: The edits to apply to the syntax tree - /// - tree: he syntax tree to which the edits should be applied. - /// - Returns: A `String` representation of the modified syntax tree after applying the edits. + /// - edits: The edits to apply. + /// - tree: The syntax tree to which the edits should be applied. + /// - Returns: A `String` representation of the modified syntax tree. public static func apply( edits: [SourceEdit], to tree: some SyntaxProtocol @@ -62,17 +62,27 @@ public enum FixItApplier { while let edit = edits.first { edits = Array(edits.dropFirst()) + // Empty edits do nothing. + guard !edit.isEmpty else { + continue + } + let startIndex = source.utf8.index(source.utf8.startIndex, offsetBy: edit.startUtf8Offset) let endIndex = source.utf8.index(source.utf8.startIndex, offsetBy: edit.endUtf8Offset) source.replaceSubrange(startIndex.. SourceEdit? in - if remainingEdit.range.overlaps(edit.range) { + // Drop any subsequent edits that conflict with one we just applied, and + // adjust the range of the rest. + for i in edits.indices { + let remainingEdit = edits[i] + + guard !remainingEdit.range.overlaps(edit.range) else { // The edit overlaps with the previous edit. We can't apply both - // without conflicts. Apply the one that's listed first and drop the - // later edit. - return nil + // without conflicts. Drop this one by swapping it for a no-op + // edit. + edits[i] = SourceEdit() + continue } // If the remaining edit starts after or at the end of the edit that we just applied, @@ -82,10 +92,8 @@ public enum FixItApplier { let startPosition = AbsolutePosition(utf8Offset: remainingEdit.startUtf8Offset + shift) let endPosition = AbsolutePosition(utf8Offset: remainingEdit.endUtf8Offset + shift) - return SourceEdit(range: startPosition.. Date: Wed, 2 Jul 2025 07:09:05 +0100 Subject: [PATCH 5/7] FixItApplier: Eliminate quadratic `dropFirst` (cherry picked from commit b3270feeefa08ae0ddb48613d3d93837932042a4) --- Sources/SwiftIDEUtils/FixItApplier.swift | 33 +++++++++++++++++------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/Sources/SwiftIDEUtils/FixItApplier.swift b/Sources/SwiftIDEUtils/FixItApplier.swift index 539e4a310b6..22d873e5435 100644 --- a/Sources/SwiftIDEUtils/FixItApplier.swift +++ b/Sources/SwiftIDEUtils/FixItApplier.swift @@ -59,29 +59,37 @@ public enum FixItApplier { var edits = edits var source = tree.description - while let edit = edits.first { - edits = Array(edits.dropFirst()) + for var editIndex in edits.indices { + let edit = edits[editIndex] // Empty edits do nothing. guard !edit.isEmpty else { continue } - let startIndex = source.utf8.index(source.utf8.startIndex, offsetBy: edit.startUtf8Offset) - let endIndex = source.utf8.index(source.utf8.startIndex, offsetBy: edit.endUtf8Offset) + do { + let utf8 = source.utf8 + let startIndex = utf8.index(utf8.startIndex, offsetBy: edit.startUtf8Offset) + let endIndex = utf8.index(utf8.startIndex, offsetBy: edit.endUtf8Offset) - source.replaceSubrange(startIndex.. Index { + self.formIndex(after: &index) as Void + return index + } +} + private extension SourceEdit { var startUtf8Offset: Int { return range.lowerBound.utf8Offset From 21364de2449f73da9674c26b8b5aabce6dca6143 Mon Sep 17 00:00:00 2001 From: Anthony Latsis Date: Wed, 2 Jul 2025 15:55:34 +0100 Subject: [PATCH 6/7] FixItApplier: Add parameter for skipping duplicate insertions (cherry picked from commit df0f832da66c7f509674c725b048d3fbf5286045) --- Sources/SwiftIDEUtils/FixItApplier.swift | 33 ++++- .../SwiftIDEUtilsTest/FixItApplierTests.swift | 138 ++++++++++++++---- 2 files changed, 134 insertions(+), 37 deletions(-) diff --git a/Sources/SwiftIDEUtils/FixItApplier.swift b/Sources/SwiftIDEUtils/FixItApplier.swift index 22d873e5435..241a57c89bc 100644 --- a/Sources/SwiftIDEUtils/FixItApplier.swift +++ b/Sources/SwiftIDEUtils/FixItApplier.swift @@ -27,12 +27,15 @@ public enum FixItApplier { /// - filterByMessages: An optional array of message strings to filter which Fix-Its to apply. /// If `nil`, the first Fix-It from each diagnostic is applied. /// - tree: The syntax tree to which the Fix-Its will be applied. + /// - allowDuplicateInsertions: Whether to apply duplicate insertions. + /// Defaults to `true`. /// /// - Returns: A `String` representation of the modified syntax tree after applying the Fix-Its. public static func applyFixes( from diagnostics: [Diagnostic], filterByMessages messages: [String]?, - to tree: some SyntaxProtocol + to tree: some SyntaxProtocol, + allowDuplicateInsertions: Bool = true ) -> String { let messages = messages ?? diagnostics.compactMap { $0.fixIts.first?.message.message } @@ -43,7 +46,7 @@ public enum FixItApplier { .filter { messages.contains($0.message.message) } .flatMap(\.edits) - return self.apply(edits: edits, to: tree) + return self.apply(edits: edits, to: tree, allowDuplicateInsertions: allowDuplicateInsertions) } /// Applies the given edits to the given syntax tree. @@ -51,10 +54,14 @@ public enum FixItApplier { /// - Parameters: /// - edits: The edits to apply. /// - tree: The syntax tree to which the edits should be applied. + /// - allowDuplicateInsertions: Whether to apply duplicate insertions. + /// Defaults to `true`. + /// /// - Returns: A `String` representation of the modified syntax tree. public static func apply( edits: [SourceEdit], - to tree: some SyntaxProtocol + to tree: some SyntaxProtocol, + allowDuplicateInsertions: Bool = true ) -> String { var edits = edits var source = tree.description @@ -85,10 +92,22 @@ public enum FixItApplier { continue } - guard !remainingEdit.range.overlaps(edit.range) else { - // The edit overlaps with the previous edit. We can't apply both - // without conflicts. Drop this one by swapping it for a no-op - // edit. + func shouldDropRemainingEdit() -> Bool { + // Insertions never conflict between themselves, unless we were asked + // to drop duplicate insertions. + if edit.range.isEmpty && remainingEdit.range.isEmpty { + if allowDuplicateInsertions { + return false + } + + return edit == remainingEdit + } + + return remainingEdit.range.overlaps(edit.range) + } + + guard !shouldDropRemainingEdit() else { + // Drop the edit by swapping it for an empty one. edits[editIndex] = SourceEdit() continue } diff --git a/Tests/SwiftIDEUtilsTest/FixItApplierTests.swift b/Tests/SwiftIDEUtilsTest/FixItApplierTests.swift index 51540be17bf..0761b3978a8 100644 --- a/Tests/SwiftIDEUtilsTest/FixItApplierTests.swift +++ b/Tests/SwiftIDEUtilsTest/FixItApplierTests.swift @@ -166,7 +166,9 @@ class FixItApplierApplyEditsTests: XCTestCase { .init(range: 3..<7, replacement: "cd"), ], // The second edit is skipped. - possibleOutputs: ["aboo = 1", "varcd = 1"] + outputs: [ + .init(oneOf: "aboo = 1", "varcd = 1") + ] ) } @@ -180,19 +182,36 @@ class FixItApplierApplyEditsTests: XCTestCase { .init(range: 0..<5, replacement: "_"), .init(range: 0..<3, replacement: "let"), ], - possibleOutputs: ["_ = 11", "let x = 11"] + outputs: [ + .init(oneOf: "_ = 11", "let x = 11") + ] ) } func testOverlappingInsertions() { + assertAppliedEdits( + to: "x = 1", + edits: [ + .init(range: 1..<1, replacement: "y"), + .init(range: 1..<1, replacement: "z"), + ], + outputs: [ + .init(oneOf: "xyz = 1", "xzy = 1") + ] + ) + assertAppliedEdits( to: "x = 1", edits: [ .init(range: 0..<0, replacement: "var "), .init(range: 0..<0, replacement: "var "), + .init(range: 4..<5, replacement: "2"), .init(range: 0..<0, replacement: "var "), ], - output: "var var var x = 1" + outputs: [ + .init(deterministic: "var var var x = 2", allowDuplicateInsertions: true), + .init(deterministic: "var x = 2", allowDuplicateInsertions: false), + ] ) } @@ -214,47 +233,105 @@ class FixItApplierApplyEditsTests: XCTestCase { .init(range: 2..<2, replacement: "a"), // Insertion ], // FIXME: This behavior where these edits are not considered overlapping doesn't feel desirable - possibleOutputs: ["_x = 1", "_ a= 1"] + outputs: [ + .init(oneOf: "_x = 1", "_ a= 1") + ] ) } } -/// Asserts that at least one element in `possibleOutputs` matches the result -/// of applying an array of edits to `input`, for all permutations of `edits`. +/// Represents an output expectation. +private struct OutputExpectation { + var possibleOutputs: [String] + var allowDuplicateInsertions: Bool? + var line: UInt + + /// Create a deterministic output expectation for the given value of + /// `allowDuplicateInsertions`. If `allowDuplicateInsertions` is `nil`, + /// the expectation holds for both `true` and `false`. + init( + deterministic output: String, + allowDuplicateInsertions: Bool? = nil, + line: UInt = #line + ) { + self.possibleOutputs = [output] + self.allowDuplicateInsertions = allowDuplicateInsertions + self.line = line + } + + /// Create a "one of the given possible outputs" expectation for the given + /// value of `allowDuplicateInsertions`. If `allowDuplicateInsertions` is + /// `nil`, the expectation holds for both `true` and `false`. + init( + oneOf possibleOutputs: String..., + allowDuplicateInsertions: Bool? = nil, + line: UInt = #line + ) { + self.possibleOutputs = possibleOutputs + self.allowDuplicateInsertions = allowDuplicateInsertions + self.line = line + } +} + +/// Asserts that the given outputs match the result of applying an array of +/// edits to `input`, for all permutations of `edits`. private func assertAppliedEdits( to tree: SourceFileSyntax, edits: [SourceEdit], - possibleOutputs: [String], - line: UInt = #line + outputs: [OutputExpectation] ) { - precondition(!possibleOutputs.isEmpty) + precondition(!outputs.isEmpty) - var indices = Array(edits.indices) - while true { - let editsPermutation = indices.map { edits[$0] } + NEXT_OUTPUT: for output in outputs { + let allowDuplicateInsertionsValues = + if let value = output.allowDuplicateInsertions { + [value] + } else { + [true, false] + } - let actualOutput = FixItApplier.apply(edits: editsPermutation, to: tree) - guard possibleOutputs.contains(actualOutput) else { - XCTFail( - """ - Actual output \"\(actualOutput)\" does not match one of \(possibleOutputs) - Edits: - \(editsPermutation) - """, - line: line - ) - return - } + let possibleOutputs = output.possibleOutputs + + // Check this output against all permutations of edits. + var indices = Array(edits.indices) + while true { + let editsPermutation = indices.map { edits[$0] } - let keepGoing = indices.nextPermutation() - guard keepGoing else { - break + for allowDuplicateInsertionsValue in allowDuplicateInsertionsValues { + let actualOutput = FixItApplier.apply( + edits: editsPermutation, + to: tree, + allowDuplicateInsertions: allowDuplicateInsertionsValue + ) + + guard possibleOutputs.contains(actualOutput) else { + XCTFail( + """ + Actual output \"\(actualOutput)\" does not match one of \(possibleOutputs) + Edits: + \(editsPermutation) + allowDuplicateInsertions: + \(allowDuplicateInsertionsValue) + """, + line: output.line + ) + + // Fail once for all permutations to avoid excessive logging. + continue NEXT_OUTPUT + } + } + + let keepGoing = indices.nextPermutation() + guard keepGoing else { + break + } } } } /// Asserts that `output` matches the result of applying an array of edits to -/// `input`, for all permutations of `edits`. +/// `input`, for all permutations of `edits` and for `allowDuplicateInsertions` +/// both `true` and `false`. private func assertAppliedEdits( to tree: SourceFileSyntax, edits: [SourceEdit], @@ -264,8 +341,9 @@ private func assertAppliedEdits( assertAppliedEdits( to: tree, edits: edits, - possibleOutputs: [output], - line: line + outputs: [ + .init(deterministic: output, allowDuplicateInsertions: nil, line: line) + ] ) } From aafe013985a6942133f3a068c46d93aeaa6ee570 Mon Sep 17 00:00:00 2001 From: Anthony Latsis Date: Wed, 2 Jul 2025 16:30:11 +0100 Subject: [PATCH 7/7] FixItApplier: Non-empty insertions can conflict with other edits (cherry picked from commit a00cbca02ca7997e722228846966d5492871f47f) --- Sources/SwiftIDEUtils/FixItApplier.swift | 10 +++++++++- Tests/SwiftIDEUtilsTest/FixItApplierTests.swift | 3 +-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/Sources/SwiftIDEUtils/FixItApplier.swift b/Sources/SwiftIDEUtils/FixItApplier.swift index 241a57c89bc..6253710930d 100644 --- a/Sources/SwiftIDEUtils/FixItApplier.swift +++ b/Sources/SwiftIDEUtils/FixItApplier.swift @@ -103,7 +103,15 @@ public enum FixItApplier { return edit == remainingEdit } - return remainingEdit.range.overlaps(edit.range) + // Edits conflict in the following cases: + // + // - Their ranges have a common element. + // - One's range is empty and its lower bound is strictly within the + // other's range. So 0..<2 also conflicts with 1..<1, but not with + // 0..<0 or 2..<2. + // + return edit.endUtf8Offset > remainingEdit.startUtf8Offset + && edit.startUtf8Offset < remainingEdit.endUtf8Offset } guard !shouldDropRemainingEdit() else { diff --git a/Tests/SwiftIDEUtilsTest/FixItApplierTests.swift b/Tests/SwiftIDEUtilsTest/FixItApplierTests.swift index 0761b3978a8..d45be14f3c5 100644 --- a/Tests/SwiftIDEUtilsTest/FixItApplierTests.swift +++ b/Tests/SwiftIDEUtilsTest/FixItApplierTests.swift @@ -232,9 +232,8 @@ class FixItApplierApplyEditsTests: XCTestCase { .init(range: 0..<5, replacement: "_"), // Replacement .init(range: 2..<2, replacement: "a"), // Insertion ], - // FIXME: This behavior where these edits are not considered overlapping doesn't feel desirable outputs: [ - .init(oneOf: "_x = 1", "_ a= 1") + .init(oneOf: "_ = 1", "vaar x = 1") ] ) }