Error parsing Django with unclosed tag inside if statement

[adamghill]

Describe the bug

Running curlylint on the following example Django template throws an exception. It appears that having an un-closed tag inside of the if statement causes an issue for curlylint. While probably not a great practice (there are other ways to achieve the same result) it came as a surprise that it wouldn't even parse it.

{% for thing in things %}
	{% if forloop.first %}
	<ul>
	{% endif %}

	<li>{{ thing }}</li>
	
	{% if forloop.last %}
	</ul>
	{% endif %}
{% endfor %}

Which terms did you search for in the documentation and issue tracker?

I looked through all of the open and closed issues.

Environment

Python 3.6.6
curlylint 0.12.0

Steps to reproduce

1. First, create a template with the above example.
2. Then, run curlylint pointing to the template created.

Expected behavior

Parse and lint the template without throwing an exception.

Actual behavior

Exception thrown:

  File ".venv/lib/python3.6/site-packages/parsy/__init__.py", line 275, in seq_parser
    result = parser(stream, index).aggregate(result)
  File ".venv/lib/python3.6/site-packages/parsy/__init__.py", line 80, in __call__
    return self.wrapped_fn(stream, index)
  File ".venv/lib/python3.6/site-packages/parsy/__init__.py", line 338, in generated
    next_parser = iterator.send(value)
  File ".venv/lib/python3.6/site-packages/curlylint/parse.py", line 541, in opt_container_impl
    yield P.fail("expected `{% if " + content + " %}`")
TypeError: must be str, not Parser

Reproducible demo

I can fork this repo and add a sample template to the tests directory if that is helpful. Or create a minimal repo demonstrating the issue. Let me know which might be the most useful!

[kdeldycke]

Got the exact same issue, in which the following Jinja template:

{% for article in dates %}

   (...)

   {% if article.date.month != current_month %}
       {% if not loop.first %}
           </dl>
       {% endif %}
       <dl class="dl-horizontal">
   {% endif %}

   (...)

   {% if loop.last %}
       </dl>
   {% endif %}

{% endfor %}

Leads to the following error, in which curlylint stumble upon the </dl> tag:

❯ curlylint --verbose .
(...)
47:41	Parse error: expected one of '[:a-zA-Z]', 'animate', 'animateMotion', 'animateTransform', 'area', 'base', 'br', 'circle', 'col', 'ellipse', 'embed', 'feBlend', 'feColorMatrix', 'feComposite', 'feConvolveMatrix', 'feDisplacementMap', 'feDistantLight', 'feDropShadow', 'feFlood', 'feFuncA', 'feFuncB', 'feFuncG', 'feFuncR', 'feGaussianBlur', 'feImage', 'feMergeNode', 'feMorphology', 'feOffset', 'fePointLight', 'feSpotLight', 'feTile', 'feTurbulence', 'hr', 'image', 'img', 'input', 'line', 'link', 'meta', 'mpath', 'param', 'path', 'polygon', 'polyline', 'rect', 'script', 'set', 'source', 'stop', 'style', 'track', 'use', 'wbr', '{#', '{%', '{{' at 47:41	parse_error

Oh no! 💥 💔 💥
1 error reported

The full source of the Jinja template I'm trying to validate is available here: https://github.com/kdeldycke/plumage/blob/3f25667919594aadbd29caeaa5465b50185b830d/plumage/templates/archives.html#L48

[tunetheweb]

Hmmm looks like this is not implemented, except in very specific, limited circumstances:

curlylint/curlylint/parse.py

Lines 537 to 548 in 35a1412

	# Awkward hack to handle optional HTML containers, for example:
	#
	# {% if a %}
	# <div>
	# {% endif %}
	# foo
	# {% if a %}
	# </div>
	# {% endif %}
	#
	# Currently, this only works with `if` statements and the two conditions
	# must be exactly the same.

And this came over from the original jinja-lint.

Guessing that means not easy to fix as lots of variations of this, which can produce valid HTML, such as:

{% if test %}
<div>
{% elsif %}
<div>
{% endif %}
...
</div>

or even

{% if test >= 0 %}
<div>
{% endif %}
...
{% if test < 0 %}
<div>
{% endif %}
...
</div>

I do wonder if this will ever be possible to lint?

Or, perhaps it's best handled with exceptions, using syntax something like below for example:

{% if test >= 0 %}
<div>{# curly_ignore #}
{% endif %}
...
{% if test < 0 %}
<div>{# curly_ignore #}
{% endif %}
...
</div>{# curly_ignore #}

[thibaudcolas]

Thank you for the detailed issue report 👌 I have noticed this in the past, but didn’t give it much thought since as @adamghill mentions there were ways to rewrite the templates to avoid this style (although that’s not always a clear-cut option).

As @bazzadp mentions it would require much more complex static analysis than what the parser currently does to determine whether any arbitrary combination of conditionals does end up outputting valid HTML or not, however I think it would be realistic to support some of those patterns. I haven’t had to work on that part of the parser just yet, so I’m not sure what kind of effort would be needed. It certainly would be nice if simple cases like {% if test %}<div class="foo">{% else %}<div class="bar">{% endif %} "just worked".

I would be very keen to add support for those proposed {# curly_ignore #} comments as well, as currently it’s really annoying to have to ignore whole files for single linting issues (or curlylint bugs), so this might be the more pragmatic way forward.

---

In the meantime I would also welcome adding this to the website in a new "Known issues" page, either at the end of the Introduction section or under References, with prominent links to it from the README’s "Usage" section, and perhaps a link right at the end of the "Getting started" page on the site.

I think it would also be good to:

• Change the current implementation so an error message displays, instead of the exception / stack trace
• @adamghill if you or anyone else is up for it – the parser badly needs more unit tests written for it. If you want to add test cases for your examples they could go next to https://github.com/thibaudcolas/curlylint/blob/main/curlylint/parse_test.py#L324-L333.

[CAM-Gerlach]

Hey @thibaudcolas , thanks for your excellent work and maintainership thus far. I ran into this issue myself, with the simplest reproducible test case being:

{%- if this %}
<div>
{%- else %}
<div>
{%- endif %}
</div>

At the very minimum, how difficult would it be to add support for checking that a <div> matches in simple cases like this, where you can statically guarantee that a <div> will be output? I.e. would it be practical to add a simple hack like what already exists for identical if statements? My instances of this issue would all be covered by this scenario (although I was fortunately able to rewrite the affected templates to avoid it without too much trouble; if its more for you to support this then I don't mean to impose).

[thibaudcolas]

Hey @CAM-Gerlach, thanks for chiming in :) Supporting this simplest case sounds reasonable to me, and is something I will feel comfortable embarking on once I’ve written more tests for the parser. The parser comes almost straight from jinjalint, on which Curlylint is originally based, and is the code I’m the least familiar with.

I’ll try to dive into the parser code again this week to see how much work it would be to fix all of the known parser issues, and at least write tests for those well-known failure cases.

[sanchit-mathew]

Hi @thibaudcolas. I made a PR #53 for the minimum case described by @CAM-Gerlach. Please have a look and let me know if it's fine. Thanks :)