Skip to content

Commit 43c2f27

Browse files
author
Nathan Hawes
committed
[Refactoring][Migrator] Fix rename of callsites with a trailing closure argument
A label range of 0 length was being reported as the label of trailing closure arguments, just before the opening '{'. For the rename refactoring, this meant that if the corresponding parameter had an external label (e.g. 'a') the occurrence would be treated as not matching the expected symbol name, and so not be updated at all. For the migrator, when renaming a function with an unlabelled closure for its last parameter to have a label, it would incorrectly insert the new label in front of the opening '{' on all of that function's callsites with trailing closures. Resolves rdar://problem/42162571
1 parent f85bb3e commit 43c2f27

16 files changed

+212
-4
lines changed

‎include/swift/IDE/Utils.h

+3
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

+13-4
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

+11
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

+1
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

+4
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

+4
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

+4
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

+4
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

+70
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

+4
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

+4
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

+4
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

+70
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

+4
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

+4
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

+8
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)