Remove Hybrid Tag - #1309
Remove Hybrid Tag#1309stephanie-shopify wants to merge 4 commits into
Conversation
| * +section+ is a standalone tag, so +{% endsection %}+ parses as an unknown | ||
| * tag. Ruby Liquid reports it the same way. | ||
| */ | ||
| export function checkEndsectionTag(node: LiquidTag, context: Context): void { |
There was a problem hiding this comment.
not sure if this is correct behavior to add
There was a problem hiding this comment.
This moves existing behavior rather than adding new behavior. On main, LiquidSyntaxError already reports Unknown tag 'endsection', from inside checkSectionTag via the section node's blockEndPosition. That only exists because of the hybrid block form. With section standalone, endsection is its own LiquidTag node, so the report moves to a checker keyed on endsection. The message and range are the same, and it matches Ruby Liquid (Shopify core asserts Unknown tag 'endsection' for the legacy block form, and the parity corpus expects it).
That said, it's only here because the parser change moves endsection. If we keep the hybrid tag and only fix the scan (see the thread on ast.test.ts), this goes away and checkSectionTag stays as it is on main.
| // Re-export all tag definition types so existing imports from './environment' keep working. | ||
| export { | ||
| TagKind, | ||
| isStructuralEndTag, |
There was a problem hiding this comment.
Plumbing, not API. liquid-tags.ts already imports TagKind from ../environment, so I put the helper there too. But the re-export comment says it's there "so existing imports keep working", and the tag files import from ../tag-definitions directly, so a new symbol arguably doesn't belong here. environment isn't exported from the package index.ts, so nothing external sees it. If we keep this approach I'll import from ../tag-definitions and drop the re-export.
| } | ||
| }); | ||
|
|
||
| it('should throw on orphaned endsection', () => { |
There was a problem hiding this comment.
why was this here? why does removing hybrid tag change it?
There was a problem hiding this comment.
On main, any end tag for a registered tag with no opener is a parse error ({% endrender %}, {% endif %}, …). section is registered, so a lone {% endsection %} threw too. With the hybrid form, a paired endsection was legitimate, and this test pinned that difference: paired parses, orphan throws.
Once section isn't a block, every endsection is effectively an orphan. Left alone, the legacy {% section %}…{% endsection %} form would go from "Unknown tag, other checks still run" to "whole file fails to parse" (theme-check skips every other check on files that fail to parse). So I exempted endsection from that throw, which is what flips this test. Other tags' end-tag errors are unchanged, and there's a regression test for that.
This, the checker move and the new export all follow from removing the block form. They also cause a few remaining differences on already-invalid Liquid, which the tophat caught: different messages for a lone endsection, one in {% liquid %} and one inside HTML, and Prettier output changes for those. <p>{% section 'x' %}y{% endsection %}</p> gets mangled.
Alternative with zero behavior change: keep section hybrid and replace the per-tag forward scan with one pass that pairs each section with its endsection (like matching parentheses), per document and per {% liquid %} block. That's still linear in the worst case, and theme-check, Prettier and all existing tests stay as on main. Removing the block form could then be a separate follow-up where the behavior change is the explicit point. I'm leaning that way. I'll check with CP, since removing hybrid was their suggestion.
There was a problem hiding this comment.
Alternative with zero behavior change: keep section hybrid and replace the per-tag forward scan with one pass that pairs each section with its endsection
dont want to do this. defeats purpose of removing hybrid section tag.
Ruby Liquid has no block form for section, but the parser treated it as a
hybrid tag and scanned forward for a matching endsection on every section
tag (both in documents and in {% liquid %} bodies). Files with many section
tags parsed in quadratic time.
Make section a plain TagKind.Tag and remove the hybrid tag kind. Theme
Check still reports a stray endsection as "Unknown tag 'endsection'" by
mapping the parser error.
The previous commit made a stray endsection throw "Attempting to close LiquidTag 'section'". In a full Theme Check run that turns the file's AST into an error, so the only offense was LiquidHTMLSyntaxError and every other check skipped the file. Only tags with a body (Block/Raw) have end tags, so only those now throw the structural close errors. endsection parses as an unknown tag like any other end* name, and LiquidSyntaxError reports it as "Unknown tag 'endsection'".
laxRecoverTagMarkup only recovers Tag/Block kinds, so section (previously
Hybrid) was never recovered. Now that section is a Tag it would be, and
{% section 'x' junk %} would recover and render, while Ruby's section tag
raises in every error mode. Exclude it explicitly to keep the previous
behavior.
The previous change stopped throwing structural close errors for every
registered tag without a body, so stray {% endrender %}, {% endecho %},
{% endsections %}, etc. silently parsed as unknown tags instead of failing
as they do on main. Only endsection needs the exception. Restore the
existing behavior for everything else and add a regression test.
75220dc to
334cac7
Compare
stephanie-shopify
changed the title
This branch has not been deployed
1 participant
What are you adding in this PR?
Parses
sectionas a standalone tag and removes the hybrid tag kind.The hand-written parser treated
sectionas a hybrid tag: every{% section %}scanned forward through the rest of the document (or the rest of a{% liquid %}body) looking for a matching{% endsection %}. Files with manysectiontags parsed in quadratic time.Ruby Liquid has no block form for
section(the parity corpus already records{% section 'foo' %}{% endsection %}asUnknown tag 'endsection'), so the hybrid path wasn't modelling real Liquid. Its only consumer was Theme Check, which used the block form to report that error.Changes:
sectionTagis nowTagKind.Tag. RemovedTagKind.Hybrid,TagDefinitionHybrid,liquid-hybrid.tsandparseLineHybridTag.{% endsection %}now parses as an unknown tag, andLiquidSyntaxErrorreports it asUnknown tag 'endsection'(same message as before). Stray{% endrender %},{% endsections %}, etc. are unchanged.laxRecoverTagMarkuponly recoversTag/Blockkinds, so hybridsectionwas never recovered. It now excludessectionexplicitly to keep that behavior (Ruby'ssectionraises in every error mode).Intentional behavior differences, all on Liquid that Ruby already rejects:
{% endsection %}used to be a parse error, so every other check skipped the file. Now it's an unknown-tag offense and the other checks still run.endsectionline inside{% liquid %}reportsUnexpected end tag 'endsection' in {% liquid %} block, same as a strayendif.endsectioninside an HTML element (<p>{% section 'x' %}y{% endsection %}</p>) reportsLiquidHTMLSyntaxError. That's the existing behavior for any unknownend*tag inside HTML (<p>{% endfoo %}</p>does the same onmain).Parse time for N consecutive
sectiontags:{% section 'x' %}before{% liquid %}lines beforeWhat's next? Any followup issues?
None.
Tophatting
Before/after harness (theme-check CLI + Prettier + parser timing,
mainbuild vs this branch):sectionusage (incl.{% liquid %}and nested inif), missing section files (MissingTemplate), bad markup and each stray-endsectioncase. The only theme-check diffs are the intentional ones listed above. Prettier output also changes for those stray-endsectioninputs. Most changes are cosmetic, but<p>{% section 'x' %}y{% endsection %}</p>gets mangled (see review thread onast.test.ts)..liquidfile.sectiontags: 7.3s → 26ms (document), 1.0s → 26ms ({% liquid %}).Manual (VS Code extension, Debug Node Extension):
sectiontags. Cmd-click on the section name opens the section file.MissingTemplateon missing sections.Unknown tag 'endsection'on strayendsection.{% section %}references.Tests:
vitestacross liquid-html-parser, theme-check-common, prettier-plugin-liquid, theme-graph and theme-language-server-common all pass (6714 tests, rebased on2066abfe).Before you deploy
changeset