From 155773c38e02e71d446e434ff125f4e21cf7f742 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Sun, 9 Jun 2024 11:15:42 -0700 Subject: [PATCH] Fix renaming enum case parameters with unnamed associated arguments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We treated enum case parameters the same way as function parameters and weren’t considering that they can be unlabeled. That caused us to insert eg. `_ ` in front of the case’s type, producing `case myCase(_ String)`, which is invalid. When we are inside an enum case parameter and the parameter label is empty, treat it the same as a function call, which will leave the label untouched if it isn’t modified and insert a label including a colon if a new label is introduced. https://github.com/apple/sourcekit-lsp/issues/1228 --- include/swift/IDE/IDEBridging.h | 7 +++ include/swift/IDE/Utils.h | 37 +++++++++++---- .../NameMatcherBridging.swift | 2 + .../SyntacticRenameRangeDetails.cpp | 13 ++++++ ...d_rename_ranges_enum_case_with_label.swift | 46 +++++++++++++++++++ ...ename_ranges_enum_case_without_label.swift | 44 ++++++++++++++++++ ...thout_underscore_as_first_name_label.swift | 45 ++++++++++++++++++ .../Outputs/types/case-other.swift.expected | 2 +- 8 files changed, 186 insertions(+), 10 deletions(-) create mode 100644 test/SourceKit/Refactoring/find_rename_ranges_enum_case_with_label.swift create mode 100644 test/SourceKit/Refactoring/find_rename_ranges_enum_case_without_label.swift create mode 100644 test/SourceKit/Refactoring/find_rename_ranges_enum_case_without_underscore_as_first_name_label.swift diff --git a/include/swift/IDE/IDEBridging.h b/include/swift/IDE/IDEBridging.h index 379783f82ec82..dbbed37e48cbb 100644 --- a/include/swift/IDE/IDEBridging.h +++ b/include/swift/IDE/IDEBridging.h @@ -38,6 +38,13 @@ enum class LabelRangeType { /// `func foo([a b]: Int)` Param, + /// The parameter of an enum case declaration + /// + /// ### Examples + /// - `case myCase([a]: Int)` + /// - `case myCase([]Int)` + EnumCaseParam, + /// 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 diff --git a/include/swift/IDE/Utils.h b/include/swift/IDE/Utils.h index 3ca9b5de3b796..eb697d0615e1e 100644 --- a/include/swift/IDE/Utils.h +++ b/include/swift/IDE/Utils.h @@ -465,15 +465,34 @@ enum class RegionType { }; enum class RefactoringRangeKind { - BaseName, // func [foo](a b: Int) - KeywordBaseName, // [init](a: Int) - ParameterName, // func foo(a[ b]: Int) - NoncollapsibleParameterName, // subscript(a[ a]: Int) - DeclArgumentLabel, // func foo([a] b: Int) - CallArgumentLabel, // foo([a]: 1) - CallArgumentColon, // foo(a[: ]1) - CallArgumentCombined, // foo([]1) could expand to foo([a: ]1) - SelectorArgumentLabel, // foo([a]:) + /// `func [foo](a b: Int)` + BaseName, + + /// `[init](a: Int)` + KeywordBaseName, + + /// `func foo(a[ b]: Int)` + ParameterName, + + /// `subscript(a[ a]: Int)` + NoncollapsibleParameterName, + + /// `func foo([a] b: Int)` + DeclArgumentLabel, + + /// `foo([a]: 1)` + CallArgumentLabel, + + /// `foo(a[: ]1)` + CallArgumentColon, + + /// `foo([]1) could expand to foo([a: ]1)` + /// Also used for enum case declarations without a label, eg. + /// `case foo([]String)` should expand to `case foo([a: ]String)`. + CallArgumentCombined, + + /// `foo([a]:)` + SelectorArgumentLabel, }; struct NoteRegion { diff --git a/lib/ASTGen/Sources/SwiftIDEUtilsBridging/NameMatcherBridging.swift b/lib/ASTGen/Sources/SwiftIDEUtilsBridging/NameMatcherBridging.swift index 2f81023a3a8e3..30d198f48ffc1 100644 --- a/lib/ASTGen/Sources/SwiftIDEUtilsBridging/NameMatcherBridging.swift +++ b/lib/ASTGen/Sources/SwiftIDEUtilsBridging/NameMatcherBridging.swift @@ -50,6 +50,7 @@ fileprivate extension IDEBridging.LabelRangeType { case .noArguments: self = .None case .call: self = .CallArg case .parameters: self = .Param + case .enumCaseParameters: self = .EnumCaseParam case .noncollapsibleParameters: self = .NoncollapsibleParam case .selector: self = .CompoundName #if RESILIENT_SWIFT_SYNTAX @@ -72,6 +73,7 @@ extension BridgedResolvedLoc { case .noArguments: arguments = [] case .call(let arguments2, _): arguments = arguments2 case .parameters(let arguments2): arguments = arguments2 + case .enumCaseParameters(let arguments2): arguments = arguments2 case .noncollapsibleParameters(let arguments2): arguments = arguments2 case .selector(let arguments2): arguments = arguments2 #if RESILIENT_SWIFT_SYNTAX diff --git a/lib/Refactoring/SyntacticRenameRangeDetails.cpp b/lib/Refactoring/SyntacticRenameRangeDetails.cpp index 1e36c85c6c646..b22820f789349 100644 --- a/lib/Refactoring/SyntacticRenameRangeDetails.cpp +++ b/lib/Refactoring/SyntacticRenameRangeDetails.cpp @@ -146,6 +146,18 @@ void RenameRangeDetailCollector::splitAndRenameLabel(CharSourceRange Range, return splitAndRenameCallArg(Range, NameIndex); case LabelRangeType::Param: return splitAndRenameParamLabel(Range, NameIndex, /*IsCollapsible=*/true); + case LabelRangeType::EnumCaseParam: + if (Range.getByteLength() == 0) { + // If the associated value currently doesn't have a label, emit a + // `CallArgumentCombined` range, which will cause a new label followed by + // `:` to be inserted in the same fashion that call arguments get inserted + // to calls + return addRenameRange(Range, RefactoringRangeKind::CallArgumentCombined, NameIndex); + } else { + // If the associated value has a label already, we are in the same case as + // function parameters. + return splitAndRenameParamLabel(Range, NameIndex, /*IsCollapsible=*/true); + } case LabelRangeType::NoncollapsibleParam: return splitAndRenameParamLabel(Range, NameIndex, /*IsCollapsible=*/false); @@ -248,6 +260,7 @@ bool RenameRangeDetailCollector::labelRangeMatches(CharSourceRange Range, LLVM_FALLTHROUGH; case LabelRangeType::CallArg: case LabelRangeType::Param: + case LabelRangeType::EnumCaseParam: case LabelRangeType::CompoundName: return ExistingLabel == (Expected.empty() ? "_" : Expected); case LabelRangeType::None: diff --git a/test/SourceKit/Refactoring/find_rename_ranges_enum_case_with_label.swift b/test/SourceKit/Refactoring/find_rename_ranges_enum_case_with_label.swift new file mode 100644 index 0000000000000..4657564a87733 --- /dev/null +++ b/test/SourceKit/Refactoring/find_rename_ranges_enum_case_with_label.swift @@ -0,0 +1,46 @@ +// RUN: %empty-directory(%t) +// RUN: split-file %s %t +// RUN: %sourcekitd-test -req=find-rename-ranges -rename-spec %t/rename-spec.json %t/input.swift | %FileCheck %s + +// CHECK: source.edit.kind.active: +// CHECK-NEXT: 2:8-2:14 source.refactoring.range.kind.basename +// CHECK-NEXT: 2:15-2:20 source.refactoring.range.kind.decl-argument-label arg-index=0 +// CHECK-NEXT: 2:20-2:20 source.refactoring.range.kind.parameter-and-whitespace arg-index=0 +// CHECK-NEXT: source.edit.kind.active: +// CHECK-NEXT: 6:14-6:20 source.refactoring.range.kind.basename +// CHECK-NEXT: 6:21-6:26 source.refactoring.range.kind.call-argument-label arg-index=0 +// CHECK-NEXT: 6:26-6:28 source.refactoring.range.kind.call-argument-colon arg-index=0 + +//--- input.swift +enum MyEnum { + case myCase(label: String) +} + +func test() { + _ = MyEnum.myCase(label: "abc") +} + +//--- rename-spec.json + +[ + { + "key.name": "myCase(label:)", + "key.locations": [ + { + "key.line": 2, + "key.column": 8, + "key.nametype": source.syntacticrename.definition + } + ] + }, + { + "key.name": "myCase(label:)", + "key.locations": [ + { + "key.line": 6, + "key.column": 14, + "key.nametype": source.syntacticrename.call + } + ] + } +] diff --git a/test/SourceKit/Refactoring/find_rename_ranges_enum_case_without_label.swift b/test/SourceKit/Refactoring/find_rename_ranges_enum_case_without_label.swift new file mode 100644 index 0000000000000..60bc091b856c8 --- /dev/null +++ b/test/SourceKit/Refactoring/find_rename_ranges_enum_case_without_label.swift @@ -0,0 +1,44 @@ +// RUN: %empty-directory(%t) +// RUN: split-file %s %t +// RUN: %sourcekitd-test -req=find-rename-ranges -rename-spec %t/rename-spec.json %t/input.swift | %FileCheck %s + +// CHECK: source.edit.kind.active: +// CHECK-NEXT: 2:8-2:14 source.refactoring.range.kind.basename +// CHECK-NEXT: 2:15-2:15 source.refactoring.range.kind.call-argument-combined arg-index=0 +// CHECK-NEXT: source.edit.kind.active: +// CHECK-NEXT: 6:14-6:20 source.refactoring.range.kind.basename +// CHECK-NEXT: 6:21-6:21 source.refactoring.range.kind.call-argument-combined arg-index=0 + +//--- input.swift +enum MyEnum { + case myCase(String) +} + +func test() { + _ = MyEnum.myCase("abc") +} + +//--- rename-spec.json + +[ + { + "key.name": "myCase(_:)", + "key.locations": [ + { + "key.line": 2, + "key.column": 8, + "key.nametype": source.syntacticrename.definition + } + ] + }, + { + "key.name": "myCase(_:)", + "key.locations": [ + { + "key.line": 6, + "key.column": 14, + "key.nametype": source.syntacticrename.call + } + ] + } +] diff --git a/test/SourceKit/Refactoring/find_rename_ranges_enum_case_without_underscore_as_first_name_label.swift b/test/SourceKit/Refactoring/find_rename_ranges_enum_case_without_underscore_as_first_name_label.swift new file mode 100644 index 0000000000000..8dbe54c425191 --- /dev/null +++ b/test/SourceKit/Refactoring/find_rename_ranges_enum_case_without_underscore_as_first_name_label.swift @@ -0,0 +1,45 @@ +// RUN: %empty-directory(%t) +// RUN: split-file %s %t +// RUN: %sourcekitd-test -req=find-rename-ranges -rename-spec %t/rename-spec.json %t/input.swift | %FileCheck %s + +// CHECK: source.edit.kind.active: +// CHECK-NEXT: 2:8-2:14 source.refactoring.range.kind.basename +// CHECK-NEXT: 2:15-2:16 source.refactoring.range.kind.decl-argument-label arg-index=0 +// CHECK-NEXT: 2:16-2:22 source.refactoring.range.kind.parameter-and-whitespace arg-index=0 +// CHECK-NEXT: source.edit.kind.active: +// CHECK-NEXT: 6:14-6:20 source.refactoring.range.kind.basename +// CHECK-NEXT: 6:21-6:21 source.refactoring.range.kind.call-argument-combined arg-index=0 + +//--- input.swift +enum MyEnum { + case myCase(_ label: String) +} + +func test() { + _ = MyEnum.myCase("abc") +} + +//--- rename-spec.json + +[ + { + "key.name": "myCase(:)", + "key.locations": [ + { + "key.line": 2, + "key.column": 8, + "key.nametype": source.syntacticrename.definition + } + ] + }, + { + "key.name": "myCase(:)", + "key.locations": [ + { + "key.line": 6, + "key.column": 14, + "key.nametype": source.syntacticrename.call + } + ] + } +] diff --git a/test/refactoring/SyntacticRename/Outputs/types/case-other.swift.expected b/test/refactoring/SyntacticRename/Outputs/types/case-other.swift.expected index 915828f17a671..2a1786116f14d 100644 --- a/test/refactoring/SyntacticRename/Outputs/types/case-other.swift.expected +++ b/test/refactoring/SyntacticRename/Outputs/types/case-other.swift.expected @@ -27,7 +27,7 @@ extension /*class-Animal:def*/Animal { enum /*enum-Barcode:def*/Barcode { case upc(Int, Int, Int, Int) case /*case-qrCode:def*/qrCode(code: String) - case /*case-other:def*/other(Int) + case /*case-other:def*/other(Int) case /*case-another:def*/another } var barCode: /*enum-Barcode*/Barcode = /*enum-Barcode*/Barcode.upc(1, 1, 1, 1)