fix <rdar://25178926> QoI: Warn about cases where switch statement "ignores" where clause

It is a common point of confusion that code like:

      switch value {
      case .Foo, .Bar where someNumber != 100:

Only applies the where clause to the second pattern, not every pattern in the case.

Resolve this by warning about the ambiguity, providing two notes (with fixits) that
resolve the issue in different ways:

t.swift:25:17: warning: 'where' only applies to the second pattern match in this case
case .Foo, .Bar where someNumber != 100:
           ~~~~ ^     ~~~~~~~~~~~~~~~~~
t.swift:25:12: note: disambiguate by adding a line break between them if this is desired
case .Foo, .Bar where someNumber != 100:
           ^
t.swift:25:6: note: duplicate the 'where' on both patterns to check both patterns
case .Foo, .Bar where someNumber != 100:
     ^~~~
          where someNumber != 100

Author: lattner

include/swift/AST/DiagnosticsSema.def

@@ -2176,6 +2176,15 @@ ERROR(unnecessary_cast_over_optionset,none,
 ERROR(type_mismatch_multiple_pattern_list,none,
       "pattern variable bound to type %0, expected type %1", (Type, Type))
 
+WARNING(where_on_one_item, none,
+        "'where' only applies to the second pattern match in this case", ())
+
+NOTE(add_where_newline, none,
+     "disambiguate by adding a line break between them if this is desired", ())
+
+NOTE(duplicate_where, none,
+     "duplicate the 'where' on both patterns to check both patterns",())
+
 //------------------------------------------------------------------------------
 // Type Check Patterns
 //------------------------------------------------------------------------------

lib/Sema/MiscDiagnostics.cpp

@@ -1825,7 +1825,8 @@ static bool plusEqualOneIncrementForConvertingCStyleForLoop(TypeChecker &TC, con
 }
 
 static void checkCStyleForLoop(TypeChecker &TC, const ForStmt *FS) {
-  // If we're missing semi-colons we'll already be erroring out, and this may not even have been intended as C-style.
+  // If we're missing semi-colons we'll already be erroring out, and this may
+  // not even have been intended as C-style.
   if (FS->getFirstSemicolonLoc().isInvalid() || FS->getSecondSemicolonLoc().isInvalid())
     return;
 
@@ -1835,15 +1836,16 @@ static void checkCStyleForLoop(TypeChecker &TC, const ForStmt *FS) {
 
   // Verify that there is only one loop variable, and it is declared here.
   auto initializers = FS->getInitializerVarDecls();
-  PatternBindingDecl *loopVarDecl = initializers.size() == 2 ? dyn_cast<PatternBindingDecl>(initializers[0]) : nullptr;
+  PatternBindingDecl *loopVarDecl = initializers.size() == 2 ?
+    dyn_cast<PatternBindingDecl>(initializers[0]) : nullptr;
   if (!loopVarDecl || loopVarDecl->getNumPatternEntries() != 1)
     return;
 
   VarDecl *loopVar = dyn_cast<VarDecl>(initializers[1]);
   Expr *startValue = loopVarDecl->getInit(0);
   Expr *endValue = endConditionValueForConvertingCStyleForLoop(FS, loopVar);
   bool strideByOne = unaryIncrementForConvertingCStyleForLoop(FS, loopVar) ||
-                     plusEqualOneIncrementForConvertingCStyleForLoop(TC, FS, loopVar);
+               plusEqualOneIncrementForConvertingCStyleForLoop(TC, FS, loopVar);
 
   if (!loopVar || !startValue || !endValue || !strideByOne)
     return;
@@ -1859,16 +1861,78 @@ static void checkCStyleForLoop(TypeChecker &TC, const ForStmt *FS) {
     return;
   }
 
-  SourceLoc loopPatternEnd = Lexer::getLocForEndOfToken(TC.Context.SourceMgr, loopVarDecl->getPattern(0)->getEndLoc());
-  SourceLoc endOfIncrementLoc = Lexer::getLocForEndOfToken(TC.Context.SourceMgr, FS->getIncrement().getPtrOrNull()->getEndLoc());
+  SourceLoc loopPatternEnd =
+    Lexer::getLocForEndOfToken(TC.Context.SourceMgr,
+                               loopVarDecl->getPattern(0)->getEndLoc());
+  SourceLoc endOfIncrementLoc =
+    Lexer::getLocForEndOfToken(TC.Context.SourceMgr,
+                               FS->getIncrement().getPtrOrNull()->getEndLoc());
 
   diagnostic
    .fixItRemoveChars(loopVarDecl->getLoc(), loopVar->getLoc())
    .fixItReplaceChars(loopPatternEnd, startValue->getStartLoc(), " in ")
-   .fixItReplaceChars(FS->getFirstSemicolonLoc(), endValue->getStartLoc(), " ..< ")
+   .fixItReplaceChars(FS->getFirstSemicolonLoc(), endValue->getStartLoc(),
+                      " ..< ")
    .fixItRemoveChars(FS->getSecondSemicolonLoc(), endOfIncrementLoc);
 }
 
+
+// Perform MiscDiagnostics on Switch Statements.
+static void checkSwitch(TypeChecker &TC, const SwitchStmt *stmt) {
+  // We want to warn about "case .Foo, .Bar where 1 != 100:" since the where
+  // clause only applies to the second case, and this is surprising.
+  for (auto cs : stmt->getCases()) {
+    // The case statement can have multiple case items, each can have a where.
+    // If we find a "where", and there is a preceding item without a where, and
+    // if they are on the same source line, then warn.
+    auto items = cs->getCaseLabelItems();
+    
+    // Don't do any work for the vastly most common case.
+    if (items.size() == 1) continue;
+    
+    // Ignore the first item, since it can't have preceding ones.
+    for (unsigned i = 1, e = items.size(); i != e; ++i) {
+      // Must have a where clause.
+      auto where = items[i].getGuardExpr();
+      if (!where)
+        continue;
+      
+      // Preceding item must not.
+      if (items[i-1].getGuardExpr())
+        continue;
+      
+      // Must be on the same source line.
+      auto prevLoc = items[i-1].getStartLoc();
+      auto thisLoc = items[i].getStartLoc();
+      if (prevLoc.isInvalid() || thisLoc.isInvalid())
+        continue;
+      
+      auto &SM = TC.Context.SourceMgr;
+      auto prevLineCol = SM.getLineAndColumn(prevLoc);
+      if (SM.getLineNumber(thisLoc) != prevLineCol.first)
+        continue;
+      
+      TC.diagnose(items[i].getWhereLoc(), diag::where_on_one_item)
+        .highlight(items[i].getPattern()->getSourceRange())
+        .highlight(where->getSourceRange());
+      
+      // Whitespace it out to the same column as the previous item.
+      std::string whitespace(prevLineCol.second-1, ' ');
+      TC.diagnose(thisLoc, diag::add_where_newline)
+        .fixItInsert(thisLoc, "\n"+whitespace);
+
+      auto whereRange = SourceRange(items[i].getWhereLoc(),
+                                    where->getEndLoc());
+      auto charRange = Lexer::getCharSourceRangeFromSourceRange(SM, whereRange);
+      auto whereText = SM.extractText(charRange);
+      TC.diagnose(prevLoc, diag::duplicate_where)
+        .fixItInsertAfter(items[i-1].getEndLoc(), " " + whereText.str())
+        .highlight(items[i-1].getSourceRange());
+    }
+  }
+}
+
+
 static Optional<ObjCSelector>
 parseObjCSelector(ASTContext &ctx, StringRef string) {
   // Find the first colon.
@@ -2250,6 +2314,9 @@ void swift::performStmtDiagnostics(TypeChecker &TC, const Stmt *S) {
 
   if (auto forStmt = dyn_cast<ForStmt>(S))
     checkCStyleForLoop(TC, forStmt);
+  
+  if (auto switchStmt = dyn_cast<SwitchStmt>(S))
+    checkSwitch(TC, switchStmt);
 }
 
 //===----------------------------------------------------------------------===//

test/stmt/statements.swift

@@ -480,6 +480,37 @@ func f(x : Int, y : Int) {
 
 
 
+// <rdar://problem/25178926> QoI: Warn about cases where switch statement "ignores" where clause
+enum Type {
+  case Foo
+  case Bar
+}
+func r25178926(a : Type) {
+  switch a {
+  case .Foo, .Bar where 1 != 100:
+    // expected-warning @-1 {{'where' only applies to the second pattern match in this case}}
+    // expected-note @-2 {{disambiguate by adding a line break between them if this is desired}} {{14-14=\n       }}
+    // expected-note @-3 {{duplicate the 'where' on both patterns to check both patterns}} {{12-12= where 1 != 100}}
+    break
+  }
+
+  switch a {
+  case .Foo: break
+  case .Bar where 1 != 100: break
+  }
+
+  switch a {
+  case .Foo,  // no warn
+       .Bar where 1 != 100:
+    break
+  }
+
+  switch a {
+  case .Foo where 1 != 100, .Bar where 1 != 100:
+    break
+  }
+}
+
 
 
 // Errors in case syntax