Possible alternative to block capturing from RubyWorld conference

[adam12]

Hi Jeremy,

I was watching the RubyWorld conference and there was a very short talk by @seki about an alternative to block capturing. The use of a regular expression was mentioned, which we know isn't the best solution.

As part of the talk, there was an alternative shown that switched the method (I think? seems to use += instead of <<) when evaluating blocks, and it seems to work. I wonder if it's something worth considering for Erubi.

I haven't dug into it too deeply yet, and the translations weren't much help yet, but here's the Gist with the code example and the start of the relevant presentation piece.

https://gist.github.com/seki/610a42932a85209aaa33547ae983bbdf
https://www.youtube.com/live/ZTkxcAFs2oI?si=9JxN1sBOoURP_FRO&t=22886

What do you think?

[jeremyevans]

Erubi already has support for capturing via erubi/capture_end, though it does require alternative syntax. However, support for @seki's approach could probably be added (not by default, but as an alternative similar to erubi/capture_end).

[adam12]

Circling back to this, I managed to get it working, using the example from @seki as most of the base. It doesn't support most of the engine options currently but it seems feasible.

The custom buffer object is necessary because once we switch to +=, we need to drop the (<code>).to_s statements to ensure the precedence still works. So we need to coerce String when appending. There are probably going to be performance implications here with unnecessary coercion attempts or extra method calls.

Also, I shimmed h as CGI.escape but had to coerce to String here too. I am not sure if that's normal or if ruby/erb does something different here.

diff --git a/lib/erubi/capture_out.rb b/lib/erubi/capture_out.rb
new file mode 100644
index 0000000..155da84
--- /dev/null
+++ b/lib/erubi/capture_out.rb
@@ -0,0 +1,46 @@
+require 'erubi'
+
+module Erubi
+  class CaptureOutEngine < Engine
+    class OutputBuffer
+      Buffer = String
+
+      def initialize(s = "")
+        @str = Buffer.new(s)
+      end
+
+      def to_s
+        @str
+      end
+
+      def <<(other)
+        @str << other.to_s
+        self
+      end
+
+      def +(other)
+        @str << other.to_s
+        self
+      end
+
+      def capture(*arg, &block)
+        save = @str
+        @str = Buffer.new
+        yield(*arg)
+        return @str
+      ensure
+        @str = save
+      end
+    end
+
+    def initialize(input, properties = {})
+      properties[:bufval] = 'Erubi::CaptureOutEngine::OutputBuffer.new'
+      super
+    end
+
+    # Add the result of Ruby expression to the template
+    def add_expression_result(code)
+      @src << " #{@bufvar} += " << code << ';'
+    end
+  end
+end
diff --git a/test/test_two.rb b/test/test_two.rb
new file mode 100644
index 0000000..e1b35fe
--- /dev/null
+++ b/test/test_two.rb
@@ -0,0 +1,44 @@
+require 'minitest/autorun'
+require 'erubi'
+require 'erubi/capture_out'
+
+describe Erubi::CaptureOutEngine do
+  it do
+    def capture(*arg, &block)
+      block.binding.local_variable_get(:_buf).capture(*arg, &block)
+    end
+
+    def form(*arg, &block)
+      "<form>" + capture(*arg, &block).upcase + "</form>"
+    end
+
+    def h(value)
+      CGI.escape(value.to_s)
+    end
+
+    erb = <<~ERB
+    <%= form 'x' do |it| %>
+      <foo>
+      <%=h it %>
+      </foo>
+      <%= Time.utc(1999) %>
+      <%=h 1.0 %>
+      <%= 3.0 + 0.14 %>
+    <% end %>
+    ERB
+
+    src = ::Erubi::CaptureOutEngine.new(erb).src
+    output = eval(src)
+
+    assert_equal <<~OUTPUT.strip, output.strip
+      <form>
+        <FOO>
+        X
+        </FOO>
+        1999-01-01 00:00:00 UTC
+        1.0
+        3.14
+      </form>
+    OUTPUT
+  end
+end

[jeremyevans]

