Skip to content

Commit 625cd94

Browse files
author
Nathan Hawes
authored
Merge pull request #21447 from nathawes/r42162571-rename-calls-with-trailing-closure-ignored-if-param-has-label
[Refactoring][Migrator] Fix rename of callsites with a trailing closure argument
2 parents b697b2c + 43c2f27 commit 625cd94

16 files changed

+212
-4
lines changed

‎include/swift/IDE/Utils.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -598,13 +598,16 @@ enum class LabelRangeEndAt: int8_t {
598598
struct CallArgInfo {
599599
Expr *ArgExp;
600600
CharSourceRange LabelRange;
601+
bool IsTrailingClosure;
601602
CharSourceRange getEntireCharRange(const SourceManager &SM) const;
602603
};
603604

604605
std::vector<CallArgInfo>
605606
getCallArgInfo(SourceManager &SM, Expr *Arg, LabelRangeEndAt EndKind);
606607

607608
// Get the ranges of argument labels from an Arg, either tuple or paren.
609+
// This includes empty ranges for any unlabelled arguments, and excludes
610+
// trailing closures.
608611
std::vector<CharSourceRange>
609612
getCallArgLabelRanges(SourceManager &SM, Expr *Arg, LabelRangeEndAt EndKind);
610613

‎lib/IDE/SwiftSourceDocInfo.cpp

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1640,15 +1640,18 @@ getCallArgInfo(SourceManager &SM, Expr *Arg, LabelRangeEndAt EndKind) {
16401640
if (EndKind == LabelRangeEndAt::LabelNameOnly)
16411641
LabelEnd = LabelStart.getAdvancedLoc(NameIdentifier.getLength());
16421642
}
1643-
1643+
bool IsTrailingClosure = TE->hasTrailingClosure() &&
1644+
ElemIndex == TE->getNumElements() - 1;
16441645
InfoVec.push_back({getSingleNonImplicitChild(Elem),
1645-
CharSourceRange(SM, LabelStart, LabelEnd)});
1646+
CharSourceRange(SM, LabelStart, LabelEnd), IsTrailingClosure});
16461647
++ElemIndex;
16471648
}
16481649
} else if (auto *PE = dyn_cast<ParenExpr>(Arg)) {
16491650
if (auto Sub = PE->getSubExpr())
16501651
InfoVec.push_back({getSingleNonImplicitChild(Sub),
1651-
CharSourceRange(Sub->getStartLoc(), 0)});
1652+
CharSourceRange(Sub->getStartLoc(), 0),
1653+
PE->hasTrailingClosure()
1654+
});
16521655
}
16531656
return InfoVec;
16541657
}
@@ -1657,7 +1660,13 @@ std::vector<CharSourceRange> swift::ide::
16571660
getCallArgLabelRanges(SourceManager &SM, Expr *Arg, LabelRangeEndAt EndKind) {
16581661
std::vector<CharSourceRange> Ranges;
16591662
auto InfoVec = getCallArgInfo(SM, Arg, EndKind);
1660-
std::transform(InfoVec.begin(), InfoVec.end(), std::back_inserter(Ranges),
1663+
1664+
auto EndWithoutTrailing = std::remove_if(InfoVec.begin(), InfoVec.end(),
1665+
[](CallArgInfo &Info) {
1666+
return Info.IsTrailingClosure;
1667+
});
1668+
std::transform(InfoVec.begin(), EndWithoutTrailing,
1669+
std::back_inserter(Ranges),
16611670
[](CallArgInfo &Info) { return Info.LabelRange; });
16621671
return Ranges;
16631672
}

‎test/Migrator/Inputs/API.json

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -529,6 +529,17 @@
529529
"RightComment": "Int",
530530
"ModuleName": "Cities"
531531
},
532+
{
533+
"DiffItemKind": "CommonDiffItem",
534+
"NodeKind": "Function",
535+
"NodeAnnotation": "Rename",
536+
"ChildIndex": "0",
537+
"LeftUsr": "s:6Cities12ToplevelTypeC8trailingyyySayypGSgcF",
538+
"LeftComment": "",
539+
"RightUsr": "",
540+
"RightComment": "trailing2(a:)",
541+
"ModuleName": "Cities"
542+
},
532543
{
533544
"DiffItemKind": "CommonDiffItem",
534545
"NodeKind": "TypeDecl",

‎test/Migrator/Inputs/Cities.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ open class ToplevelType {
6262
public init() {}
6363
public init(recordName: String) {}
6464
open func member(_ x: @escaping ([Any]?) -> Void) {}
65+
open func trailing(_ x: @escaping ([Any]?) -> Void) {}
6566
}
6667

6768
public var GlobalAttribute: String = ""

‎test/Migrator/rename-func-decl.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,7 @@ class MySubTopLevelType2: ToplevelType {
2222
class SubCities: Cities {
2323
override var yogurt: Int { return 2 }
2424
}
25+
26+
func boo(_ a: ToplevelType) {
27+
a.trailing { print($0!) }
28+
}

‎test/Migrator/rename-func-decl.swift.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,7 @@ class MySubTopLevelType2: ToplevelType {
2222
class SubCities: Cities {
2323
override var cheese: Int { return 2 }
2424
}
25+
26+
func boo(_ a: ToplevelType) {
27+
a.trailing2 { print($0!) }
28+
}

‎test/refactoring/SyntacticRename/FindRangeOutputs/callsites/defaults.swift.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ func /*trailing:def*/withTrailingClosure(x: Int, y: () -> Int) {}
2828
/*trailing:call*/withTrailingClosure(x: 2)
2929
{ return 1}
3030

31+
func /*trailing-only:def*/trailingOnly(a: () -> ()) {}
32+
/*trailing-only:call*/trailingOnly(a: {})
33+
/*trailing-only:call*/trailingOnly {}
34+
3135

3236
func /*varargs:def*/withVarargs(x: Int..., y: Int, _: Int) {}
3337

‎test/refactoring/SyntacticRename/FindRangeOutputs/callsites/trailing.swift.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ func /*trailing:def*/<base>withTrailingClosure</base>(<arglabel index=0>x</argla
2828
/*trailing:call*/<base>withTrailingClosure</base>(<callarg index=0>x</callarg><callcolon index=0>: </callcolon>2)
2929
{ return 1}
3030

31+
func /*trailing-only:def*/trailingOnly(a: () -> ()) {}
32+
/*trailing-only:call*/trailingOnly(a: {})
33+
/*trailing-only:call*/trailingOnly {}
34+
3135

3236
func /*varargs:def*/withVarargs(x: Int..., y: Int, _: Int) {}
3337

‎test/refactoring/SyntacticRename/FindRangeOutputs/callsites/trailing_only.swift.expected

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
func /*defaults:def*/withDefaults(_ xx: Int = 4, y: Int = 2, x: Int = 1) {}
2+
3+
// valid
4+
/*defaults:call*/withDefaults()
5+
/*defaults:call*/withDefaults(2)
6+
/*defaults:call*/withDefaults(y: 2)
7+
/*defaults:call*/withDefaults(2, x: 3)
8+
/*defaults:call*/withDefaults(y: 2, x: 3)
9+
/*defaults:call*/withDefaults(2, y: 1, x: 4)
10+
11+
// false positives
12+
/*defaults:call*/withDefaults(y: 2, 3)
13+
/*defaults:call*/withDefaults(y: 2, 4, x: 3)
14+
15+
// invalid
16+
/*defaults:call*/withDefaults(x: 2, y: 3)
17+
18+
19+
func /*trailing:def*/withTrailingClosure(x: Int, y: () -> Int) {}
20+
21+
// valid
22+
/*trailing:call*/withTrailingClosure(x: 2, y: { return 1})
23+
/*trailing:call*/withTrailingClosure(x: 2) { return 1}
24+
25+
// false positives
26+
/*trailing:call*/withTrailingClosure(x: 1, y: 2) { return 1}
27+
/*trailing:call*/withTrailingClosure(x: 1, y: 2) { return 1}
28+
/*trailing:call*/withTrailingClosure(x: 2)
29+
{ return 1}
30+
31+
func /*trailing-only:def*/<base>trailingOnly</base>(<arglabel index=0>a</arglabel><param index=0></param>: () -> ()) {}
32+
/*trailing-only:call*/<base>trailingOnly</base>(<callarg index=0>a</callarg><callcolon index=0>: </callcolon>{})
33+
/*trailing-only:call*/<base>trailingOnly</base> {}
34+
35+
36+
func /*varargs:def*/withVarargs(x: Int..., y: Int, _: Int) {}
37+
38+
// valid
39+
/*varargs:call*/withVarargs(x: 1, 2, 3, y: 2, 4)
40+
/*varargs:call*/withVarargs(y: 2, 4)
41+
42+
// false positives
43+
/*varargs:call*/withVarargs(x: 1, y: 2, 4, 5)
44+
45+
//invalid
46+
/*varargs:call*/withVarargs(2, y: 2)
47+
48+
49+
func /*varargs2:def*/withVarargs(x: Int, y: Int, _: Int...) {}
50+
51+
// valid
52+
/*varargs2:call*/withVarargs(x: 1, y: 2, 4, 5)
53+
/*varargs2:call*/withVarargs(x: 1, y: 2)
54+
55+
// false positive
56+
/*varargs2:call*/withVarargs(x: 1, 2, y: 2, 4)
57+
58+
59+
func /*mixed:def*/withAllOfTheAbove(x: Int = 2, _: Int..., z: Int = 2, c: () -> Int) {}
60+
61+
// valid
62+
/*mixed:call*/withAllOfTheAbove(2){ return 1 }
63+
/*mixed:call*/withAllOfTheAbove(x: 1, 2, c: {return 1})
64+
/*mixed:call*/withAllOfTheAbove(x: 1, c: {return 1})
65+
/*mixed:call*/withAllOfTheAbove(1, z: 1) { return 1 }
66+
/*mixed:call*/withAllOfTheAbove(1, 2, c: {return 1})
67+
68+
// false positives
69+
/*mixed:call*/withAllOfTheAbove(z: 1, 2, c: {return 1})
70+

‎test/refactoring/SyntacticRename/Outputs/callsites/defaults.swift.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ func /*trailing:def*/withTrailingClosure(x: Int, y: () -> Int) {}
2828
/*trailing:call*/withTrailingClosure(x: 2)
2929
{ return 1}
3030

31+
func /*trailing-only:def*/trailingOnly(a: () -> ()) {}
32+
/*trailing-only:call*/trailingOnly(a: {})
33+
/*trailing-only:call*/trailingOnly {}
34+
3135

3236
func /*varargs:def*/withVarargs(x: Int..., y: Int, _: Int) {}
3337

‎test/refactoring/SyntacticRename/Outputs/callsites/mixed.swift.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ func /*trailing:def*/withTrailingClosure(x: Int, y: () -> Int) {}
2828
/*trailing:call*/withTrailingClosure(x: 2)
2929
{ return 1}
3030

31+
func /*trailing-only:def*/trailingOnly(a: () -> ()) {}
32+
/*trailing-only:call*/trailingOnly(a: {})
33+
/*trailing-only:call*/trailingOnly {}
34+
3135

3236
func /*varargs:def*/withVarargs(x: Int..., y: Int, _: Int) {}
3337

‎test/refactoring/SyntacticRename/Outputs/callsites/trailing.swift.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ func /*trailing:def*/betterName(a: Int, b: () -> Int) {}
2828
/*trailing:call*/betterName(a: 2)
2929
{ return 1}
3030

31+
func /*trailing-only:def*/trailingOnly(a: () -> ()) {}
32+
/*trailing-only:call*/trailingOnly(a: {})
33+
/*trailing-only:call*/trailingOnly {}
34+
3135

3236
func /*varargs:def*/withVarargs(x: Int..., y: Int, _: Int) {}
3337

‎test/refactoring/SyntacticRename/Outputs/callsites/trailing_only.swift.expected

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
func /*defaults:def*/withDefaults(_ xx: Int = 4, y: Int = 2, x: Int = 1) {}
2+
3+
// valid
4+
/*defaults:call*/withDefaults()
5+
/*defaults:call*/withDefaults(2)
6+
/*defaults:call*/withDefaults(y: 2)
7+
/*defaults:call*/withDefaults(2, x: 3)
8+
/*defaults:call*/withDefaults(y: 2, x: 3)
9+
/*defaults:call*/withDefaults(2, y: 1, x: 4)
10+
11+
// false positives
12+
/*defaults:call*/withDefaults(y: 2, 3)
13+
/*defaults:call*/withDefaults(y: 2, 4, x: 3)
14+
15+
// invalid
16+
/*defaults:call*/withDefaults(x: 2, y: 3)
17+
18+
19+
func /*trailing:def*/withTrailingClosure(x: Int, y: () -> Int) {}
20+
21+
// valid
22+
/*trailing:call*/withTrailingClosure(x: 2, y: { return 1})
23+
/*trailing:call*/withTrailingClosure(x: 2) { return 1}
24+
25+
// false positives
26+
/*trailing:call*/withTrailingClosure(x: 1, y: 2) { return 1}
27+
/*trailing:call*/withTrailingClosure(x: 1, y: 2) { return 1}
28+
/*trailing:call*/withTrailingClosure(x: 2)
29+
{ return 1}
30+
31+
func /*trailing-only:def*/betterName(b: () -> ()) {}
32+
/*trailing-only:call*/betterName(b: {})
33+
/*trailing-only:call*/betterName {}
34+
35+
36+
func /*varargs:def*/withVarargs(x: Int..., y: Int, _: Int) {}
37+
38+
// valid
39+
/*varargs:call*/withVarargs(x: 1, 2, 3, y: 2, 4)
40+
/*varargs:call*/withVarargs(y: 2, 4)
41+
42+
// false positives
43+
/*varargs:call*/withVarargs(x: 1, y: 2, 4, 5)
44+
45+
//invalid
46+
/*varargs:call*/withVarargs(2, y: 2)
47+
48+
49+
func /*varargs2:def*/withVarargs(x: Int, y: Int, _: Int...) {}
50+
51+
// valid
52+
/*varargs2:call*/withVarargs(x: 1, y: 2, 4, 5)
53+
/*varargs2:call*/withVarargs(x: 1, y: 2)
54+
55+
// false positive
56+
/*varargs2:call*/withVarargs(x: 1, 2, y: 2, 4)
57+
58+
59+
func /*mixed:def*/withAllOfTheAbove(x: Int = 2, _: Int..., z: Int = 2, c: () -> Int) {}
60+
61+
// valid
62+
/*mixed:call*/withAllOfTheAbove(2){ return 1 }
63+
/*mixed:call*/withAllOfTheAbove(x: 1, 2, c: {return 1})
64+
/*mixed:call*/withAllOfTheAbove(x: 1, c: {return 1})
65+
/*mixed:call*/withAllOfTheAbove(1, z: 1) { return 1 }
66+
/*mixed:call*/withAllOfTheAbove(1, 2, c: {return 1})
67+
68+
// false positives
69+
/*mixed:call*/withAllOfTheAbove(z: 1, 2, c: {return 1})
70+

‎test/refactoring/SyntacticRename/Outputs/callsites/varargs.swift.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ func /*trailing:def*/withTrailingClosure(x: Int, y: () -> Int) {}
2828
/*trailing:call*/withTrailingClosure(x: 2)
2929
{ return 1}
3030

31+
func /*trailing-only:def*/trailingOnly(a: () -> ()) {}
32+
/*trailing-only:call*/trailingOnly(a: {})
33+
/*trailing-only:call*/trailingOnly {}
34+
3135

3236
func /*varargs:def*/betterName(a: Int..., b: Int, c: Int) {}
3337

‎test/refactoring/SyntacticRename/Outputs/callsites/varargs2.swift.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ func /*trailing:def*/withTrailingClosure(x: Int, y: () -> Int) {}
2828
/*trailing:call*/withTrailingClosure(x: 2)
2929
{ return 1}
3030

31+
func /*trailing-only:def*/trailingOnly(a: () -> ()) {}
32+
/*trailing-only:call*/trailingOnly(a: {})
33+
/*trailing-only:call*/trailingOnly {}
34+
3135

3236
func /*varargs:def*/withVarargs(x: Int..., y: Int, _: Int) {}
3337

‎test/refactoring/SyntacticRename/callsites.swift

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ func /*trailing:def*/withTrailingClosure(x: Int, y: () -> Int) {}
2828
/*trailing:call*/withTrailingClosure(x: 2)
2929
{ return 1}
3030

31+
func /*trailing-only:def*/trailingOnly(a: () -> ()) {}
32+
/*trailing-only:call*/trailingOnly(a: {})
33+
/*trailing-only:call*/trailingOnly {}
34+
3135

3236
func /*varargs:def*/withVarargs(x: Int..., y: Int, _: Int) {}
3337

@@ -69,6 +73,8 @@ func /*mixed:def*/withAllOfTheAbove(x: Int = 2, _: Int..., z: Int = 2, c: () ->
6973
// RUN: diff -u %S/Outputs/callsites/defaults.swift.expected %t.result/callsites_defaults.swift
7074
// RUN: %refactor -syntactic-rename -source-filename %s -pos="trailing" -is-function-like -old-name "withTrailingClosure(x:y:)" -new-name "betterName(a:b:)" >> %t.result/callsites_trailing.swift
7175
// RUN: diff -u %S/Outputs/callsites/trailing.swift.expected %t.result/callsites_trailing.swift
76+
// RUN: %refactor -syntactic-rename -source-filename %s -pos="trailing-only" -is-function-like -old-name "trailingOnly(a:)" -new-name "betterName(b:)" >> %t.result/callsites_trailing_only.swift
77+
// RUN: diff -u %S/Outputs/callsites/trailing_only.swift.expected %t.result/callsites_trailing_only.swift
7278
// RUN: %refactor -syntactic-rename -source-filename %s -pos="varargs" -is-function-like -old-name "withVarargs(x:y:_:)" -new-name "betterName(a:b:c:)" >> %t.result/callsites_varargs.swift
7379
// RUN: diff -u %S/Outputs/callsites/varargs.swift.expected %t.result/callsites_varargs.swift
7480
// RUN: %refactor -syntactic-rename -source-filename %s -pos="varargs2" -is-function-like -old-name "withVarargs(x:y:_:)" -new-name "betterName(a:b:c:)" >> %t.result/callsites_varargs2.swift
@@ -80,3 +86,5 @@ func /*mixed:def*/withAllOfTheAbove(x: Int = 2, _: Int..., z: Int = 2, c: () ->
8086
// RUN: diff -u %S/FindRangeOutputs/callsites/defaults.swift.expected %t.ranges/callsites_defaults.swift
8187
// RUN: %refactor -find-rename-ranges -source-filename %s -pos="trailing" -is-function-like -old-name "withTrailingClosure(x:y:)" >> %t.ranges/callsites_trailing.swift
8288
// RUN: diff -u %S/FindRangeOutputs/callsites/trailing.swift.expected %t.ranges/callsites_trailing.swift
89+
// RUN: %refactor -find-rename-ranges -source-filename %s -pos="trailing-only" -is-function-like -old-name "trailingOnly(a:)" >> %t.ranges/callsites_trailing_only.swift
90+
// RUN: diff -u %S/FindRangeOutputs/callsites/trailing_only.swift.expected %t.ranges/callsites_trailing_only.swift

0 commit comments

Comments
 (0)