fix: report error for malformed else/elsif/endif/endfor, #713

Author: harttle

docs/source/tutorials/differences.md

@@ -34,7 +34,7 @@ Though we're trying to be compatible with the Ruby version, there are still some
     * LiquidJS-defined filters: [json][json], group_by, group_by_exp, where_exp, jsonify, inspect, etc.
     * Tags/filters that don't depend on Shopify platform are borrowed from [Shopify][shopify-tags].
     * Tags/filters that don't depend on Jekyll framework are borrowed from [Jekyll][jekyll-filters].
-* Some tags/filters behave differently: [date][date] filter.
+* Some tags/filters behave differently: [date][date] filter, malformed tags (like duplicated `else`, extra args for `endif`) throw errors in LiquidJS. 
 
 [date]: https://liquidjs.com/filters/date.html
 [layout]: ../tags/layout.html

docs/source/zh-cn/tutorials/differences.md

@@ -34,7 +34,7 @@ LiquidJS 一直很重视兼容于 Ruby 版本的 Liquid。Liquid 模板语言最
     * LiquidJS 自己定义的过滤器：[json][json]。
     * 从 [Shopify][shopify-tags] 借来的不依赖 Shopify 平台的标签/过滤器。
     * 从 [Jekyll][jekyll-filters] 借来的不依赖 Jekyll 框架的标签/过滤器。
-* 有些过滤器和标签表现不同：比如 [date][date]。
+* 有些过滤器和标签表现不同：比如 [date][date]，非法的标签（比如重复的 `else`，`endif` 的多余参数）在 LiquidJS 中会抛出异常。
 
 [layout]: ../tags/layout.html
 [render]: ../tags/render.html

src/tags/for.ts

@@ -1,10 +1,10 @@
 import { Hash, ValueToken, Liquid, Tag, evalToken, Emitter, TagToken, TopLevelToken, Context, Template, ParseStream } from '..'
-import { toEnumerable } from '../util'
+import { assertEmpty, toEnumerable } from '../util'
 import { ForloopDrop } from '../drop/forloop-drop'
 
 const MODIFIERS = ['offset', 'limit', 'reversed']
 
