Engine: Delegate context-aware expressions to overridable engine methods (#1421)

This pull request moves expression handling logic from the compiler into
overridable engine methods, making `Herb::Engine` an easier drop-in
replacement for `Erubi::Engine`.

Previously, the compiler bypassed the engine's `add_expression` method
in two cases:

1. **Block expressions** (`<%= link_to do %>...<% end %>`), the compiler
called `add_expression_block` which dispatched directly to
`add_expression_block_result`, skipping any subclass override of
`add_expression`.

2. **Context-aware expressions** (inside `<script>`, `<style>`, or
attribute values), the compiler's `add_context_aware_expression` wrote
directly to `@engine.src`, bypassing the engine entirely (see #1419)

This meant subclasses that overrode `add_expression` (the Erubi pattern
used by ActionView) would not have their override apply to blocks or
context-aware expressions, leading to incorrect compiled output.

So the idea of this pull request is to move the logic from the compiler
into overridable engine methods so that subclasses (like ReActionView)
get correct behavior by only overriding `add_expression`. The compiler
becomes a thin dispatcher that routes tokens to the engine and no longer
manipulates `@engine.src` directly.

Resolves #1419 

Related marcoroth/reactionview#78
Related marcoroth/reactionview#80
Related marcoroth/reactionview#83

Author: marcoroth

lib/herb/engine.rb

@@ -55,6 +55,9 @@ def initialize(input, properties = {})
       @bufvar = properties[:bufvar] || properties[:outvar] || "_buf"
       @escape = properties.fetch(:escape) { properties.fetch(:escape_html, false) }
       @escapefunc = properties.fetch(:escapefunc, @escape ? "__herb.h" : "::Herb::Engine.h")
+      @attrfunc = properties.fetch(:attrfunc, @escape ? "__herb.attr" : "::Herb::Engine.attr")
+      @jsfunc = properties.fetch(:jsfunc, @escape ? "__herb.js" : "::Herb::Engine.js")
+      @cssfunc = properties.fetch(:cssfunc, @escape ? "__herb.css" : "::Herb::Engine.css")
       @src = properties[:src] || String.new
       @chain_appends = properties[:chain_appends]
       @buffer_on_stack = false
@@ -244,6 +247,24 @@ def add_expression(indicator, code)
       end
     end
 
+    def add_context_aware_expression(indicator, code, context)
+      escapefunc = context_escape_function(context)
+
+      if escapefunc.nil?
+        add_expression(indicator, code)
+      else
+        with_buffer { @src << " << #{escapefunc}((" << code << trailing_newline(code) << "))" }
+      end
+    end
+
+    def context_escape_function(context)
+      case context
+      when :attribute_value then @attrfunc
+      when :script_content then @jsfunc
+      when :style_content then @cssfunc
+      end
+    end
+
     def add_expression_result(code)
       with_buffer {
         @src << " << (" << code << trailing_newline(code) << ").to_s"

lib/herb/engine/compiler.rb

@@ -10,9 +10,6 @@ def initialize(engine, options = {})
 
         @engine = engine
         @escape = options.fetch(:escape) { options.fetch(:escape_html, false) }
-        @attrfunc = options.fetch(:attrfunc, @escape ? "__herb.attr" : "::Herb::Engine.attr")
-        @jsfunc = options.fetch(:jsfunc, @escape ? "__herb.js" : "::Herb::Engine.js")
-        @cssfunc = options.fetch(:cssfunc, @escape ? "__herb.css" : "::Herb::Engine.css")
         @tokens = [] #: Array[untyped]
         @element_stack = [] #: Array[String]
         @context_stack = [:html_content]
@@ -28,26 +25,16 @@ def generate_output
             @engine.send(:add_text, value)
           when :code
             @engine.send(:add_code, value)
-          when :expr
-            if [:attribute_value, :script_content, :style_content].include?(context)
-              add_context_aware_expression(value, context)
-            else
-              indicator = @escape ? "==" : "="
-              @engine.send(:add_expression, indicator, value)
-            end
-          when :expr_escaped
-            if [:attribute_value, :script_content, :style_content].include?(context)
-              add_context_aware_expression(value, context)
+          when :expr, :expr_escaped
+            indicator = indicator_for(type)
+
+            if context_aware_context?(context)
+              @engine.send(:add_context_aware_expression, indicator, value, context)
             else
-              indicator = @escape ? "=" : "=="
               @engine.send(:add_expression, indicator, value)
             end
-          when :expr_block
-            indicator = @escape ? "==" : "="
-            @engine.send(:add_expression_block, indicator, value)
-          when :expr_block_escaped
-            indicator = @escape ? "=" : "=="
-            @engine.send(:add_expression_block, indicator, value)
+          when :expr_block, :expr_block_escaped
+            @engine.send(:add_expression_block, indicator_for(type), value)
           when :expr_block_end
             @engine.send(:add_expression_block_end, value, escaped: escaped)
           end
@@ -342,27 +329,6 @@ def pop_context
         @context_stack.pop
       end
 
-      def add_context_aware_expression(code, context)
-        closing = code.include?("#") ? "\n))" : "))"
-
-        case context
-        when :attribute_value
-          @engine.send(:with_buffer) {
-            @engine.src << " << #{@attrfunc}((" << code << closing
-          }
-        when :script_content
-          @engine.send(:with_buffer) {
-            @engine.src << " << #{@jsfunc}((" << code << closing
-          }
-        when :style_content
-          @engine.send(:with_buffer) {
-            @engine.src << " << #{@cssfunc}((" << code << closing
-          }
-        else
-          @engine.send(:add_expression_result_escaped, code)
-        end
-      end
-
       def process_erb_tag(node, skip_comment_check: false)
         opening = node.tag_opening.value
 
@@ -503,6 +469,16 @@ def process_erb_output(node, opening, code)
         @trim_next_whitespace = true if has_right_trim
       end
 
+      def indicator_for(type)
+        escaped = [:expr_escaped, :expr_block_escaped].include?(type)
+
+        escaped ^ @escape ? "==" : "="
+      end
+
+      def context_aware_context?(context)
+        [:attribute_value, :script_content, :style_content].include?(context)
+      end
+
       def should_escape_output?(opening)
         is_double_equals = opening == "<%=="
         is_double_equals ? !@escape : @escape