bkiers / Liqp

An ANTLR based 'Liquid Template' parser and rendering engine.
MIT License
165 stars 94 forks source link

`include` with `with` syntax should throw an expection in Jekyll flavour #196

Closed msangel closed 3 years ago

msangel commented 3 years ago

Like that:

Traceback (most recent call last):
        10: from cases_include_with_syntax_with.rb:10:in `<main>'
         9: from /srv/jekyll/_helpers.rb:88:in `render'
         8: from /usr/lib/ruby/gems/2.7.0/gems/liquid-4.0.3/lib/liquid/template.rb:116:in `parse'
         7: from /usr/lib/ruby/gems/2.7.0/gems/liquid-4.0.3/lib/liquid/template.rb:132:in `parse'
         6: from /usr/lib/ruby/gems/2.7.0/gems/liquid-4.0.3/lib/liquid/document.rb:5:in `parse'
         5: from /usr/lib/ruby/gems/2.7.0/gems/liquid-4.0.3/lib/liquid/document.rb:10:in `parse'
         4: from /usr/lib/ruby/gems/2.7.0/gems/liquid-4.0.3/lib/liquid/block_body.rb:34:in `parse'
         3: from /usr/lib/ruby/gems/2.7.0/gems/liquid-4.0.3/lib/liquid/tag.rb:9:in `parse'
         2: from /usr/lib/ruby/gems/2.7.0/gems/liquid-4.0.3/lib/liquid/tag.rb:9:in `new'
         1: from /usr/lib/ruby/gems/2.7.0/gems/jekyll-4.1.1/lib/jekyll/tags/include.rb:29:in `initialize'
/usr/lib/ruby/gems/2.7.0/gems/jekyll-4.1.1/lib/jekyll/tags/include.rb:74:in `validate_params': Invalid syntax for include tag: (ArgumentError)

with 'red'

Valid syntax:

{% include file.ext param='value' param2='value' %}
msangel commented 3 years ago

This bug is more complicated than it seems so. Problem is: Invalid is a fact of whitespace character in resource name for Jekyll, but the rest of possible items there is a valid one. A very simple example: {% include some.file %} What is some.file? according to existed lexer rules, this is an expression. But that's not true. This should be a filename. We don't have a lexeme for a filename but we can add one. And it will be parsable, but then it will break parsing of all the expressions that look like filename: {% for item in site.pages %}. So there is not a proper solution. We can go further and create a new lexer mode depending on lexer flavor:

IncludeResourceStart : 'include' WhitespaceChar+ {!isLekyll}? -> pushMode(IN_INCLUDE_JEKYLL_RESOURCE) ;
...
mode IN_INCLUDE_JEKYLL_RESOURCE;
   IncludeEnd : '%}' -> popMode, popMode;
   OutStart3 : '{{' -> pushMode(IN_TAG);
   ExitIncludeResource : WhitespaceChar+ -> popMode;
   IncludeResource: .+?;

Then it will be tokenized properly and so we can do safe parsing:

include_tag
 : {isLiquid()}? tagStart liquid=Include expr (With Str)? TagEnd
 | {isJekyll()}? tagStart jekyll=Include file_name_or_output (jekyll_include_params)* TagEnd
 ;

// only valid for Flavor.JEKYLL
file_name_or_output
 : output   #jekyll_include_output
 | filename #jekyll_include_filename
 ;

// most important part now
filename
 : IncludeResource+
 ;

And I believe this will be the most correct solution, but, this will increase the required changes amount dramatically and will require a lot of time to code and test it. Also, this will made testing of vocabulary with external tools more complicated because of the new semantic predicate. And yes, it also will require a lot of new code because this is only for Jekyll, and the Liquid should behave as previously, so everywhere additional checks and so on.

So if anyone will have a willingness to fix this in a proper way, you know what to do. Meanwhile, I will fix this quite simple way - I will allow anything as filename but with greed matching so it will eat as little as possible (till not reached next matching lexeme):

filename
 : ( . )+?
 ;

And the next lexeme that will close this will be: (jekyll_include_params)* TagEnd, so once match that, the filename will ends. And yes, as those anything might be anything, I will not visit it, just read its interval as a text;

Interval interval = Interval.of(ctx.filename().start.getStartIndex(), ctx.filename().stop.getStopIndex());
String text = ctx.filename().start.getInputStream().getText(interval);