Warn on redundant comparison to 'Optional.none', like we already do for 'nil'

Author: calda

include/swift/AST/DiagnosticsSema.def

@@ -4009,6 +4009,9 @@ WARNING(use_of_qq_on_non_optional_value,none,
 WARNING(nonoptional_compare_to_nil,none,
         "comparing non-optional value of type %0 to 'nil' always returns"
         " %select{false|true}1", (Type, bool))
+WARNING(nonoptional_compare_to_optional_none_case,none,
+        "comparing non-optional value of type %0 to 'Optional.none' always returns"
+        " %select{false|true}1", (Type, bool))
 WARNING(optional_check_nonoptional,none,
         "non-optional expression of type %0 used in a check for optionals",
         (Type))

lib/Sema/MiscDiagnostics.cpp

@@ -1282,25 +1282,19 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
 
     }
 
-    /// Return true if this is a 'nil' literal.  This looks
-    /// like this if the type is Optional<T>:
-    ///
-    ///   (dot_syntax_call_expr implicit type='Int?'
-    ///     (declref_expr implicit decl=Optional.none)
-    ///     (type_expr type=Int?))
-    ///
-    /// Or like this if it is any other ExpressibleByNilLiteral type:
-    ///
-    ///   (nil_literal_expr)
-    ///
-    bool isTypeCheckedOptionalNil(Expr *E) {
+    /// Returns true if this is a 'nil' literal
+    bool isNilLiteral(Expr *E) {
       if (dyn_cast<NilLiteralExpr>(E)) return true;
-
+      return false;
+    }
+    
+    /// Returns true if this is a expression is a reference to
+    /// the `Optional.none` case specifically (e.g. not `nil`).
+    bool isOptionalNoneCase(Expr *E) {
       auto CE = dyn_cast<ApplyExpr>(E->getSemanticsProvidingExpr());
-      if (!CE || !CE->isImplicit())
+      if (!CE)
         return false;
 
-      // First case -- Optional.none
       if (auto DRE = dyn_cast<DeclRefExpr>(CE->getSemanticFn()))
         return DRE->getDecl() == Ctx.getOptionalNoneDecl();
 
@@ -1345,9 +1339,12 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
 
       if (calleeName == "==" || calleeName == "!=" ||
           calleeName == "===" || calleeName == "!==") {
+        bool isTrue = calleeName == "!=" || calleeName == "!==";
+        
+        // Diagnose a redundant comparison of a non-optional to a `nil` literal
         if (((subExpr = isImplicitPromotionToOptional(lhs)) &&
-             isTypeCheckedOptionalNil(rhs)) ||
-            (isTypeCheckedOptionalNil(lhs) &&
+             isNilLiteral(rhs)) ||
+            (isNilLiteral(lhs) &&
              (subExpr = isImplicitPromotionToOptional(rhs)))) {
           bool isTrue = calleeName == "!=" || calleeName == "!==";
 
@@ -1357,6 +1354,19 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
             .highlight(rhs->getSourceRange());
           return;
         }
+        
+        // Diagnose a redundant comparison of a non-optional to the `Optional.none` case
+        if (((subExpr = isImplicitPromotionToOptional(lhs)) &&
+             isOptionalNoneCase(rhs)) ||
+            (isOptionalNoneCase(lhs) &&
+             (subExpr = isImplicitPromotionToOptional(rhs)))) {
+          
+          Ctx.Diags.diagnose(DRE->getLoc(), diag::nonoptional_compare_to_optional_none_case,
+                             subExpr->getType(), isTrue)
+            .highlight(lhs->getSourceRange())
+            .highlight(rhs->getSourceRange());
+          return;
+        }
       }
     }
   };