andodeki
82eb6b9c73
ADR 0017 left destinations.rs at 0% coverage as an open item. The obvious
reading is "an untested module". The real one is worse: nothing called it.
It was pub use'd from lib.rs and referenced from nowhere else in the
workspace. 0% was not a gap in the tests, it was the symptom of dead code,
and nothing else was doing the job.
Meanwhile PdfAnnotation read a link's target as
dict.get_name("Dest") - a *name* /Dest and nothing else. Not
/Dest [4 0 R /Fit], and not /A << /S /GoTo /D ... >>, which is how internal
links are written in practically every real document.
The corpus has had one since Phase 6, in annotations/links.pdf, and no test
asserted where it went:
Link { uri: None, dest: None } -> action=None
Clicking it did nothing. No error, no warning - the viewer got no action and
correctly performed none. A link to nowhere and a link the reader cannot
parse look identical from outside. The viewer was already wired for this:
PdfAction::GoToPage exists, is matched in test_host.rs, and was never
constructed by anything. A complete delivery path with nothing at the source.
Now: all three legal spellings parse, named destinations resolve through the
/Names /Dests tree *and* the pre-1.2 /Root /Dests dictionary, and resolution
happens in page_annotations where the catalogue is in reach.
XYZ keeps Option per component because null is meaningful there and only
there - it means "leave unchanged". Reading it as 0.0 scrolls to the origin
at 0% magnification. Zoom 0 means the same as null and is normalised.
Lookup uses a deliberate shallow resolve. Deep-resolving a destination array
replaces [4 0 R /Fit] with the page dictionary and destroys the only thing
identifying the target - the defect that once emptied every AcroForm
(ADR 0006) and every annotation reference (ADR 0004).
GoToAction now requires /S to be GoTo. The old code ignored /S and took /D
from whatever it was handed, so a /GoToR (another file), /Launch (a program)
or /JavaScript carrying a /D was reported as a local page jump. Refuse by
verb, same policy as ADR 0012. An unresolvable destination is left
unresolved, never defaulted to page 0: silently landing on page one is the
worst outcome because it looks like the link worked.
Seven mutations, all killed. M1 - removing the /S check - reported as
surviving on the first attempt. It had not survived: the patch string
omitted an interleaved comment so the mutation never applied and I measured
the unmutated build. A harness that does not verify its own mutation says
"weak test" when the truth is "never ran", and the conclusion would have
been to delete a real security check. Every mutation now asserts it applied.
destinations.rs 0% -> 98.65%; total 83.42% -> 83.86%. Floors added for
destinations.rs and annotations.rs, verified to fail when breached.
AnnotationType::Link changes shape (dest: Option<String> ->
destination: Option<Destination>) and AnnotationAction gains
GoToDestination; the old field could not express an explicit destination, so
keeping it meant keeping the bug. AnnotationAction loses Eq because a
destination carries f64 coordinates.
pdf: 724 passed (was 695). pdf-ui: 769 passed (was 725). ADR 0018.
202 lines
9 KiB
Markdown
202 lines
9 KiB
Markdown
# ADR 0018: destinations — the module at 0% coverage was dead, and every internal link with it
|
|
|
|
- **Status:** Accepted
|
|
- **Date:** 2026-08-16
|
|
- **Review item:** the open item left by ADR 0017, *"`destinations.rs`
|
|
remains at 0% coverage and has no floor; it needs tests before it needs a
|
|
floor"*
|
|
- **Supersedes:** nothing
|
|
- **Related:** ADR 0017 (declared-versus-delivered; this is the same class of
|
|
defect, found by following its open item), ADR 0012 (refusing dangerous
|
|
actions rather than performing them), ADR 0004 / ADR 0006 (deep-resolve
|
|
destroying the references that identify things)
|
|
|
|
## Context
|
|
|
|
ADR 0017 recorded `destinations.rs` at 0% line coverage. The obvious reading
|
|
is "an untested module". The real one is worse.
|
|
|
|
**Nothing called it.** The module was `pub use`d from `lib.rs` and referenced
|
|
from nowhere else in the workspace:
|
|
|
|
```
|
|
$ grep -rn 'destinations::|GoToAction|DestinationKind' --include='*.rs' .
|
|
./pdf-document/src/lib.rs:23:pub use destinations::{Destination, GoToAction};
|
|
```
|
|
|
|
0% coverage was not a gap in the tests. It was the *symptom* of dead code,
|
|
and the thing it was supposed to do was not being done by anything else.
|
|
|
|
### Internal links did nothing
|
|
|
|
`PdfAnnotation::from_dict` read a link's target as:
|
|
|
|
```rust
|
|
dest: dict.get_name("Dest").map(|s| s.to_string()),
|
|
```
|
|
|
|
A **name** `/Dest`, and nothing else. Not `/Dest [4 0 R /Fit]` (an explicit
|
|
array), and not `/A << /S /GoTo /D ... >>` — which is how internal links are
|
|
written in practically every real document.
|
|
|
|
The corpus already contained one, in `annotations/links.pdf`, and had since
|
|
Phase 6:
|
|
|
|
```
|
|
11: << /Type /Annot /Subtype /Link /Rect [72 660 272 680]
|
|
/P 3 0 R /A << /S /GoTo /D [4 0 R /Fit] >> >>
|
|
```
|
|
|
|
Probing it before any change:
|
|
|
|
```
|
|
Link { uri: Some("https://example.org/docs"), dest: None } -> OpenUri(...)
|
|
Link { uri: None, dest: None } -> action=None
|
|
```
|
|
|
|
`action=None`. Clicking that link did nothing. No error, no warning — the
|
|
viewer received no action and correctly performed none. A link to nowhere
|
|
and a link the reader cannot parse are indistinguishable from the outside,
|
|
which is exactly the failure mode ADR 0017 is about, one layer up.
|
|
|
|
The viewer was *already wired* for this: `PdfAction::GoToPage` exists in
|
|
`interaction.rs`, is matched in `test_host.rs`, and was **never constructed
|
|
by anything**. A complete delivery path with nothing at the source end.
|
|
|
|
### Why no test caught it
|
|
|
|
The corpus had one internal link and no test asserted where it went.
|
|
`annotation_roundtrip.rs` and `corpus.rs` checked that link annotations
|
|
*parsed* and that their rectangles were right. Both passed. The assertion
|
|
that was missing is the one that names a page number.
|
|
|
|
## Decision
|
|
|
|
### Make the module real, and call it
|
|
|
|
`destinations.rs` now parses all three forms a destination is legally
|
|
written in (§12.3.2): an explicit array, a name, a byte string, and the
|
|
`/D`-wrapped dictionary a name tree leaf uses. `Destination::from_object`
|
|
accepts any of them.
|
|
|
|
`XYZ` carries `Option<f64>` per component, because **null is meaningful and
|
|
only meaningful there**: it means "leave this unchanged". Collapsing null to
|
|
`0.0` is not a harmless default — it scrolls to the origin, and for `zoom`
|
|
it requests 0% magnification. A zoom of literal `0` means the same as null
|
|
(Table 151) and is normalised to `None`, so a viewer cannot render at zero.
|
|
|
|
An unrecognised fit degrades to `/Fit` rather than failing: losing the
|
|
framing is recoverable, losing the page is not.
|
|
|
|
### Resolve names where the catalogue is in reach
|
|
|
|
`PdfDocument` gained `lookup_named_destination`, `resolve_destination` and
|
|
`named_destinations`. Both spellings are searched, because both are current:
|
|
the `/Names /Dests` **name tree** (§7.9.6, walked through `/Kids`) and the
|
|
pre-1.2 `/Root /Dests` **dictionary**. Supporting only the modern one loses
|
|
every link in an older file.
|
|
|
|
Lookup uses a deliberate `resolve_shallow`, not `resolve`. Deep-resolving a
|
|
destination array replaces `[4 0 R /Fit]` with the page *dictionary* and
|
|
destroys the only thing identifying the target — the same defect that once
|
|
emptied every AcroForm (ADR 0006) and every annotation reference (ADR 0004).
|
|
Mutation M8 below confirms this is load-bearing, not superstition.
|
|
|
|
Destinations are resolved in `page_annotations`, so annotations reach the
|
|
viewer already carrying a page index. An unresolvable one is **left
|
|
unresolved**, never defaulted to page 0: silently landing on the first page
|
|
is the worst outcome, because it looks like the link worked.
|
|
|
|
### Refuse by verb
|
|
|
|
`GoToAction::from_dict` requires `/S` to be `GoTo`. The previous version
|
|
ignored `/S` entirely and read `/D` from whatever it was given, so a
|
|
`/GoToR` (another *file*), a `/Launch` (a program) or a `/JavaScript`
|
|
carrying a `/D` was reported as an in-document page jump. Same policy as ADR
|
|
0012: refuse by name, do not silently perform.
|
|
|
|
## Verification
|
|
|
|
Every claim checked by running it.
|
|
|
|
**Before:** `action=None` for the corpus's only internal link.
|
|
**After:** `action=Some(GoToPage { page_index: 1 })`.
|
|
|
|
**Mutation testing — seven mutations, all killed:**
|
|
|
|
| # | Mutation | Result |
|
|
|---|---|---|
|
|
| M1 | `/S` verb check removed | **1 fails** — `/Launch` became `GoToPage { page_index: 1 }` |
|
|
| M2 | null in `XYZ` read as `0.0` | **1 fails** |
|
|
| M3 | zoom `0` not normalised | **1 fails** |
|
|
| M4 | name tree `/Kids` not descended | **2 fail** |
|
|
| M5 | legacy `/Root /Dests` fallback removed | **1 fails** |
|
|
| M6 | unresolvable name defaults to page 0 | **1 fails** |
|
|
| M7 | `/Dest` on the annotation ignored | **4 fail** |
|
|
| M8 | `resolve_shallow` → deep `resolve` | **5 fail** |
|
|
|
|
**M1 was a false negative on the first attempt, and that is worth
|
|
recording.** The first run reported it surviving. It had not survived — the
|
|
Python replacement string omitted an interleaved comment, so the patch never
|
|
applied and I measured the *unmutated* build. Re-run with an `assert old in
|
|
s` guard, it failed as it should. A mutation harness that does not verify
|
|
the mutation was applied reports "test is weak" when the truth is "the test
|
|
never ran against changed code", and the conclusion — deleting a real
|
|
security check as untested — would have been actively harmful. Every
|
|
mutation here now asserts its own application.
|
|
|
|
**Coverage:** `destinations.rs` **0% → 98.65%**; total 83.42% → **83.86%**.
|
|
Floors added for `destinations.rs` (94) and `annotations.rs` (70), and the
|
|
new floor verified to fail when raised above measurement.
|
|
|
|
**Suite:** `TEST_TARGET=pdf ./tools/test-rust-clean.sh` → **724 passed, 0
|
|
failed** (was 695; +29).
|
|
|
|
## Merge criteria
|
|
|
|
- [x] `destinations.rs` is called by production code, not only exported
|
|
- [x] Explicit array, name, string and `/D`-wrapped forms all parse
|
|
- [x] All eight fit kinds parse to distinct values, asserted individually
|
|
- [x] `null` in `XYZ` reads as "unchanged", not `0.0`
|
|
- [x] Zoom `0` normalised to "unchanged"; coordinate `0` kept as `0.0`
|
|
- [x] Named destinations resolved through a nested `/Names /Dests` tree
|
|
- [x] Legacy `/Root /Dests` dictionary resolved
|
|
- [x] `/GoToR`, `/Launch`, `/JavaScript` refused by verb even when carrying
|
|
a `/D`
|
|
- [x] An unresolvable destination never becomes page 0
|
|
- [x] A cyclic name tree terminates
|
|
- [x] Corpus-wide invariant: no link resolves past the end of its document
|
|
- [x] Five `destinations/` fixtures generated by `generate.py`
|
|
- [x] Seven mutations, each verified to apply and each killed
|
|
- [x] Coverage floor added for `destinations.rs`, verified to fail when
|
|
breached
|
|
- [x] `TEST_TARGET=pdf ./tools/test-rust-clean.sh` green (724)
|
|
- [x] `cargo fmt --check` and `clippy -D warnings` clean
|
|
|
|
## Consequences
|
|
|
|
**Positive.** Internal navigation works: table-of-contents links,
|
|
cross-references and outline targets now reach the viewer as page indices.
|
|
The `GoToPage` path that existed end-to-end but was never fed is now fed.
|
|
A dead module is either alive or gone.
|
|
|
|
**Negative.** `AnnotationType::Link` changed shape — `dest: Option<String>`
|
|
became `destination: Option<Destination>` — and `AnnotationAction` gained
|
|
`GoToDestination`. Both are public. The old field could not express an
|
|
explicit destination, so keeping it would have meant keeping the bug.
|
|
|
|
`AnnotationAction` also lost its `Eq` derive, because a destination carries
|
|
`f64` coordinates. Deriving `Eq` would have forced those coordinates to be
|
|
dropped or rounded, which is the wrong trade for a comparison nothing needs.
|
|
|
|
**Risk.** `resolve_destination` is called for every link annotation on every
|
|
`page_annotations`, adding a name-tree walk per named link. The tree is
|
|
traversed linearly rather than binary-searched: correctness first, and these
|
|
trees are small. If a document with thousands of named links ever shows up
|
|
in a profile, the ordered `/Limits` make a binary search straightforward.
|
|
|
|
**Out of scope, deliberately:** `/GoToR` remote navigation (parsed and
|
|
refused, not followed — acting on it means opening another file, which is a
|
|
host decision under rule 5), outline `/Outlines` trees, `/OpenAction` on the
|
|
catalogue, and structure-destination `/SD` preference. Each is a separate
|
|
feature with its own risk, not a detail of this one.
|