From c8eb37872b81958551b8a49da3f5ce6e8fe3de0d Mon Sep 17 00:00:00 2001 From: Jon Date: Fri, 26 Jan 2018 21:56:10 -0500 Subject: [PATCH 1/2] [interpreter,spec] (#109) Fix `self` and scope resolution for blocks on static methods. `Invocation` was incorrectly determining the value of `self` to use when calling a function. Before #131, it was only checking if the Call had a receiver, and would use that value if it did. After #131, if the Call did _not_ have a receiver, it would then check if the functor was a closure, and use the closed value of `self` if it was. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now, Invocations _always_ perform both checks, with closures taking higher precedence than Call receivers (though, both will be pushed to the stack if the conditions are met). Before this commit, I was very confused why static methods weren't working while non-static methods were. I assumed it was an issue with `scope` vs `instance_scope` on Types or something like that, but had no idea. Now, though, I'm even more confused how _any_ case was working previously, as the Call receiver was _always_ being pushed as the value of `self`. At least with this version of the code, the value of `self` that will be present is more explicit and well-specified. ¯\_(ツ)_/¯ --- spec/interpreter/misc/closures_spec.cr | 217 ++++++++++++++++++ .../nodes/anonymous_function_spec.cr | 14 -- spec/interpreter/nodes/block_spec.cr | 10 - src/myst/interpreter/invocation.cr | 14 +- test.mt | 13 ++ 5 files changed, 235 insertions(+), 33 deletions(-) create mode 100644 spec/interpreter/misc/closures_spec.cr create mode 100644 test.mt diff --git a/spec/interpreter/misc/closures_spec.cr b/spec/interpreter/misc/closures_spec.cr new file mode 100644 index 0000000..2a75ebb --- /dev/null +++ b/spec/interpreter/misc/closures_spec.cr @@ -0,0 +1,217 @@ +require "../../spec_helper.cr" +require "../../support/nodes.cr" +require "../../support/interpret.cr" + +describe "Interpreter - Closures" do + describe "on blocks" do + it "captures the value of `self` as part of the closure" do + itr = parse_and_interpret %q( + @sum = 0 + [1, 2, 3].each{ |e| @sum += e } + @sum + ) + + itr.stack.last.should eq(val(6)) + end + + it "can access variables from blocks on static methods (see #109)" do + itr = parse_and_interpret %q( + deftype Test + defstatic hello(&block) + block() + end + end + + i = 0 + Test.hello do + i += 6 + end + i + ) + + itr.stack.last.should eq(val(6)) + end + + it "can access variables from within nested blocks" do + itr = parse_and_interpret %q( + defmodule Test + def hello(&block) + block() + end + end + + i = 0 + Test.hello do + [1, 2, 3].each{ |e| i += e } + end + i + ) + + itr.stack.last.should eq(val(6)) + end + + it "can access variables from within nested blocks on static methods (see #109)" do + itr = parse_and_interpret %q( + deftype Test + defstatic hello(&block) + block() + end + end + + i = 0 + Test.hello do + [1, 2, 3].each{ |e| i += e } + end + i + ) + + itr.stack.last.should eq(val(6)) + end + + it "maintains the top level value of `self` from within nested blocks" do + itr = parse_and_interpret %q( + deftype Test + defstatic hello(&block) + block() + end + end + + @sum = 0 + Test.hello do + [1, 2, 3].each{ |e| @sum += e } + end + @sum + ) + + itr.stack.last.should eq(val(6)) + end + + it "can access variables from a block within an anonymous function" do + itr = parse_and_interpret %q( + defmodule Test + def hello(&block) + block() + end + end + + i = 0 + + func = fn + ->() { [1, 2, 3].each{ |e| i += e }; i } + end + + Test.hello(&func) + ) + + itr.stack.last.should eq(val(6)) + end + + it "can access variables from a block within an anonymous function on static methods (see #109)" do + itr = parse_and_interpret %q( + deftype Test + defstatic hello(&block) + block() + end + end + + i = 0 + + func = fn + ->() { [1, 2, 3].each{ |e| i += e }; i } + end + + Test.hello(&func) + ) + + itr.stack.last.should eq(val(6)) + end + end + + + describe "on anonymous functions" do + it "captures the value of `self` as part of the closure" do + itr = parse_and_interpret %q( + @sum = 0 + func = fn + ->(a) { @sum += a } + end + + func(6) + @sum + ) + + itr.stack.last.should eq(val(6)) + end + + it "can access variables from anonymous functions on static methods (see #109)" do + itr = parse_and_interpret %q( + deftype Test + defstatic hello(&block) + block() + end + end + + i = 0 + Test.hello(&fn ->() { i += 6 } end) + i + ) + + itr.stack.last.should eq(val(6)) + end + + it "can access variables from within a nested block" do + itr = parse_and_interpret %q( + defmodule Test + def hello(&block) + block() + end + end + + i = 0 + Test.hello do + func = fn ->(e) { i += e } end + [1, 2, 3].each(&func) + end + i + ) + + itr.stack.last.should eq(val(6)) + end + + it "can access variables from within a nested block on static methods (see #109)" do + itr = parse_and_interpret %q( + deftype Test + defstatic hello(&block) + block() + end + end + + i = 0 + Test.hello do + func = fn ->(e) { i += e } end + [1, 2, 3].each(&func) + end + i + ) + + itr.stack.last.should eq(val(6)) + end + + it "maintains the top level value of `self` from within a nested block" do + itr = parse_and_interpret %q( + deftype Test + defstatic hello(&block) + block() + end + end + + @sum = 0 + Test.hello do + func = fn ->(e) { @sum += e } end + [1, 2, 3].each(&func) + end + @sum + ) + end + end +end diff --git a/spec/interpreter/nodes/anonymous_function_spec.cr b/spec/interpreter/nodes/anonymous_function_spec.cr index c4503c0..82e3d24 100644 --- a/spec/interpreter/nodes/anonymous_function_spec.cr +++ b/spec/interpreter/nodes/anonymous_function_spec.cr @@ -30,20 +30,6 @@ describe "Interpreter - AnonymousFunction" do itr.stack.last.should eq(val(:called_foo)) end - it "captures the value of `self` as part of the closure" do - itr = parse_and_interpret %q( - @sum = 0 - func = fn - ->(a) { @sum += a } - end - - func(6) - @sum - ) - - itr.stack.last.should eq(val(6)) - end - it "allows clauses of various arities" do itr = parse_and_interpret %q( fn diff --git a/spec/interpreter/nodes/block_spec.cr b/spec/interpreter/nodes/block_spec.cr index 874a1bf..64f6ad8 100644 --- a/spec/interpreter/nodes/block_spec.cr +++ b/spec/interpreter/nodes/block_spec.cr @@ -63,14 +63,4 @@ describe "Interpreter - Block" do itr.stack.last.should eq(val(6)) end - - it "captures the value of `self` as part of the closure" do - itr = parse_and_interpret %q( - @sum = 0 - [1, 2, 3].each{ |e| @sum += e } - @sum - ) - - itr.stack.last.should eq(val(6)) - end end diff --git a/src/myst/interpreter/invocation.cr b/src/myst/interpreter/invocation.cr index 5163c95..c55d2ed 100644 --- a/src/myst/interpreter/invocation.cr +++ b/src/myst/interpreter/invocation.cr @@ -21,15 +21,11 @@ module Myst def invoke @selfstack_size_at_entry = @itr.self_stack.size - case - when @receiver - # If the invocation has a receiver, use it as the current value of `self` - # for the duration of the Invocation. - @itr.push_self(@receiver.not_nil!) - when @func.closure? - # If the invoked functor is a closure, use the closed value of `self`. - @itr.push_self(@func.closed_self) - end + # If the invocation has a receiver, use it as the current value of `self` + # for the duration of the Invocation. + @itr.push_self(@receiver.not_nil!) if @receiver + # If the invoked functor is a closure, use the closed value of `self`. + @itr.push_self(@func.closed_self) if @func.closure? result = @func.clauses.each do |clause| @itr.push_scope_override(@func.new_scope) diff --git a/test.mt b/test.mt new file mode 100644 index 0000000..e8f8f88 --- /dev/null +++ b/test.mt @@ -0,0 +1,13 @@ +deftype Test + defstatic hello(&block) + block() + end +end + +i = 0 + +func = fn + ->() { 10.times{ i += 1 } } +end + +Test.hello(&func) From a583ff65ce8ae0b7f8b93de670b083ab5576bf9a Mon Sep 17 00:00:00 2001 From: Jon Date: Fri, 26 Jan 2018 23:29:17 -0500 Subject: [PATCH 2/2] [interpreter,stdlib] Fix Const lookup to work with blocks capturing `self`. Const lookup was only operating on the current scope and current `self`, rather than looking at the full lexical scope as well. Const lookup now does full recursive lookup (like a Call) to properly resolve in most cases. This commit also makes some changes to the Spec library to work with the new semantics of capturing `self` with closures. --- spec/myst/spec.mt | 1 + src/myst/interpreter/nodes/references.cr | 2 +- stdlib/spec.mt | 1 + stdlib/spec/dsl.mt | 41 ++++++++++++++++++++++++ stdlib/spec/errors.mt | 6 ++-- stdlib/spec/single_spec.mt | 39 ---------------------- test.mt | 13 -------- 7 files changed, 46 insertions(+), 57 deletions(-) create mode 100644 stdlib/spec/dsl.mt delete mode 100644 test.mt diff --git a/spec/myst/spec.mt b/spec/myst/spec.mt index 90b7daf..c410df1 100644 --- a/spec/myst/spec.mt +++ b/spec/myst/spec.mt @@ -1,6 +1,7 @@ require "stdlib/spec.mt" include Spec +include Spec.DSL # TODO: add Dir globbing to automatically detect and require all `*_spec.mt` # files under this directory. diff --git a/src/myst/interpreter/nodes/references.cr b/src/myst/interpreter/nodes/references.cr index 335de92..466221f 100644 --- a/src/myst/interpreter/nodes/references.cr +++ b/src/myst/interpreter/nodes/references.cr @@ -20,7 +20,7 @@ module Myst end def visit(node : Const) - if value = current_scope[node.name]? || __typeof(current_self).scope[node.name] + if value = (current_scope[node.name]? || __typeof(current_self).scope[node.name]? || recursive_lookup(current_self, node.name)) stack.push(value) else @callstack.push(node) diff --git a/stdlib/spec.mt b/stdlib/spec.mt index 4f33243..32fb2fc 100644 --- a/stdlib/spec.mt +++ b/stdlib/spec.mt @@ -1,3 +1,4 @@ +require "./spec/dsl.mt" require "./spec/errors.mt" require "./spec/single_spec.mt" diff --git a/stdlib/spec/dsl.mt b/stdlib/spec/dsl.mt new file mode 100644 index 0000000..3ff6932 --- /dev/null +++ b/stdlib/spec/dsl.mt @@ -0,0 +1,41 @@ +defmodule Spec + defmodule DSL + def assert(assertion) + unless assertion + raise %AssertionFailure{true, assertion} + end + end + + def refute(assertion) + when assertion + raise %AssertionFailure{false, assertion} + end + end + + # Expect the given block to raise an error matching the given value. If no + # error, or an error with a different value, is raised, the assertion fails. + def expect_raises(expected_error, &block) + block() + raise %AssertionFailure{expected_error, "no error"} + rescue + # If the raised error matches what was expected, the assertion passes. + rescue received_error + # For any other error + raise %AssertionFailure{expected_error, received_error} + end + + # Same as `expect_raises(expected, &block)`, but without the expectation of + # a specific error. + def expect_raises(&block) + block() + raise %AssertionFailure{"Any Error", "no error"} + rescue ex : AssertionFailure + # Rescuing an AssertionFailure implies that `block` did + # not raise an exception, so the exception is re-raised. + raise ex + rescue + # Otherwise, the error must have come from `block`, so the + # assertion is successful. + end + end +end diff --git a/stdlib/spec/errors.mt b/stdlib/spec/errors.mt index 474206c..25d49ab 100644 --- a/stdlib/spec/errors.mt +++ b/stdlib/spec/errors.mt @@ -7,18 +7,16 @@ defmodule Spec # made within an `it` block fails. The failure contains the Spec object that # failed, the value that was expected, and the value that was received. deftype AssertionFailure - def initialize(name : String, expected, got) - @name = name + def initialize(expected, got) @expected = expected @got = got end - def name; @name; end def expected; @expected; end def got; @got; end def to_s - "Assertion failed: <(@name)>.\n" + + "Assertion failed.\n" + " Expected: <(@expected)>\n" + " Got: <(@got)>\n" end diff --git a/stdlib/spec/single_spec.mt b/stdlib/spec/single_spec.mt index 796e8a2..a0f29d2 100644 --- a/stdlib/spec/single_spec.mt +++ b/stdlib/spec/single_spec.mt @@ -14,44 +14,5 @@ defmodule Spec IO.puts(failure) exit(1) end - - - def assert(assertion) - unless assertion - raise %AssertionFailure{@name, true, assertion} - end - end - - def refute(assertion) - when assertion - raise %AssertionFailure{@name, false, assertion} - end - end - - # Expect the given block to raise an error matching the given value. If no - # error, or an error with a different value, is raised, the assertion fails. - def expect_raises(expected_error, &block) - block() - raise %AssertionFailure{@name, expected_error, "no error"} - rescue - # If the raised error matches what was expected, the assertion passes. - rescue received_error - # For any other error - raise %AssertionFailure{@name, expected_error, received_error} - end - - # Same as `expect_raises(expected, &block)`, but without the expectation of - # a specific error. - def expect_raises(&block) - block() - raise %AssertionFailure{@name, "Any Error", "no error"} - rescue ex : AssertionFailure - # Rescuing an AssertionFailure implies that `block` did - # not raise an exception, so the exception is re-raised. - raise ex - rescue - # Otherwise, the error must have come from `block`, so the - # assertion is successful. - end end end diff --git a/test.mt b/test.mt deleted file mode 100644 index e8f8f88..0000000 --- a/test.mt +++ /dev/null @@ -1,13 +0,0 @@ -deftype Test - defstatic hello(&block) - block() - end -end - -i = 0 - -func = fn - ->() { 10.times{ i += 1 } } -end - -Test.hello(&func)