That's pretty cool. I'm definitely open to adding erubi/capture_out. Performance implications are not that important, as it would be optional and not the default. Ideally, it should pass the tests as default erubi, in addition to the new test in your example. I should restructure the tests so erubi/capture_end also passes the same tests as default erubi, then you could add erubi/capture_out test support easily.

[jeremyevans]

I restructured the tests in 7300487, so it should be easy to see if erubi/capture_out passes the same tests expected from default erubi.

[adam12]

Great. I’ll look at implementing.

[adam12]

I'm looking at this again, but having issues reusing many of the specs where they perform a comparison on the generated Ruby code.

Two main issues:

1. I am using += instead of << to perform the concat.
2. I dropped some parenthesis around some code blocks to fix precedence.

Do you have any preferences on how I proceed? I'd hate to unroll the loop here. Perhaps skip or add a conditional on the test if it's a specific engine, and then add similar tests with adjusted markup for the new engine?

Example of the failing test:

 19) Failure:
Erubi::CaptureOut#test_0016_should handle :escape_html and :escapefunc options [test/test.rb:45]:
--- expected
+++ actual
@@ -1,15 +1,17 @@
-"_buf = ::String.new; _buf << '<table>
+# encoding: ASCII-8BIT
+#    valid: true
+"_buf = Erubi::CaptureOutEngine::OutputBuffer.new; _buf << '<table>
  <tbody>
 ';   i = 0
      list.each_with_index do |item, i|
  _buf << '  <tr>
-   <td>'; _buf << h.call(( i+1 )); _buf << '</td>
-   <td>'; _buf << ( item ).to_s; _buf << '</td>
+   <td>'; _buf += h.call(( i+1 )); _buf << '</td>
+   <td>'; _buf += ( item ); _buf << '</td>
   </tr>
 ';  end
  _buf << ' </tbody>
 </table>
-'; _buf << ( i+1 ).to_s; _buf << '
+'; _buf += ( i+1 ); _buf << '
 ';
 _buf.to_s
 "

[jeremyevans]

Since the generated code is not designed to be the same, copying and pasting the existing tests and modifying the generated code as appropriate is fine.

Alternatively, if only the generated code is different, refactoring things so the generated code is not checked in the Erubi::CaptureOutEngine case should be fairly easy. We would probably still want at least a few specs that check the generated code, which could be copied, pasted, and then modified.

[jeremyevans]

I saw @seki's talk at RubyKaigi. I agree that it would be good for Erubi to implement this. Below is a patch.

Some differences from @seki's implementation (which you used above):

1. Make the Buffer class a String subclass for better compatibility with code that calls string methods on it.
2. Use <<= instead of +=, so that we don't have to override Buffer#+, since for strings that should not mutate the receiver.
3. Support escaped tags <%== via |= and have Buffer#| do the escaping and concatenating (this is an extension to ERB added by Erubis and retained by Erubi).
4. Handle empty tags by treating them as containing the empty string for compatibility.

I made this pass most existing tests by ignoring the generated code and only checking the resulting output. Compared to CaptureEndEngine, there are some whitespace differences, but I think those are unavoidable as <%= for blocks cannot be differentiated from non blocks. I added some CaptureBlockEngine (new engine) specific tests that check the generated code.

Please review and let me know what you think.

