From 8d1cd4145335d9ee0c7cc0b99da0143c8334bba2 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Fri, 15 Mar 2019 14:49:31 -0400 Subject: [PATCH 1/5] Add {% liquid %}, {% echo %}, and {% local %} tags --- Gemfile | 2 +- lib/liquid/block_body.rb | 60 +++++++++++++-- lib/liquid/locales/en.yml | 1 + lib/liquid/tag.rb | 4 +- lib/liquid/tags/assign.rb | 6 +- lib/liquid/tags/echo.rb | 24 ++++++ lib/liquid/tags/local.rb | 30 ++++++++ lib/liquid/tokenizer.rb | 18 +++-- test/integration/tags/echo_test.rb | 11 +++ test/integration/tags/liquid_tag_test.rb | 98 ++++++++++++++++++++++++ test/integration/tags/local_test.rb | 15 ++++ test/test_helper.rb | 6 +- 12 files changed, 257 insertions(+), 18 deletions(-) create mode 100644 lib/liquid/tags/echo.rb create mode 100644 lib/liquid/tags/local.rb create mode 100644 test/integration/tags/echo_test.rb create mode 100644 test/integration/tags/liquid_tag_test.rb create mode 100644 test/integration/tags/local_test.rb diff --git a/Gemfile b/Gemfile index bdeefac..60f1a7a 100644 --- a/Gemfile +++ b/Gemfile @@ -15,6 +15,6 @@ group :test do gem 'rubocop', '~> 0.49.0' platform :mri do - gem 'liquid-c', github: 'Shopify/liquid-c', ref: '9168659de45d6d576fce30c735f857e597fa26f6' + gem 'liquid-c', github: 'Shopify/liquid-c', ref: 'liquid-tag' end end diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 266d8ed..5f86545 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -1,6 +1,7 @@ module Liquid class BlockBody - FullToken = /\A#{TagStart}#{WhitespaceControl}?\s*(\w+)\s*(.*?)#{WhitespaceControl}?#{TagEnd}\z/om + LiquidTagToken = /\A\s*(\w+)\s*(.*?)\z/o + FullToken = /\A#{TagStart}#{WhitespaceControl}?(\s*)(\w+)(\s*)(.*?)#{WhitespaceControl}?#{TagEnd}\z/om ContentOfVariable = /\A#{VariableStart}#{WhitespaceControl}?(.*?)#{WhitespaceControl}?#{VariableEnd}\z/om WhitespaceOrNothing = /\A\s*\z/ TAGSTART = "{%".freeze @@ -13,8 +14,46 @@ module Liquid @blank = true end - def parse(tokenizer, parse_context) + def parse(tokenizer, parse_context, &block) parse_context.line_number = tokenizer.line_number + + if tokenizer.for_liquid_tag + parse_for_liquid_tag(tokenizer, parse_context, &block) + else + parse_for_document(tokenizer, parse_context, &block) + end + end + + private def parse_for_liquid_tag(tokenizer, parse_context) + while token = tokenizer.shift + case + when token.empty? + # pass + else + unless token =~ LiquidTagToken + next if token =~ WhitespaceOrNothing + # line isn't empty but didn't match tag syntax, yield and let the + # caller raise a syntax error + return yield token, token + end + tag_name = $1 + markup = $2 + unless tag = registered_tags[tag_name] + # end parsing if we reach an unknown tag and let the caller decide + # determine how to proceed + return yield tag_name, markup + end + new_tag = tag.parse(tag_name, markup, tokenizer, parse_context) + @blank &&= new_tag.blank? + @nodelist << new_tag + end + parse_context.line_number = tokenizer.line_number + end + + yield nil, nil + end + + private def parse_for_document(tokenizer, parse_context, &block) while token = tokenizer.shift next if token.empty? case @@ -23,9 +62,20 @@ module Liquid unless token =~ FullToken raise_missing_tag_terminator(token, parse_context) end - tag_name = $1 - markup = $2 - # fetch the tag from registered blocks + tag_name = $2 + markup = $4 + + if parse_context.line_number + # newlines inside the tag should increase the line number, + # particularly important for multiline {% liquid %} tags + parse_context.line_number += $1.count("\n".freeze) + $3.count("\n".freeze) + end + + if tag_name == 'liquid'.freeze + liquid_tag_tokenizer = Tokenizer.new(markup, line_number: parse_context.line_number, for_liquid_tag: true) + next parse(liquid_tag_tokenizer, parse_context, &block) + end + unless tag = registered_tags[tag_name] # end parsing if we reach an unknown tag and let the caller decide # determine how to proceed diff --git a/lib/liquid/locales/en.yml b/lib/liquid/locales/en.yml index 48b3b1d..56c068f 100644 --- a/lib/liquid/locales/en.yml +++ b/lib/liquid/locales/en.yml @@ -3,6 +3,7 @@ syntax: tag_unexpected_args: "Syntax Error in '%{tag}' - Valid syntax: %{tag}" assign: "Syntax Error in 'assign' - Valid syntax: assign [var] = [source]" + local: "Syntax Error in 'local' - Valid syntax: local [var] = [source]" capture: "Syntax Error in 'capture' - Valid syntax: capture [var]" case: "Syntax Error in 'case' - Valid syntax: case [condition]" case_invalid_when: "Syntax Error in tag 'case' - Valid when condition: {% when [condition] [or condition2...] %}" diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index 06970c1..ac99ecd 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -5,8 +5,8 @@ module Liquid include ParserSwitching class << self - def parse(tag_name, markup, tokenizer, options) - tag = new(tag_name, markup, options) + def parse(tag_name, markup, tokenizer, parse_context) + tag = new(tag_name, markup, parse_context) tag.parse(tokenizer) tag end diff --git a/lib/liquid/tags/assign.rb b/lib/liquid/tags/assign.rb index c8d0574..55753d8 100644 --- a/lib/liquid/tags/assign.rb +++ b/lib/liquid/tags/assign.rb @@ -10,6 +10,10 @@ module Liquid class Assign < Tag Syntax = /(#{VariableSignature}+)\s*=\s*(.*)\s*/om + def self.syntax_error_translation_key + "errors.syntax.assign".freeze + end + attr_reader :to, :from def initialize(tag_name, markup, options) @@ -18,7 +22,7 @@ module Liquid @to = $1 @from = Variable.new($2, options) else - raise SyntaxError.new options[:locale].t("errors.syntax.assign".freeze) + raise SyntaxError.new(options[:locale].t(self.class.syntax_error_translation_key)) end end diff --git a/lib/liquid/tags/echo.rb b/lib/liquid/tags/echo.rb new file mode 100644 index 0000000..acb9ab4 --- /dev/null +++ b/lib/liquid/tags/echo.rb @@ -0,0 +1,24 @@ +module Liquid + # Echo outputs an expression + # + # {% echo monkey %} + # {% echo user.name %} + # + # This is identical to variable output syntax, like {{ foo }}, but works + # inside {% liquid %} tags. The full syntax is supported, including filters: + # + # {% echo user | link %} + # + class Echo < Tag + def initialize(tag_name, markup, parse_context) + super + @variable = Variable.new(markup, parse_context) + end + + def render(context) + @variable.render(context) + end + end + + Template.register_tag('echo'.freeze, Echo) +end diff --git a/lib/liquid/tags/local.rb b/lib/liquid/tags/local.rb new file mode 100644 index 0000000..572920e --- /dev/null +++ b/lib/liquid/tags/local.rb @@ -0,0 +1,30 @@ +require_relative 'assign' + +module Liquid + # Local sets a variable in the current scope. + # + # {% local foo = 'monkey' %} + # + # You can then use the variable later in the scope. + # + # {% if true %} + # {% local foo = 'monkey' %} + # {{ foo }} outputs monkey + # {% endif %} + # {{ foo }} outputs nothing + # + class Local < Assign + def self.syntax_error_translation_key + "errors.syntax.local".freeze + end + + def render(context) + val = @from.render(context) + context[@to] = val + context.resource_limits.assign_score += assign_score_of(val) + ''.freeze + end + end + + Template.register_tag('local'.freeze, Local) +end diff --git a/lib/liquid/tokenizer.rb b/lib/liquid/tokenizer.rb index d03657e..d3fd676 100644 --- a/lib/liquid/tokenizer.rb +++ b/lib/liquid/tokenizer.rb @@ -1,25 +1,31 @@ module Liquid class Tokenizer - attr_reader :line_number + attr_reader :line_number, :for_liquid_tag - def initialize(source, line_numbers = false) + def initialize(source, line_numbers = false, line_number: nil, for_liquid_tag: false) @source = source - @line_number = line_numbers ? 1 : nil + @line_number = line_number || (line_numbers ? 1 : nil) + @for_liquid_tag = for_liquid_tag @tokens = tokenize end def shift - token = @tokens.shift - @line_number += token.count("\n") if @line_number && token + token = @tokens.shift or return + + if @line_number + @line_number += @for_liquid_tag ? 1 : token.count("\n") + end + token end private def tokenize - @source = @source.source if @source.respond_to?(:source) return [] if @source.to_s.empty? + return @source.split("\n") if @for_liquid_tag + tokens = @source.split(TemplateParser) # removes the rogue empty element at the beginning of the array diff --git a/test/integration/tags/echo_test.rb b/test/integration/tags/echo_test.rb new file mode 100644 index 0000000..ed5b821 --- /dev/null +++ b/test/integration/tags/echo_test.rb @@ -0,0 +1,11 @@ +require 'test_helper' + +class EchoTest < Minitest::Test + include Liquid + + def test_echo_outputs_its_input + assert_template_result('BAR', <<~LIQUID, { 'variable-name' => 'bar' }) + {%- echo variable-name | upcase -%} + LIQUID + end +end diff --git a/test/integration/tags/liquid_tag_test.rb b/test/integration/tags/liquid_tag_test.rb new file mode 100644 index 0000000..2cfd13a --- /dev/null +++ b/test/integration/tags/liquid_tag_test.rb @@ -0,0 +1,98 @@ +require 'test_helper' + +class LiquidTagTest < Minitest::Test + include Liquid + + def test_liquid_tag + assert_template_result('1 2 3', <<~LIQUID, 'array' => [1, 2, 3]) + {%- liquid + echo array | join: " " + -%} + LIQUID + + assert_template_result('1 2 3', <<~LIQUID, 'array' => [1, 2, 3]) + {%- liquid + for value in array + echo value + unless forloop.last + echo " " + endunless + endfor + -%} + LIQUID + + assert_template_result('4 8 12', <<~LIQUID, 'array' => [1, 2, 3]) + {%- liquid + for value in array + local double_value = value | times: 2 + echo double_value | times: 2 + unless forloop.last + echo " " + endunless + endfor + + echo double_value + -%} + LIQUID + + assert_template_result('abc', <<~LIQUID) + {%- liquid echo "a" -%} + b + {%- liquid echo "c" -%} + LIQUID + end + + def test_liquid_tag_errors + assert_match_syntax_error("syntax error (line 1): Unknown tag 'error'", <<~LIQUID) + {%- liquid error no such tag -%} + LIQUID + + assert_match_syntax_error("syntax error (line 7): Unknown tag 'error'", <<~LIQUID) + {{ test }} + + {%- + liquid + for value in array + + error no such tag + endfor + -%} + LIQUID + + assert_match_syntax_error("syntax error (line 2): Unknown tag '!!! the guards are vigilant'", <<~LIQUID) + {%- liquid + !!! the guards are vigilant + -%} + LIQUID + + assert_match_syntax_error("syntax error (line 4): 'for' tag was never closed", <<~LIQUID) + {%- liquid + for value in array + echo 'forgot to close the for tag' + -%} + LIQUID + end + + def test_cannot_open_blocks_living_past_a_liquid_tag + assert_match_syntax_error("syntax error (line 3): 'if' tag was never closed", <<~LIQUID) + {%- liquid + if true + -%} + {%- endif -%} + LIQUID + end + + def test_quirk_can_close_blocks_created_before_a_liquid_tag + assert_template_result("42", <<~LIQUID) + {%- if true -%} + 42 + {%- liquid endif -%} + LIQUID + end + + def test_liquid_tag_in_raw + assert_template_result("{% liquid echo 'test' %}\n", <<~LIQUID) + {% raw %}{% liquid echo 'test' %}{% endraw %} + LIQUID + end +end diff --git a/test/integration/tags/local_test.rb b/test/integration/tags/local_test.rb new file mode 100644 index 0000000..412ee26 --- /dev/null +++ b/test/integration/tags/local_test.rb @@ -0,0 +1,15 @@ +require 'test_helper' + +class LocalTest < Minitest::Test + include Liquid + + def test_local_is_scope_aware + assert_template_result('value', <<~LIQUID) + {%- if true -%} + {%- local variable-name = 'value' -%} + {{- variable-name -}} + {%- endif -%} + {{- variable-name -}} + LIQUID + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index ac5ab53..c55328d 100755 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -37,18 +37,18 @@ module Minitest include Liquid def assert_template_result(expected, template, assigns = {}, message = nil) - assert_equal expected, Template.parse(template).render!(assigns), message + assert_equal expected, Template.parse(template, line_numbers: true).render!(assigns), message end def assert_template_result_matches(expected, template, assigns = {}, message = nil) return assert_template_result(expected, template, assigns, message) unless expected.is_a? Regexp - assert_match expected, Template.parse(template).render!(assigns), message + assert_match expected, Template.parse(template, line_numbers: true).render!(assigns), message end def assert_match_syntax_error(match, template, assigns = {}) exception = assert_raises(Liquid::SyntaxError) do - Template.parse(template).render(assigns) + Template.parse(template, line_numbers: true).render(assigns) end assert_match match, exception.message end From 951abb67ee29af54378960815eb7118c90414653 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Mon, 8 Apr 2019 18:34:39 -0400 Subject: [PATCH 2/5] Remove {% local %} tag --- lib/liquid/tags/local.rb | 30 ------------------------ test/integration/tags/liquid_tag_test.rb | 5 ++-- test/integration/tags/local_test.rb | 15 ------------ 3 files changed, 3 insertions(+), 47 deletions(-) delete mode 100644 lib/liquid/tags/local.rb delete mode 100644 test/integration/tags/local_test.rb diff --git a/lib/liquid/tags/local.rb b/lib/liquid/tags/local.rb deleted file mode 100644 index 572920e..0000000 --- a/lib/liquid/tags/local.rb +++ /dev/null @@ -1,30 +0,0 @@ -require_relative 'assign' - -module Liquid - # Local sets a variable in the current scope. - # - # {% local foo = 'monkey' %} - # - # You can then use the variable later in the scope. - # - # {% if true %} - # {% local foo = 'monkey' %} - # {{ foo }} outputs monkey - # {% endif %} - # {{ foo }} outputs nothing - # - class Local < Assign - def self.syntax_error_translation_key - "errors.syntax.local".freeze - end - - def render(context) - val = @from.render(context) - context[@to] = val - context.resource_limits.assign_score += assign_score_of(val) - ''.freeze - end - end - - Template.register_tag('local'.freeze, Local) -end diff --git a/test/integration/tags/liquid_tag_test.rb b/test/integration/tags/liquid_tag_test.rb index 2cfd13a..56ca712 100644 --- a/test/integration/tags/liquid_tag_test.rb +++ b/test/integration/tags/liquid_tag_test.rb @@ -21,16 +21,17 @@ class LiquidTagTest < Minitest::Test -%} LIQUID - assert_template_result('4 8 12', <<~LIQUID, 'array' => [1, 2, 3]) + assert_template_result('4 8 12 6', <<~LIQUID, 'array' => [1, 2, 3]) {%- liquid for value in array - local double_value = value | times: 2 + assign double_value = value | times: 2 echo double_value | times: 2 unless forloop.last echo " " endunless endfor + echo " " echo double_value -%} LIQUID diff --git a/test/integration/tags/local_test.rb b/test/integration/tags/local_test.rb deleted file mode 100644 index 412ee26..0000000 --- a/test/integration/tags/local_test.rb +++ /dev/null @@ -1,15 +0,0 @@ -require 'test_helper' - -class LocalTest < Minitest::Test - include Liquid - - def test_local_is_scope_aware - assert_template_result('value', <<~LIQUID) - {%- if true -%} - {%- local variable-name = 'value' -%} - {{- variable-name -}} - {%- endif -%} - {{- variable-name -}} - LIQUID - end -end From e6ed804ca56d6bcc818e439bee372cd5d8a379c3 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Mon, 8 Apr 2019 18:43:09 -0400 Subject: [PATCH 3/5] Fix line number tracking after a non-empty blank token --- lib/liquid/block_body.rb | 6 +++--- test/integration/tags/liquid_tag_test.rb | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 5f86545..4861bca 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -27,11 +27,11 @@ module Liquid private def parse_for_liquid_tag(tokenizer, parse_context) while token = tokenizer.shift case - when token.empty? - # pass + when token.empty? || token =~ WhitespaceOrNothing + # pass, but assign line_number below, since it could change even when + # the token is empty else unless token =~ LiquidTagToken - next if token =~ WhitespaceOrNothing # line isn't empty but didn't match tag syntax, yield and let the # caller raise a syntax error return yield token, token diff --git a/test/integration/tags/liquid_tag_test.rb b/test/integration/tags/liquid_tag_test.rb index 56ca712..d4be128 100644 --- a/test/integration/tags/liquid_tag_test.rb +++ b/test/integration/tags/liquid_tag_test.rb @@ -74,6 +74,11 @@ class LiquidTagTest < Minitest::Test LIQUID end + def test_line_number_is_correct_after_a_blank_token + assert_match_syntax_error("syntax error (line 3): Unknown tag 'error'", "{% liquid echo ''\n\n error %}") + assert_match_syntax_error("syntax error (line 3): Unknown tag 'error'", "{% liquid echo ''\n \n error %}") + end + def test_cannot_open_blocks_living_past_a_liquid_tag assert_match_syntax_error("syntax error (line 3): 'if' tag was never closed", <<~LIQUID) {%- liquid From 7dc488a73b8bbe8717f318ed510bacc090abe3be Mon Sep 17 00:00:00 2001 From: Justin Li Date: Tue, 9 Apr 2019 15:19:47 -0400 Subject: [PATCH 4/5] Simplifications from review --- lib/liquid/block_body.rb | 8 ++------ lib/liquid/locales/en.yml | 1 - 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 4861bca..cffb15b 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -26,11 +26,7 @@ module Liquid private def parse_for_liquid_tag(tokenizer, parse_context) while token = tokenizer.shift - case - when token.empty? || token =~ WhitespaceOrNothing - # pass, but assign line_number below, since it could change even when - # the token is empty - else + unless token.empty? || token =~ WhitespaceOrNothing unless token =~ LiquidTagToken # line isn't empty but didn't match tag syntax, yield and let the # caller raise a syntax error @@ -73,7 +69,7 @@ module Liquid if tag_name == 'liquid'.freeze liquid_tag_tokenizer = Tokenizer.new(markup, line_number: parse_context.line_number, for_liquid_tag: true) - next parse(liquid_tag_tokenizer, parse_context, &block) + next parse_for_liquid_tag(liquid_tag_tokenizer, parse_context, &block) end unless tag = registered_tags[tag_name] diff --git a/lib/liquid/locales/en.yml b/lib/liquid/locales/en.yml index 56c068f..48b3b1d 100644 --- a/lib/liquid/locales/en.yml +++ b/lib/liquid/locales/en.yml @@ -3,7 +3,6 @@ syntax: tag_unexpected_args: "Syntax Error in '%{tag}' - Valid syntax: %{tag}" assign: "Syntax Error in 'assign' - Valid syntax: assign [var] = [source]" - local: "Syntax Error in 'local' - Valid syntax: local [var] = [source]" capture: "Syntax Error in 'capture' - Valid syntax: capture [var]" case: "Syntax Error in 'case' - Valid syntax: case [condition]" case_invalid_when: "Syntax Error in tag 'case' - Valid when condition: {% when [condition] [or condition2...] %}" From ab698191b9681228f8a50946fbd79e8779af09f2 Mon Sep 17 00:00:00 2001 From: Ashwin Maroli Date: Thu, 16 May 2019 19:14:27 +0530 Subject: [PATCH 5/5] Add a CI job to profile memory usage of commit --- .travis.yml | 7 ++-- Gemfile | 1 + performance/memory_profile.rb | 64 +++++++++++++++++++++++++++-------- 3 files changed, 56 insertions(+), 16 deletions(-) diff --git a/.travis.yml b/.travis.yml index 3a01754..24e755b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,18 +6,21 @@ rvm: - 2.3 - 2.4 - 2.5 + - &latest_ruby 2.6 - ruby-head - jruby-head # - rbx-2 -sudo: false - addons: apt: packages: - libgmp3-dev matrix: + include: + - rvm: *latest_ruby + script: bundle exec rake memory_profile:run + name: Profiling Memory Usage allow_failures: - rvm: ruby-head - rvm: jruby-head diff --git a/Gemfile b/Gemfile index 37ffe1d..ddff94a 100644 --- a/Gemfile +++ b/Gemfile @@ -8,6 +8,7 @@ gemspec group :benchmark, :test do gem 'benchmark-ips' gem 'memory_profiler' + gem 'terminal-table' install_if -> { RUBY_PLATFORM !~ /mingw|mswin|java/ } do gem 'stackprof' diff --git a/performance/memory_profile.rb b/performance/memory_profile.rb index bfacde8..9a15375 100644 --- a/performance/memory_profile.rb +++ b/performance/memory_profile.rb @@ -2,25 +2,61 @@ require 'benchmark/ips' require 'memory_profiler' +require 'terminal-table' require_relative 'theme_runner' -def profile(phase, &block) - puts - puts "#{phase}:" - puts +class Profiler + LOG_LABEL = "Profiling: ".rjust(14).freeze + REPORTS_DIR = File.expand_path('.memprof', __dir__).freeze - report = MemoryProfiler.report(&block) + def self.run + puts + yield new + end - report.pretty_print( - color_output: true, - scale_bytes: true, - detailed_report: true - ) + def initialize + @allocated = [] + @retained = [] + @headings = [] + end + + def profile(phase, &block) + print LOG_LABEL + print "#{phase}.. ".ljust(10) + report = MemoryProfiler.report(&block) + puts 'Done.' + @headings << phase.capitalize + @allocated << "#{report.scale_bytes(report.total_allocated_memsize)} (#{report.total_allocated} objects)" + @retained << "#{report.scale_bytes(report.total_retained_memsize)} (#{report.total_retained} objects)" + + return if ENV['CI'] + require 'fileutils' + report_file = File.join(REPORTS_DIR, "#{sanitize(phase)}.txt") + FileUtils.mkdir_p(REPORTS_DIR) + report.pretty_print(to_file: report_file, scale_bytes: true) + end + + def tabulate + table = Terminal::Table.new(headings: @headings.unshift('Phase')) do |t| + t << @allocated.unshift('Total allocated') + t << @retained.unshift('Total retained') + end + + puts + puts table + puts "\nDetailed report(s) saved to #{REPORTS_DIR}/" unless ENV['CI'] + end + + def sanitize(string) + string.downcase.gsub(/[\W]/, '-').squeeze('-') + end end Liquid::Template.error_mode = ARGV.first.to_sym if ARGV.first -profiler = ThemeRunner.new - -profile("Parsing") { profiler.compile } -profile("Rendering") { profiler.render } +runner = ThemeRunner.new +Profiler.run do |x| + x.profile('parse') { runner.compile } + x.profile('render') { runner.render } + x.tabulate +end