From 3e0394c4c4127d2845e5353af2d212e5a692c4b9 Mon Sep 17 00:00:00 2001 From: Robert Widmann Date: Mon, 3 Oct 2016 17:08:11 -0400 Subject: [PATCH] Warn on optionals in string interpolation segments Basic extension of the optional-to-any AST walker to incorporate warnings for optionally-typed expressions in string interpolation segments that can lead to unintended behavior. Specifically let x = Optional.some(2) print(x) // "Optional(2)" --- include/swift/AST/DiagnosticsSema.def | 6 +++ lib/Sema/MiscDiagnostics.cpp | 45 ++++++++++++++++--- ...> diag_unintended_optional_behavior.swift} | 15 +++++++ 3 files changed, 59 insertions(+), 7 deletions(-) rename test/Sema/{diag_optional_to_any.swift => diag_unintended_optional_behavior.swift} (73%) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index ba8eadcaecfbc..ac57627caa847 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -2403,6 +2403,12 @@ NOTE(force_optional_to_any,none, "force-unwrap the value to avoid this warning", ()) NOTE(silence_optional_to_any,none, "explicitly cast to Any with 'as Any' to silence this warning", ()) +WARNING(optional_in_string_interpolation_segment,none, + "string interpolation produces a debug description for an optional " + "value; did you mean to make this explicit?", + ()) +NOTE(silence_optional_in_interpolation_segment_call,none, + "use 'String(describing:)' to silence this warning", ()) ERROR(invalid_noescape_use,none, "non-escaping %select{value|parameter}1 %0 may only be called", diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index f80ce22d2a737..7d979c45420ec 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -3667,12 +3667,12 @@ checkImplicitPromotionsInCondition(const StmtConditionElement &cond, } } -static void diagnoseOptionalToAnyCoercion(TypeChecker &TC, const Expr *E, - const DeclContext *DC) { +static void diagnoseUnintendedOptionalBehavior(TypeChecker &TC, const Expr *E, + const DeclContext *DC) { if (!E || isa(E) || !E->getType()) return; - class OptionalToAnyCoercionWalker : public ASTWalker { + class UnintendedOptionalBehaviorWalker : public ASTWalker { TypeChecker &TC; SmallPtrSet ErasureCoercedToAny; @@ -3702,16 +3702,47 @@ static void diagnoseOptionalToAnyCoercion(TypeChecker &TC, const Expr *E, .highlight(subExpr->getSourceRange()) .fixItInsertAfter(subExpr->getEndLoc(), " as Any"); } - } + } else if (auto *literal = dyn_cast(E)) { + // Warn about interpolated segments that contain optionals. + for (auto &segment : literal->getSegments()) { + // Allow explicit casts. + if (auto paren = dyn_cast(segment)) { + if (isa(paren->getSubExpr())) { + continue; + } + } + // Bail out if we don't have an optional. + if (!segment->getType()->getOptionalObjectType()) { + continue; + } + + TC.diagnose(segment->getStartLoc(), + diag::optional_in_string_interpolation_segment) + .highlight(segment->getSourceRange()); + + // Suggest 'String(describing: )'. + auto segmentStart = segment->getStartLoc().getAdvancedLoc(1); + TC.diagnose(segment->getLoc(), + diag::silence_optional_in_interpolation_segment_call) + .highlight(segment->getSourceRange()) + .fixItInsert(segmentStart, "String(describing: ") + .fixItInsert(segment->getEndLoc(), ")"); + + // Suggest inserting a default value. + TC.diagnose(segment->getLoc(), diag::default_optional_to_any) + .highlight(segment->getSourceRange()) + .fixItInsert(segment->getEndLoc(), " ?? <#default value#>"); + } + } return { true, E }; } public: - OptionalToAnyCoercionWalker(TypeChecker &tc) : TC(tc) { } + UnintendedOptionalBehaviorWalker(TypeChecker &tc) : TC(tc) { } }; - OptionalToAnyCoercionWalker Walker(TC); + UnintendedOptionalBehaviorWalker Walker(TC); const_cast(E)->walk(Walker); } @@ -3727,7 +3758,7 @@ void swift::performSyntacticExprDiagnostics(TypeChecker &TC, const Expr *E, diagSyntacticUseRestrictions(TC, E, DC, isExprStmt); diagRecursivePropertyAccess(TC, E, DC); diagnoseImplicitSelfUseInClosure(TC, E, DC); - diagnoseOptionalToAnyCoercion(TC, E, DC); + diagnoseUnintendedOptionalBehavior(TC, E, DC); if (!TC.getLangOpts().DisableAvailabilityChecking) diagAvailability(TC, E, const_cast(DC)); if (TC.Context.LangOpts.EnableObjCInterop) diff --git a/test/Sema/diag_optional_to_any.swift b/test/Sema/diag_unintended_optional_behavior.swift similarity index 73% rename from test/Sema/diag_optional_to_any.swift rename to test/Sema/diag_unintended_optional_behavior.swift index 79e1bbb9177ea..56686592bd388 100644 --- a/test/Sema/diag_optional_to_any.swift +++ b/test/Sema/diag_unintended_optional_behavior.swift @@ -58,3 +58,18 @@ func warnOptionalToAnyCoercion(value x: Int?) -> Any { // expected-note@-2 {{force-unwrap the value to avoid this warning}} // expected-note@-3 {{explicitly cast to Any with 'as Any' to silence this warning}} } + +func warnOptionalInStringInterpolationSegment(_ o : Int?) { + print("Always some, Always some, Always some: \(o)") + // expected-warning@-1 {{string interpolation produces a debug description for an optional value; did you mean to make this explicit?}} + // expected-note@-2 {{use 'String(describing:)' to silence this warning}} {{51-51=String(describing: }} {{52-52=)}} + // expected-note@-3 {{provide a default value to avoid this warning}} {{52-52= ?? <#default value#>}} + print("Always some, Always some, Always some: \(o.map { $0 + 1 })") + // expected-warning@-1 {{string interpolation produces a debug description for an optional value; did you mean to make this explicit?}} + // expected-note@-2 {{use 'String(describing:)' to silence this warning}} {{51-51=String(describing: }} {{67-67=)}} + // expected-note@-3 {{provide a default value to avoid this warning}} {{67-67= ?? <#default value#>}} + + print("Always some, Always some, Always some: \(o as Int?)") // No warning + print("Always some, Always some, Always some: \(o.debugDescription)") // No warning. +} +