Skip to content

Commit f51d925

Browse files
rudkxrudkx
authored
Merge pull request #14221 from hamishknight/optional-to-any-diag
[Sema] Improve Optional-to-Any diagnostics for collections, nested optionals & IUOs
2 parents b2f1260 + 416cb7f commit f51d925

File tree

7 files changed

+360
-113
lines changed

7 files changed

+360
-113
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2753,13 +2753,14 @@ WARNING(optional_pattern_match_promotion,none,
27532753
"pattern match introduces an implicit promotion from %0 to %1",
27542754
(Type, Type))
27552755
WARNING(optional_to_any_coercion,none,
2756-
"expression implicitly coerced from %0 to Any", (Type))
2756+
"expression implicitly coerced from %0 to %1", (Type, Type))
27572757
NOTE(default_optional_to_any,none,
27582758
"provide a default value to avoid this warning", ())
27592759
NOTE(force_optional_to_any,none,
27602760
"force-unwrap the value to avoid this warning", ())
27612761
NOTE(silence_optional_to_any,none,
2762-
"explicitly cast to Any with 'as Any' to silence this warning", ())
2762+
"explicitly cast to %0 with '%1' to silence this warning",
2763+
(Type, StringRef))
27632764
WARNING(optional_in_string_interpolation_segment,none,
27642765
"string interpolation produces a debug description for an optional "
27652766
"value; did you mean to make this explicit?",

‎lib/Sema/MiscDiagnostics.cpp

Lines changed: 218 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -3449,7 +3449,211 @@ static void diagnoseUnintendedOptionalBehavior(TypeChecker &TC, const Expr *E,
34493449

34503450
class UnintendedOptionalBehaviorWalker : public ASTWalker {
34513451
TypeChecker &TC;
3452-
SmallPtrSet<Expr *, 4> ErasureCoercedToAny;
3452+
SmallPtrSet<Expr *, 16> IgnoredExprs;
3453+
3454+
class OptionalToAnyCoercion {
3455+
public:
3456+
Type DestType;
3457+
CoerceExpr *ParentCoercion;
3458+
3459+
bool shouldSuppressDiagnostic() {
3460+
// If we have a parent CoerceExpr that has the same type as our
3461+
// Optional-to-Any coercion, don't emit a diagnostic.
3462+
return ParentCoercion && ParentCoercion->getType()->isEqual(DestType);
3463+
}
3464+
};
3465+
3466+
/// Returns true iff a coercion from srcType to destType is an
3467+
/// Optional-to-Any coercion.
3468+
bool isOptionalToAnyCoercion(Type srcType, Type destType) {
3469+
size_t difference = 0;
3470+
return isOptionalToAnyCoercion(srcType, destType, difference);
3471+
}
3472+
3473+
/// Returns true iff a coercion from srcType to destType is an
3474+
/// Optional-to-Any coercion. On returning true, the value of 'difference'
3475+
/// will be the difference in the levels of optionality.
3476+
bool isOptionalToAnyCoercion(Type srcType, Type destType,
3477+
size_t &difference) {
3478+
SmallVector<Type, 4> destOptionals;
3479+
auto destValueType =
3480+
destType->lookThroughAllAnyOptionalTypes(destOptionals);
3481+
3482+
if (!destValueType->isAny())
3483+
return false;
3484+
3485+
SmallVector<Type, 4> srcOptionals;
3486+
srcType->lookThroughAllAnyOptionalTypes(srcOptionals);
3487+
3488+
if (srcOptionals.size() > destOptionals.size()) {
3489+
difference = srcOptionals.size() - destOptionals.size();
3490+
return true;
3491+
} else {
3492+
return false;
3493+
}
3494+
}
3495+
3496+
/// Returns true iff the collection upcast coercion is an Optional-to-Any
3497+
/// coercion.
3498+
bool isOptionalToAnyCoercion(CollectionUpcastConversionExpr::ConversionPair
3499+
conversion) {
3500+
if (!conversion.OrigValue || !conversion.Conversion)
3501+
return false;
3502+
3503+
auto srcType = conversion.OrigValue->getType();
3504+
auto destType = conversion.Conversion->getType();
3505+
return isOptionalToAnyCoercion(srcType, destType);
3506+
}
3507+
3508+
/// Looks through OptionalEvaluationExprs and InjectIntoOptionalExprs to
3509+
/// find a child ErasureExpr, returning nullptr if no such child is found.
3510+
/// Any intermediate OptionalEvaluationExprs will be marked as ignored.
3511+
ErasureExpr *findErasureExprThroughOptionalInjections(Expr *E) {
3512+
while (true) {
3513+
if (auto *next = dyn_cast<OptionalEvaluationExpr>(E)) {
3514+
// We don't want to re-visit any intermediate optional evaluations.
3515+
IgnoredExprs.insert(next);
3516+
E = next->getSubExpr();
3517+
} else if (auto *next = dyn_cast<InjectIntoOptionalExpr>(E)) {
3518+
E = next->getSubExpr();
3519+
} else {
3520+
break;
3521+
}
3522+
}
3523+
return dyn_cast<ErasureExpr>(E);
3524+
}
3525+
3526+
void emitSilenceOptionalAnyWarningWithCoercion(Expr *E, Type destType) {
3527+
SmallString<16> coercionString;
3528+
coercionString += " as ";
3529+
coercionString += destType->getWithoutParens()->getString();
3530+
3531+
TC.diagnose(E->getLoc(), diag::silence_optional_to_any,
3532+
destType, coercionString.substr(1))
3533+
.highlight(E->getSourceRange())
3534+
.fixItInsertAfter(E->getEndLoc(), coercionString);
3535+
}
3536+
3537+
void visitErasureExpr(ErasureExpr *E, OptionalToAnyCoercion coercion) {
3538+
if (coercion.shouldSuppressDiagnostic())
3539+
return;
3540+
3541+
auto subExpr = E->getSubExpr();
3542+
3543+
// Look through any BindOptionalExprs, as the coercion may have started
3544+
// from a higher level of optionality.
3545+
while (auto *bindExpr = dyn_cast<BindOptionalExpr>(subExpr))
3546+
subExpr = bindExpr->getSubExpr();
3547+
3548+
// We're taking the source type from the child of any BindOptionalExprs,
3549+
// and the destination from the parent of any
3550+
// (InjectIntoOptional/OptionalEvaluation)Exprs in order to take into
3551+
// account any bindings that need to be done for nested Optional-to-Any
3552+
// coercions, e.g Int??? to Any?.
3553+
auto srcType = subExpr->getType();
3554+
auto destType = coercion.DestType;
3555+
3556+
size_t optionalityDifference = 0;
3557+
if (!isOptionalToAnyCoercion(srcType, destType, optionalityDifference))
3558+
return;
3559+
3560+
TC.diagnose(subExpr->getStartLoc(), diag::optional_to_any_coercion,
3561+
/* from */ srcType, /* to */ destType)
3562+
.highlight(subExpr->getSourceRange());
3563+
3564+
if (optionalityDifference == 1) {
3565+
TC.diagnose(subExpr->getLoc(), diag::default_optional_to_any)
3566+
.highlight(subExpr->getSourceRange())
3567+
.fixItInsertAfter(subExpr->getEndLoc(), " ?? <#default value#>");
3568+
}
3569+
3570+
SmallString<4> forceUnwrapString;
3571+
for (size_t i = 0; i < optionalityDifference; i++)
3572+
forceUnwrapString += "!";
3573+
3574+
TC.diagnose(subExpr->getLoc(), diag::force_optional_to_any)
3575+
.highlight(subExpr->getSourceRange())
3576+
.fixItInsertAfter(subExpr->getEndLoc(), forceUnwrapString);
3577+
3578+
emitSilenceOptionalAnyWarningWithCoercion(subExpr, destType);
3579+
}
3580+
3581+
void visitCollectionUpcastExpr(CollectionUpcastConversionExpr *E,
3582+
OptionalToAnyCoercion coercion) {
3583+
// We only need to consider the valueConversion, as the Key type of a
3584+
// Dictionary cannot be implicitly coerced to Any.
3585+
auto valueConversion = E->getValueConversion();
3586+
3587+
// We're handling the coercion of the entire collection, so we don't need
3588+
// to re-visit a nested ErasureExpr for the value.
3589+
if (auto conversionExpr = valueConversion.Conversion)
3590+
if (auto *erasureExpr =
3591+
findErasureExprThroughOptionalInjections(conversionExpr))
3592+
IgnoredExprs.insert(erasureExpr);
3593+
3594+
if (coercion.shouldSuppressDiagnostic() ||
3595+
!isOptionalToAnyCoercion(valueConversion))
3596+
return;
3597+
3598+
auto subExpr = E->getSubExpr();
3599+
3600+
TC.diagnose(subExpr->getStartLoc(), diag::optional_to_any_coercion,
3601+
/* from */ subExpr->getType(), /* to */ E->getType())
3602+
.highlight(subExpr->getSourceRange());
3603+
3604+
emitSilenceOptionalAnyWarningWithCoercion(subExpr, E->getType());
3605+
}
3606+
3607+
void visitPossibleOptionalToAnyExpr(Expr *E,
3608+
OptionalToAnyCoercion coercion) {
3609+
if (auto *upcastExpr =
3610+
dyn_cast<CollectionUpcastConversionExpr>(E)) {
3611+
visitCollectionUpcastExpr(upcastExpr, coercion);
3612+
} else if (auto *erasureExpr = dyn_cast<ErasureExpr>(E)) {
3613+
visitErasureExpr(erasureExpr, coercion);
3614+
} else if (auto *optionalEvalExpr = dyn_cast<OptionalEvaluationExpr>(E)) {
3615+
// The ErasureExpr could be nested within optional injections and
3616+
// bindings, such as is the case for e.g Int??? to Any?. Try and find
3617+
// and visit it directly, making sure we don't re-visit it later.
3618+
auto subExpr = optionalEvalExpr->getSubExpr();
3619+
if (auto *erasureExpr =
3620+
findErasureExprThroughOptionalInjections(subExpr)) {
3621+
visitErasureExpr(erasureExpr, coercion);
3622+
IgnoredExprs.insert(erasureExpr);
3623+
}
3624+
}
3625+
}
3626+
3627+
void visitInterpolatedStringLiteralExpr(InterpolatedStringLiteralExpr *E) {
3628+
// Warn about interpolated segments that contain optionals.
3629+
for (auto &segment : E->getSegments()) {
3630+
// Allow explicit casts.
3631+
if (auto paren = dyn_cast<ParenExpr>(segment))
3632+
if (isa<ExplicitCastExpr>(paren->getSubExpr()))
3633+
continue;
3634+
3635+
// Bail out if we don't have an optional.
3636+
if (!segment->getType()->getRValueType()->getOptionalObjectType())
3637+
continue;
3638+
3639+
TC.diagnose(segment->getStartLoc(),
3640+
diag::optional_in_string_interpolation_segment)
3641+
.highlight(segment->getSourceRange());
3642+
3643+
// Suggest 'String(describing: <expr>)'.
3644+
auto segmentStart = segment->getStartLoc().getAdvancedLoc(1);
3645+
TC.diagnose(segment->getLoc(),
3646+
diag::silence_optional_in_interpolation_segment_call)
3647+
.highlight(segment->getSourceRange())
3648+
.fixItInsert(segmentStart, "String(describing: ")
3649+
.fixItInsert(segment->getEndLoc(), ")");
3650+
3651+
// Suggest inserting a default value.
3652+
TC.diagnose(segment->getLoc(), diag::default_optional_to_any)
3653+
.highlight(segment->getSourceRange())
3654+
.fixItInsert(segment->getEndLoc(), " ?? <#default value#>");
3655+
}
3656+
}
34533657

34543658
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
34553659
if (!E || isa<ErrorExpr>(E) || !E->getType())
@@ -3459,60 +3663,20 @@ static void diagnoseUnintendedOptionalBehavior(TypeChecker &TC, const Expr *E,
34593663
if (!CE->hasSingleExpressionBody())
34603664
return { false, E };
34613665

3462-
if (auto *coercion = dyn_cast<CoerceExpr>(E)) {
3463-
if (E->getType()->isAny() && isa<ErasureExpr>(coercion->getSubExpr()))
3464-
ErasureCoercedToAny.insert(coercion->getSubExpr());
3465-
} else if (isa<ErasureExpr>(E) && !ErasureCoercedToAny.count(E) &&
3466-
E->getType()->isAny()) {
3467-
auto subExpr = cast<ErasureExpr>(E)->getSubExpr();
3468-
auto erasedTy = subExpr->getType();
3469-
if (erasedTy->getOptionalObjectType()) {
3470-
TC.diagnose(subExpr->getStartLoc(), diag::optional_to_any_coercion,
3471-
erasedTy)
3472-
.highlight(subExpr->getSourceRange());
3473-
3474-
TC.diagnose(subExpr->getLoc(), diag::default_optional_to_any)
3475-
.highlight(subExpr->getSourceRange())
3476-
.fixItInsertAfter(subExpr->getEndLoc(), " ?? <#default value#>");
3477-
TC.diagnose(subExpr->getLoc(), diag::force_optional_to_any)
3478-
.highlight(subExpr->getSourceRange())
3479-
.fixItInsertAfter(subExpr->getEndLoc(), "!");
3480-
TC.diagnose(subExpr->getLoc(), diag::silence_optional_to_any)
3481-
.highlight(subExpr->getSourceRange())
3482-
.fixItInsertAfter(subExpr->getEndLoc(), " as Any");
3483-
}
3484-
} else if (auto *literal = dyn_cast<InterpolatedStringLiteralExpr>(E)) {
3485-
// Warn about interpolated segments that contain optionals.
3486-
for (auto &segment : literal->getSegments()) {
3487-
// Allow explicit casts.
3488-
if (auto paren = dyn_cast<ParenExpr>(segment)) {
3489-
if (isa<ExplicitCastExpr>(paren->getSubExpr())) {
3490-
continue;
3491-
}
3492-
}
3493-
3494-
// Bail out if we don't have an optional.
3495-
if (!segment->getType()->getRValueType()->getOptionalObjectType()) {
3496-
continue;
3497-
}
3666+
if (IgnoredExprs.count(E))
3667+
return { true, E };
34983668

3499-
TC.diagnose(segment->getStartLoc(),
3500-
diag::optional_in_string_interpolation_segment)
3501-
.highlight(segment->getSourceRange());
3502-
3503-
// Suggest 'String(describing: <expr>)'.
3504-
auto segmentStart = segment->getStartLoc().getAdvancedLoc(1);
3505-
TC.diagnose(segment->getLoc(),
3506-
diag::silence_optional_in_interpolation_segment_call)
3507-
.highlight(segment->getSourceRange())
3508-
.fixItInsert(segmentStart, "String(describing: ")
3509-
.fixItInsert(segment->getEndLoc(), ")");
3510-
3511-
// Suggest inserting a default value.
3512-
TC.diagnose(segment->getLoc(), diag::default_optional_to_any)
3513-
.highlight(segment->getSourceRange())
3514-
.fixItInsert(segment->getEndLoc(), " ?? <#default value#>");
3515-
}
3669+
if (auto *literal = dyn_cast<InterpolatedStringLiteralExpr>(E)) {
3670+
visitInterpolatedStringLiteralExpr(literal);
3671+
} else if (auto *coercion = dyn_cast<CoerceExpr>(E)) {
3672+
// If we come across a CoerceExpr, visit its subExpr with the coercion
3673+
// as the parent, making sure we don't re-visit the subExpr later.
3674+
auto subExpr = coercion->getSubExpr();
3675+
visitPossibleOptionalToAnyExpr(subExpr,
3676+
{ subExpr->getType(), coercion });
3677+
IgnoredExprs.insert(subExpr);
3678+
} else {
3679+
visitPossibleOptionalToAnyExpr(E, { E->getType(), nullptr });
35163680
}
35173681
return { true, E };
35183682
}

‎test/ClangImporter/objc_parse.swift

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -247,27 +247,27 @@ func almostSubscriptableValueMismatch(_ as1: AlmostSubscriptable, a: A) {
247247
func almostSubscriptableKeyMismatch(_ bc: BadCollection, key: NSString) {
248248
// FIXME: We end up importing this as read-only due to the mismatch between
249249
// getter/setter element types.
250-
var _ : Any = bc[key] // expected-warning {{expression implicitly coerced from 'Any?' to Any}}
250+
var _ : Any = bc[key] // expected-warning {{expression implicitly coerced from 'Any?' to 'Any'}}
251251
// expected-note@-1 {{force-unwrap the value to avoid this warning}}
252252
// expected-note@-2 {{provide a default value to avoid this warning}}
253-
// expected-note@-3 {{explicitly cast to Any with 'as Any' to silence this warning}}
253+
// expected-note@-3 {{explicitly cast to 'Any' with 'as Any' to silence this warning}}
254254
}
255255

256256
func almostSubscriptableKeyMismatchInherited(_ bc: BadCollectionChild,
257257
key: String) {
258-
var value : Any = bc[key] // expected-warning {{expression implicitly coerced from 'Any?' to Any}}
258+
var value : Any = bc[key] // expected-warning {{expression implicitly coerced from 'Any?' to 'Any'}}
259259
// expected-note@-1 {{force-unwrap the value to avoid this warning}}
260260
// expected-note@-2 {{provide a default value to avoid this warning}}
261-
// expected-note@-3 {{explicitly cast to Any with 'as Any' to silence this warning}}
261+
// expected-note@-3 {{explicitly cast to 'Any' with 'as Any' to silence this warning}}
262262
bc[key] = value // expected-error{{cannot assign through subscript: subscript is get-only}}
263263
}
264264

265265
func almostSubscriptableKeyMismatchInherited(_ roc: ReadOnlyCollectionChild,
266266
key: String) {
267-
var value : Any = roc[key] // expected-warning {{expression implicitly coerced from 'Any?' to Any}}
267+
var value : Any = roc[key] // expected-warning {{expression implicitly coerced from 'Any?' to 'Any'}}
268268
// expected-note@-1 {{force-unwrap the value to avoid this warning}}
269269
// expected-note@-2 {{provide a default value to avoid this warning}}
270-
// expected-note@-3 {{explicitly cast to Any with 'as Any' to silence this warning}}
270+
// expected-note@-3 {{explicitly cast to 'Any' with 'as Any' to silence this warning}}
271271
roc[key] = value // expected-error{{cannot assign through subscript: subscript is get-only}}
272272
}
273273

@@ -407,22 +407,22 @@ func testPropertyAndMethodCollision(_ obj: PropertyAndMethodCollision,
407407
type(of: rev).classRef(rev, doSomething:#selector(getter: NSMenuItem.action))
408408

409409
var value: Any
410-
value = obj.protoProp() // expected-warning {{expression implicitly coerced from 'Any?' to Any}}
410+
value = obj.protoProp() // expected-warning {{expression implicitly coerced from 'Any?' to 'Any'}}
411411
// expected-note@-1 {{force-unwrap the value to avoid this warning}}
412412
// expected-note@-2 {{provide a default value to avoid this warning}}
413-
// expected-note@-3 {{explicitly cast to Any with 'as Any' to silence this warning}}
414-
value = obj.protoPropRO() // expected-warning {{expression implicitly coerced from 'Any?' to Any}}
413+
// expected-note@-3 {{explicitly cast to 'Any' with 'as Any' to silence this warning}}
414+
value = obj.protoPropRO() // expected-warning {{expression implicitly coerced from 'Any?' to 'Any'}}
415415
// expected-note@-1 {{force-unwrap the value to avoid this warning}}
416416
// expected-note@-2 {{provide a default value to avoid this warning}}
417-
// expected-note@-3 {{explicitly cast to Any with 'as Any' to silence this warning}}
418-
value = type(of: obj).protoClassProp() // expected-warning {{expression implicitly coerced from 'Any?' to Any}}
417+
// expected-note@-3 {{explicitly cast to 'Any' with 'as Any' to silence this warning}}
418+
value = type(of: obj).protoClassProp() // expected-warning {{expression implicitly coerced from 'Any?' to 'Any'}}
419419
// expected-note@-1 {{force-unwrap the value to avoid this warning}}
420420
// expected-note@-2 {{provide a default value to avoid this warning}}
421-
// expected-note@-3 {{explicitly cast to Any with 'as Any' to silence this warning}}
422-
value = type(of: obj).protoClassPropRO() // expected-warning {{expression implicitly coerced from 'Any?' to Any}}
421+
// expected-note@-3 {{explicitly cast to 'Any' with 'as Any' to silence this warning}}
422+
value = type(of: obj).protoClassPropRO() // expected-warning {{expression implicitly coerced from 'Any?' to 'Any'}}
423423
// expected-note@-1 {{force-unwrap the value to avoid this warning}}
424424
// expected-note@-2 {{provide a default value to avoid this warning}}
425-
// expected-note@-3 {{explicitly cast to Any with 'as Any' to silence this warning}}
425+
// expected-note@-3 {{explicitly cast to 'Any' with 'as Any' to silence this warning}}
426426
_ = value
427427
}
428428

‎test/Constraints/casts_objc.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,10 @@ func nsobject_as_class_cast<T>(_ x: NSObject, _: T) {
3434
func test(_ a : CFString!, b : CFString) {
3535
let dict = NSMutableDictionary()
3636
let object = NSObject()
37-
dict[a] = object // expected-warning {{expression implicitly coerced from 'CFString?' to Any}}
37+
dict[a] = object // expected-warning {{expression implicitly coerced from 'CFString?' to 'Any'}}
3838
// expected-note@-1 {{force-unwrap the value to avoid this warning}}
3939
// expected-note@-2 {{provide a default value to avoid this warning}}
40-
// expected-note@-3 {{explicitly cast to Any with 'as Any' to silence this warning}}
40+
// expected-note@-3 {{explicitly cast to 'Any' with 'as Any' to silence this warning}}
4141

4242

4343
dict[b] = object

‎test/Constraints/diag_ambiguities.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,10 @@ struct SR3715 {
5151
func take(_ a: [Any]) {}
5252

5353
func test() {
54-
take([overloaded]) // expected-warning {{expression implicitly coerced from 'Int?' to Any}}
54+
take([overloaded]) // expected-warning {{expression implicitly coerced from 'Int?' to 'Any'}}
5555
// expected-note@-1 {{force-unwrap the value to avoid this warning}}
5656
// expected-note@-2 {{provide a default value to avoid this warning}}
57-
// expected-note@-3 {{explicitly cast to Any with 'as Any' to silence this warning}}
57+
// expected-note@-3 {{explicitly cast to 'Any' with 'as Any' to silence this warning}}
5858
}
5959
}
6060

0 commit comments

Comments
 (0)