Skip to content

Returns errors as data from parsing and XPath - #36

Merged
redvers merged 2 commits into
mainfrom
feature/error-handling-redesign
May 22, 2026
Merged

Returns errors as data from parsing and XPath#36
redvers merged 2 commits into
mainfrom
feature/error-handling-redesign

Conversation

@redvers

@redvers redvers commented May 22, 2026

Copy link
Copy Markdown
Collaborator

Closes #17 and addresses review H10. Replaces the partial-function error-signalling on the parsing and XPath surface with union returns that carry an Xml2Error value alongside the success type.

API changes

Removed:

  • Xml2Doc.parseDoc(...)?
  • Xml2Doc.parseFile(...)?

Added:

  • Xml2Parser.parseDoc(xml, options): (Xml2Doc | Xml2Error)
  • Xml2Parser.parseFile(auth, path, options): (Xml2Doc | Xml2Error)
  • Xml2Doc._from_ptr(...): package-private constructor used by Xml2Parser

Changed signatures (now return union with Xml2Error):

  • Xml2Doc.xpathEvalNodes / String / F64 / Bool
  • Xml2Node.xpathEvalNodes / String / F64 / Bool

Xml2XPathResult gains an Xml2Error variant; empty nodesets now return an empty Array[Xml2Node] rather than None (closes review M2).

Xml2Error refactor

class val with let fields throughout; safe to share across actors.

  • domain: Xml2ErrorDomain (typed primitive union over libxml2's ~31 documented domains plus an Unknown fall-through)
  • level: Xml2ErrorLevel (None / Warning / Error / Fatal / Unknown)
  • from_last_error()?: partial constructor reading libxml2's per-thread state; raises when no error is current rather than fabricating one
  • _synthetic(...): package-private constructor for the impossible case where libxml2 reports a parse failure but xmlGetLastError returns null; lets the public union return types stay total
  • string(): human-readable rendering for logging

Xml2Parser captures errors eagerly on the same Pony behaviour as the parse call, so callers receiving the union don't have to worry about libxml2's thread-local last-error state surviving actor migration.

Tests

TestParseError now asserts on the error fields directly: the prior "non-deterministic" caveat is gone because Xml2Parser delivers the error of this parse, not whatever happens to be in TLS later.

Added:

  • xml2doc/parse-error (rewritten with real assertions)
  • xml2error/string-rendering
  • xml2error/domain-mapping
  • xml2error/level-mapping

All other tests migrated mechanically to the new APIs:
Xml2Doc.parseDoc(...)? -> Xml2Parser.parseDoc(...) as Xml2Doc
doc.xpathEvalNodes(q)? -> doc.xpathEvalNodes(q) as Array[Xml2Node]
etc.

README example updated to the new union shape.

Counterfactual: replacing _capture_error to always synthesise (i.e. ignore libxml2 last-error) fails parse-error and string-rendering tests, confirming the rich-error path is on the hot path and not silently shadowed by the synthetic fallback.

61/61 tests pass.

Deferred to follow-up

  • xmlSetStructuredErrorFunc callback installation (per-thread buffer + module-init timing; overlaps review H5)
  • xmlXPathOrderDocElems / xmlXPathTreeOrder (the secondary item on issue 6. Performance and error handling hooks #17)

redvers added 2 commits May 21, 2026 23:50
Closes #17 and addresses review H10. Replaces the partial-function
error-signalling on the parsing and XPath surface with union returns
that carry an Xml2Error value alongside the success type.

API changes
-----------

Removed:
  - Xml2Doc.parseDoc(...)?
  - Xml2Doc.parseFile(...)?

Added:
  - Xml2Parser.parseDoc(xml, options): (Xml2Doc | Xml2Error)
  - Xml2Parser.parseFile(auth, path, options): (Xml2Doc | Xml2Error)
  - Xml2Doc._from_ptr(...): package-private constructor used by
    Xml2Parser

Changed signatures (now return union with Xml2Error):
  - Xml2Doc.xpathEvalNodes / String / F64 / Bool
  - Xml2Node.xpathEvalNodes / String / F64 / Bool

Xml2XPathResult gains an Xml2Error variant; empty nodesets now
return an empty Array[Xml2Node] rather than None (closes review M2).

Xml2Error refactor
------------------

class val with let fields throughout; safe to share across actors.

  - domain: Xml2ErrorDomain (typed primitive union over libxml2's
    ~31 documented domains plus an Unknown fall-through)
  - level: Xml2ErrorLevel (None / Warning / Error / Fatal /
    Unknown)
  - from_last_error()?: partial constructor reading libxml2's
    per-thread state; raises when no error is current rather than
    fabricating one
  - _synthetic(...): package-private constructor for the
    impossible case where libxml2 reports a parse failure but
    xmlGetLastError returns null; lets the public union return
    types stay total
  - string(): human-readable rendering for logging

Xml2Parser captures errors eagerly on the same Pony behaviour as
the parse call, so callers receiving the union don't have to worry
about libxml2's thread-local last-error state surviving actor
migration.

Tests
-----

TestParseError now asserts on the error fields directly: the prior
"non-deterministic" caveat is gone because Xml2Parser delivers the
error of *this* parse, not whatever happens to be in TLS later.

Added:
  - xml2doc/parse-error (rewritten with real assertions)
  - xml2error/string-rendering
  - xml2error/domain-mapping
  - xml2error/level-mapping

All other tests migrated mechanically to the new APIs:
  Xml2Doc.parseDoc(...)? -> Xml2Parser.parseDoc(...) as Xml2Doc
  doc.xpathEvalNodes(q)? -> doc.xpathEvalNodes(q) as Array[Xml2Node]
  etc.

README example updated to the new union shape.

Counterfactual: replacing _capture_error to always synthesise (i.e.
ignore libxml2 last-error) fails parse-error and string-rendering
tests, confirming the rich-error path is on the hot path and not
silently shadowed by the synthetic fallback.

61/61 tests pass.

Deferred to follow-up
---------------------

  - xmlSetStructuredErrorFunc callback installation (per-thread
    buffer + module-init timing; overlaps review H5)
  - xmlXPathOrderDocElems / xmlXPathTreeOrder (the secondary item
    on issue #17)
@redvers
redvers merged commit ae532e3 into main May 22, 2026
8 checks passed
@redvers
redvers deleted the feature/error-handling-redesign branch May 22, 2026 03:55
github-actions Bot pushed a commit that referenced this pull request May 22, 2026
github-actions Bot pushed a commit that referenced this pull request May 22, 2026
@redvers redvers mentioned this pull request May 22, 2026
redvers added a commit that referenced this pull request May 22, 2026
The original FuzzParseDoc property was removed when crash-resistance
fuzz tests were added (PR #32) because feeding arbitrary byte strings
to Xml2Doc.parseDoc triggered cumulative state corruption inside
libxml2 - a segfault or "free(): invalid pointer" abort after ~5-10
distinct failed parses. The crash was cumulative across inputs but
did not reproduce when the same input was reparsed in a loop,
suggesting state buildup in libxml2's internal error context across
failed parses.

PR #36 (errors-as-data refactor) routes parse failures through
Xml2Error.from_last_error, which calls xmlResetError after reading
the per-thread last-error. This appears to clear the state buildup
that previously caused the corruption.

Re-enabling the property with the default 100-sample budget. Five
consecutive `make test` runs (500 distinct failed-parse attempts)
complete without incident, well past the original ~5-10 crash
threshold. The crash-resistance contract is back in force for the
parse path.

No release note: test-only change, follows PR #32 / #22 precedent
of not generating CHANGELOG entries for test additions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6. Performance and error handling hooks

1 participant