Fix <rdar://22774938> QoI: "never used" in an "if let" should rewrite expression to use != nil

When we see an unused variable in a simple-enough "if/let" (also guard and
while of course), fixit it into a comparison against nil instead of replacing
the name of the variable with "_".  Also special case initialization with an
as? expression, since we can transform that into an "is" boolean test.

Author: lattner

include/swift/AST/DiagnosticsSema.def

@@ -2612,10 +2612,17 @@ NOTE(availability_conformance_introduced_here, sema_avail, none,
 // Variable usage diagnostics
 //------------------------------------------------------------------------------
 
+WARNING(pbd_never_used_stmtcond, sema_varusage, none,
+        "value %0 was defined but never used; consider replacing "
+        "with boolean test",
+        (Identifier))
+
 WARNING(pbd_never_used, sema_varusage, none,
         "initialization of %select{variable|immutable value}1 %0 was never used"
         "; consider replacing with assignment to '_' or removing it",
         (Identifier, unsigned))
+
+
 WARNING(capture_never_used, sema_varusage, none,
         "capture %0 was never used",
         (Identifier))

lib/Sema/MiscDiagnostics.cpp

@@ -1216,6 +1216,11 @@ class VarDeclUsageChecker : public ASTWalker {
   /// This is a mapping from an OpaqueValue to the expression that initialized
   /// it.
   llvm::SmallDenseMap<OpaqueValueExpr*, Expr*> OpaqueValueMap;
+  
+  /// This is a mapping from VarDecls to the if/while/guard statement that they
+  /// occur in, when they are in a pattern in a StmtCondition.
+  llvm::SmallDenseMap<VarDecl*, LabeledConditionalStmt*> StmtConditionForVD;
+  
   bool sawError = false;
 
   VarDeclUsageChecker(const VarDeclUsageChecker &) = delete;
@@ -1340,6 +1345,18 @@ class VarDeclUsageChecker : public ASTWalker {
     // for them.
     if (auto *ICS = dyn_cast<IfConfigStmt>(S))
       handleIfConfig(ICS);
+      
+    // Keep track of an association between vardecls and the StmtCondition that
+    // they are bound in for IfStmt, GuardStmt, WhileStmt, etc.
+    if (auto LCS = dyn_cast<LabeledConditionalStmt>(S)) {
+      for (auto &cond : LCS->getCond())
+        if (auto pat = cond.getPatternOrNull()) {
+          pat->forEachVariable([&](VarDecl *VD) {
+            StmtConditionForVD[VD] = LCS;
+          });
+        }
+    }
+      
     return { true, S };
   }
 
@@ -1409,6 +1426,60 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
           continue;
         }
 
+      // If the variable is defined in a pattern in an if/while/guard statement,
+      // see if we can produce a tuned fixit.  When we have something like:
+      //
+      //    if let x = <expr> {
+      //
+      // we prefer to rewrite it to:
+      //
+      //    if <expr> != nil {
+      //
+      if (auto SC = StmtConditionForVD[var]) {
+        // We only handle the "if let" case right now, since it is vastly the
+        // most common situation that people run into.
+        if (SC->getCond().size() == 1) {
+          auto pattern = SC->getCond()[0].getPattern();
+          if (auto OSP = dyn_cast<OptionalSomePattern>(pattern))
+            if (auto LP = dyn_cast<VarPattern>(OSP->getSubPattern()))
+              if (isa<NamedPattern>(LP->getSubPattern())) {
+                auto initExpr = SC->getCond()[0].getInitializer();
+                auto beforeExprLoc =
+                  initExpr->getStartLoc().getAdvancedLocOrInvalid(-1);
+                if (beforeExprLoc.isValid()) {
+                  unsigned noParens = initExpr->canAppendCallParentheses();
+                  
+                  // If the subexpr is an "as?" cast, we can rewrite it to
+                  // be an "is" test.
+                  bool isIsTest = false;
+                  if (isa<ConditionalCheckedCastExpr>(initExpr) &&
+                      !initExpr->isImplicit()) {
+                    noParens = isIsTest = true;
+                  }
+                  
+                  auto diagIF = TC.diagnose(var->getLoc(),
+                                            diag::pbd_never_used_stmtcond,
+                                            var->getName());
+                  auto introducerLoc = SC->getCond()[0].getIntroducerLoc();
+                  diagIF.fixItReplace(SourceRange(introducerLoc, beforeExprLoc),
+                                      &"("[noParens]);
+                  
+                  if (isIsTest) {
+                    // If this was an "x as? T" check, rewrite it to "x is T".
+                    auto CCE = cast<ConditionalCheckedCastExpr>(initExpr);
+                    diagIF.fixItReplace(SourceRange(CCE->getLoc(),
+                                                    CCE->getQuestionLoc()),
+                                        "is");
+                  } else {
+                    diagIF.fixItInsertAfter(initExpr->getEndLoc(),
+                                            &") != nil"[noParens]);
+                  }
+                  continue;
+                }
+              }
+        }
+      }
+      
       // Otherwise, this is something more complex, perhaps
       //    let (a,b) = foo()
       // Just rewrite the one variable with a _.

test/decl/var/usage.swift

@@ -219,3 +219,28 @@ func testFixitsInStatementsWithPatterns(a : Int?) {
       _ = b
   }
 }
+
+
+// <rdar://22774938> QoI: "never used" in an "if let" should rewrite expression to use != nil
+func test(a : Int?, b : Any) {
+  if true == true, let x = a {   // expected-warning {{immutable value 'x' was never used; consider replacing with '_' or removing it}} {{24-25=_}}
+  }
+  if let x = a, y = a {  // expected-warning {{immutable value 'x' was never used; consider replacing with '_' or removing it}} {{10-11=_}}
+    _ = y
+  }
+
+  // Simple case, insert a comparison with nil.
+  if let x = a {  // expected-warning {{value 'x' was defined but never used; consider replacing with boolean test}} {{6-14=}} {{15-15= != nil}}
+  }
+
+  // General case, need to insert parentheses.
+  if let x = a ?? a {}  // expected-warning {{value 'x' was defined but never used; consider replacing with boolean test}} {{6-14=(}} {{20-20=) != nil}}
+  
+  // Special case, we can turn this into an 'is' test.
+  if let x = b as? Int {  // expected-warning {{value 'x' was defined but never used; consider replacing with boolean test}} {{6-14=}} {{16-19=is}}
+  }
+
+  
+}
+
+