From fe66edb8254a4af4af3a69a32884178e2263c421 Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Fri, 23 Oct 2020 10:53:16 -0400 Subject: [PATCH 1/4] Freeze block body after parsing completes --- Gemfile | 2 +- lib/liquid/block.rb | 1 + lib/liquid/block_body.rb | 9 +++++++++ lib/liquid/document.rb | 1 + lib/liquid/tags/case.rb | 6 +++++- lib/liquid/tags/for.rb | 2 ++ lib/liquid/tags/if.rb | 1 + 7 files changed, 20 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index 14537c6..338fe56 100644 --- a/Gemfile +++ b/Gemfile @@ -22,6 +22,6 @@ group :test do gem 'rubocop-performance', require: false platform :mri, :truffleruby do - gem 'liquid-c', github: 'Shopify/liquid-c', ref: 'master' + gem 'liquid-c', github: 'Shopify/liquid-c', ref: 'pz-block-body-buffer' end end diff --git a/lib/liquid/block.rb b/lib/liquid/block.rb index 47d5f25..d6619f9 100644 --- a/lib/liquid/block.rb +++ b/lib/liquid/block.rb @@ -13,6 +13,7 @@ module Liquid @body = new_body while parse_body(@body, tokens) end + @body.freeze(parse_context) end # For backwards compatibility diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 67d5d63..f61ae87 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -16,9 +16,12 @@ module Liquid def initialize @nodelist = [] @blank = true + @frozen = false end def parse(tokenizer, parse_context, &block) + raise FrozenError, "can't modify frozen Liquid::BlockBody" if @frozen + parse_context.line_number = tokenizer.line_number if tokenizer.for_liquid_tag @@ -28,6 +31,10 @@ module Liquid end end + def freeze(_context) + @frozen = true + end + private def parse_for_liquid_tag(tokenizer, parse_context) while (token = tokenizer.shift) unless token.empty? || token =~ WhitespaceOrNothing @@ -192,6 +199,8 @@ module Liquid end def render_to_output_buffer(context, output) + raise "Can only render when frozen" unless @frozen + context.resource_limits.increment_render_score(@nodelist.length) idx = 0 diff --git a/lib/liquid/document.rb b/lib/liquid/document.rb index 2e47ffb..21ddec2 100644 --- a/lib/liquid/document.rb +++ b/lib/liquid/document.rb @@ -22,6 +22,7 @@ module Liquid def parse(tokenizer, parse_context) while parse_body(tokenizer) end + @body.freeze(parse_context) rescue SyntaxError => e e.line_number ||= parse_context.line_number raise diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index b46fa05..cc85ed4 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -20,7 +20,11 @@ module Liquid def parse(tokens) body = new_body - body = @blocks.last.attachment while parse_body(body, tokens) + while parse_body(body, tokens) + body.freeze(parse_context) + body = @blocks.last.attachment + end + body.freeze(parse_context) if blank? @blocks.each { |condition| condition.attachment.remove_blank_strings } end diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 6f66ebd..9a81b4d 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -61,7 +61,9 @@ module Liquid def parse(tokens) if parse_body(@for_block, tokens) parse_body(@else_block, tokens) + @else_block.freeze(parse_context) end + @for_block.freeze(parse_context) if blank? @for_block.remove_blank_strings @else_block&.remove_blank_strings diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index c910b0b..7632be9 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -31,6 +31,7 @@ module Liquid def parse(tokens) while parse_body(@blocks.last.attachment, tokens) end + @blocks.each { |block| block.attachment.freeze(parse_context) } if blank? @blocks.each { |condition| condition.attachment.remove_blank_strings } end From 0bedc71854196ebb0164a6eaccc93f8c8e889aa3 Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Mon, 26 Oct 2020 15:11:00 -0400 Subject: [PATCH 2/4] Address comments --- Gemfile | 2 +- lib/liquid/block.rb | 20 +++++++++++++++----- lib/liquid/block_body.rb | 10 +++++----- lib/liquid/document.rb | 2 +- lib/liquid/tags/case.rb | 9 +-------- lib/liquid/tags/for.rb | 6 ------ lib/liquid/tags/if.rb | 4 ---- 7 files changed, 23 insertions(+), 30 deletions(-) diff --git a/Gemfile b/Gemfile index 338fe56..14537c6 100644 --- a/Gemfile +++ b/Gemfile @@ -22,6 +22,6 @@ group :test do gem 'rubocop-performance', require: false platform :mri, :truffleruby do - gem 'liquid-c', github: 'Shopify/liquid-c', ref: 'pz-block-body-buffer' + gem 'liquid-c', github: 'Shopify/liquid-c', ref: 'master' end end diff --git a/lib/liquid/block.rb b/lib/liquid/block.rb index d6619f9..4299915 100644 --- a/lib/liquid/block.rb +++ b/lib/liquid/block.rb @@ -11,9 +11,9 @@ module Liquid def parse(tokens) @body = new_body - while parse_body(@body, tokens) + while parse_body(@body, tokens, partial: true) end - @body.freeze(parse_context) + @body.freeze end # For backwards compatibility @@ -68,7 +68,9 @@ module Liquid end # @api public - def parse_body(body, tokens) + def parse_body(body, tokens, partial: false) + block_terminated = true + if parse_context.depth >= MAX_DEPTH raise StackLevelError, "Nesting too deep" end @@ -77,7 +79,10 @@ module Liquid body.parse(tokens, parse_context) do |end_tag_name, end_tag_params| @blank &&= body.blank? - return false if end_tag_name == block_delimiter + if end_tag_name == block_delimiter + block_terminated = false + next + end raise_tag_never_closed(block_name) unless end_tag_name # this tag is not registered with the system @@ -88,7 +93,12 @@ module Liquid parse_context.depth -= 1 end - true + unless partial + body.remove_blank_strings if body.blank? + body.freeze + end + + block_terminated end end end diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index f61ae87..b8350a3 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -16,11 +16,10 @@ module Liquid def initialize @nodelist = [] @blank = true - @frozen = false end def parse(tokenizer, parse_context, &block) - raise FrozenError, "can't modify frozen Liquid::BlockBody" if @frozen + raise FrozenError, "can't modify frozen Liquid::BlockBody" if frozen? parse_context.line_number = tokenizer.line_number @@ -31,8 +30,9 @@ module Liquid end end - def freeze(_context) - @frozen = true + def freeze + @nodelist.freeze + super end private def parse_for_liquid_tag(tokenizer, parse_context) @@ -199,7 +199,7 @@ module Liquid end def render_to_output_buffer(context, output) - raise "Can only render when frozen" unless @frozen + raise "Can only render when frozen" unless frozen? context.resource_limits.increment_render_score(@nodelist.length) diff --git a/lib/liquid/document.rb b/lib/liquid/document.rb index 21ddec2..9bf13b4 100644 --- a/lib/liquid/document.rb +++ b/lib/liquid/document.rb @@ -22,7 +22,7 @@ module Liquid def parse(tokenizer, parse_context) while parse_body(tokenizer) end - @body.freeze(parse_context) + @body.freeze rescue SyntaxError => e e.line_number ||= parse_context.line_number raise diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index cc85ed4..734f166 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -20,14 +20,7 @@ module Liquid def parse(tokens) body = new_body - while parse_body(body, tokens) - body.freeze(parse_context) - body = @blocks.last.attachment - end - body.freeze(parse_context) - if blank? - @blocks.each { |condition| condition.attachment.remove_blank_strings } - end + body = @blocks.last.attachment while parse_body(body, tokens) end def nodelist diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 9a81b4d..d7e93bb 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -61,12 +61,6 @@ module Liquid def parse(tokens) if parse_body(@for_block, tokens) parse_body(@else_block, tokens) - @else_block.freeze(parse_context) - end - @for_block.freeze(parse_context) - if blank? - @for_block.remove_blank_strings - @else_block&.remove_blank_strings end end diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 7632be9..c3b8915 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -31,10 +31,6 @@ module Liquid def parse(tokens) while parse_body(@blocks.last.attachment, tokens) end - @blocks.each { |block| block.attachment.freeze(parse_context) } - if blank? - @blocks.each { |condition| condition.attachment.remove_blank_strings } - end end def unknown_tag(tag, markup, tokens) From 5c082472a10a7795fa188dfd01b0a2763fea006d Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Mon, 26 Oct 2020 16:07:43 -0400 Subject: [PATCH 3/4] Address comments --- lib/liquid/block.rb | 18 ++++-------------- lib/liquid/block_body.rb | 2 +- lib/liquid/tags/case.rb | 4 ++++ lib/liquid/tags/for.rb | 6 ++++++ lib/liquid/tags/if.rb | 4 ++++ 5 files changed, 19 insertions(+), 15 deletions(-) diff --git a/lib/liquid/block.rb b/lib/liquid/block.rb index 4299915..dc22a47 100644 --- a/lib/liquid/block.rb +++ b/lib/liquid/block.rb @@ -11,7 +11,7 @@ module Liquid def parse(tokens) @body = new_body - while parse_body(@body, tokens, partial: true) + while parse_body(@body, tokens) end @body.freeze end @@ -68,9 +68,7 @@ module Liquid end # @api public - def parse_body(body, tokens, partial: false) - block_terminated = true - + def parse_body(body, tokens) if parse_context.depth >= MAX_DEPTH raise StackLevelError, "Nesting too deep" end @@ -79,10 +77,7 @@ module Liquid body.parse(tokens, parse_context) do |end_tag_name, end_tag_params| @blank &&= body.blank? - if end_tag_name == block_delimiter - block_terminated = false - next - end + return false if end_tag_name == block_delimiter raise_tag_never_closed(block_name) unless end_tag_name # this tag is not registered with the system @@ -93,12 +88,7 @@ module Liquid parse_context.depth -= 1 end - unless partial - body.remove_blank_strings if body.blank? - body.freeze - end - - block_terminated + true end end end diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index b8350a3..af62a14 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -199,7 +199,7 @@ module Liquid end def render_to_output_buffer(context, output) - raise "Can only render when frozen" unless frozen? + freeze unless frozen? context.resource_limits.increment_render_score(@nodelist.length) diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index 734f166..caa6fff 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -21,6 +21,10 @@ module Liquid def parse(tokens) body = new_body body = @blocks.last.attachment while parse_body(body, tokens) + if blank? + @blocks.each { |condition| condition.attachment.remove_blank_strings } + end + @blocks.each { |condition| condition.attachment.freeze } end def nodelist diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index d7e93bb..df2d4db 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -62,6 +62,12 @@ module Liquid if parse_body(@for_block, tokens) parse_body(@else_block, tokens) end + if blank? + @for_block.remove_blank_strings + @else_block&.remove_blank_strings + end + @for_block.freeze + @else_block&.freeze end def nodelist diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index c3b8915..7bef1af 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -31,6 +31,10 @@ module Liquid def parse(tokens) while parse_body(@blocks.last.attachment, tokens) end + if blank? + @blocks.each { |condition| condition.attachment.remove_blank_strings } + end + @blocks.each { |block| block.attachment.freeze } end def unknown_tag(tag, markup, tokens) From 292d971937e418283d68788502ad09acfd799f6d Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Tue, 27 Oct 2020 10:42:30 -0400 Subject: [PATCH 4/4] Merge loops --- lib/liquid/tags/case.rb | 6 +++--- lib/liquid/tags/if.rb | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index caa6fff..12d1712 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -21,10 +21,10 @@ module Liquid def parse(tokens) body = new_body body = @blocks.last.attachment while parse_body(body, tokens) - if blank? - @blocks.each { |condition| condition.attachment.remove_blank_strings } + @blocks.each do |condition| + condition.attachment.remove_blank_strings if blank? + condition.attachment.freeze end - @blocks.each { |condition| condition.attachment.freeze } end def nodelist diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 7bef1af..d4de9c2 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -31,10 +31,10 @@ module Liquid def parse(tokens) while parse_body(@blocks.last.attachment, tokens) end - if blank? - @blocks.each { |condition| condition.attachment.remove_blank_strings } + @blocks.each do |block| + block.attachment.remove_blank_strings if blank? + block.attachment.freeze end - @blocks.each { |block| block.attachment.freeze } end def unknown_tag(tag, markup, tokens)