Skip to content

Commit f166b0d

Browse files
bjhomerbjhomer
committed
Make the try? migration simpler and more reliable.
We were trying to get fancy rewriting cases where multiple 'if let' clauses were used to unwrap a double optional, but in testing that turned out to be too unreliable. Now we simply detect any 'try?' expressions whose behavior will change, and add an explicit 'as OldType' to the expression, in order to preserve the original behavior.
1 parent c371c06 commit f166b0d

File tree

3 files changed

+226
-111
lines changed

3 files changed

+226
-111
lines changed

Diff for: ‎lib/Migrator/OptionalTryMigratorPass.cpp

+44-62
Original file line numberDiff line numberDiff line change
@@ -28,76 +28,58 @@ namespace {
2828
class OptionalTryMigratorPass: public ASTMigratorPass,
2929
public SourceEntityWalker {
3030

31-
std::vector<DeclContext *> ConditionStack;
31+
bool explicitCastActiveForOptionalTry = false;
3232

33-
void handleIfAndGuard(const LabeledConditionalStmt *IF) {
34-
// Look for cases like this:
35-
// if let optX = try? somethingOptional(),
36-
// let x = optX { }
37-
38-
NamedPattern *tempVarNamePattern = nullptr;
39-
SourceLoc endOfPrecedingElement;
33+
bool walkToExprPre(Expr *E) override {
34+
if (dyn_cast<ParenExpr>(E) || E->isImplicit()) {
35+
// Look through parentheses and implicit expressions.
36+
return true;
37+
}
4038

41-
for (auto conditionElement : IF->getCond()) {
42-
if (conditionElement.getKind() != StmtConditionElement::CK_PatternBinding) {
43-
continue;
44-
}
45-
46-
const auto pattern = dyn_cast<OptionalSomePattern>(conditionElement.getPattern());
47-
if (!pattern) {
48-
// We are only concerned with patterns that are unwrapping optionals.
49-
continue;
50-
}
51-
52-
const auto initializer = conditionElement.getInitializer();
53-
54-
if (tempVarNamePattern && initializer->getKind() == ExprKind::DeclRef) {
55-
const auto declExpr = static_cast<DeclRefExpr *>(initializer);
56-
57-
if (const auto rhsVarDecl = dyn_cast<VarDecl>(declExpr->getDecl())) {
58-
if (rhsVarDecl->getName() == tempVarNamePattern->getBoundName()) {
59-
60-
// We found an unwrap binding of our previous 'try?' variable
61-
auto thisVarPattern = dyn_cast<VarPattern>(pattern->getSubPattern());
62-
if (!thisVarPattern) { continue; }
63-
64-
// Use the final unwrapped variable name for the original 'try?' expression
65-
auto tempVarRange = Lexer::getCharSourceRangeFromSourceRange(SM, tempVarNamePattern->getSourceRange());
66-
Editor.replace(tempVarRange, thisVarPattern->getBoundName().str());
67-
68-
// Remove this condition element that only served to unwrap the try.
69-
auto rangeToRemove = SourceRange(endOfPrecedingElement, conditionElement.getEndLoc());
70-
auto charRangeToRemove = Lexer::getCharSourceRangeFromSourceRange(SM, rangeToRemove);
71-
Editor.remove(charRangeToRemove);
72-
73-
// We've made our change. Clear out the tempVarNamePattern to allow for
74-
// finding another pattern in this 'if'
75-
tempVarNamePattern = nullptr;
76-
}
77-
}
78-
}
79-
else if (const auto optTryExpr = dyn_cast<OptionalTryExpr>(initializer)) {
80-
if (!optTryExpr->getSubExpr()->getType()->getOptionalObjectType()) {
81-
continue;
82-
}
83-
// This is a `try?` wrapping an optional. It's behavior has changed in Swift 5
84-
const auto VP = dyn_cast<VarPattern>(pattern->getSubPattern());
85-
const auto NP = dyn_cast<NamedPattern>(VP->getSubPattern());
86-
if (!NP) { continue; }
87-
88-
tempVarNamePattern = NP;
89-
}
90-
endOfPrecedingElement = conditionElement.getEndLoc().getAdvancedLoc(1);
39+
if (const auto *explicitCastExpr = dyn_cast<ExplicitCastExpr>(E)) {
40+
// If the user has already provided an explicit cast for the
41+
// 'try?', then we don't need to add one. So let's track whether
42+
// one is active
43+
explicitCastActiveForOptionalTry = true;
44+
}
45+
else if (const auto *optTryExpr = dyn_cast<OptionalTryExpr>(E)) {
46+
wrapTryInCastIfNeeded(optTryExpr);
47+
return false;
48+
}
49+
else if (explicitCastActiveForOptionalTry) {
50+
// If an explicit cast is active and we are entering a new
51+
// expression that is not an OptionalTryExpr, then the cast
52+
// does not apply to the OptionalTryExpr.
53+
explicitCastActiveForOptionalTry = false;
9154
}
55+
return true;
9256
}
9357

94-
bool walkToStmtPre(Stmt *S) override {
95-
if (const auto *IF = dyn_cast<LabeledConditionalStmt>(S)) {
96-
handleIfAndGuard(IF);
97-
}
58+
bool walkToExprPost(Expr *E) override {
59+
explicitCastActiveForOptionalTry = false;
9860
return true;
9961
}
10062

63+
void wrapTryInCastIfNeeded(const OptionalTryExpr *optTryExpr) {
64+
if (explicitCastActiveForOptionalTry) {
65+
// There's already an explicit cast here; we don't need to add anything
66+
return;
67+
}
68+
69+
if (!optTryExpr->getSubExpr()->getType()->getOptionalObjectType()) {
70+
// This 'try?' doesn't wrap an optional, so its behavior does not
71+
// change from Swift 4 to Swift 5
72+
return;
73+
}
74+
75+
Type typeToPreserve = optTryExpr->getType();
76+
auto typeName = typeToPreserve->getStringAsComponent();
77+
78+
auto range = optTryExpr->getSourceRange();
79+
auto charRange = Lexer::getCharSourceRangeFromSourceRange(SM, range);
80+
Editor.insertWrap("((", charRange, (Twine(") as ") + typeName + ")").str());
81+
}
82+
10183
public:
10284
OptionalTryMigratorPass(EditorAdapter &Editor,
10385
SourceFile *SF,

Diff for: ‎test/Migrator/optional_try_migration.swift

+91-25
Original file line numberDiff line numberDiff line change
@@ -1,46 +1,112 @@
11
// RUN: %empty-directory(%t)
2-
// RUN: %target-swift-frontend -c -update-code -swift-version 4 -primary-file %s -emit-migrated-file-path %t/optional_try_migration.result.swift -o %t/rename-func-decl.swift.remap
2+
// RUN: %target-swift-frontend -c -swift-version 4 -primary-file %s -emit-migrated-file-path %t/optional_try_migration.result.swift
33
// RUN: diff -u %S/optional_try_migration.swift.expected %t/optional_try_migration.result.swift
44

5-
func throwOrOptional() throws -> Int? {
5+
func fetchOptInt() throws -> Int? {
66
return 3
77
}
88

9-
func throwOrInt() throws -> Int {
9+
func fetchInt() throws -> Int {
1010
return 3
1111
}
1212

13-
func doubleOptional() -> Int?? {
13+
func fetchAny() throws -> Any {
1414
return 3
1515
}
1616

17-
func testIfLet() {
18-
if let optionalX = try? throwOrOptional(), // This comment should be preserved
19-
let y = Optional(3), // This will stay here
20-
let x = optionalX // this comment will be moved
21-
{
22-
print(x, y)
23-
return
17+
func testOnlyMigrateChangedBehavior() {
18+
// No migration needed
19+
let _ = try? fetchInt()
20+
21+
// Migration needed
22+
let _ = try? fetchOptInt()
23+
}
24+
25+
func testExplicitCasts() {
26+
27+
// No migration needed, because there's an explicit cast on the try already
28+
let _ = (try? fetchOptInt()) as? Int
29+
30+
// Migration needed; the 'as? Int' is part of the sub-expression
31+
let _ = try? fetchAny() as? Int
32+
33+
// No migration needed; the subexpression is non-optional so behavior has not changed
34+
let _ = (try? fetchAny()) as? Int
35+
36+
// No migration needed, because there's an explicit cast on the try already
37+
let _ = (try? fetchOptInt()) as! Int // expected-warning {{forced cast from 'Int??' to 'Int' only unwraps optionals; did you mean to use '!!'?}}
38+
39+
// No migration needed; the subexpression is non-optional
40+
let _ = try? fetchAny() as! Int
41+
42+
// No migration needed; the subexpression is non-optional so behavior has not changed
43+
let _ = (try? fetchAny()) as! Int
44+
45+
// Migration needed; the explicit cast is not directly on the try?
46+
let _ = String(describing: try? fetchOptInt()) as Any
47+
48+
// No migration needed, because the try's subexpression is non-optional
49+
let _ = String(describing: try? fetchInt()) as Any
50+
51+
}
52+
53+
func testOptionalChaining() {
54+
struct Thing {
55+
func fetchInt() throws -> Int { return 3 }
56+
func fetchOptInt() throws -> Int { return 3 }
2457
}
2558

26-
if let optionalX = doubleOptional(),
27-
let x = optionalX
59+
let thing = Thing()
60+
let optThing: Thing? = Thing()
61+
62+
// Migration needed
63+
let _ = try? optThing?.fetchInt()
64+
65+
// Migration needed
66+
let _ = try? optThing?.fetchOptInt()
67+
68+
// No migration needed
69+
let _ = try? optThing!.fetchOptInt()
70+
71+
// No migration needed, because of the explicit cast
72+
let _ = (try? optThing?.fetchOptInt()) as? Int
73+
74+
// Migration needed
75+
let _ = try? thing.fetchInt()
76+
77+
// Migration needed
78+
let _ = try? thing.fetchOptInt()
79+
80+
// No migration needed, because of the explicit cast
81+
let _ = (try? thing.fetchOptInt()) as! Int // expected-warning {{forced cast from 'Int?' to 'Int' only unwraps optionals; did you mean to use '!'?}}
82+
}
83+
84+
85+
func testIfLet() {
86+
87+
// Migration needed
88+
if let optionalX = try? fetchOptInt(),
89+
let x = optionalX
2890
{
29-
// This one shouldn't be changed
3091
print(x)
3192
}
32-
33-
if let x = try? throwOrInt(),
34-
let y = try? throwOrInt() {
35-
// This one shouldn't be changed
93+
94+
// Don't change 'try?'s that haven't changed behavior
95+
if let x = try? fetchInt(),
96+
let y = try? fetchInt() {
3697
print(x, y)
3798
}
99+
}
38100

39-
if let optionalX = try? throwOrOptional(),
40-
let optionalY = try? throwOrOptional(),
41-
let x = optionalX,
42-
let y = optionalY
43-
{
44-
print(x, y)
101+
102+
func testCaseMatching() {
103+
// Migration needed
104+
if case let x?? = try? fetchOptInt() {
105+
print(x)
45106
}
46-
}
107+
108+
// No migration needed
109+
if case let x? = try? fetchInt() {
110+
print(x)
111+
}
112+
}

Diff for: ‎test/Migrator/optional_try_migration.swift.expected

+91-24
Original file line numberDiff line numberDiff line change
@@ -1,45 +1,112 @@
11
// RUN: %empty-directory(%t)
2-
// RUN: %target-swift-frontend -c -update-code -swift-version 4 -primary-file %s -emit-migrated-file-path %t/optional_try_migration.result.swift -o %t/rename-func-decl.swift.remap
2+
// RUN: %target-swift-frontend -c -swift-version 4 -primary-file %s -emit-migrated-file-path %t/optional_try_migration.result.swift
33
// RUN: diff -u %S/optional_try_migration.swift.expected %t/optional_try_migration.result.swift
44

5-
func throwOrOptional() throws -> Int? {
5+
func fetchOptInt() throws -> Int? {
66
return 3
77
}
88

9-
func throwOrInt() throws -> Int {
9+
func fetchInt() throws -> Int {
1010
return 3
1111
}
1212

13-
func doubleOptional() -> Int?? {
13+
func fetchAny() throws -> Any {
1414
return 3
1515
}
1616

17-
func testIfLet() {
18-
if let x = try? throwOrOptional(), // This comment should be preserved
19-
let y = Optional(3) // this will stay here // this comment will be moved
20-
{
21-
print(x, y)
22-
return
17+
func testOnlyMigrateChangedBehavior() {
18+
// No migration needed
19+
let _ = try? fetchInt()
20+
21+
// Migration needed
22+
let _ = ((try? fetchOptInt()) as Int??)
23+
}
24+
25+
func testExplicitCasts() {
26+
27+
// No migration needed, because there's an explicit cast on the try already
28+
let _ = (try? fetchOptInt()) as? Int
29+
30+
// Migration needed; the 'as? Int' is part of the sub-expression
31+
let _ = ((try? fetchAny() as? Int) as Int??)
32+
33+
// No migration needed; the subexpression is non-optional so behavior has not changed
34+
let _ = (try? fetchAny()) as? Int
35+
36+
// No migration needed, because there's an explicit cast on the try already
37+
let _ = (try? fetchOptInt()) as! Int // expected-warning {{forced cast from 'Int??' to 'Int' only unwraps optionals; did you mean to use '!!'?}}
38+
39+
// No migration needed; the subexpression is non-optional
40+
let _ = try? fetchAny() as! Int
41+
42+
// No migration needed; the subexpression is non-optional so behavior has not changed
43+
let _ = (try? fetchAny()) as! Int
44+
45+
// Migration needed; the explicit cast is not directly on the try?
46+
let _ = String(describing: ((try? fetchOptInt()) as Int??)) as Any
47+
48+
// No migration needed, because the try's subexpression is non-optional
49+
let _ = String(describing: try? fetchInt()) as Any
50+
51+
}
52+
53+
func testOptionalChaining() {
54+
struct Thing {
55+
func fetchInt() throws -> Int { return 3 }
56+
func fetchOptInt() throws -> Int { return 3 }
2357
}
2458

25-
if let optionalX = doubleOptional(),
26-
let x = optionalX
59+
let thing = Thing()
60+
let optThing: Thing? = Thing()
61+
62+
// Migration needed
63+
let _ = ((try? optThing?.fetchInt()) as Int??)
64+
65+
// Migration needed
66+
let _ = ((try? optThing?.fetchOptInt()) as Int??)
67+
68+
// No migration needed
69+
let _ = try? optThing!.fetchOptInt()
70+
71+
// No migration needed, because of the explicit cast
72+
let _ = (try? optThing?.fetchOptInt()) as? Int
73+
74+
// Migration needed
75+
let _ = try? thing.fetchInt()
76+
77+
// Migration needed
78+
let _ = try? thing.fetchOptInt()
79+
80+
// No migration needed, because of the explicit cast
81+
let _ = (try? thing.fetchOptInt()) as! Int // expected-warning {{forced cast from 'Int?' to 'Int' only unwraps optionals; did you mean to use '!'?}}
82+
}
83+
84+
85+
func testIfLet() {
86+
87+
// Migration needed
88+
if let optionalX = ((try? fetchOptInt()) as Int??),
89+
let x = optionalX
2790
{
28-
// This one shouldn't be changed
2991
print(x)
3092
}
31-
32-
if let x = try? throwOrInt(),
33-
let y = try? throwOrInt() {
34-
// This one shouldn't be changed
93+
94+
// Don't change 'try?'s that haven't changed behavior
95+
if let x = try? fetchInt(),
96+
let y = try? fetchInt() {
3597
print(x, y)
3698
}
99+
}
37100

38-
if let optionalX = try? throwOrOptional(),
39-
let optionalY = try? throwOrOptional(),
40-
let x = optionalX,
41-
let y = optionalY
42-
{
43-
print(x, y)
101+
102+
func testCaseMatching() {
103+
// Migration needed
104+
if case let x?? = ((try? fetchOptInt()) as Int??) {
105+
print(x)
44106
}
45-
}
107+
108+
// No migration needed
109+
if case let x? = try? fetchInt() {
110+
print(x)
111+
}
112+
}

0 commit comments

Comments
 (0)