Let a caller bring their own data dictionary, and keep it alive past the read. (#82)

#81 put the dictionary behind an interface. Nothing could supply one. A private
element's VR depends on the vendor who wrote the file, and the only way to teach
godicom a vendor's block was AddPrivateDictEntry, which mutates a process-global
table: every caller in the process sees the entry, and the only way to remove it
clears whatever anyone else registered too. Right for a program that owns its
process, wrong for a library.

ReadOptions.Dictionary is the per-read version, nil meaning PS3.6. It changes what
an element means rather than how it is described -- an implicit VR file carries no
VRs at all, so every element's VR is whatever the dictionary says, and for a
private element that answer depends on who wrote it. NewPrivateDictionary builds a
set of vendor entries that touches no global state, and NewDictionary composes:
each member is tried in turn and the first entry found wins, so the argument order
is the precedence. Compose rather than replace -- NewDictionary(vendor, Standard())
keeps the other 5,189 entries, and handing over the vendor's dictionary on its own
would leave every standard element as UN.

Three places the plumbing had to reach, none of them obvious from the signature
that needed changing:

The dictionary lives on readContext, not codecContext where the rest of the
per-element decode state lives, because it has to outlive the parse. A deferred
value is decoded on Get, long after the read returned, and deferred.go compares
the VR it re-resolves against the one the element was first read under. A
dictionary scoped to the parse would turn every deferred implicit-VR private
element into "deferred read VR UN does not match original US". readContext is the
only piece of parse state a Dataset retains, which is what makes it the right
home. Replacing rc.dictionary() with Standard() in datasetResolver reproduces that
error in both subtests of TestReadOptionsDictionarySurvivesADeferredLoad.

finishDeflated copies ReadOptions field by field, so a Deflated file would have
read its inflated dataset against PS3.6 while the same file undeflated resolved
properly. Only readReaderAt takes that path -- readBytes inflates in place and
keeps its readCtx -- so the test drives ReadFile as well as ReadBytes, and
dropping the copy fails it.

The VR-mismatch diagnostic is now judged against the dictionary the read was
given rather than always PS3.6. A caller who overrode an entry said what they
expect the file to contain; measuring against a different expectation than the
parse used would be reporting on nothing. Private tags stay exempt even when the
supplied dictionary has an entry, because the creator is not in hand at that point
and resolving one per element would charge the quiet path for a diagnostic nobody
asked for.

dictionaryVRForRead and lookupVRWithCreator are replaced by vrResolver, which
pairs the dictionary with the private-creator lookup because neither answers
alone -- a creator is only worth resolving against a dictionary that has that
vendor's block in it. Its zero value resolves against Standard with no creator,
which is what a header decoded outside a parse should get. LookupVR is now one
call to it.

AddPrivateDictEntry and ResetExtraPrivateDictionaries behave exactly as before and
are not deprecated; they are now implemented on top of PrivateDictionary rather
than a parallel table, and document the global mutation they perform.

Three gaps left deliberately, each recorded where someone would look for it:

  - No WriteOptions.Dictionary. The write path resolves an ambiguous VR from the
    dataset's own values -- Pixel Representation decides between US and SS -- and
    never consults the dictionary, so the field would have nothing to do.
  - DecodeDataset and friends take no options at all and so still resolve against
    Standard(). Giving four functions an options parameter is a wider change than
    the dictionary alone justifies.
  - dictionaryHasTag stays a direct probe of the exact standard table. It runs
    before the transfer syntax is known, to guess a byte order from the first tag;
    a private block cannot help, and letting a supplied dictionary answer would
    make the guess depend on how many entries the caller happened to add.

19 new tests in dictionary_composition_test.go, and ExampleNewPrivateDictionary
compile-checks the README snippet.

Closes #71.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: xbmlz

CHANGELOG.md

@@ -52,8 +52,58 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   so nearly every image ever written — and `(0028,1200)` Gray Lookup Table Data
   permits three, `US or SS or OW`. `vrDisagreesWithDictionary` now uses it instead
   of splitting the string itself
+- `ReadOptions.Dictionary` resolves tags for one read, defaulting to `Standard()`
+  when nil. It changes what an element *means* rather than how it is described: an
+  implicit VR file carries no VRs, so every element's VR is whatever the dictionary
+  says, and for a private element that answer depends on the vendor who wrote the
+  file. The dataset that comes back retains it, because a deferred value is decoded
+  on `Get` — long after the read returned — and the reload rejects an element whose
+  VR no longer matches the one it was first read under; a dictionary that went out
+  of scope with the parse would turn every deferred private element into a mismatch
+  error. It is also carried into the second read a Deflated transfer syntax starts,
+  so a file means the same thing deflated and inflated. There is deliberately no
+  `WriteOptions.Dictionary`: the write path resolves an ambiguous VR from the
+  dataset's own values — Pixel Representation decides between US and SS — and never
+  consults the dictionary, so the field would have nothing to do. `DecodeDataset`
+  and `DecodeDatasetContext` take no options at all and so still resolve against
+  `Standard()`. Closes
+  [#71](https://github.com/godicom-dev/godicom/issues/71)
+- `NewDictionary(dicts ...Dictionary) Dictionary` composes dictionaries, trying each
+  in turn and returning the first entry found, so the argument order is the
+  precedence: `NewDictionary(vendor, godicom.Standard())` reads the vendor's entries
+  where it has them and PS3.6's everywhere else. First match wins rather than most
+  specific, because that is the only rule a caller can read off the argument order.
+  Composing rather than replacing is the point — a caller adding one vendor block
+  should not have to carry the other 5,189 entries. A nil member is skipped and an
+  empty composition answers nothing, both of which a caller threading a built-up
+  list will produce; the members are held rather than copied, so a `PrivateDictionary`
+  among them stays live, while the slice itself is copied
+- `NewPrivateDictionary` returns a `*PrivateDictionary`, a set of private entries
+  keyed by Private Creator that satisfies `Dictionary`. `Add(creator, tag, vr, name,
+  vm...)` mirrors `AddPrivateDictEntry` so moving from one to the other is
+  mechanical, and stores the entry against the tag's block byte, so it resolves
+  wherever the vendor's block lands — `(0041,1001)` in one file and `(0041,2001)` in
+  the next. Unlike `AddPrivateDictEntry` it touches no process-global state: a
+  library can build one, compose it with `Standard()`, and hand it to
+  `ReadOptions.Dictionary` without changing what any other caller in the process
+  reads. Safe for concurrent use. Documented in the README with
+  `ExampleNewPrivateDictionary` behind it, so the snippet is compile-checked
 
 ### Changed
+- the VR-mismatch diagnostic is judged against the dictionary the read was given
+  rather than always against PS3.6. A caller who overrode an entry said what they
+  expect the file to contain, and measuring against a different expectation than the
+  parse used would be reporting on nothing. Private tags stay exempt even when the
+  supplied dictionary has an entry for them: the creator is not in hand at that
+  point, and resolving one per element would charge the quiet path for a diagnostic
+  nobody asked for
+- `AddPrivateDictEntry` and `ResetExtraPrivateDictionaries` now document that they
+  mutate process-global state — every caller in the process sees an added entry, and
+  `Reset` clears entries registered by code that has nothing to do with the caller —
+  and point at `NewPrivateDictionary` plus `ReadOptions.Dictionary` for the version
+  that does not. Neither is deprecated: a command-line tool registering its vendor's
+  blocks at startup is what they are for. Behaviour is unchanged, and they are now
+  implemented on top of `PrivateDictionary` rather than a parallel table
 - **Breaking:** the three exported dictionary maps are unexported —
   `DicomDictionaryGo`, `RepeatersDictionaryGo` and `PrivateDictionaries` become
   `dicomDictionary`, `repeatersDictionary` and `privateDictionaries`, and

README.md

@@ -104,6 +104,39 @@ entry, _ := godicom.Standard().Lookup(tag.PixelData, "")
 // entry.VRs() []VR{"OB", "OW"}
 ```
 
+**Your own dictionary**
+
+`AddPrivateDictEntry` mutates process-global state, which is right for a program
+that owns its process and wrong for a library. `NewPrivateDictionary` builds one
+that belongs to a single read instead, and `ReadOptions.Dictionary` is how it
+gets there:
+
+```go
+vendor := godicom.NewPrivateDictionary()
+if err := vendor.Add("ACME 3.2", godicom.NewTag(0x0041, 0x1001), godicom.VRUS, "Some Number"); err != nil {
+	return err
+}
+ds, err := godicom.ReadFile("ct.dcm", &godicom.ReadOptions{
+	Dictionary: godicom.NewDictionary(vendor, godicom.Standard()),
+})
+```
+
+`NewDictionary` composes: each dictionary is tried in turn and the first entry
+found wins, so the order is the precedence. Compose rather than replace —
+`Standard()` behind the vendor's dictionary is what keeps the other 5,189
+entries, and handing over the vendor's on its own would leave every standard
+element as UN.
+
+This changes what an element *means*, not merely how it is described. An implicit
+VR file carries no VRs, so every element's VR is whatever the dictionary says it
+is; the dataset that comes back retains the dictionary, because a deferred value
+is decoded on `Get`, long after the read returned, and has to resolve to the VR
+it was first read under.
+
+There is no `WriteOptions.Dictionary`. The write path resolves an ambiguous VR
+from the dataset's own values — Pixel Representation decides between US and SS —
+and never asks the dictionary, so the field would have nothing to do.
+
 **Truncated and malformed files**
 
 By default a read keeps whatever it parsed before the file stopped making sense,

codec.go

@@ -6,9 +6,15 @@ package godicom
 // These travelled the write chain as three separate parameters. That cost two
 // things worth fixing. The two bools are adjacent and interchangeable, so
 // transposing them at a call site compiles and silently writes a file in the
-// wrong encoding. And every new piece of codec state -- a diagnostic hook, a
-// dictionary, a validation policy -- had to be threaded through every signature
-// between writeDataset and the leaf that needed it.
+// wrong encoding. And every new piece of codec state -- a validation policy, a
+// character-set fallback -- had to be threaded through every signature between
+// writeDataset and the leaf that needed it.
+//
+// Not everything the read path carries around belongs here. State that has to
+// outlive the parse lives on readContext instead, which is what a Dataset retains:
+// the data dictionary in effect is there rather than here because a deferred load
+// re-resolves an element's VR long after the codecContext it was first read under
+// is gone.
 //
 // EncodingInfo is embedded rather than restated field by field: it is already
 // the type for the implicit-VR/endianness pair, and it is what a Dataset stores

dataset.go

@@ -46,6 +46,14 @@ type readContext struct {
 	// onDiag is ReadOptions.OnDiagnostic, kept so deferred loads can report
 	// through the same hook after the read has returned.
 	onDiag func(Diagnostic) error
+	// dict is ReadOptions.Dictionary, nil for PS3.6. It lives here rather than on
+	// codecContext -- where the rest of the per-element decoding state lives --
+	// because it has to outlive the parse. A deferred load reruns the header
+	// decode after the read returned, and a private element's VR has to resolve
+	// the same way it did the first time or the reload is rejected as a mismatch.
+	// readContext is the only piece of parse state that survives, as
+	// Dataset.readCtx.
+	dict Dictionary
 	// seqPath is the sequences currently being descended into, and the item of
 	// each, used to stamp Diagnostic.Path.
 	seqPath []PathStep
@@ -54,6 +62,16 @@ type readContext struct {
 	baseOffset int64
 }
 
+// dictionary returns the data dictionary this parse resolves against: the one
+// ReadOptions.Dictionary supplied, or PS3.6. Nil-safe, because a decoder handed
+// no readContext at all still has to resolve VRs.
+func (rc *readContext) dictionary() Dictionary {
+	if rc == nil {
+		return Standard()
+	}
+	return dictionaryOrStandard(rc.dict)
+}
+
 func (rc *readContext) logCtx() context.Context {
 	if rc != nil && rc.ctx != nil {
 		return rc.ctx

decode.go

@@ -54,6 +54,10 @@ func DecodeDatasetEncodingContext(ctx context.Context, data []byte, isImplicitVR
 		return ds, nil
 	}
 	ds := NewDataset()
+	// No dict: these entry points take no ReadOptions, so they resolve against
+	// PS3.6. A caller with a vendor's private dictionary to apply reads through
+	// ReadBytes with Force set, which takes one; giving these four functions an
+	// options parameter is a wider change than the dictionary alone justifies.
 	rc := &readContext{data: data, ctx: ctx}
 	enc := EncodingInfo{IsImplicitVR: isImplicitVR, IsLittleEndian: isLittleEndian}
 	_, err := readDatasetElements(data, 0, int64(len(data)), ds, codecContext{EncodingInfo: enc}, nil, rc)

deferred.go

@@ -106,8 +106,12 @@ func loadDeferredElement(ctx *readContext, ds *Dataset, elem *Element) error {
 	}
 
 	// The private creator lives in the dataset, not necessarily in data: a
-	// deferred load may have re-read only the element's own bytes.
-	raw, err := readRawDataElementAt(data, elementStart, cc.EncodingInfo, datasetCreator(ds))
+	// deferred load may have re-read only the element's own bytes. The dictionary
+	// comes from ctx, which is why it lives there: the VR resolved below is
+	// compared against the one from the original read, so a private element
+	// resolved through a caller's dictionary the first time has to resolve through
+	// the same one now.
+	raw, err := readRawDataElementAt(data, elementStart, cc.EncodingInfo, datasetResolver(ctx, ds))
 	if err != nil {
 		return err
 	}
@@ -128,16 +132,17 @@ func loadDeferredElement(ctx *readContext, ds *Dataset, elem *Element) error {
 
 // readRawDataElementAt reads a single defined-length element at tag position pos.
 // Mirrors pydicom.filereader.data_element_generator for one element with
-// defer_size=None. creator resolves private creators so an implicit VR private
-// element resolves to the same VR it did on the original read.
+// defer_size=None. vr carries the dictionary and the private creator lookup, so
+// an implicit VR private element resolves to the same VR it did on the original
+// read.
 func readRawDataElementAt(
 	data []byte,
 	pos int64,
 	enc EncodingInfo,
-	creator creatorFunc,
+	vr vrResolver,
 ) (*RawDataElement, error) {
 	tag := readTagBytes(data, pos, enc.IsLittleEndian)
-	h, need, ok := decodeElementHeader(data, pos, tag, enc, creator)
+	h, need, ok := decodeElementHeader(data, pos, tag, enc, vr)
 	if !ok {
 		return nil, fmt.Errorf("godicom: unexpected EOF reading deferred element header for %s: need %d bytes, have %d",
 			tag, need, int64(len(data))-pos)

diagnostic.go

@@ -184,8 +184,13 @@ func truncatedValue(tag Tag, vr VR, valueStart, need, total int64) Diagnostic {
 }
 
 // reportVRMismatch offers a disagreement between the VR encoded for tag at pos
-// and the VR the data dictionary gives it. Nothing about the parse changes, so
-// unlike the truncation diagnostics this one is pure information.
+// and the VR the dictionary this read was given gives it. Nothing about the parse
+// changes, so unlike the truncation diagnostics this one is pure information.
+//
+// Judged against the same dictionary the header decoder resolved against, not
+// always PS3.6: a caller who overrode an entry through ReadOptions.Dictionary said
+// what they expect the file to contain, and a diagnostic measuring against a
+// different expectation than the parse used would be reporting on nothing.
 //
 // Because it is only information, nobody who is not listening should pay for it:
 // the dictionary lookup runs once per explicit VR element, which is once per
@@ -206,7 +211,7 @@ func (rc *readContext) reportVRMismatch(tag Tag, encoded VR, pos int64, isImplic
 			return nil
 		}
 	}
-	want := vrDisagreesWithDictionary(tag, encoded)
+	want := vrDisagreesWithDictionary(rc.dictionary(), tag, encoded)
 	if want == "" {
 		return nil
 	}