Merge pull request #17967 from vguerra/SR7307

[TypeChecker] SR-7307 : Warn when a block contains defer as last statement

Author: rudkx

include/swift/AST/DiagnosticsSema.def

@@ -3033,6 +3033,10 @@ ERROR(jump_out_of_defer,none,
       "'%0' cannot transfer control out of a defer statement",
       (StringRef))
 
+WARNING(defer_stmt_at_block_end,none,
+     "'defer' statement before end of scope always executes immediately; "
+     "replace with 'do' statement to silence this warning", ())
+
 ERROR(return_invalid_outside_func,none,
       "return invalid outside of a func", ())
 

lib/Sema/TypeCheckStmt.cpp

@@ -313,6 +313,9 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
   /// expressions are not discarded.
   bool IsREPL;
 
+  /// Used to distinguish the first BraceStmt that starts a TopLevelCodeDecl.
+  bool IsBraceStmtFromTopLevelDecl;
+
   struct AddLabeledStmt {
     StmtChecker &SC;
     AddLabeledStmt(StmtChecker &SC, LabeledStmt *LS) : SC(SC) {
@@ -352,16 +355,21 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
   };
 
   StmtChecker(TypeChecker &TC, AbstractFunctionDecl *AFD)
-    : TC(TC), TheFunc(AFD), DC(AFD), IsREPL(false) { }
+      : TC(TC), TheFunc(AFD), DC(AFD), IsREPL(false),
+        IsBraceStmtFromTopLevelDecl(false) {}
 
   StmtChecker(TypeChecker &TC, ClosureExpr *TheClosure)
-    : TC(TC), TheFunc(TheClosure), DC(TheClosure), IsREPL(false) { }
+      : TC(TC), TheFunc(TheClosure), DC(TheClosure), IsREPL(false),
+        IsBraceStmtFromTopLevelDecl(false) {}
 
   StmtChecker(TypeChecker &TC, DeclContext *DC)
-    : TC(TC), TheFunc(), DC(DC), IsREPL(false) {
+      : TC(TC), TheFunc(), DC(DC), IsREPL(false),
+        IsBraceStmtFromTopLevelDecl(false) {
     if (const SourceFile *SF = DC->getParentSourceFile())
       if (SF->Kind == SourceFileKind::REPL)
         IsREPL = true;
+
+    IsBraceStmtFromTopLevelDecl = isa<TopLevelCodeDecl>(DC);
   }
 
   //===--------------------------------------------------------------------===//
@@ -550,7 +558,7 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
     Expr *theCall = DS->getCallExpr();
     TC.typeCheckExpression(theCall, DC);
     DS->setCallExpr(theCall);
-    
+
     return DS;
   }
 
@@ -1397,6 +1405,21 @@ void TypeChecker::checkIgnoredExpr(Expr *E) {
 
 Stmt *StmtChecker::visitBraceStmt(BraceStmt *BS) {
   const SourceManager &SM = TC.Context.SourceMgr;
+
+  // Diagnose defer statement being last one in block (only if
+  // BraceStmt does not start a TopLevelDecl).
+  if (IsBraceStmtFromTopLevelDecl) {
+    IsBraceStmtFromTopLevelDecl = false;
+  } else if (BS->getNumElements() > 0) {
+    if (auto stmt =
+            BS->getElement(BS->getNumElements() - 1).dyn_cast<Stmt *>()) {
+      if (auto deferStmt = dyn_cast<DeferStmt>(stmt)) {
+        TC.diagnose(deferStmt->getStartLoc(), diag::defer_stmt_at_block_end)
+            .fixItReplace(deferStmt->getStartLoc(), "do");
+      }
+    }
+  }
+
   for (auto &elem : BS->getElements()) {
     if (auto *SubExpr = elem.dyn_cast<Expr*>()) {
       SourceLoc Loc = SubExpr->getStartLoc();
@@ -1452,7 +1475,7 @@ Stmt *StmtChecker::visitBraceStmt(BraceStmt *BS) {
 
     TC.typeCheckDecl(SubDecl);
   }
-  
+
   return BS;
 }
 

test/SILGen/statements.swift

@@ -524,7 +524,7 @@ func defer_in_closure_in_generic<T>(_ x: T) {
   // CHECK-LABEL: sil private @$S10statements017defer_in_closure_C8_genericyyxlFyycfU_ : $@convention(thin) <T> () -> ()
   _ = {
     // CHECK-LABEL: sil private @$S10statements017defer_in_closure_C8_genericyyxlFyycfU_6$deferL_yylF : $@convention(thin) <T> () -> ()
-    defer { generic_callee_1(T.self) }
+    defer { generic_callee_1(T.self) } // expected-warning {{'defer' statement before end of scope always executes immediately}}{{5-10=do}}
   }
 }
 
@@ -537,7 +537,7 @@ func defer_mutable(_ x: Int) {
   // CHECK: function_ref @$S10statements13defer_mutableyySiF6$deferL_yyF : $@convention(thin) (@inout_aliasable Int) -> ()
   // CHECK-NOT: [[BOX]]
   // CHECK: destroy_value [[BOX]]
-  defer { _ = x }
+  defer { _ = x } // expected-warning {{'defer' statement before end of scope always executes immediately}}{{3-8=do}}
 }
 
 protocol StaticFooProtocol { static func foo() }

test/SILOptimizer/definite_init_diagnostics_globals.swift

@@ -21,7 +21,7 @@ defer { print(y) } // expected-error {{constant 'y' used in defer before being i
 // Test top-level functions.
 
 func testFunc() {       // expected-error {{variable 'x' used by function definition before being initialized}}
-  defer { print(x) }
+  defer { print(x) }    // expected-warning {{'defer' statement before end of scope always executes immediately}}{{3-8=do}}
 }
 
 // Test top-level closures.

test/SILOptimizer/unreachable_code.swift

@@ -328,7 +328,7 @@ func deferTryNoReturn() throws {
 }
 
 func noReturnInDefer() {
-  defer {
+  defer { // expected-warning {{'defer' statement before end of scope always executes immediately}}{{3-8=do}}
     _ = Lisp()
     die() // expected-note {{a call to a never-returning function}}
     die() // expected-warning {{will never be executed}}

test/Sema/diag_defer_block_end.swift

@@ -0,0 +1,23 @@
+// RUN: %target-swift-frontend -typecheck -verify %s
+
+let x = 1
+let y = 2
+if (x > y) {
+    defer { // expected-warning {{'defer' statement before end of scope always executes immediately}}{{5-10=do}}
+        print("not so useful defer stmt.")
+    }
+}
+
+func sr7307(_ value: Bool) {
+    let negated = !value 
+    defer { // expected-warning {{'defer' statement before end of scope always executes immediately}}{{5-10=do}}
+        print("negated value is {negated}")
+    }
+}
+
+sr7307(true)
+
+defer { // No note
+    print("end of program.")
+}
+

test/stmt/statements.swift

@@ -342,14 +342,17 @@ func test_defer(_ a : Int) {
 
   // Not ok.
   while false { defer { break } }   // expected-error {{'break' cannot transfer control out of a defer statement}}
+  // expected-warning@-1 {{'defer' statement before end of scope always executes immediately}}{{17-22=do}}
   defer { return }  // expected-error {{'return' cannot transfer control out of a defer statement}}
+  // expected-warning@-1 {{'defer' statement before end of scope always executes immediately}}{{3-8=do}}
 }
 
 class SomeTestClass {
   var x = 42
 
   func method() {
     defer { x = 97 }  // self. not required here!
+    // expected-warning@-1 {{'defer' statement before end of scope always executes immediately}}{{5-10=do}}
   }
 }
 

test/stmt/yield.swift

@@ -82,7 +82,7 @@ func call_yield() {
 struct YieldInDefer {
   var property: String {
     _read {
-      defer {
+      defer { // expected-warning {{'defer' statement before end of scope always executes immediately}}{{7-12=do}}
         // FIXME: this recovery is terrible
         yield ""
         // expected-error@-1 {{expression resolves to an unused function}}