Skip to content

XML: keep HTML void element support from changing how malformed XML recovers - #8563

Closed
timtebeek wants to merge 1 commit into
mainfrom
tim/xml-apim-policy-attr-npe
Closed

XML: keep HTML void element support from changing how malformed XML recovers#8563
timtebeek wants to merge 1 commit into
mainfrom
tim/xml-apim-policy-attr-npe

Conversation

@timtebeek

@timtebeek timtebeek commented Aug 19, 2026

Copy link
Copy Markdown
Member

What's wrong

element
    :   OPEN name=Name attribute*
        (   '/>'
        |   CLOSE
            (   {isVoidElement($name.text)}? voidClose
            |   content* OPEN '/' Name CLOSE
            )
        )
    ;

That PR argued this was safe for XML because the predicate is gated on htmlMode. The predicate is — but the rule shape isn't, and the shape is what determines the resynchronization sets ANTLR's DefaultErrorStrategy uses. Recovery changed for every source the XML parser touches, including strict XML, where the void alternative is never taken.

The input that exposes it: XMLLexer has STRING : '"' ~[<"]* '"', so an attribute value may contain neither " nor <. A value carrying an unescaped quote — routine in embedded expressions such as Azure API Management policies, whose bodies span many lines — ends its STRING at the first embedded quote, and the remainder re-lexes as further attributes.

Under the new shape those recover into an attribute with a synthesized =. Once built, the printer cannot tell it from a real one and writes it back into the source:

     <set-variable name="allowList" value="@{
         string list = "";
-        list += "AAA,";
+        list += "AAA=,";

One level of nesting deeper, the same input instead abandons the rule with a null EQUALS, and visitAttribute throws:

java.lang.NullPointerException: Cannot invoke "org.antlr.v4.runtime.tree.TerminalNode.getSymbol()" because "node" is null
  org.openrewrite.xml.internal.XmlParserVisitor.convert(XmlParserVisitor.java:530)
  org.openrewrite.xml.internal.XmlParserVisitor.lambda$visitAttribute$25(XmlParserVisitor.java:395)

Two symptoms, one cause; nesting alone decides which you get. Neither reproduces on any released version — I checked 8.20.0, 8.40.0 and 8.56.1, and confirmed by bisect and by reverting only rewrite-xml/ onto current main.

The fix

  • Give strict XML back its original rule, gated by isHtmlMode(), so HTML keeps the left-factored shape and XML keeps the recovery behaviour it had before rewrite-xml: support HTML void elements in JSP/HTML parsing #7906. This is what makes "HTML support does not affect XML parsing" true of recovery and not only of matching.

  • Reject attributes and elements that recovery left incomplete or invented. visitAttribute now rejects a context with a missing Name/STRING or a synthesized EQUALS, and visitElement rejects a nameless element, so the document falls back to a ParseError that preserves the original text — the contract XML: harden parser against malformed input crashes #7555 established for other malformed input. A synthesized token is detectable (getTokenIndex() < 0); a synthesized = in a built Xml.Attribute is not, which is why this has to happen at the parse tree.

These files genuinely are not well-formed XML — xmllint rejects them — so a ParseError is the right outcome. What isn't right is a crash, or a reprint that differs from the input.

I deliberately did not blanket-null-guard convert(TerminalNode, ...). On its own that converts the crash into a silently wrong LST and hides the grammar problem, which is strictly worse; the print-idempotency check only catches corruption that happens to be visible in the reprint.

Tests

Three tests in XmlParserTest, each verified to fail without the main-source change and pass with it:

  • attributeValueWithUnescapedQuoteIsNotGivenAnExtraEquals — round-trips again instead of gaining an =

  • policyExpressionAttributeValueIsNeverRewritten — a policy-style document; ParseError, text byte for byte, and asserts no AAA= in the reported diff

  • nestedAttributeValueWithUnescapedQuoteAndAngleBracketDoesNotThrowParseError rather than an NPE

  • :rewrite-xml:test and :rewrite-maven:test are green, including the void element and JSP tests from rewrite-xml: support HTML void elements in JSP/HTML parsing #7906.

Notes

  • Draft: I'd like a second opinion on gating the element rule on isHtmlMode() versus keeping one shape and constraining recovery some other way. Duplicating the alternatives grows the ATN, but it is the only version I found that leaves XML recovery provably untouched.
  • The C# grammar sources under rewrite-csharp/ are left alone, matching rewrite-xml: support HTML void elements in JSP/HTML parsing #7906, which also did not regenerate them; XMLParserBase has no C# counterpart yet.

…ecovers

The `element` rule was left-factored when void element support was added in
#7906, so strict XML and HTML-like sources began sharing one shape. The shape
of the rule, not just which alternative matches, determines the
resynchronization sets ANTLR's error recovery uses, so the change altered
recovery for XML too -- despite the void element predicate itself being gated
on `htmlMode`.

`XMLLexer` ends a `STRING` at the first embedded quote, because an attribute
value may contain neither `"` nor `<`. An attribute whose value carries an
unescaped quote -- common in embedded expressions such as Azure API Management
policies -- therefore re-lexes the rest of the value as further attributes.
Under the new shape those recovered into an `attribute` with a synthesized
`=`, which the printer cannot distinguish from a real one and wrote back into
the value:

    -    list += "AAA,";
    +    list += "AAA=,";

One level of nesting deeper the same input instead abandoned the rule with a
null `EQUALS`, and `visitAttribute` threw a `NullPointerException`.

Give strict XML back the two plain alternatives it had before void element
support existed, gated by `isHtmlMode()`, and reject attributes and elements
that error recovery left incomplete or invented so such input falls back to a
`ParseError` preserving the original text, as #7555 established for other
malformed input.

The C# grammar sources are left as they are, matching #7906, which also did
not regenerate them.
@timtebeek

Copy link
Copy Markdown
Member Author

Closing as a duplicate of #8562, which covers this and does it better.

I ran both against a shared corpus. #8562 alone already resolves the two symptoms this PR was opened for — the invented = and the TerminalNode.getSymbol() NPE both land on a clean Malformed attribute message. Gating the element rule on isHtmlMode() on top of it makes two cases worse, because #8562's guards are tuned to the current left-factored recovery shape and reverting XML to the older shape routes those inputs down paths the guards don't cover:

Input #8562 alone #8562 + this PR
nested unescaped quote + < Malformed attribute not print idempotent
multi-line policy expression Malformed attribute NullPointerException

Everything else in the corpus was identical, including void element handling for .html vs .xml, so the gate isn't buying anything there either.

The #7906 attribution is already captured in #8562's description via the 8.84.4 bisect, so nothing here is additive.

@timtebeek timtebeek closed this Aug 19, 2026
@github-project-automation github-project-automation Bot moved this from In Progress to Done in OpenRewrite Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant