From 8957feedb44b272f1343ec847ae0322d49a54c62 Mon Sep 17 00:00:00 2001 From: Jeremy Ashkenas Date: Sat, 16 Jan 2010 12:10:31 -0500 Subject: [PATCH] expression closure wrappers are now safer -- they won't be generated if there's a statement_only inside --- lib/coffee_script/grammar.y | 2 +- lib/coffee_script/nodes.rb | 29 +++++++++--------- lib/coffee_script/scope.rb | 2 +- lib/coffee_script/value.rb | 6 ++++ .../execution/test_expressions.coffee | 30 +++++++++++++++++++ test/fixtures/execution/test_functions.coffee | 18 ----------- 6 files changed, 52 insertions(+), 35 deletions(-) create mode 100644 test/fixtures/execution/test_expressions.coffee diff --git a/lib/coffee_script/grammar.y b/lib/coffee_script/grammar.y index d7ed1c3c..799f04d4 100644 --- a/lib/coffee_script/grammar.y +++ b/lib/coffee_script/grammar.y @@ -294,7 +294,7 @@ rule # Calling super. Super: - SUPER "(" ArgList ")" { result = CallNode.new(:super, val[2]) } + SUPER "(" ArgList ")" { result = CallNode.new(Value.new('super'), val[2]) } ; # The range literal. diff --git a/lib/coffee_script/nodes.rb b/lib/coffee_script/nodes.rb index bc5c86b1..62c586d3 100644 --- a/lib/coffee_script/nodes.rb +++ b/lib/coffee_script/nodes.rb @@ -44,7 +44,8 @@ module CoffeeScript @options = o.dup @indent = o[:indent] top = self.top_sensitive? ? @options[:top] : @options.delete(:top) - closure = statement? && !statement_only? && !top && !@options[:return] + closure = statement? && !statement_only? && !top && !@options[:return] && !self.is_a?(CommentNode) + closure &&= !contains? {|n| n.statement_only? } closure ? compile_closure(@options) : compile_node(@options) end @@ -65,18 +66,14 @@ module CoffeeScript def contains?(&block) children.each do |node| return true if yield(node) - node.is_a?(Node) && node.contains?(&block) - false + return true if node.is_a?(Node) && node.contains?(&block) end - end - - # All Nodes must implement a "children" method that returns child nodes. - def children - raise NotImplementedError, "#{self.class} is missing a 'children' method" + false end # Default implementations of the common node methods. def unwrap; self; end + def children; []; end def statement?; false; end def statement_only?; false; end def top_sensitive?; false; end @@ -230,7 +227,7 @@ module CoffeeScript # Pass through CoffeeScript comments into JavaScript comments at the # same position. class CommentNode < Node - statement_only + statement def initialize(lines) @lines = lines.value @@ -258,7 +255,7 @@ module CoffeeScript end def super? - @variable == :super + @variable == 'super' end def prefix @@ -517,7 +514,7 @@ module CoffeeScript if obj.is_a?(SplatNode) val = LiteralNode.wrap(obj.compile_value(o, val_var, @variable.base.objects.index(obj))) else - val = ValueNode.new(Value.new(val_var), [access_class.new(Value.new(i.to_s))]) + val = ValueNode.new(val_var, [access_class.new(Value.new(i.to_s))]) end assigns << AssignNode.new(obj, val).compile(o) end @@ -582,8 +579,9 @@ module CoffeeScript end # A function definition. The only node that creates a new Scope. + # A CodeNode does not have any children -- they're within the new scope. class CodeNode < Node - children :params, :body + attr_reader :params, :body attr_reader :bound def initialize(params, body, tag=nil) @@ -696,10 +694,11 @@ module CoffeeScript # code generation to generate a quick "array.push(value)" tree of nodes. class PushNode def self.wrap(array, expressions) - return expressions if expressions.unwrap.statement_only? + expr = expressions.unwrap + return expressions if expr.statement_only? || expr.contains? {|n| n.statement_only? } Expressions.wrap(CallNode.new( - ValueNode.new(LiteralNode.new(array), [AccessorNode.new('push')]), - [expressions.unwrap] + ValueNode.new(LiteralNode.new(array), [AccessorNode.new(Value.new('push'))]), + [expr] )) end end diff --git a/lib/coffee_script/scope.rb b/lib/coffee_script/scope.rb index 2b34cea9..19c5ad13 100644 --- a/lib/coffee_script/scope.rb +++ b/lib/coffee_script/scope.rb @@ -44,7 +44,7 @@ module CoffeeScript def free_variable @temp_variable.succ! while check(@temp_variable) @variables[@temp_variable.to_sym] = :var - @temp_variable.dup + Value.new(@temp_variable.dup) end # Ensure that an assignment is made at the top of scope (or top-level diff --git a/lib/coffee_script/value.rb b/lib/coffee_script/value.rb index 0ceda17f..05598145 100644 --- a/lib/coffee_script/value.rb +++ b/lib/coffee_script/value.rb @@ -2,6 +2,8 @@ module CoffeeScript # Instead of producing raw Ruby objects, the Lexer produces values of this # class, wrapping native objects tagged with line number information. + # Values masquerade as both strings and nodes -- being used both as nodes in + # the AST, and as literally-interpolated values in the generated code. class Value attr_reader :value, :line @@ -49,6 +51,10 @@ module CoffeeScript def children [] end + + def statement_only? + false + end end end \ No newline at end of file diff --git a/test/fixtures/execution/test_expressions.coffee b/test/fixtures/execution/test_expressions.coffee new file mode 100644 index 00000000..54729061 --- /dev/null +++ b/test/fixtures/execution/test_expressions.coffee @@ -0,0 +1,30 @@ +# Ensure that we don't wrap Nodes that are "statement_only" in a closure. + +items: [1, 2, 3, "bacon", 4, 5] + +for item in items + break if item is "bacon" + +findit: items => + for item in items + return item if item is "bacon" + +print(findit(items) is "bacon") + + +# When when a closure wrapper is generated for expression conversion, make sure +# that references to "this" within the wrapper are safely converted as well. + +obj: { + num: 5 + func: => + this.result: if false + 10 + else + "a" + "b" + this.num +} + +print(obj.num is obj.func()) +print(obj.num is obj.result) \ No newline at end of file diff --git a/test/fixtures/execution/test_functions.coffee b/test/fixtures/execution/test_functions.coffee index d64b4c25..39b2a7a7 100644 --- a/test/fixtures/execution/test_functions.coffee +++ b/test/fixtures/execution/test_functions.coffee @@ -20,21 +20,3 @@ obj: { obj.unbound() obj.bound() - - -# When when a closure wrapper is generated for expression conversion, make sure -# that references to "this" within the wrapper are safely converted as well. - -obj: { - num: 5 - func: => - this.result: if false - 10 - else - "a" - "b" - this.num -} - -print(obj.num is obj.func()) -print(obj.num is obj.result) \ No newline at end of file