-type valueof<T> = T[keyof T]
+type valueOf<T> = T[keyof T]
 
 export default class extends Tag {
   variable: string
@@ -31,12 +31,10 @@ export default class extends Tag {
     let p
     const stream: ParseStream = this.liquid.parser.parseStream(remainTokens)
       .on('start', () => (p = this.templates))
-      .on('tag:else', () => (p = this.elseTemplates))
-      .on('tag:endfor', () => stream.stop())
+      .on<TagToken>('tag:else', tag => { assertEmpty(tag.args); p = this.elseTemplates })
+      .on<TagToken>('tag:endfor', tag => { assertEmpty(tag.args); stream.stop() })
       .on('template', (tpl: Template) => p.push(tpl))
-      .on('end', () => {
-        throw new Error(`tag ${token.getText()} not closed`)
-      })
+      .on('end', () => { throw new Error(`tag ${token.getText()} not closed`) })
 
     stream.start()
   }
@@ -58,7 +56,7 @@ export default class extends Tag {
       ? Object.keys(hash).filter(x => MODIFIERS.includes(x))
       : MODIFIERS.filter(x => hash[x] !== undefined)
 
-    collection = modifiers.reduce((collection, modifier: valueof<typeof MODIFIERS>) => {
+    collection = modifiers.reduce((collection, modifier: valueOf<typeof MODIFIERS>) => {
       if (modifier === 'offset') return offset(collection, hash['offset'])
       if (modifier === 'limit') return limit(collection, hash['limit'])
       return reversed(collection)

src/tags/if.ts

@@ -1,38 +1,32 @@
 import { Liquid, Tag, Value, Emitter, isTruthy, TagToken, TopLevelToken, Context, Template } from '..'
+import { assert, assertEmpty } from '../util'
 
 export default class extends Tag {
   branches: { value: Value, templates: Template[] }[] = []
-  elseTemplates: Template[] = []
+  elseTemplates: Template[] | undefined
 
   constructor (tagToken: TagToken, remainTokens: TopLevelToken[], liquid: Liquid) {
     super(tagToken, remainTokens, liquid)
     let p: Template[] = []
-    let elseCount = 0
     liquid.parser.parseStream(remainTokens)
       .on('start', () => this.branches.push({
         value: new Value(tagToken.args, this.liquid),
         templates: (p = [])
       }))
       .on('tag:elsif', (token: TagToken) => {
-        if (elseCount > 0) {
-          p = []
-          return
-        }
+        assert(!this.elseTemplates, 'unexpected elsif after else')
         this.branches.push({
           value: new Value(token.args, this.liquid),
           templates: (p = [])
         })
       })
-      .on('tag:else', () => {
-        elseCount++
-        p = this.elseTemplates
-      })
-      .on('tag:endif', function () { this.stop() })
-      .on('template', (tpl: Template) => {
-        if (p !== this.elseTemplates || elseCount === 1) {
-          p.push(tpl)
-        }
-      })
+      .on<TagToken>('tag:else', tag => {
+      assertEmpty(tag.args)
+      assert(!this.elseTemplates, 'duplicated else')
+      p = this.elseTemplates = []
+    })
+      .on<TagToken>('tag:endif', function (tag) { assertEmpty(tag.args); this.stop() })
+      .on('template', (tpl: Template) => p.push(tpl))
       .on('end', () => { throw new Error(`tag ${tagToken.getText()} not closed`) })
       .start()
   }
@@ -47,6 +41,6 @@ export default class extends Tag {
         return
       }
     }
-    yield r.renderTemplates(this.elseTemplates, ctx, emitter)
+    yield r.renderTemplates(this.elseTemplates || [], ctx, emitter)
   }
 }

src/util/assert.ts

@@ -8,3 +8,7 @@ export function assert <T> (predicate: T | null | undefined, message?: string |
     throw new AssertionError(msg)
   }
 }
+
+export function assertEmpty<T> (predicate: T | null | undefined, message = `unexpected ${JSON.stringify(predicate)}`) {
+  assert(!predicate, message)
+}

test/e2e/issues.spec.ts

@@ -471,19 +471,11 @@ describe('Issues', function () {
   })
   it('#670 Should not render anything after an else branch', () => {
     const engine = new Liquid()
-    const result = engine.parseAndRenderSync('{% assign value = "this" %}' +
-      '{% if false %}don\'t show' +
-      '{% else %}show {{ value }}' +
-    '{% else %}don\'t show{% endif %}', {})
-    expect(result).toEqual('show this')
+    expect(() => engine.parseAndRenderSync('{% assign value = "this" %}{% if false %}{% else %}{% else %}{% endif %}')).toThrow('duplicated else')
   })
   it('#672 Should not render an elseif after an else branch', () => {
     const engine = new Liquid()
-    const result = engine.parseAndRenderSync('{% if false %}don\'t show' +
-      '{% else %}show' +
-      '{% elsif true %}don\'t show' +
-    '{% endif %}', {})
-    expect(result).toEqual('show')
+    expect(() => engine.parseAndRenderSync('{% if false %}{% else %}{% elsif true %}{% endif %}')).toThrow('unexpected elsif after else')
   })
   it('#675 10.10.1 Operator: contains regression', () => {
     const engine = new Liquid()

test/integration/tags/for.spec.ts

@@ -78,10 +78,10 @@ describe('tags/for', function () {
         .rejects.toThrow('illegal tag: {%for c alpha%}, line:1, col:1')
     })
 
-    it('should reject when inner templates rejected', function () {
-      const src = '{%for c in alpha%}{%throwingTag%}{%endfor%}'
+    it('should throw for additional args', function () {
+      const src = "{% for f in foo %} foo {% else foo = 'blah' %} {% endfor %}"
       return expect(liquid.parseAndRender(src, scope))
-        .rejects.toThrow(/intended render error/)
+        .rejects.toThrow(`unexpected "foo = 'blah'", line:1, col:1`)
     })
   })
 

test/integration/tags/if.spec.ts

@@ -26,6 +26,12 @@ describe('tags/if', function () {
     return expect(html).toBe('')
   })
 
+  it('should throw for additional args', function () {
+    const src = "{% if foo %} foo {% else foo = 'blah' %} {% endif %}"
+    return expect(liquid.parseAndRender(src, scope))
+      .rejects.toThrow(`unexpected "foo = 'blah'", line:1, col:1`)
+  })
+
   describe('single value as condition', function () {
     it('should support boolean', async function () {
       const src = '{% if false %}1{%elsif true%}2{%else%}3{%endif%}'
@@ -155,12 +161,12 @@ describe('tags/if', function () {
     const html = await liquid.parseAndRender(src, scope)
     return expect(html).toBe('no')
   })
-  it('should not render anything after an else branch even when first else branch is empty', () => {
-    const engine = new Liquid()
-    const result = engine.parseAndRenderSync('{% if false %}don\'t show' +
-      '{% else %}' +
-      '{% else %}don\'t show' +
-    '%{% endif %}', {})
-    expect(result).toEqual('')
+  it('should throw for duplicated else', () => {
+    expect(() => liquid.parseAndRenderSync('{% if false %}{% else %}{% else %}{% endif %}'))
+      .toThrow(`duplicated else`)
+  })
+  it('should throw for unexpected elsif', () => {
+    expect(() => liquid.parseAndRenderSync('{% if false %}{% else %}{% elsif true %}{% endif %}'))
+      .toThrow(`unexpected elsif after else`)
   })
 })