Skip to content

Commit d86f41a

Browse files
committed
Improve Fix-It for if let x where x is a reference to an unsafe value
When we encounter unsafe code in `if let x`, we would produce a Fix-It that would change it to the ill-formed `if let unsafe x`. Improve tracking of the expressions that are synthesized for the right-hand side of these conditions, so that we can produce a Fix-It that turns this into the proper if let x = unsafe x Fixes rdar://147944243.
1 parent 90a2b3d commit d86f41a

File tree

3 files changed

+94
-9
lines changed

3 files changed

+94
-9
lines changed

lib/Sema/TypeCheckEffects.cpp

Lines changed: 74 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -668,7 +668,10 @@ class EffectsHandlingWalker : public ASTWalker {
668668
recurse = asImpl().checkThrow(thr);
669669
} else if (auto forEach = dyn_cast<ForEachStmt>(S)) {
670670
recurse = asImpl().checkForEach(forEach);
671+
} else if (auto labeled = dyn_cast<LabeledConditionalStmt>(S)) {
672+
asImpl().noteLabeledConditionalStmt(labeled);
671673
}
674+
672675
if (!recurse)
673676
return Action::SkipNode(S);
674677

@@ -690,6 +693,8 @@ class EffectsHandlingWalker : public ASTWalker {
690693
}
691694

692695
void visitExprPre(Expr *expr) { asImpl().visitExprPre(expr); }
696+
697+
void noteLabeledConditionalStmt(LabeledConditionalStmt *stmt) { }
693698
};
694699

695700
/// A potential reason why something might have an effect.
@@ -3416,6 +3421,10 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
34163421
/// passed directly into an explicitly `@safe` function.
34173422
llvm::DenseSet<const Expr *> assumedSafeArguments;
34183423

