Parse processing instructions that carry no data - #8361
Open
renechoi wants to merge 1 commit into
Open
Conversation
`processinginstruction` required `PI_TEXT+`, but the XML specification makes
a processing instruction's data optional:
PI ::= '<?' PITarget (S (Char* - (Char* '?>' Char*)))? '?>'
So `<?target?>` and `<?target ?>` are well formed, yet both failed with
`mismatched input '?>' expecting PI_TEXT`. Because that is a parse failure
rather than a recipe failure, the whole document became unparseable and no
recipe could run on the file at all.
Relax the rule to `PI_TEXT*` and make `Xml.ProcessingInstruction#processingInstructions`
nullable to model the absent data. The printer, `XmlVisitor`, and the RPC
sender/receiver already route that child through null-tolerant calls, so the
round trip stays byte for byte.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's changed?
XMLParser.g4spelled the processing instruction rule asSPECIAL_OPEN PI_TEXT+ SPECIAL_CLOSE, requiring at least one data token. The XML specification makes a processing instruction's data optional:The rule is now
PI_TEXT*, andXml.ProcessingInstruction#processingInstructionsis nullable to model the absent data.What's your motivation?
<?target?>and<?target ?>are well formed, but both failed to parse:This is a parse failure rather than a recipe failure, so the blast radius is the whole file: the document never becomes an
Xml.Document, and no recipe can run on it. A single data-less processing instruction anywhere in a POM or config file takes the entire file out of scope.Existing coverage did not catch it because every processing instruction fixture in the suite carries data (
<?xml-stylesheet href="mystyle.css" type="text/css"?>,<?m2e execute onConfiguration,onIncremental?>).Anything in particular you'd like reviewers to focus on?
The nullable LST field. I checked every consumer of that child before changing it; all four already route through null-tolerant calls, so nothing needed adapting:
XmlPrintervisit(pi.getProcessingInstructions(), p)TreeVisitor.visit(@Nullable Tree, P)XmlVisitorvisitAndCast(pi.getProcessingInstructions(), p)visitAndCast(@Nullable Tree, P)XmlSenderq.getAndSend(pi, ...)Function<T, @Nullable U>; skipsonChangewhenafter == nullXmlReceiverq.receive(pi.getProcessingInstructions(), ...)receive(@Nullable T before, ...);onChangeonly on ADD/CHANGENo other code reads the field.
TabsAndIndentsVisitor,CountLinesVisitor,XPathMatcher,XmlWhitespaceValidationService, andMavenIsoVisitorreferenceXml.ProcessingInstructionbut only its prefix or its type.XmlSendReceiveTest.sendReceiveProcessingInstructionWithoutDatacovers the RPC path specifically because of this change.Guard strength. Reverting only the grammar hunk while keeping the Java changes turns exactly the three new tests red and leaves the other 298 in
rewrite-xmlgreen, so the tests are guarding the grammar and not something incidental.Have you considered any alternatives or workarounds?
Synthesizing an empty
Xml.CharDatainstead ofnullwould have avoided touching the model, but it puts a node in the LST that has no counterpart in the source.Xml.Tag#closingandXml.Prolog#xmlDeclare already nullable for optional children, sonullfollows the existing convention.Any additional context
Two adjacent things I found while verifying, both left out of this PR because they are independent of it:
token recognition error at: '<?'. This is a lexer mode gap rather than the parser rule changed here: I confirmed it by control, and<?m2e execute onConfiguration?>(data present) fails identically on unpatchedmain.markupdeclpermitsprocessinginstruction, but noINSIDE_DTDrule ever emitsSPECIAL_OPEN.LineBreaksVisitor.visitProcessingInstructionunconditionally prepends a newline, so<exclude>a</exclude> <?why-a?>is reflowed onto its own line. This is the same shape as the comment case fixed in Keep XML comments trailing an element on the same line #8353, which added anisTrailingCommentguard tovisitCommentbut leftvisitProcessingInstructionuntouched. It affects processing instructions that already parsed today, so it is not a consequence of this change.I did not regenerate
rewrite-csharp/csharp/OpenRewrite/Xml/Grammar, which is generated from this same.g4bygenerateAntlrCSharpSources. Those checked-in files are already stale relative tomain: regenerating them from an unmodified grammar produces 602 insertions and 545 deletions across 10 files, so including them would bury this change. Happy to do it in a separate PR if you want them resynced.Verification
Local, JDK 21:
:rewrite-xml:test:rewrite-maven:test@Disabled):rewrite-xml:licensegenerateAntlrSources:rewrite-xml:recipeCsvValidateCompletenessfails on this branch, but it fails identically on unmodifiedmain(org.openrewrite.xml.CreateXmlFileandorg.openrewrite.xml.style.AutodetectDebuglisted inrecipes.csvbut absent from the environment). Unrelated to this change.Checklist