Skip to content

Commit 731ac28

Browse files
committed
docs(xml): document and pin the processing-instruction position limit (#2202)
Rule-14 sweep. The ticket's recorded gate named "/rv is ABSENT" and "consumers were not inspected", both stale -- but neither was ever the real gate. The real one is the vendored substrate, and it stands. What was actually outstanding is this ticket's OWN acceptance criteria, whose fallback path needs no approval and had simply never been executed: "if the substrate cannot express it, the correct outcome is an explicit documented limitation on XProcessingInstruction and XmlWriter::WriteProcessingInstruction plus a pin, not silence." No production statement changed. This is a declaration and its evidence, the shape of #2015 and #2324. This runtime writes a PI correctly in any position and reads one back only before every other node -- an XML declaration may precede it and nothing else may, not even a comment -- so SR-AUD-349's closure property does not hold for this one node kind. The cause is re-measured directly rather than asserted: called with no port code involved, vendor/tinyxml2 produces exactly the port's five verdicts, and the leading "<?p d?>" comes back as a Declaration node valued "p d". tinyxml2 has no processing-instruction node type at all, so every "<?" inherits the rule that a declaration may appear only before anything else. The fifth row is new evidence this ticket adds: a declaration MAY precede the PI, so the rule is "before every other node" rather than "first token", and that is now pinned. .NET differs by exactly that model distinction -- XmlLoader.cs:203-209 switches on XmlDeclaration and ProcessingInstruction as separate cases in the same general node loop, which runs for element content too. Both repair routes are refused, the second now priced by measurement rather than judgement: patching vendor/ is forbidden, and pre-rewriting non-leading "<?...?>" around the substrate forks its semantics for every document -- mutation M3 implements a naive version and it breaks the shipped XPathSelectTests.ProcessingInstructionNodeTest_SelectsPI. Three mutations, all caught, two of them also by pre-existing tests. Pinned at both layers as the criteria require; the asymmetry pin is written to fail the moment the limitation lifts, naming both doc-comments. Gate: 17,424 run, 17,424 passed, 0 failed, 0 skipped across 38 executables (+3 on 17,421; SharpRuntimeTests_Xml 515 -> 518; no other executable moved).
1 parent 3b86d95 commit 731ac28

6 files changed

Lines changed: 218 additions & 1 deletion

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Declaration — a processing instruction can be read back only before every other node (ticket #2202)
5+
6+
*2026-08-19.* This runtime writes a processing instruction correctly in any position and can
7+
parse one back **only at the very start of the document**. That asymmetry is now documented at
8+
both doors and pinned at both layers.
9+
10+
**This ticket changed no production statement.** It is a declaration and its evidence, the same
11+
shape as #2015 and #2324. Nothing you can write, read or emit behaves differently.
12+
13+
---
14+
15+
## 1. The limitation
16+
17+
| Document | Loads? |
18+
|---|---|
19+
| `<?p d?><root/>` | **yes** |
20+
| `<?xml version="1.0"?><?p d?><root/>` | **yes** — an XML declaration may precede it |
21+
| `<root><?p d?></root>` | no — `XML_ERROR_PARSING_DECLARATION` |
22+
| `<root/><?p d?>` | no |
23+
| `<!--c--><?p d?><root/>` | no — not even a comment may precede it |
24+
25+
Both writer families — `System::Xml::XmlWriter::WriteProcessingInstruction` and
26+
`System::Xml::Linq::XProcessingInstruction` — emit the instruction in **every** one of those
27+
positions, and the text they emit is well-formed XML. So SR-AUD-349's closure property (whatever
28+
this runtime writes, this runtime must be able to read) does **not** hold for this one node kind.
29+
30+
## 2. The cause is the substrate's node-type model
31+
32+
Not a check in the wrong place, and not a defect in the port. Measured directly against
33+
`vendor/tinyxml2`, with no port code involved
34+
(`build-probe/2202_probe1_tinyxml2_direct.cpp`):
35+
36+
```
37+
<?p d?><root/> -> OK
38+
<root><?p d?></root> -> XML_ERROR_PARSING_DECLARATION
39+
<root/><?p d?> -> XML_ERROR_PARSING_DECLARATION
40+
<!--c--><?p d?><root/> -> XML_ERROR_PARSING_DECLARATION
41+
<?xml version="1.0"?><?p d?><root/> -> OK
42+
first node of '<?p d?><root/>': Declaration=1 Unknown=0 Element=0 value=p d
43+
```
44+
45+
Exactly the verdicts the port produces, and the last line is the reason: **tinyxml2 has no
46+
processing-instruction node type at all.** Every `<?` becomes an `XMLDeclaration` — the leading
47+
`<?p d?>` is a *Declaration* whose value is the string `p d` — so ordinary PIs inherit tinyxml2's
48+
rule that a declaration is allowed only at document level and before anything else.
49+
50+
The port therefore neither loses nor rebases position information. There is no local projection
51+
bug to repair.
52+
53+
## 3. .NET has no such limitation, and the difference is the same model distinction
54+
55+
`XmlLoader.cs:203-209` switches on `XmlNodeType.XmlDeclaration` and
56+
`XmlNodeType.ProcessingInstruction` as **separate cases in the same general node loop** — a loop
57+
that runs for element content, not only for the prolog. Two node types, two rules. tinyxml2 has
58+
one node type and therefore one rule.
59+
60+
## 4. Why it is documented rather than repaired
61+
62+
Two routes exist and both are refused:
63+
64+
1. **Patch `vendor/tinyxml2` to add a PI node type.** `vendor/` is third-party source, kept
65+
unmodified from upstream, and CLAUDE.md forbids editing it.
66+
2. **Pre-rewrite non-leading `<?...?>` before handing text to the substrate and map it back.**
67+
This forks the substrate's parsing semantics for every document that passes through
68+
`LoadXml`, to serve one node kind. Measured as mutation M3 (§5), a naive version of exactly
69+
this shim breaks `XPathSelectTests.ProcessingInstructionNodeTest_SelectsPI` — shipped,
70+
working functionality — which is what a semantics fork costs in practice.
71+
72+
The ticket's own acceptance criteria prescribe this outcome: *"if the substrate cannot express
73+
it, the correct outcome is an explicit documented limitation on `XProcessingInstruction` and
74+
`XmlWriter::WriteProcessingInstruction` plus a pin, not silence."* The substrate cannot express
75+
it, and that had been confirmed; the documentation and the second pin were simply never written.
76+
77+
## 5. Evidence
78+
79+
Three mutations, **all caught**:
80+
81+
| Mutation | Caught by |
82+
|---|---|
83+
| M1 — the writer silently drops a PI written inside an element | `Decl2202_TheWriterEmitsWhatTheParserCannotReadBack` |
84+
| M2 — `LoadXml` swallows the substrate's parse error | both new pins **and four pre-existing tests** |
85+
| M3 — strip non-leading PIs before parsing (the realistic future "repair") | both new pins **and `XPathSelectTests.ProcessingInstructionNodeTest_SelectsPI`** |
86+
87+
Pins, at both layers as the acceptance criteria require:
88+
89+
* `System::Xml``XmlWriterValidationTests.Decl2202_*` (three cases, **new**: the four verdicts,
90+
the declaration exception, and the write-then-fail-to-read asymmetry end to end).
91+
* `System::Xml::Linq`
92+
`XLinqLexicalSerializationTests.ProcessingInstruction_ParserPositionLimitIsSubstrateNotSerialization`
93+
(pre-existing).
94+
95+
Doc-comments: `XmlWriter::WriteProcessingInstruction` and `XProcessingInstruction`, both stating
96+
the limitation, its cause, and that .NET does not share it.
97+
98+
Gate: **17,424 run, 17,424 passed, 0 failed, 0 skipped** across 38 executables — `+3` on 17,421,
99+
exactly the three new cases (`SharpRuntimeTests_Xml` 515 → 518). No other executable moved.
100+
101+
## 6. What would reopen this
102+
103+
A substrate with a real processing-instruction node type, or an upstream tinyxml2 that grows one.
104+
The asymmetry pin is written so it fails the moment the limitation lifts:
105+
106+
> *"if this stops throwing, #2202's limitation is gone and the note on
107+
> `WriteProcessingInstruction` and `XProcessingInstruction` must be removed"*
108+
109+
## 7. Downstream
110+
111+
No behaviour changed, so nothing downstream can break. Neither `cna` nor `mobile-eggbert` was
112+
modified.

modules/xml-linq/include/System/Xml/Linq/XProcessingInstruction.hpp

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,25 @@ namespace System::Xml::Linq {
2626
* matching real .NET's `XmlEncodedRawTextWriter.WriteCommentOrPi`. Before #2196 the direct
2727
* door emitted the raw `?>`, which closed the instruction early and made the resulting text
2828
* unparseable — this node kind is the one whose corruption was not silent.
29+
*
30+
* @note **This runtime cannot parse back a processing instruction placed anywhere except
31+
* before every other node.** Serialization is correct in every position and the text is
32+
* well-formed XML, but `XDocument::Parse` accepts a PI only at the very start of the
33+
* document — an XML declaration may precede it, and nothing else may, not even a comment.
34+
* `<root><?p d?></root>`, `<root/><?p d?>` and `<!--c--><?p d?><root/>` all throw.
35+
*
36+
* The cause is the vendored substrate's node-type model rather than anything in this port:
37+
* `vendor/tinyxml2` has no processing-instruction type, so every `<?` becomes an XML
38+
* *declaration* and inherits the rule that a declaration may appear only at document level
39+
* and before anything else. `vendor/` is third-party source and is never edited.
40+
*
41+
* **.NET has no such limitation** — its loader handles `XmlNodeType.XmlDeclaration` and
42+
* `XmlNodeType.ProcessingInstruction` as separate cases in the same general node loop
43+
* (`XmlLoader.cs:203-209`), which runs for element content too.
44+
*
45+
* Ticket **#2202**; pinned by
46+
* `XLinqLexicalSerializationTests.ProcessingInstruction_ParserPositionLimitIsSubstrateNotSerialization`
47+
* and, at the `System::Xml` layer, by `XmlWriterValidationTests.Decl2202_*`.
2948
*/
3049
class XProcessingInstruction : public XNode {
3150
std::string target_;

modules/xml/include/System/Xml/XmlWriter.hpp

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,31 @@ namespace System::Xml {
139139
* @throws System::Xml::XmlException if @p target is not a valid XML name.
140140
* @throws System::ArgumentException if @p target is empty.
141141
* @throws System::InvalidOperationException if @c Close() has already been called.
142+
*
143+
* @note **This runtime cannot read back a processing instruction it writes anywhere
144+
* except before every other node.** The instruction is emitted correctly in any
145+
* position and is well-formed XML, but this runtime's own parser accepts one only at
146+
* the very start of the document — an XML declaration may precede it, and nothing
147+
* else may, not even a comment. `<root><?p d?></root>`, `<root/><?p d?>` and
148+
* `<!--c--><?p d?><root/>` all fail to load. So SR-AUD-349's closure property —
149+
* whatever this writer emits, this module's reader must consume — does **not** hold
150+
* for this one node kind.
151+
*
152+
* This is a limitation of the vendored substrate's node-type model, not a check in
153+
* the wrong place: `vendor/tinyxml2` has no processing-instruction type at all, so
154+
* every `<?` becomes an XML *declaration* and inherits the rule that a declaration is
155+
* allowed only at document level and before anything else. `vendor/` is third-party
156+
* source and is never edited, and pre-rewriting non-leading `<?...?>` around the
157+
* substrate would fork its semantics for every document.
158+
*
159+
* **.NET has no such limitation**, because its model separates the two: its loader
160+
* switches on `XmlNodeType.XmlDeclaration` and `XmlNodeType.ProcessingInstruction` as
161+
* distinct cases in the same general node loop (`XmlLoader.cs:203-209`), which runs
162+
* for element content and not only for the prolog.
163+
*
164+
* Ticket **#2202**; pinned by `XmlWriterValidationTests.Decl2202_*` and, at the
165+
* Xml.Linq layer, by
166+
* `XLinqLexicalSerializationTests.ProcessingInstruction_ParserPositionLimitIsSubstrateNotSerialization`.
142167
*/
143168
void WriteProcessingInstruction(const std::string& target, const std::string& data);
144169

modules/xml/tests/System/Xml/XmlWriterValidationTests.cpp

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -784,3 +784,64 @@ TEST(XmlWriterValidationTests, Fix2348_TheCheckIsQuoteAware) {
784784
System::Xml::XmlException);
785785
}
786786
}
787+
788+
// ===========================================================================================
789+
// #2202 — the parser accepts a processing instruction ONLY before every other node.
790+
//
791+
// This is a DOCUMENTED LIMITATION, not a repair, and it is pinned at this layer because the
792+
// ticket's acceptance criteria ask for it "at both layers" and only the Xml.Linq layer had it
793+
// (XLinqLexicalSerializationTests.ProcessingInstruction_ParserPositionLimitIsSubstrateNotSerialization).
794+
//
795+
// THE ASYMMETRY: both of this runtime's writer families emit a PI in any position, and this
796+
// runtime's own parser can then read back only the leading one. SR-AUD-349's closure property
797+
// -- whatever the writer emits, the reader must consume -- does not hold for this node kind.
798+
//
799+
// THE CAUSE IS THE SUBSTRATE'S NODE-TYPE MODEL, not a misplaced check and not a defect in the
800+
// port. vendor/tinyxml2 has NO processing-instruction node type at all: every "<?" becomes an
801+
// XMLDeclaration, so tinyxml2's rule that a declaration is allowed only at document level and
802+
// before anything else catches ordinary PIs too. Called directly, with no port code involved,
803+
// tinyxml2 produces exactly the four verdicts below. vendor/ is third-party source and is never
804+
// edited (CLAUDE.md), and the alternative -- rewriting non-leading "<?...?>" before handing text
805+
// to the substrate and mapping it back -- forks the substrate's semantics for every document.
806+
//
807+
// .NET HAS NO SUCH LIMITATION, and the reason is precisely the model difference: its loader
808+
// switches on XmlNodeType.XmlDeclaration and XmlNodeType.ProcessingInstruction as SEPARATE
809+
// cases in the same general node loop (XmlLoader.cs:203-209), which runs for element content,
810+
// not only the prolog.
811+
// ===========================================================================================
812+
813+
TEST(XmlWriterValidationTests, Decl2202_TheParserAcceptsAPiOnlyBeforeEveryOtherNode) {
814+
const auto parses = [](const std::string& xml) {
815+
System::Xml::XmlDocument doc;
816+
doc.LoadXml(xml);
817+
};
818+
EXPECT_NO_THROW(parses("<?p d?><root/>"));
819+
EXPECT_THROW(parses("<root><?p d?></root>"), XmlException);
820+
EXPECT_THROW(parses("<root/><?p d?>"), XmlException);
821+
// Not even a comment may precede it -- which is what shows the rule is "before every other
822+
// node" rather than "outside the root element".
823+
EXPECT_THROW(parses("<!--c--><?p d?><root/>"), XmlException);
824+
}
825+
826+
TEST(XmlWriterValidationTests, Decl2202_AnXmlDeclarationMayStillPrecedeThePi) {
827+
// The one node that may come first, because the substrate's single "<?" node type is the
828+
// declaration. Recorded so the rule above is not read as stricter than it is.
829+
System::Xml::XmlDocument doc;
830+
EXPECT_NO_THROW(doc.LoadXml("<?xml version=\"1.0\"?><?p d?><root/>"));
831+
}
832+
833+
TEST(XmlWriterValidationTests, Decl2202_TheWriterEmitsWhatTheParserCannotReadBack) {
834+
// The asymmetry itself, asserted end to end rather than inferred from the two halves.
835+
auto w = NewWriter();
836+
w->WriteStartElement("root");
837+
ASSERT_NO_THROW(w->WriteProcessingInstruction("p", "d"));
838+
w->WriteEndElement();
839+
const std::string emitted = w->ToString();
840+
EXPECT_NE(emitted.find("<?p d?>"), std::string::npos)
841+
<< "the writer must still emit the instruction: " << emitted;
842+
843+
System::Xml::XmlDocument doc;
844+
EXPECT_THROW(doc.LoadXml(emitted), XmlException)
845+
<< "if this stops throwing, #2202's limitation is gone and the note on "
846+
"WriteProcessingInstruction and XProcessingInstruction must be removed";
847+
}

plan.sqlite3

4 KB
Binary file not shown.

0 commit comments

Comments
 (0)