Skip to content

Commit a517327

Browse files
author
Nathan Hawes
authored
Merge pull request #21467 from nathawes/r42162571-rename-callsites-with-trailing-closure-ignored-if-param-has-label-5.0
[5.0] Fix rename refactoring/migration issues with (1) trailing closures and (2) overridden properties
2 parents 8cff778 + 3238485 commit a517327

17 files changed

+246
-5
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
}

‎lib/Migrator/APIDiffMigratorPass.cpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1404,6 +1404,19 @@ struct APIDiffMigratorPass : public ASTMigratorPass, public SourceEntityWalker {
14041404
D->walk(Removal);
14051405
}
14061406
}
1407+
1408+
// Handle property overriding migration.
1409+
if (auto *VD = dyn_cast<VarDecl>(D)) {
1410+
for (auto *Item: getRelatedDiffItems(VD)) {
1411+
if (auto *CD = dyn_cast<CommonDiffItem>(Item)) {
1412+
// If the overriden property has been renamed, we should rename
1413+
// this property decl as well.
1414+
if (CD->isRename() && VD->getNameLoc().isValid()) {
1415+
Editor.replaceToken(VD->getNameLoc(), CD->getNewName());
1416+
}
1417+
}
1418+
}
1419+
}
14071420
return true;
14081421
}
14091422
};

‎test/Migrator/Inputs/API.json

Lines changed: 23 additions & 1 deletion
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",
@@ -539,5 +550,16 @@
539550
"RightUsr": "",
540551
"RightComment": "FontWeighting",
541552
"ModuleName": "Cities"
542-
}
553+
},
554+
{
555+
"DiffItemKind": "CommonDiffItem",
556+
"NodeKind": "Var",
557+
"NodeAnnotation": "Rename",
558+
"ChildIndex": "0",
559+
"LeftUsr": "s:6CitiesAAC6yogurtSivp",
560+
"LeftComment": "yogurt",
561+
"RightUsr": "",
562+
"RightComment": "cheese",
563+
"ModuleName": "Cities"
564+
},
543565
]

‎test/Migrator/Inputs/Cities.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ open class Cities {
99
open func buderim() -> Cities? { return Cities(x: 1) }
1010
open func noosa() -> [[String : Cities]?] { return [] }
1111
open func maroochy(x: Int?, y: Int?) {}
12+
open var yogurt: Int { return 1 }
1213
public struct CityKind {
1314
public static let Town = 1
1415
public static let Village = 1
@@ -61,6 +62,7 @@ open class ToplevelType {
6162
public init() {}
6263
public init(recordName: String) {}
6364
open func member(_ x: @escaping ([Any]?) -> Void) {}
65+
open func trailing(_ x: @escaping ([Any]?) -> Void) {}
6466
}
6567

6668
public var GlobalAttribute: String = ""

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,11 @@ class MySubTopLevelType: ToplevelType {
1818
class MySubTopLevelType2: ToplevelType {
1919
override func member(_ x: @escaping (((([(Any)])?))) -> Void) {}
2020
}
21+
22+
class SubCities: Cities {
23+
override var yogurt: Int { return 2 }
24+
}
25+
26+
func boo(_ a: ToplevelType) {
27+
a.trailing { print($0!) }
28+
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,11 @@ class MySubTopLevelType: ToplevelType {
1818
class MySubTopLevelType2: ToplevelType {
1919
override func member(_ x: @escaping (((([(Int)])?))) -> Void) {}
2020
}
21+
22+
class SubCities: Cities {
23+
override var cheese: Int { return 2 }
24+
}
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+

0 commit comments

Comments
 (0)