diff --git a/lib/erubi/capture_block.rb b/lib/erubi/capture_block.rb
new file mode 100644
index 0000000..7242a3d
--- /dev/null
+++ b/lib/erubi/capture_block.rb
@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+require 'erubi'
+
+module Erubi
+  # An engine class that supports capturing blocks via the <tt><%=</tt> and <tt><%==</tt> tags:
+  #
+  #   <%= upcase_form do %>
+  #     <%= 'foo' %>
+  #   <% end %>
+  #
+  # Where +upcase_form+ is defined like:
+  #
+  #   def upcase_form(&block)
+  #     "<form>#{@bufvar.capture(&block).upcase}</form>"
+  #   end
+  #
+  # With output being:
+  #
+  #   <form>
+  #     FOO
+  #   </form>
+  #
+  # This requires using a string subclass as the buffer value, provided by the
+  # CaptureBlockEngine::Buffer class.
+  #
+  # This engine does not support the :escapefunc option.  To change the escaping function,
+  # use a subclass of CaptureBlockEngine::Buffer and override the #| method.
+  #
+  # This engine does not support the :chain_appends option, and ignores it if present.
+  class CaptureBlockEngine < Engine
+    class Buffer < ::String
+      # Convert argument to string when concatening
+      def <<(v)
+        super(v.to_s)
+      end
+
+      # Escape argument using Erubi.h then then concatenate it to the receiver.
+      def |(v)
+        self << ::Erubi.h(v)
+      end
+
+      # Temporarily clear the receiver before yielding to the block, yield the
+      # given args to the block, return any data captured by the receiver, and
+      # restore the original data the receiver contained before returning.
+      def capture(*args)
+        prev = dup
+        clear
+        yield(*args)
+        dup
+      ensure
+        replace(prev)
+      end
+    end
+
+    def initialize(input, properties={})
+      properties = Hash[properties]
+      properties[:bufval] ||= '::Erubi::CaptureBlockEngine::Buffer.new'
+      properties[:chain_appends] = false
+      super
+    end
+
+    private
+
+    def add_expression_result(code)
+      add_expression_op(' <<= ', code)
+    end
+
+    def add_expression_result_escaped(code)
+      add_expression_op(' |= ', code)
+    end
+
+    def add_expression_op(op, code)
+      check = /\A\s*\z/.send(MATCH_METHOD, code) ? "''" : ''
+      with_buffer{@src << op  << check << code}
+    end
+  end
+end
diff --git a/test/test.rb b/test/test.rb
index fbdd069..3bdcba6 100644
--- a/test/test.rb
+++ b/test/test.rb
@@ -25,6 +25,7 @@ end
 
 require 'erubi'
 require 'erubi/capture_end'
+require 'erubi/capture_block'
 
 ENV['MT_NO_PLUGINS'] = '1' # Work around stupid autoloading of plugins
 gem 'minitest'
@@ -39,6 +40,7 @@ describe 'Erubi' do
     t = @engine.new(input, @options)
     tsrc = t.src
     eval(tsrc, block.binding).must_equal result
+    return if @engine <= Erubi::CaptureBlockEngine && !@check_src
     strip_freeze = defined?(@strip_freeze) ? @strip_freeze : RUBY_VERSION >= '2.1'
     tsrc = tsrc.gsub(/\.freeze/, '') if strip_freeze
     tsrc.must_equal src
@@ -57,6 +59,20 @@ describe 'Erubi' do
   end
 
   def setup_bar
+    if @engine <= Erubi::CaptureBlockEngine
+      def self.bar(&block)
+        "a#{@a.capture(&block)}b".upcase
+      end
+      def self.baz(&block)
+        "c#{@a.capture(&block)}d" * 2
+      end
+      def self.quux(&block)
+        v = 3.times.map do |i|
+          "c#{i}#{@a.capture(i, &block)}d#{i}"
+        end.join
+        "a#{v}b".upcase
+      end
+    else
       def self.bar
         @a << "a"
         yield
@@ -80,8 +96,9 @@ describe 'Erubi' do
         @a.upcase
       end
     end
+  end
 
-  {'Engine'=>Erubi::Engine, 'CaptureEndEngine'=>Erubi::CaptureEndEngine}.each do |desc, engine|
+  {'Engine'=>Erubi::Engine, 'CaptureEndEngine'=>Erubi::CaptureEndEngine, 'CaptureBlockEngine'=>Erubi::CaptureBlockEngine}.each do |desc, engine|
     describe desc do
       before do
         @engine = engine
@@ -680,7 +697,7 @@ END2
 1
 END3
         end
-      end
+      end unless engine == Erubi::CaptureBlockEngine
 
       it "should handle :escape option without :escapefunc option" do
         @options[:escape] = true
@@ -759,7 +776,7 @@ END2
 1<table>
  <tbody>
 END3
