Deprecation warnings for C-style for loops

Warns of deprecation, checks all the appropriate bits to see if we can
do an automatic fix, and generates fix-its if that is valid.

Also adds a note if the loop looks like it ought to be a simple
for-each, but really isn’t because the loop var is modified inside the
loop.

Author: gregomni

include/swift/AST/DiagnosticsSema.def

@@ -2014,6 +2014,12 @@ NOTE(change_to_mutating,sema_tcs,none,
      "mark %select{method|accessor}0 'mutating' to make 'self' mutable",
      (bool))
 
+// For Stmt
+WARNING(deprecated_c_style_for_stmt,sema_tcs,none,
+"C-style for statement is deprecated and will be removed in a future version of Swift", ())
+NOTE(cant_fix_c_style_for_stmt,sema_tcs,none,
+"C-style for statement can't be automatically fixed to for-in, because the loop variable is modified inside the loop", ())
+
 // ForEach Stmt
 ERROR(sequence_protocol_broken,sema_tcs,none,
       "SequenceType protocol definition is broken", ())

include/swift/AST/Stmt.h

@@ -800,6 +800,9 @@ class ForStmt : public LabeledStmt {
 
   SourceLoc getStartLoc() const { return getLabelLocOrKeywordLoc(ForLoc); }
   SourceLoc getEndLoc() const { return Body->getEndLoc(); }
+
+  SourceLoc getFirstSemicolonLoc() const { return Semi1Loc; }
+  SourceLoc getSecondSemicolonLoc() const { return Semi2Loc; }
 
   NullablePtr<Expr> getInitializer() const { return Initializer; }
   void setInitializer(Expr *V) { Initializer = V; }

lib/Sema/MiscDiagnostics.cpp

@@ -1139,25 +1139,6 @@ static void diagAvailability(TypeChecker &TC, const Expr *E,
   const_cast<Expr*>(E)->walk(walker);
 }
 
-//===--------------------------------------------------------------------===//
-// High-level entry points.
-//===--------------------------------------------------------------------===//
-
-/// \brief Emit diagnostics for syntactic restrictions on a given expression.
-void swift::performSyntacticExprDiagnostics(TypeChecker &TC, const Expr *E,
-                                            const DeclContext *DC,
-                                            bool isExprStmt) {
-  diagSelfAssignment(TC, E);
-  diagSyntacticUseRestrictions(TC, E, DC, isExprStmt);
-  diagRecursivePropertyAccess(TC, E, DC);
-  diagnoseImplicitSelfUseInClosure(TC, E, DC);
-  diagAvailability(TC, E, DC);
-}
-
-void swift::performStmtDiagnostics(TypeChecker &TC, const Stmt *S) {
-  TC.checkUnsupportedProtocolType(const_cast<Stmt *>(S));
-}
-
 //===--------------------------------------------------------------------===//
 // Per func/init diagnostics
 //===--------------------------------------------------------------------===//
@@ -1201,7 +1182,16 @@ class VarDeclUsageChecker : public ASTWalker {
           VarDecls[VD] = 0;
       });
   }
-
+    
+  VarDeclUsageChecker(TypeChecker &TC, VarDecl *VD) : TC(TC) {
+    // Track a specific VarDecl
+    VarDecls[VD] = 0;
+  }
+    
+  void suppressDiagnostics() {
+    sawError = true; // set this flag so that no diagnostics will be emitted on delete.
+  }
+    
   // After we have scanned the entire region, diagnose variables that could be
   // declared with a narrower usage kind.
   ~VarDeclUsageChecker();
@@ -1224,6 +1214,10 @@ class VarDeclUsageChecker : public ASTWalker {
     }
     return sawMutation;
   }