3424+
/// Keeps track of the expressions that were synthesized as initializers for
3425+
/// the "if let x" shorthand syntax.
3426+
llvm::SmallPtrSet<const Expr *, 4> synthesizedIfLetInitializers;
3427+
34193428
/// Tracks all of the uncovered uses of unsafe constructs based on their
34203429
/// anchor expression, so we can emit diagnostics at the end.
34213430
llvm::MapVector<Expr *, std::vector<UnsafeUse>> uncoveredUnsafeUses;
@@ -4425,7 +4434,67 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
44254434
Ctx.Diags.diagnose(E->getUnsafeLoc(), diag::no_unsafe_in_unsafe)
44264435
.fixItRemove(E->getUnsafeLoc());
44274436
}
4428-
4437+
4438+
void noteLabeledConditionalStmt(LabeledConditionalStmt *stmt) {
4439+
// Make a note of any initializers that are the synthesized right-hand side
4440+
// for an "if let x".
4441+
for (const auto &condition: stmt->getCond()) {
4442+
switch (condition.getKind()) {
4443+
case StmtConditionElement::CK_Availability:
4444+
case StmtConditionElement::CK_Boolean:
4445+
case StmtConditionElement::CK_HasSymbol:
4446+
continue;
4447+
4448+
case StmtConditionElement::CK_PatternBinding:
4449+
break;
4450+
}
4451+
4452+
auto init = condition.getInitializer();
4453+
if (!init)
4454+
continue;
4455+
4456+
auto pattern = condition.getPattern();
4457+
if (!pattern)
4458+
continue;
4459+
4460+
auto optPattern = dyn_cast<OptionalSomePattern>(pattern);
4461+
if (!optPattern)
4462+
continue;
4463+
4464+
auto var = optPattern->getSubPattern()->getSingleVar();
4465+
if (!var)
4466+
continue;
4467+
4468+
// If the right-hand side has the same location as the variable, it was
4469+
// synthesized.
4470+
if (var->getLoc().isValid() &&
4471+
var->getLoc() == init->getStartLoc() &&
4472+
init->getStartLoc() == init->getEndLoc())
4473+
synthesizedIfLetInitializers.insert(init);
4474+
}
4475+
}
4476+
4477+
/// Determine whether this is the synthesized right-hand-side when we have
4478+
/// expanded an "if let x" into its semantic equivalent, "if let x = x".
4479+
VarDecl *isShorthandIfLetSyntax(const Expr *expr) const {
4480+
// Check whether this is referencing a variable.
4481+
VarDecl *var = nullptr;
4482+
if (auto declRef = dyn_cast<DeclRefExpr>(expr)) {
4483+
var = dyn_cast_or_null<VarDecl>(declRef->getDecl());
4484+
} else if (auto memberRef = dyn_cast<MemberRefExpr>(expr)) {
4485+
var = dyn_cast_or_null<VarDecl>(memberRef->getMember().getDecl());
4486+
}
4487+
4488+
if (!var)
4489+
return nullptr;
4490+
4491+
// If we identified this as one of the bindings, return the variable.
4492+
if (synthesizedIfLetInitializers.contains(expr))
4493+
return var;
4494+
4495+
return nullptr;
4496+
}
4497+
44294498
std::pair<SourceLoc, std::string>
44304499
getFixItForUncoveredSite(const Expr *anchor, StringRef keyword) const {
44314500
SourceLoc insertLoc = anchor->getStartLoc();
@@ -4438,13 +4507,10 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
44384507
insertLoc = tryExpr->getSubExpr()->getStartLoc();
44394508
// Supply a tailored fixIt including the identifier if we are
44404509
// looking at a shorthand optional binding.
4441-
} else if (anchor->isImplicit()) {
4442-
if (auto declRef = dyn_cast<DeclRefExpr>(anchor))
4443-
if (auto var = dyn_cast_or_null<VarDecl>(declRef->getDecl())) {
4444-
insertText = (" = " + keyword).str() + " " + var->getNameStr().str();
4445-
insertLoc = Lexer::getLocForEndOfToken(Ctx.Diags.SourceMgr,
4446-
anchor->getStartLoc());
4447-
}
4510+
} else if (auto var = isShorthandIfLetSyntax(anchor)) {
4511+
insertText = (" = " + keyword).str() + " " + var->getNameStr().str();
4512+
insertLoc = Lexer::getLocForEndOfToken(Ctx.Diags.SourceMgr,
4513+
anchor->getStartLoc());
44484514
}
44494515
return std::make_pair(insertLoc, insertText);
44504516
}

test/Unsafe/safe.swift

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,3 +249,22 @@ func testUnsafeLHS() {
249249
default: 0
250250
}
251251
}
252+
253+
@safe
254+
struct UnsafeWrapTest {
255+
@unsafe var pointer: UnsafeMutablePointer<Int>?
256+
257+
func test() {
258+
if let pointer { // expected-warning{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{19-19= = unsafe pointer}}
259+
// expected-note@-1{{reference to unsafe property 'pointer'}}
260+
_ = unsafe pointer
261+
}
262+
}
263+
264+
func otherTest(pointer: UnsafeMutablePointer<Int>?) {
265+
if let pointer { // expected-warning{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{19-19= = unsafe pointer}}
266+
// expected-note@-1{{reference to parameter 'pointer' involves unsafe type 'UnsafeMutablePointer<Int>}}
267+
_ = unsafe pointer
268+
}
269+
}
270+
}

test/expr/unary/async_await.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ func testAsyncExprWithoutAwait() async {
232232
if let result = result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{19-19=await }}
233233
// expected-warning@-1 {{value 'result' was defined but never used; consider replacing with boolean test}}
234234
// expected-note@-2 {{reference to async let 'result' is 'async'}}
235-
if let result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{10-10=await }}
235+
if let result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{16-16= = await result}}
236236
// expected-warning@-1 {{value 'result' was defined but never used; consider replacing with boolean test}}
237237
// expected-note@-2 {{reference to async let 'result' is 'async'}}
238238
let a = f("a") // expected-error {{expression is 'async' but is not marked with 'await'}} {{11-11=await }}

0 commit comments

Comments
 (0)