-      end
+      end unless engine == Erubi::CaptureBlockEngine
 
       it "should have working filename accessor" do
         engine.new('', :filename=>'foo.rb').filename.must_equal 'foo.rb'
@@ -790,9 +807,31 @@ END3
     end
   end
 
-  describe 'CaptureEndEngine' do
+  # Hack to allow CaptureEndEngine tests to pass with minimal changes on CaptureBlock engine
+  capture_block_engine = Class.new(::Erubi::CaptureBlockEngine) do
+    def initialize(input, opts={})
+      input = input.gsub('<%|', '<%')
+      super
+    end
+  end
+
+  {
+    ::Erubi::CaptureEndEngine=>'CaptureEndEngine',
+    capture_block_engine=>'CaptureBlockEngine'
+  }.each do |engine, desc|
+    describe desc do
       before do
-      @engine = ::Erubi::CaptureEndEngine
+        @engine = engine
+      end
+
+      if engine == capture_block_engine
+        def self.it(desc)
+          desc = desc.gsub('<%|', '<%')
+          super
+        end
+      else
+        space = ' '
+        nl = "\n"
       end
 
       it "should handle trailing rspace with - modifier in <%|= and <%|" do
@@ -806,19 +845,22 @@ END3
       it "should have <%|= not escape by default" do
         eval(@engine.new('<%|= "&" %><%| %>').src).must_equal '&'
         eval(@engine.new('<%|= "&" %><%| %>', :escape=>false).src).must_equal '&'
-      eval(@engine.new('<%|= "&" %><%| %>', :escape_capture=>false).src).must_equal '&'
         eval(@engine.new('<%|= "&" %><%| %>', :escape=>true).src).must_equal '&amp;'
-      eval(@engine.new('<%|= "&" %><%| %>', :escape_capture=>true).src).must_equal '&amp;'
       end
 
       it "should have <%|== escape by default" do
         eval(@engine.new('<%|== "&" %><%| %>').src).must_equal '&amp;'
         eval(@engine.new('<%|== "&" %><%| %>', :escape=>true).src).must_equal '&'
-      eval(@engine.new('<%|== "&" %><%| %>', :escape_capture=>true).src).must_equal '&'
         eval(@engine.new('<%|== "&" %><%| %>', :escape=>false).src).must_equal '&amp;'
-      eval(@engine.new('<%|== "&" %><%| %>', :escape_capture=>false).src).must_equal '&amp;'
       end
 
+      it "should have <%|= and <%|== respect :escape_capture option" do
+        eval(@engine.new('<%|= "&" %><%| %>', :escape_capture=>false).src).must_equal '&'
+        eval(@engine.new('<%|= "&" %><%| %>', :escape_capture=>true).src).must_equal '&amp;'
+        eval(@engine.new('<%|== "&" %><%| %>', :escape_capture=>true).src).must_equal '&'
+        eval(@engine.new('<%|== "&" %><%| %>', :escape_capture=>false).src).must_equal '&amp;'
+      end unless engine == capture_block_engine
+
       [['', false], ['=', true]].each do |ind, escape|
         it "should allow <%|=#{ind} and <%| for capturing with :escape_capture => #{escape} and :escape => #{!escape}" do
           @options[:bufvar] = '@a'
@@ -854,7 +896,7 @@ END2
 </table>
 END3
         end
-    end
+      end unless engine == capture_block_engine
 
       [['', true], ['=', false]].each do |ind, escape|
         it "should allow <%|=#{ind} and <%| for capturing when with :escape => #{escape}" do
@@ -884,8 +926,7 @@ END2
  <tbody>
   A
    &lt;B&gt;&amp;AMP;&lt;/B&gt;
- B
- </tbody>
+#{space}B#{nl} </tbody>
 </table>
 END3
         end
@@ -917,12 +958,11 @@ END2
  <tbody>
   AC0
    &lt;B&gt;0&amp;AMP;&lt;/B&gt;
- D0C1
+#{space}D0C1
    &lt;B&gt;1&amp;AMP;&lt;/B&gt;
- D1C2
+#{space}D1C2
    &lt;B&gt;2&amp;AMP;&lt;/B&gt;
