Error parsing Django with unclosed tag inside if statement

[thibaudcolas]

Error parsing Django with unclosed tag inside if statement

[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 :)