+    
+  bool isVarDeclEverWritten(VarDecl *VD) {
+    return (VarDecls[VD] & RK_Written) != 0;
+  }
 
   bool shouldTrackVarDecl(VarDecl *VD) {
     // If the variable is implicit, ignore it.
@@ -1689,8 +1683,128 @@ void swift::performAbstractFuncDeclDiagnostics(TypeChecker &TC,
   AFD->getBody()->walk(VarDeclUsageChecker(TC, AFD));
 }
 
+/// Diagnose C style for loops.
+
+static Expr *endConditionValueForConvertingCStyleForLoop(const ForStmt *FS, VarDecl *loopVar) {
+  auto *Cond = FS->getCond().getPtrOrNull();
+  if (!Cond)
+    return nullptr;
+  auto callExpr = dyn_cast<CallExpr>(Cond);
+  if (!callExpr)
+    return nullptr;
+  auto dotSyntaxExpr = dyn_cast<DotSyntaxCallExpr>(callExpr->getFn());
+  if (!dotSyntaxExpr)
+    return nullptr;
+  auto binaryExpr = dyn_cast<BinaryExpr>(dotSyntaxExpr->getBase());
+  if (!binaryExpr)
+    return nullptr;
+  auto binaryFuncExpr = dyn_cast<DeclRefExpr>(binaryExpr->getFn());
+  if (!binaryFuncExpr)
+    return nullptr;
+
+  // Verify that the condition is a simple != or < comparison to the loop variable.
+  auto comparisonOpName = binaryFuncExpr->getDecl()->getNameStr();
+  if (comparisonOpName != "!=" && comparisonOpName != "<")
+    return nullptr;
+  auto args = binaryExpr->getArg()->getElements();
+  auto loadExpr = dyn_cast<LoadExpr>(args[0]);
+  if (!loadExpr)
+    return nullptr;
+  auto declRefExpr = dyn_cast<DeclRefExpr>(loadExpr->getSubExpr());
+  if (!declRefExpr)
+    return nullptr;
+  if (declRefExpr->getDecl() != loopVar)
+    return nullptr;
+  return args[1];
+}
 
+static bool simpleIncrementForConvertingCStyleForLoop(const ForStmt *FS, VarDecl *loopVar) {
+  auto *Increment = FS->getIncrement().getPtrOrNull();
+  if (!Increment)
+    return false;
+  ApplyExpr *unaryExpr = dyn_cast<PrefixUnaryExpr>(Increment);
+  if (!unaryExpr)
+    unaryExpr = dyn_cast<PostfixUnaryExpr>(Increment);
+  if (!unaryExpr)
+    return false;
+  auto inoutExpr = dyn_cast<InOutExpr>(unaryExpr->getArg());
+  if (!inoutExpr)
+   return false;
+  auto incrementDeclRefExpr = dyn_cast<DeclRefExpr>(inoutExpr->getSubExpr());
+  if (!incrementDeclRefExpr)
+    return false;
+  auto unaryFuncExpr = dyn_cast<DeclRefExpr>(unaryExpr->getFn());
+  if (!unaryFuncExpr)
+    return false;
+  if (unaryFuncExpr->getDecl()->getNameStr() != "++")
+    return false;
+  return incrementDeclRefExpr->getDecl() == loopVar;    
+}
 
+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 (FS->getFirstSemicolonLoc().isInvalid() || FS->getSecondSemicolonLoc().isInvalid())
+    return;
+    
+  InFlightDiagnostic diagnostic = TC.diagnose(FS->getStartLoc(), diag::deprecated_c_style_for_stmt);
+    
+  // Try to construct a fix it using for-each:
+
+  // 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;
+  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 = simpleIncrementForConvertingCStyleForLoop(FS, loopVar);
+
+  if (!loopVar || !startValue || !endValue || !strideByOne)
+    return;
+    
+  // Verify that the loop variable is invariant inside the body.
+  VarDeclUsageChecker checker(TC, loopVar);
+  checker.suppressDiagnostics();
+  FS->getBody()->walk(checker);
+    
+  if (checker.isVarDeclEverWritten(loopVar)) {
+    diagnostic.flush();
+    TC.diagnose(FS->getStartLoc(), diag::cant_fix_c_style_for_stmt);
+    return;
+  }
+    
+  SourceLoc loopPatternEnd = Lexer::getLocForEndOfToken(TC.Context.SourceMgr, loopVarDecl->getPattern(0)->getEndLoc());
+  SourceLoc endOfIncrementLoc = Lexer::getLocForEndOfToken(TC.Context.SourceMgr, FS->getIncrement().getPtrOrNull()->getEndLoc());
+    
+  diagnostic
+   .fixItReplaceChars(loopPatternEnd, startValue->getStartLoc(), " in ")
+   .fixItReplaceChars(FS->getFirstSemicolonLoc(), endValue->getStartLoc(), " ..< ")
+   .fixItRemoveChars(FS->getSecondSemicolonLoc(), endOfIncrementLoc);
+}
+
+//===--------------------------------------------------------------------===//
+// High-level entry points.
+//===--------------------------------------------------------------------===//
+
+/// \brief Emit diagnostics for syntactic restrictions on a given expression.
+void swift::performSyntacticExprDiagnostics(TypeChecker &TC, const Expr *E,
+                                            const DeclContext *DC,
+                                            bool isExprStmt) {
+  diagSelfAssignment(TC, E);
+  diagSyntacticUseRestrictions(TC, E, DC, isExprStmt);
+  diagRecursivePropertyAccess(TC, E, DC);
+  diagnoseImplicitSelfUseInClosure(TC, E, DC);
+  diagAvailability(TC, E, DC);
+}
+
+void swift::performStmtDiagnostics(TypeChecker &TC, const Stmt *S) {
+  TC.checkUnsupportedProtocolType(const_cast<Stmt *>(S));
+    
+  if (auto forStmt = dyn_cast<ForStmt>(S))
+    checkCStyleForLoop(TC, forStmt);
+}
 
 //===--------------------------------------------------------------------===//
 // Utility functions

