From d13829044843c96154e9e2ba75cd751a419dc2d3 Mon Sep 17 00:00:00 2001 From: Chris Lattner Date: Sat, 23 Jul 2016 16:02:48 -0700 Subject: [PATCH] Produce warnings when implicit optional promotions are introduced in some common standard library operators. This is progress towards: [Type checker] Diagnose unsavory optional injections but there is more work to be done here. --- include/swift/AST/DiagnosticsSema.def | 7 +++ lib/Sema/MiscDiagnostics.cpp | 70 ++++++++++++++++++++++ test/Constraints/diagnostics.swift | 9 +-- test/expr/cast/nil_value_to_optional.swift | 6 +- test/expr/expressions.swift | 14 +++-- 5 files changed, 95 insertions(+), 11 deletions(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 7e999a95b2e86..46adcd177a7dc 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -2214,6 +2214,13 @@ NOTE(fix_unqualified_access_top_level_multi,none, "use '%0' to reference the %1 in module %2", (StringRef, DescriptiveDeclKind, Identifier)) +WARNING(use_of_qq_on_non_optional_value,none, + "left side of nil coalescing operator '?""?' is non-optional, " + "so the right side is never used", ()) +WARNING(nonoptional_compare_to_nil,none, + "comparing non-optional value to nil always returns" + " %select{false|true}0", (bool)) + ERROR(type_of_metatype,none, "'.dynamicType' is not allowed after a type name", ()) ERROR(invalid_noescape_use,none, diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 797e4aeb6eb8a..dfc5575ff5a70 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -88,6 +88,7 @@ static void diagSelfAssignment(TypeChecker &TC, const Expr *E) { /// argument lists. /// - 'self.init' and 'super.init' cannot be wrapped in a larger expression /// or statement. +/// - Warn about promotions to optional in specific syntactic forms. /// static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E, const DeclContext *DC, @@ -268,6 +269,9 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E, // Check function calls, looking through implicit conversions on the // function and inspecting the arguments directly. if (auto *Call = dyn_cast(E)) { + // Warn about surprising implicit optional promotions. + checkOptionalPromotions(Call); + // Check for tuple splat. checkTupleSplat(Call); @@ -594,6 +598,72 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E, .fixItInsert(DRE->getStartLoc(), namePlusDot); } } + + /// Return true if this expression is an implicit promotion from T to T?. + static bool isImplicitPromotionToOptional(Expr *E) { + return E->isImplicit() && isa(E); + } + + /// Return true if this is 'nil' type checked as an Optional. This looks + /// like this: + /// (call_expr implicit type='Int?' + /// (constructor_ref_call_expr implicit + /// (declref_expr implicit decl=Optional.init(nilLiteral:) + static bool isTypeCheckedOptionalNil(Expr *E) { + auto CE = dyn_cast(E->getSemanticsProvidingExpr()); + if (!CE || !CE->isImplicit() || + !CE->getType()->getAnyOptionalObjectType()) + return false; + + auto CRCE = dyn_cast(CE->getSemanticFn()); + if (!CRCE || !CRCE->isImplicit()) return false; + + auto DRE = dyn_cast(CRCE->getSemanticFn()); + + SmallString<32> NameBuffer; + auto name = DRE->getDecl()->getFullName().getString(NameBuffer); + return name == "init(nilLiteral:)"; + } + + + /// Warn about surprising implicit optional promotions involving operands to + /// calls. Specifically, we warn about these expressions when the 'x' + /// operand is implicitly promoted to optional: + /// + /// x ?? y + /// x == nil // also != + /// + void checkOptionalPromotions(ApplyExpr *call) { + auto DRE = dyn_cast(call->getSemanticFn()); + auto args = dyn_cast(call->getArg()); + if (!DRE || !DRE->getDecl()->isOperator() || + !args || args->getNumElements() != 2) + return; + + auto lhs = args->getElement(0); + auto rhs = args->getElement(1); + auto calleeName = DRE->getDecl()->getName().str(); + + if (calleeName == "??" && isImplicitPromotionToOptional(lhs)) { + TC.diagnose(DRE->getLoc(), diag::use_of_qq_on_non_optional_value) + .highlight(lhs->getSourceRange()) + .fixItRemove(SourceRange(DRE->getLoc(), rhs->getEndLoc())); + return; + } + + if (calleeName == "==" || calleeName == "!=") { + if ((isImplicitPromotionToOptional(lhs) && + isTypeCheckedOptionalNil(rhs)) || + (isTypeCheckedOptionalNil(lhs) && + isImplicitPromotionToOptional(rhs))) { + TC.diagnose(DRE->getLoc(), diag::nonoptional_compare_to_nil, + calleeName == "!=") + .highlight(lhs->getSourceRange()) + .highlight(rhs->getSourceRange()); + return; + } + } + } }; DiagnoseWalker Walker(TC, DC, isExprStmt); diff --git a/test/Constraints/diagnostics.swift b/test/Constraints/diagnostics.swift index fc3d0b188eaa6..0b9d151198adf 100644 --- a/test/Constraints/diagnostics.swift +++ b/test/Constraints/diagnostics.swift @@ -214,6 +214,7 @@ class r20201968C { // QoI: Poor compilation error calling assert func r21459429(_ a : Int) { assert(a != nil, "ASSERT COMPILATION ERROR") + // expected-warning @-1 {{comparing non-optional value to nil always returns true}} } @@ -781,10 +782,10 @@ class SR1594 { } func nilComparison(i: Int, o: AnyObject) { - _ = i == nil - _ = nil == i - _ = i != nil - _ = nil != i + _ = i == nil // expected-warning {{comparing non-optional value to nil always returns false}} + _ = nil == i // expected-warning {{comparing non-optional value to nil always returns false}} + _ = i != nil // expected-warning {{comparing non-optional value to nil always returns true}} + _ = nil != i // expected-warning {{comparing non-optional value to nil always returns true}} _ = i < nil _ = nil < i _ = i <= nil diff --git a/test/expr/cast/nil_value_to_optional.swift b/test/expr/cast/nil_value_to_optional.swift index c5cbc450814ff..a2fbeab5a1054 100644 --- a/test/expr/cast/nil_value_to_optional.swift +++ b/test/expr/cast/nil_value_to_optional.swift @@ -5,8 +5,8 @@ var f = false func markUsed(_ t: T) {} -markUsed(t != nil) -markUsed(f != nil) +markUsed(t != nil) // expected-warning {{comparing non-optional value to nil always returns true}} +markUsed(f != nil) // expected-warning {{comparing non-optional value to nil always returns true}} class C : Equatable {} @@ -15,7 +15,7 @@ func == (lhs: C, rhs: C) -> Bool { } func test(_ c: C) { - if c == nil {} + if c == nil {} // expected-warning {{comparing non-optional value to nil always returns false}} } class D {} diff --git a/test/expr/expressions.swift b/test/expr/expressions.swift index 7f88a127d5504..74034fc88497b 100644 --- a/test/expr/expressions.swift +++ b/test/expr/expressions.swift @@ -705,10 +705,10 @@ func invalidDictionaryLiteral() { //===----------------------------------------------------------------------===// // nil/metatype comparisons //===----------------------------------------------------------------------===// -_ = Int.self == nil -_ = nil == Int.self -_ = Int.self != nil -_ = nil != Int.self +_ = Int.self == nil // expected-warning {{comparing non-optional value to nil always returns false}} +_ = nil == Int.self // expected-warning {{comparing non-optional value to nil always returns false}} +_ = Int.self != nil // expected-warning {{comparing non-optional value to nil always returns true}} +_ = nil != Int.self // expected-warning {{comparing non-optional value to nil always returns true}} // Disallow postfix ? when not chaining func testOptionalChaining(_ a : Int?, b : Int!, c : Int??) { @@ -737,6 +737,12 @@ func testNilCoalescePrecedence(cond: Bool, a: Int?, r: CountableClosedRange let r2 = (r ?? 0)...42 // not ok: expected-error {{binary operator '??' cannot be applied to operands of type 'CountableClosedRange?' and 'Int'}} // expected-note @-1 {{overloads for '??' exist with these partially matching parameter lists:}} let r3 = r ?? 0...42 // parses as the first one, not the second. + + + // [Type checker] Diagnose unsavory optional injections + // Accidental optional injection for ??. + let i = 42 + _ = i ?? 17 // expected-warning {{left side of nil coalescing operator '??' is non-optional, so the right side is never used}} {{9-15=}} } // Parsing of as and ?? regressed