Put the data dictionary behind an interface, and delete the 10,380 lines nobody read.

CHANGELOG.md

@@ -29,8 +29,50 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   CI: it depends on the network and on a document that changes independently of
   this repository, so a new DICOM edition would otherwise turn an unrelated pull
   request red. Run it after bumping the pydicom submodule
+- `Dictionary`, a one-method interface over the data dictionary —
+  `Lookup(tag Tag, creator string) (DictEntry, bool)` — and `Standard()`, which
+  returns the dictionary PS3.6 defines. One method answers for both halves of the
+  dictionary: `creator` names the Private Creator of a private tag and is ignored
+  for a standard one. `Lookup` resolves an exact entry ahead of a repeating-group
+  pattern, which is not arbitrary — `(7FE0,0010)` is Pixel Data exactly and retired
+  Variable Pixel Data through the `7Fxx,0010` mask — and never resolves a private
+  tag against the standard tables, because the same tag means different things to
+  different vendors. The package's own lookups go through it, so it is the path
+  godicom itself takes rather than a wrapper around one; the type is empty and its
+  method set is known, so the compiler inlines `Standard` and devirtualizes each
+  internal call back into a map access. It is also the first public way to read a
+  standard entry's VM, name or retired flag: `LookupVR` gave the VR and laundered a
+  missing entry into `UN`, and the `PrivateDictionary*` functions covered only
+  private tags. Groundwork for
+  [#71](https://github.com/godicom-dev/godicom/issues/71). Documented in the
+  README with `ExampleStandard`, `ExampleDictEntry_VRs` and
+  `ExampleAddPrivateDictEntry` behind it, so the snippets are compile-checked
+- `DictEntry.VRs()` returns the VRs an entry permits, splitting the compound forms
+  PS3.6 writes as prose. Thirty-seven entries permit two — `OB or OW` is PixelData,
+  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
+
+### Changed
+- **Breaking:** the three exported dictionary maps are unexported —
+  `DicomDictionaryGo`, `RepeatersDictionaryGo` and `PrivateDictionaries` become
+  `dicomDictionary`, `repeatersDictionary` and `privateDictionaries`, and
+  `PrivateDictEntry` becomes `privateDictEntry` along with the table it populates.
+  They were read-only by convention and mutable by type, with no synchronisation of
+  any kind, so any caller could have corrupted the dictionary for the whole
+  process. Read access is now `Standard().Lookup`, which answers everything the
+  maps were reachable for; `AddPrivateDictEntry` remains the way to add a private
+  entry at runtime. Nothing in the godicom-dev organisation referenced the maps.
+  `DictEntry` stays exported — it is what `Lookup` returns
 
 ### Fixed
+- `generate_dict.py` wrote to `dicom_dict_generated.go`, a name nothing in the tree
+  has: the checked-in file is `dictionary_generated.go`. Running the generator
+  therefore produced a second file redeclaring `DictEntry` and every map in it, so
+  the package stopped compiling instead of the dictionary being updated
+- both dictionary generators now `gofmt` their output. The checked-in files are
+  formatted and the generators' output was not, so regenerating produced a
+  5,182-line whitespace diff on top of whatever the data change was
 - `godicom`'s usage text listed only `-debug`, having never been updated as `show`
   grew `-no-meta`, `-top`, `-t` and `-tag`. All five are now documented, in the
   usage text and in the README's CLI section. `TestPrintUsageMatchesShowFlags`
@@ -47,6 +89,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   search with and without `-top`
 
 ### Removed
+- `dictionaryIsRetired`, which consulted only the exact table and so answered "not
+  retired" for all 72 retired repeating-group entries. It had no caller outside its
+  own test, so nothing ever noticed. `Standard().Lookup` returns the entry and
+  `Retired` is read off it, which gets the repeaters right;
+  `TestLookupReportsRetired` covers one of them
+- the generated `tagToKeyword` and `tagToName` maps, 10,380 lines between them.
+  Both were a second copy of data `dicomDictionary` already held, reachable by one
+  map access; `tagToName` had no reader at all and `keywordForTag` was the only
+  reader of `tagToKeyword`. Two 5,000-entry maps are no longer built at init.
+  `keywordToTag` stays, because that direction is the one that needs an index.
+  staticcheck skips generated files, which is why `tagToName` sat unread without
+  `-checks=all` minding
 - `var _ = regexp.Compile` in `dictionary.go`, kept by a comment claiming it forced
   an init that the `regexp` package does not have. Nothing in the file used
   `regexp`, so the import went with it

README.md

@@ -79,6 +79,31 @@ Context-aware variants (`ReadFileContext`, `WriteContext`,
 `DecodeDatasetContext`, …) accept a `context.Context` for cancellation and
 structured logging.
 
+**The data dictionary**
+
+`godicom.Standard()` returns the dictionary from PS3.6, and `Lookup` reads an
+entry out of it:
+
+```go
+entry, ok := godicom.Standard().Lookup(tag.PatientName, "")
+// entry.VR "PN", entry.VM "1", entry.Name "Patient's Name",
+// entry.Keyword "PatientName", entry.Retired false
+```
+
+The second argument is the Private Creator, which a standard tag ignores and a
+private tag needs — the same tag means different things to different vendors, so
+without a creator there is no entry to find. Register a vendor's element with
+`AddPrivateDictEntry` and `Lookup` finds it too.
+
+A few entries permit more than one VR, which PS3.6 writes as prose. Pixel Data
+is one of them, so this is not a corner case:
+
+```go
+entry, _ := godicom.Standard().Lookup(tag.PixelData, "")
+// entry.VR "OB or OW", which is not a VR anything matches
+// entry.VRs() []VR{"OB", "OW"}
+```
+
 **Truncated and malformed files**
 
 By default a read keeps whatever it parsed before the file stopped making sense,

dictionary.go

@@ -5,108 +5,186 @@ import (
 	"strings"
 )
 
+// Dictionary resolves a tag to its data dictionary entry. Standard returns the
+// one PS3.6 defines, which is what godicom reads and writes with.
+//
+// creator names the Private Creator of a private tag and is ignored for a
+// standard one, so a single method answers for both halves of the dictionary.
+// Pass "" for a standard tag, or for a private tag whose creator is unknown -- a
+// private tag on its own has no entry to find, because the same tag means
+// different things to different vendors.
+type Dictionary interface {
+	Lookup(tag Tag, creator string) (DictEntry, bool)
+}
+
+// Standard returns the DICOM data dictionary from PS3.6: the standard elements,
+// the repeating-group elements, and the registered private creators together with
+// anything added by AddPrivateDictEntry.
+func Standard() Dictionary { return standardDictionary{} }
+
+// standardDictionary has no fields because the tables it reads are package state.
+// It is a named type anyway: without one there is nothing for a caller to hold,
+// wrap, or count lookups on.
+//
+// Routing the package's own lookups through Standard costs nothing, which is why
+// they do rather than reaching for the maps directly: the type is empty and the
+// method set is known, so the compiler inlines Standard and devirtualizes every
+// internal Lookup back into the map accesses below.
+type standardDictionary struct{}
+
+// Lookup resolves tag in the order PS3.6 leaves no choice about.
+//
+// An exact entry beats a repeating-group pattern, and (7FE0,0010) is why that
+// order is not arbitrary: it is Pixel Data as an exact entry and retired Variable
+// Pixel Data through the 7Fxx,0010 mask, and every image ever written means the
+// former.
+//
+// A private tag is never resolved against the standard tables. (0029,xx10) means
+// whatever the vendor who wrote it says it means, so without a creator there is
+// no answer to give.
+func (standardDictionary) Lookup(tag Tag, creator string) (DictEntry, bool) {
+	if entry, ok := dicomDictionary[tag]; ok {
+		return entry, true
+	}
+	if tag.IsPrivate() {
+		if creator == "" {
+			return DictEntry{}, false
+		}
+		entry, ok := lookupPrivateDictEntry(tag, creator)
+		if !ok {
+			return DictEntry{}, false
+		}
+		// Keyword stays empty: PS3.6 assigns keywords to standard elements, and a
+		// private element has none to assign.
+		return DictEntry{
+			VR:      entry.VR,
+			VM:      entry.VM,
+			Name:    entry.Name,
+			Retired: entry.Retired,
+		}, true
+	}
+	if mask := maskMatch(tag); mask != "" {
+		if entry, ok := repeatersDictionary[mask]; ok {
+			return entry, true
+		}
+	}
+	return DictEntry{}, false
+}
+
+// VRs returns the VRs the entry permits, in the order PS3.6 lists them.
+//
+// Most entries permit one. Thirty-seven permit two -- "OB or OW", which is
+// PixelData and so nearly every image ever written, plus "US or SS" and
+// "US or OW" -- and (0028,1200) Gray Lookup Table Data permits three,
+// "US or SS or OW". Splitting on " or " rather than special-casing pairs is what
+// keeps that last form from reading as one VR named "US or SS or OW", which
+// matches nothing.
+//
+// Three entries say "NONE" rather than naming a VR: Item and the two delimitation
+// items, which carry no VR at all. That is returned as it stands, because it is
+// what the dictionary says.
+//
+// An entry with no VR yields no VRs rather than one empty VR: a runtime private
+// entry can be registered without one, and "" is not a VR that anything matches.
+func (e DictEntry) VRs() []VR {
+	if e.VR == "" {
+		return nil
+	}
+	parts := strings.Split(e.VR, " or ")
+	vrs := make([]VR, 0, len(parts))
+	for _, part := range parts {
+		if part = strings.TrimSpace(part); part != "" {
+			vrs = append(vrs, VR(part))
+		}
+	}
+	return vrs
+}
+
 // tagForKeyword looks up a tag by keyword string.
 func tagForKeyword(keyword string) (Tag, bool) {
 	t, ok := keywordToTag[keyword]
 	return t, ok
 }
 
 // keywordForTag looks up the keyword for a tag.
+//
+// An entry with no keyword counts as no answer rather than as an empty one. Seven
+// standard entries are retired blanks that have none, and a caller asking for a
+// keyword cannot use "" -- it wants to fall back to printing the tag.
 func keywordForTag(tag Tag) (string, bool) {
-	kw, ok := tagToKeyword[tag]
-	if ok {
-		return kw, ok
+	entry, ok := Standard().Lookup(tag, "")
+	if !ok || entry.Keyword == "" {
+		return "", false
 	}
-	// Check repeaters
-	if !tag.IsPrivate() {
-		mask := maskMatch(tag)
-		if mask != "" {
-			if entry, ok := RepeatersDictionaryGo[mask]; ok {
-				return entry.Keyword, true
-			}
-		}
-	}
-	return "", false
+	return entry.Keyword, true
 }
 
 // dictionaryVR returns the VR for a given tag.
 func dictionaryVR(tag Tag) (VR, error) {
-	if entry, ok := DicomDictionaryGo[tag]; ok {
-		return VR(entry.VR), nil
-	}
-	if !tag.IsPrivate() {
-		mask := maskMatch(tag)
-		if mask != "" {
-			if entry, ok := RepeatersDictionaryGo[mask]; ok {
-				return VR(entry.VR), nil
-			}
-		}
+	entry, ok := Standard().Lookup(tag, "")
+	if !ok {
+		return "", fmt.Errorf("godicom: tag %s not found in dictionary", tag)
 	}
-	return "", fmt.Errorf("godicom: tag %s not found in dictionary", tag)
+	return VR(entry.VR), nil
 }
 
 // vrDisagreesWithDictionary returns the VR the data dictionary gives tag when
 // that VR and encoded cannot be the same thing, or "" when they are compatible.
 //
 // Only a tag the dictionary has an entry for can disagree with anything, so this
-// consults dictionaryVR rather than LookupVR: LookupVR launders a missing entry
-// into UN, which is a decoding default rather than an expectation. An
-// unrecognised standard tag and a private tag have no dictionary VR to be wrong
-// about, and reporting every one of them would bury the disagreements that
+// consults the dictionary directly rather than LookupVR: LookupVR launders a
+// missing entry into UN, which is a decoding default rather than an expectation.
+// An unrecognised standard tag and a private tag have no dictionary VR to be
+// wrong about, and reporting every one of them would bury the disagreements that
 // matter -- a real file is full of private elements carrying perfectly good
 // explicit VRs.
 //
-// A dictionary entry may name more than one permitted VR; PS3.6 spells these
-// "US or SS", "OB or OW" and "US or OW", and any of the alternatives is correct.
-// PixelData is one of them, so skipping this would mismatch on nearly every
-// image ever written.
+// An entry may permit more than one VR, and any of them is correct. PixelData is
+// one such entry -- "OB or OW" -- so treating the whole string as a single VR
+// would mismatch on nearly every image ever written. VRs does that splitting.
 func vrDisagreesWithDictionary(tag Tag, encoded VR) VR {
 	if encoded == "" || tag.IsPrivate() {
 		return ""
 	}
-	want, err := dictionaryVR(tag)
-	if err != nil || want == "" || want == encoded {
+	entry, ok := Standard().Lookup(tag, "")
+	if !ok || entry.VR == "" {
 		return ""
 	}
-	for _, alt := range strings.Split(string(want), " or ") {
-		if VR(strings.TrimSpace(alt)) == encoded {
+	for _, permitted := range entry.VRs() {
+		if permitted == encoded {
 			return ""
 		}
 	}
-	return want
+	return VR(entry.VR)
 }
 
 // dictionaryDescription returns the name for a given tag.
 func dictionaryDescription(tag Tag) (string, bool) {
-	if entry, ok := DicomDictionaryGo[tag]; ok {
-		return entry.Name, true
-	}
-	if !tag.IsPrivate() {
-		mask := maskMatch(tag)
-		if mask != "" {
-			if entry, ok := RepeatersDictionaryGo[mask]; ok {
-				return entry.Name, true
-			}
-		}
+	entry, ok := Standard().Lookup(tag, "")
+	if !ok {
+		return "", false
 	}
-	return "", false
+	return entry.Name, true
 }
 
-// dictionaryHasTag returns true if the tag exists in the dictionary.
+// dictionaryHasTag reports whether tag has an entry of its very own.
+//
+// Deliberately not Standard().Lookup: this is the probe the endianness heuristic
+// runs, and it needs "this exact tag is a known element", not "something in the
+// dictionary covers this tag". The 88 repeating-group patterns cover 108,688 tags
+// between them against 5,189 exact entries, and the heuristic picks a byte order
+// by which of the two candidate readings is the recognised one -- the more
+// readings it recognises, the less it is deciding anything.
+//
+// No test in the suite currently tells the two apart on a real file, so nothing
+// would have caught this being widened; TestDictionaryHasTagIgnoresRepeaters is
+// what keeps the narrow question narrow.
 func dictionaryHasTag(tag Tag) bool {
-	_, ok := DicomDictionaryGo[tag]
+	_, ok := dicomDictionary[tag]
 	return ok
 }
 
-// dictionaryIsRetired returns true if the tag is retired.
-func dictionaryIsRetired(tag Tag) bool {
-	if entry, ok := DicomDictionaryGo[tag]; ok {
-		return entry.Retired
-	}
-	return false
-}
-
-// Repeater masks: precomputed from the RepeatersDictionaryGo keys
+// Repeater masks: precomputed from the repeatersDictionary keys
 type repeaterMask struct {
 	maskStr string
 	// A tag is 32 unsigned bits, so these are too. They were int, and int is 32
@@ -121,7 +199,7 @@ type repeaterMask struct {
 var repeaterMasks []repeaterMask
 
 func init() {
-	for maskStr := range RepeatersDictionaryGo {
+	for maskStr := range repeatersDictionary {
 		value, mask := repeaterMaskBits(maskStr)
 		repeaterMasks = append(repeaterMasks, repeaterMask{
 			maskStr: maskStr,