test/Parse/recovery.swift

@@ -162,16 +162,16 @@ func missingControllingExprInFor() {
   }
 
   // Ensure that we don't do recovery in the following cases.
-  for ; ; {
+  for ; ; { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
   }
 
-  for { true }(); ; {
+  for { true }(); ; { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
   }
 
-  for ; { true }() ; {
+  for ; { true }() ; { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
   }
 
-  for acceptsClosure { 42 }; ; {
+  for acceptsClosure { 42 }; ; { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
   }
 
   // A trailing closure is not accepted for the condition.

test/SILGen/statements.swift

@@ -161,15 +161,15 @@ func for_loops1(x: Int, c: Bool) {
     markUsed(i)
   }
 
-  for ; x < 40;  {
+  for ; x < 40;  { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
    markUsed(x)
    x += 1
   }
 
-  for var i = 0; i < 100; ++i {
+  for var i = 0; i < 100; ++i { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
   }
 
-  for let i = 0; i < 100; i {
+  for let i = 0; i < 100; i { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
   }
 }
 

test/SILGen/unreachable_code.swift

@@ -16,7 +16,7 @@ func testUnreachableAfterIfReturn(a: Bool) -> Int {
 }
 
 func testUnreachableForAfterContinue(b: Bool) {
-  for (var i:Int = 0; i<10; i++) { 
+  for (var i:Int = 0; i<10; i++) { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
     var y: Int = 300;
     y++;
     if b {
@@ -45,7 +45,7 @@ func testUnreachableWhileAfterContinue(b: Bool) {
 func testBreakAndContinue() {
   var i = 0;
   var m = 0;
-  for (i = 0; i < 10; ++i) {
+  for (i = 0; i < 10; ++i) { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
     m += 1
     if m == 15 {
       break

test/Sema/diag_c_style_for.swift

@@ -0,0 +1,45 @@
+// RUN: %target-parse-verify-swift
+
+for var a = 0; a < 10; a++ { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}} {{10-13= in }} {{14-20= ..< }} {{22-27=}}
+}
+
+for var b = 0; b < 10; ++b { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}} {{10-13= in }} {{14-20= ..< }} {{22-27=}}
+}
+
+for var c=1;c != 5 ;++c { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}} {{10-11= in }} {{12-18= ..< }} {{20-24=}}
+}
+
+for var d=100;d<5;d++ { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}} {{10-11= in }} {{14-17= ..< }} {{18-22=}}
+}
+
+// next three aren't auto-fixable
+for var e = 3; e > 4; e++ { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+}
+
+for var f = 3; f < 4; f-- { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+}
+
+let start = Int8(4)
+let count = Int8(10)
+var other = Int8(2)
+
+for ; other<count; other++ { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+}
+
+// this should be fixable, and keep the type
+for (var number : Int8 = start; number < count; number++) { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}} {{23-26= in }} {{31-42= ..< }} {{47-57=}}
+  print(number)
+}
+
+// should produce extra note
+for (var m : Int8 = start; m < count; ++m) { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}} expected-note {{C-style for statement can't be automatically fixed to for-in, because the loop variable is modified inside the loop}}
+  m += 3
+}
+
+// could theoretically fix this (and more like it if we auto-suggested "stride:")
+for var o = 2; o < 888; o += 1 { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+}
+
+// could theoretically fix this with "..."
+for var p = 2; p <= 8; p++ { // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+}

test/stmt/statements.swift

@@ -120,23 +120,23 @@ SomeGeneric<Int>
 
 func for_loop() {
   var x = 0
-  for ;; { }
-  for x = 1; x != 42; ++x { }
-  for infloopbooltest(); x != 12; infloopbooltest() {}
+  for ;; { } // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+  for x = 1; x != 42; ++x { } // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+  for infloopbooltest(); x != 12; infloopbooltest() {} // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
 
   for ; { } // expected-error {{expected ';' in 'for' statement}}
 
-  for var y = 1; y != 42; ++y {}
-  for (var y = 1; y != 42; ++y) {}
+  for var y = 1; y != 42; ++y {} // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+  for (var y = 1; y != 42; ++y) {} // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
   var z = 10
-  for (; z != 0; --z) {}
-  for (z = 10; z != 0; --z) {}
-  for var (a,b) = (0,12); a != b; --b {++a}
-  for (var (a,b) = (0,12); a != b; --b) {++a}
+  for (; z != 0; --z) {} // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+  for (z = 10; z != 0; --z) {} // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+  for var (a,b) = (0,12); a != b; --b {++a} // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+  for (var (a,b) = (0,12); a != b; --b) {++a} // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
   var j, k : Int
-  for ((j,k) = (0,10); j != k; --k) {}
-  for var i = 0, j = 0; i * j < 10; i++, j++ {}
-  for j = 0, k = 52; j < k; ++j, --k { }
+  for ((j,k) = (0,10); j != k; --k) {} // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+  for var i = 0, j = 0; i * j < 10; i++, j++ {} // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
+  for j = 0, k = 52; j < k; ++j, --k { } // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
   // rdar://19540536
   // expected-error@+4{{expected var declaration in a 'for' statement}}
   // expected-error@+3{{expression resolves to an unused function}}
@@ -145,7 +145,7 @@ func for_loop() {
   for @ {}
 
   // <rdar://problem/17462274> Is increment in for loop optional?
-  for (let i = 0; i < 10; ) {}
+  for (let i = 0; i < 10; ) {} // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
 }
 
 break // expected-error {{'break' is only allowed inside a loop, if, do, or switch}}
@@ -426,13 +426,13 @@ func testThrowNil() throws {
 // <rdar://problem/16650625>
 func for_ignored_lvalue_init() {
   var i = 0
-  for i;  // expected-error {{expression resolves to an unused l-value}}
+  for i;  // expected-error {{expression resolves to an unused l-value}} expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
     i < 10; ++i {}
 }
 
 // rdar://problem/18643692
 func for_loop_multi_iter() {
-  for (var i = 0, x = 0; i < 10; i++,
+  for (var i = 0, x = 0; i < 10; i++, // expected-warning {{C-style for statement is deprecated and will be removed in a future version of Swift}}
        x) { // expected-error {{expression resolves to an unused l-value}}
     x -= 1
   }