Semantic: disallow usage of return/break/next inside ensure (#4486)

makenowjust · Martin Verzilli · commit 9965a9b38ab8 · 2017-09-16T19:12:01.000-03:00
* Semantic: refactor control statement inside ensure detection Not use Array for stack and add utility method `with_block_kind`, by #4486 (comment). Correct `return` statement error message to fit other `return` statement messages. * Remove URL from error messages #4486 (comment)
diff --git a/spec/compiler/semantic/exception_spec.cr b/spec/compiler/semantic/exception_spec.cr
@@ -476,4 +476,182 @@ describe "Semantic: exception" do
     program = result.program
     program.vars["foo"].type.should be(program.nilable program.int32)
   end
+
+  it "can't return from ensure (#4470)" do
+    assert_error(%(
+      def foo
+        return 1
+      ensure
+        return 2
+      end
+
+      foo
+    ), "can't return from ensure")
+  end
+
+  it "can't return from block inside ensure (#4470)" do
+    assert_error(%(
+      def once
+        yield
+      end
+
+      def foo
+        return 1
+      ensure
+        once do
+          return 2
+        end
+      end
+
+      foo
+    ), "can't return from ensure")
+  end
+
+  it "can't return from while inside ensure (#4470)" do
+    assert_error(%(
+      def foo
+        return 1
+      ensure
+        while true
+          return 2
+        end
+      end
+
+      foo
+    ), "can't return from ensure")
+  end
+
+  it "can't use break inside while inside ensure (#4470)" do
+    assert_error(%(
+      while true
+        begin
+          break
+        ensure
+          break
+        end
+      end
+    ), "can't use break inside ensure")
+  end
+
+  it "can use break inside while inside ensure (#4470)" do
+    assert_type(%(
+      while true
+        begin
+          break
+        ensure
+          while true
+            break
+          end
+        end
+      end
+    )) { nil_type }
+  end
+
+  it "can't use break inside block inside ensure (#4470)" do
+    assert_error(%(
+      def loop
+        while true
+          yield
+        end
+      end
+
+      loop do
+        begin
+          break
+        ensure
+          break
+        end
+      end
+    ), "can't use break inside ensure")
+  end
+
+  it "can use break inside block inside ensure (#4470)" do
+    assert_type(%(
+      def loop
+        while true
+          yield
+        end
+      end
+
+      loop do
+        begin
+          break
+        ensure
+          loop do
+            break
+          end
+        end
+      end
+    )) { nil_type }
+  end
+
+  it "can't use next inside while inside ensure (#4470)" do
+    assert_error(%(
+      while true
+        begin
+          break
+        ensure
+          next
+        end
+      end
+    ), "can't use next inside ensure")
+  end
+
+  it "can't use next inside block inside ensure (#4470)" do
+    assert_error(%(
+      def loop
+        while true
+          yield
+        end
+      end
+
+      loop do
+        begin
+          break
+        ensure
+          next
+        end
+      end
+    ), "can't use next inside ensure")
+  end
+
+  it "can use next inside while inside ensure (#4470)" do
+    assert_type(%(
+      while true
+        begin
+          break
+        ensure
+          a = 0
+          while a < 1
+            a = 1
+            next
+          end
+        end
+      end
+    )) { nil_type }
+  end
+
+  it "can use next inside block inside ensure (#4470)" do
+    assert_type(%(
+      def loop
+        while true
+          yield
+        end
+      end
+
+      def once
+        yield
+      end
+
+      loop do
+        begin
+          break
+        ensure
+          once do
+            next
+          end
+        end
+      end
+    )) { nil_type }
+  end
 end
diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr
@@ -67,6 +67,26 @@ module Crystal
     property is_initialize : Bool
     property exception_handler_vars : MetaVars? = nil
 
+    # It means the last block kind, that is one of `block`, `while` and
+    # `ensure`. It is used to detect `break` or `next` from `ensure`.
+    #
+    # ```
+    # begin
+    #   # `last_block_kind == nil`
+    # ensure
+    #   # `last_block_kind == :ensure`
+    #   while true
+    #     # `last_block_kind == :while`
+    #   end
+    #   loop do
+    #     # `last_block_kind == :block`
+    #   end
+    #   # `last_block_kind == :ensure`
+    # end
+    # ```
+    property last_block_kind : Symbol?
+    property? inside_ensure : Bool = false
+
     @unreachable = false
     @is_initialize = false
     @in_type_args = 0
@@ -1031,6 +1051,9 @@ module Crystal
       block_visitor.path_lookup = path_lookup || current_type
       block_visitor.block_nest = @block_nest
 
+      block_visitor.last_block_kind = :block
+      block_visitor.inside_ensure = inside_ensure?
+
       node.body.accept block_visitor
 
       @block_nest -= 1
@@ -1628,6 +1651,10 @@ module Crystal
     end
 
     def visit(node : Return)
+      if inside_ensure?
+        node.raise "can't return from ensure"
+      end
+
       typed_def = @typed_def || node.raise("can't return from top level")
 
       if typed_def.captured_block?
@@ -1985,7 +2012,10 @@ module Crystal
       @block, old_block = nil, @block
 
       @while_stack.push node
-      node.body.accept self
+
+      with_block_kind :while do
+        node.body.accept self
+      end
 
       endless_while = node.cond.true_literal?
       merge_while_vars node.cond, endless_while, before_cond_vars_copy, before_cond_vars, after_cond_vars, @vars, node.break_vars
@@ -2149,6 +2179,10 @@ module Crystal
     end
 
     def end_visit(node : Break)
+      if last_block_kind == :ensure
+        node.raise "can't use break inside ensure"
+      end
+
       if block = @block
         node.target = block.call.not_nil!
 
@@ -2174,6 +2208,10 @@ module Crystal
     end
 
     def end_visit(node : Next)
+      if last_block_kind == :ensure
+        node.raise "can't use next inside ensure"
+      end
+
       if block = @block
         node.target = block
 
@@ -2200,6 +2238,14 @@ module Crystal
       @unreachable = true
     end
 
+    def with_block_kind(kind)
+      old_block_kind, @last_block_kind = last_block_kind, kind
+      old_inside_ensure, @inside_ensure = @inside_ensure, @inside_ensure || kind == :ensure
+      yield
+      @last_block_kind = old_block_kind
+      @inside_ensure = old_inside_ensure
+    end
+
     def visit(node : Primitive)
       case node.name
       when "binary"
@@ -2599,7 +2645,9 @@ module Crystal
           merge_rescue_vars exception_handler_vars, all_rescue_vars
 
           # And then accept the ensure part
-          node.ensure.try &.accept self
+          with_block_kind :ensure do
+            node.ensure.try &.accept self
+          end
         end
       end
 
@@ -2619,7 +2667,9 @@ module Crystal
 
           before_ensure_vars = @vars.dup
 
-          node_ensure.accept self
+          with_block_kind :ensure do
+            node_ensure.accept self
+          end
 
           @vars = after_handler_vars
 
