Simplify no-unclosed-tag rule

[AndrewJakubowicz]

Context

This was inspired (and maybe closes) #283.

The first commit went in a bad direction (which encouraged self-closing syntax (that doesn't exist in HTML)). Second commit changes the lint to use a simpler check which expects non void elements to be explicitly closed.

What is wrong with the current check?

The current no-unclosed-tag check grabs the source code locations for the tag from parse5, and then tries to guess if the tag was closed. This guess is inaccurate, and boils down to:

1. The node has no children
2. The startTag.endOffset location matches the node.endOffset location.

This results in edge cases.

Edge case 1

The first edge case can be seen with <p> which can be closed implicitly if followed by a <div>.

Consider:

<p><div></div><!--becomes:--><p></p><div></div>
<p>Some content<div></div><!--becomes:--><p>Some content</p><div></div>

Currently, the first case without content will not raise the no-unclosed-tag check, but the second case does raise the no-unclosed-tag check.
This is because the first case passes both the condition that no children are present, and the implicitly closed tag will have the same offset location as the authored tag. But this is a false negative, because the resulting DOM is probably not what the author intended.

The second case is more correct because the <p> element created contains the Some content text and thus is not empty.

Fix

Make this lint require explicit closing tags by checking whether parse5 has got an authored location for the endTag. Because void elements do not have an endTag we also must handle them.

Test plan

All these edge cases have been captured in the tests.

[43081j]

maybe we can actually just check that there is an end tag?

for example:

  <p>i never close

will have a startTag location, and no endTag location.

similarly:

  <p />

will behave the same since you can't self-close a p tag (parse5 will already be taking this into account). so it too won't have an endTag and will actually have a child text node (the whitespace following it).

if you do the same with <br/>, parse5 will give you a node with no children, and no end tag. so it will already be doing a check against a whitelist of void elements (here)

so maybe we can just check !getNodeSourceLocation(p5Node).endTag?

i'm not sure why we didn't do that all along... unless i'm missing something 🤔

[AndrewJakubowicz]

Over the weekend I came to a similar conclusion @43081j so I totally agree with you.

The check can skip void tags, and otherwise check for the presence of the endTag.
Will update today. Thanks for an amazing comment!

[AndrewJakubowicz]

cc @43081j and @rictic

This is ready for review. Thank you James for your exceptional feedback!

[43081j]

the CI failure is because one of the tests actually self-closes a custom element FYI 😬

so it already caught one!

[43081j]

looks good to me 👍

we should probably double check if it does solve the issue you mentioned too. that was more about a space tripping it up i think

[AndrewJakubowicz]

Excellent! :D Thank you!

1. Added a specific test case for <p> <div></div></p> (which continues to raise the diagnostic).
2. Fixed the self-closing custom element in the other test.

[rictic]

Do we warn on an invalid closing element for a void element? Seems like it'd be good to do so. For example, this should be a warning on the </input>

<input><div></div></input>

[AndrewJakubowicz]

I think this is a good idea but maybe something we can add incrementally to the no-unclosed-tag diagnostic.

I'll make an issue.

[rictic]

In an ideal world, we might want to allow:

<p>One
<p>Two

Or just:

<p>Hi

And we'd want to put warnings on dangling closing tags that don't match up to any opening tags.

All of those may be somewhat tricky to do with how parse5 represents the tree though, requiring us to notice spans in the original text that aren't represented in the parsed tree.

[AndrewJakubowicz]

These are very tricky, and another good case to add to an issue for followup.

Philosophically though, I wonder if the lit-analyzer should avoid HTML diagnostics and leave that to an HTML specific tool. It may even be worth scoping the no-unclosed-tag to only check custom element tags.