- D2B
- </tbody>
+#{space}D2B#{nl} </tbody>
 </table>
 END3
         end
@@ -957,13 +997,14 @@ END2
   A
    &lt;B&gt;&amp;AMP;&lt;/B&gt;
    CEDCED
- B
- </tbody>
+#{space}B#{nl} </tbody>
 </table>
 END3
         end
       end
 
+      next if engine == capture_block_engine
+
       it "should respect the :yield_returns_buffer option for making templates return the (potentially modified) buffer" do
         @options[:bufvar] = '@a'
 
@@ -1068,4 +1109,115 @@ Delicious!
 END3
       end
     end
+  end
+
+  describe 'Erubi::CaptureBlockEngine' do
+    before do
+      @engine = Erubi::CaptureBlockEngine
+      @check_src = true
+    end
+
+    it "should handle empty tags by concatening empty string" do
+      check_output(<<END1, <<END2, <<END3){}
+<%= %><%== %>
+END1
+_buf = ::Erubi::CaptureBlockEngine::Buffer.new; _buf <<= '' ; _buf |= '' ; _buf << '
+';
+_buf.to_s
+END2
+
+END3
+    end
+
+    it "should work as specified in documentation" do
+      @options[:bufvar] = '@a'
+      def self.upcase_form(&block)
+        "<form>#{@a.capture(&block).upcase}</form>"
+      end
+      check_output(<<END1, <<END2, <<END3){}
+<%= upcase_form do %>
+  <%= 'foo' %>
+<% end %>
+
+END1
+@a = ::Erubi::CaptureBlockEngine::Buffer.new; @a <<=  upcase_form do ; @a << '
+'; @a << '  '; @a <<=  'foo' ; @a << '
+'; end 
+ @a << '
+';
+@a.to_s
+END2
+<form>
+  FOO
+</form>
+END3
+  end
+
+    [['', true], ['=', false]].each do |ind, escape|
+      it "should allow <%=#{ind} and <% for escaped capturing with :escape => #{escape}" do
+        @options[:bufvar] = '@a'
+        @options[:escape] = escape
+        setup_bar
+        check_output(<<END1, <<END2, <<END3){}
+<table>
+ <tbody>
+  <%=#{ind} bar do %>
+   <b><%=#{ind} '&' %></b>
+ <% end %>
+ </tbody>
+</table>
+END1
+#{'__erubi = ::Erubi; ' if escape}@a = ::Erubi::CaptureBlockEngine::Buffer.new; @a << '<table>
+ <tbody>
+  '; @a |=  bar do ; @a << '
+'; @a << '   <b>'; @a |=  '&' ; @a << '</b>
+';  end 
+ @a << ' </tbody>
+</table>
+';
+@a.to_s
+END2
+<table>
+ <tbody>
+  A
+   &lt;B&gt;&amp;AMP;&lt;/B&gt;
+B </tbody>
+</table>
+END3
+     end
+
+      it "should allow <%=#{ind} and <% for unescaped capturing when with :escape => #{!escape}" do
+        @options[:bufvar] = '@a'
+        @options[:escape] = !escape
+        setup_bar
+        check_output(<<END1, <<END2, <<END3){}
+<table>
+ <tbody>
+  <%=#{ind} bar do %>
+   <b><%=#{ind} '&' %></b>
+ <% end %>
+ </tbody>
+</table>
+END1
+#{'__erubi = ::Erubi; ' if !escape}@a = ::Erubi::CaptureBlockEngine::Buffer.new; @a << '<table>
+ <tbody>
+  '; @a <<=  bar do ; @a << '
+'; @a << '   <b>'; @a <<=  '&' ; @a << '</b>
+';  end 
+ @a << ' </tbody>
+</table>
+';
+@a.to_s
+END2
+<table>
+ <tbody>
+  A
+   <B>&</B>
+B </tbody>
+</table>
+END3
+     end
+    end
+  end
+
 end

[adam12]

It looks good. I like the use of <<=, very creative.

[jeremyevans]

Made a few tweaks and pushed this as 0ba6e1f