From daa551c1c30d7a1ab0703e427594c41e0dfd3179 Mon Sep 17 00:00:00 2001 From: Samantha Abbott <52800387+sathabbott@users.noreply.github.com> Date: Mon, 5 Oct 2026 14:03:32 -0500 Subject: [PATCH 1/3] FileContext.type_by_name returns object: a lookup can be an annotated builtin type_by_name was annotated -> type[ROOTSerializable], but TString, TStringLong and string resolve to annotated bytes aliases (#68), and a file's context resolves TDatime to an annotated int. mypy therefore let callers call .read() on the result, the TypedTKey bug #127 fixed, and test_read.py's basket reader still did. Option 1 on #135: FileContext.type_by_name and both implementations return object, with the contract in the docstring: read the result with read_value, which now takes object, and narrow with isinstance(t, type) and issubclass(t, ROOTSerializable) where the class is needed. mypy now rejects .read() on a lookup. TKey.read_object (untyped) and TKey.read_from return object as well, since a TString record reads as bytes; so the Locator protocol's type variable is no longer bound to ROOTSerializable. The basket reader in test_read.py uses read_value. Fixes #135 Assisted-by: claude-code:claude-opus-5-5 --- src/rootfilespec/bootstrap/TKey.py | 11 ++-- src/rootfilespec/bootstrap/__init__.py | 6 +-- src/rootfilespec/dynamic.py | 6 +-- src/rootfilespec/serializable.py | 23 ++++---- tests/test_read.py | 3 +- tests/test_type_by_name.py | 75 ++++++++++++++++++++++++++ 6 files changed, 101 insertions(+), 23 deletions(-) create mode 100644 tests/test_type_by_name.py diff --git a/src/rootfilespec/bootstrap/TKey.py b/src/rootfilespec/bootstrap/TKey.py index 640baa1..47a0f42 100644 --- a/src/rootfilespec/bootstrap/TKey.py +++ b/src/rootfilespec/bootstrap/TKey.py @@ -107,7 +107,7 @@ def update_members( return members, buffer @overload - def read_object(self, fetch_data: DataFetcher) -> ROOTSerializable: ... + def read_object(self, fetch_data: DataFetcher) -> object: ... @overload def read_object( @@ -118,7 +118,7 @@ def read_object( self, fetch_data: DataFetcher, objtype: type[ObjType] | None = None, - ) -> ObjType | ROOTSerializable: + ) -> object: if self.fClassName == b"RBlob": # An RBlob key's fObjLen is decorative, and one blob can hold several # pages, each with a checksum fObjLen does not count (root-io-spec @@ -150,7 +150,7 @@ def read_object( buffer = decompress(buffer, self.header.fObjlen) else: buffer = buffer[: self.header.fObjlen] - readtype: type[ObjType] | type[ROOTSerializable] + readtype: object if objtype is None: typename = normalize(self.fClassName) readtype = buffer.file_context.type_by_name(typename) @@ -158,7 +158,6 @@ def read_object( typename = objtype.__name__ readtype = objtype # A looked-up type may be an annotated builtin, such as TString (#68) - obj: ObjType | ROOTSerializable obj, buffer = read_value(readtype, buffer) # Some types we have to handle trailing bytes if typename == "TKeyList": @@ -187,7 +186,9 @@ def offset(self) -> int: def size(self) -> int: return self.header.fNbytes - def read_from(self, buffer: ReadBuffer) -> ROOTSerializable: + def read_from(self, buffer: ReadBuffer) -> object: + """The key's object: a ROOTSerializable, or a builtin for a class that + reads as one, such as ``bytes`` for a ``TString`` (#68, #135)""" return self.read_object(lambda _seek, _size: buffer) diff --git a/src/rootfilespec/bootstrap/__init__.py b/src/rootfilespec/bootstrap/__init__.py index c4ff193..19a2e86 100644 --- a/src/rootfilespec/bootstrap/__init__.py +++ b/src/rootfilespec/bootstrap/__init__.py @@ -57,11 +57,9 @@ @dataclasses.dataclass class _BootstrapContext(FileContext): - types: dict[str, type[ROOTSerializable]] + types: dict[str, object] - def type_by_name( - self, name: str, expect_version: int | None = None - ) -> type[ROOTSerializable]: + def type_by_name(self, name: str, expect_version: int | None = None) -> object: cls = self.types.get(name) if cls is None: msg = f"Cannot find type {name} (expected version {expect_version})" diff --git a/src/rootfilespec/dynamic.py b/src/rootfilespec/dynamic.py index bcef260..03318c4 100644 --- a/src/rootfilespec/dynamic.py +++ b/src/rootfilespec/dynamic.py @@ -74,9 +74,7 @@ class DynamicFileContext(FileContext): def __repr__(self) -> str: return f"" - def type_by_name( - self, name: str, expect_version: int | None = None - ) -> type[ROOTSerializable]: + def type_by_name(self, name: str, expect_version: int | None = None) -> object: cls = self.module.__dict__.get(name) if cls is None: if name == "TLeafI": @@ -101,7 +99,7 @@ def type_by_name( # TODO: try to see if it is in BOOTSTRAP_CONTEXT ? msg = f"Unknown type {name} (version {expect_version}): not found in {self}" raise ValueError(msg) - return cls # type: ignore[no-any-return] + return cls def type_by_checksum(self, checksum: bytes) -> type[ROOTSerializable]: return super().type_by_checksum(checksum) diff --git a/src/rootfilespec/serializable.py b/src/rootfilespec/serializable.py index 9bf9b68..3ea1715 100644 --- a/src/rootfilespec/serializable.py +++ b/src/rootfilespec/serializable.py @@ -29,13 +29,18 @@ class FileContext: of types will be used to read the rest of the file """ - def type_by_name( - self, name: str, expect_version: int | None = None - ) -> type["ROOTSerializable"]: - """Lookup a ROOTSerializable-derived type by its name + def type_by_name(self, name: str, expect_version: int | None = None) -> object: + """Lookup the type of a class by its name The name should be normalized according to dispatch.normalize() + The result is a ROOTSerializable-derived class, or an annotated builtin + for a class that reads as one: ``TString`` is + ``Annotated[bytes, ROOTString("TString")]``, and ``TDatime`` an annotated + ``int``. Read a value with ``read_value(result, buffer)``, never with + ``result.read(buffer)``. A caller that needs the class narrows with + ``isinstance(result, type) and issubclass(result, ROOTSerializable)``. + If expect_version is supplied, an exception is raised if the version does not match the version that is interpretable in this context """ @@ -275,14 +280,12 @@ def build_reader(self, fname: str, ftype: type) -> ReadMembersMethod: raise NotImplementedError(msg) -def _build_read(ftype: type[MemberType]) -> ReadObjMethod: +def _build_read(ftype: object) -> ReadObjMethod: membermethod = _build_update_members("", ftype) return ReadObjMethod(membermethod) -def read_value( - ftype: type[MemberType] | type["ROOTSerializable"], buffer: ReadBuffer -) -> tuple[MemberType, ReadBuffer]: +def read_value(ftype: object, buffer: ReadBuffer) -> tuple[MemberType, ReadBuffer]: """Read one value of the given type from the buffer The type is a ROOTSerializable class, or an annotated builtin that a type @@ -364,7 +367,9 @@ def update_members( DataFetcher = Callable[[int, int], ReadBuffer] -T_co = TypeVar("T_co", bound=ROOTSerializable, covariant=True) +# Not bound to ROOTSerializable: a TKey locates a builtin when its class reads as +# one, such as a TString record read as bytes (#68, #135) +T_co = TypeVar("T_co", covariant=True) class Locator(Protocol[T_co]): diff --git a/tests/test_read.py b/tests/test_read.py index 77ca701..ccaac28 100644 --- a/tests/test_read.py +++ b/tests/test_read.py @@ -23,6 +23,7 @@ BufferContext, ReadBuffer, ROOTSerializable, + read_value, ) TESTABLE_FILES = [f for f in known_files if f.endswith(".root")] @@ -55,7 +56,7 @@ def read(self, buffer: ReadBuffer): itemheader, _ = StreamHeader.read(buffer) item_end = itemheader.fByteCount + 4 buffer, remaining = buffer[:item_end], buffer[item_end:] - item, buffer = dyntype.read(buffer) + item, buffer = read_value(dyntype, buffer) if buffer: msg = f"Expected buffer to be empty after reading {self.typename}, but got\n{buffer}" raise ValueError(msg) diff --git a/tests/test_type_by_name.py b/tests/test_type_by_name.py new file mode 100644 index 0000000..d4f1fc9 --- /dev/null +++ b/tests/test_type_by_name.py @@ -0,0 +1,75 @@ +from pathlib import Path +from typing import get_type_hints + +import pytest + +from rootfilespec.bootstrap import BOOTSTRAP_CONTEXT, TNamed, TString +from rootfilespec.bootstrap.strings import TStringLong +from rootfilespec.bootstrap.TDatime import TDatime +from rootfilespec.bootstrap.TKey import TKey +from rootfilespec.dynamic import DynamicFileContext +from rootfilespec.reader import open_path +from rootfilespec.serializable import FileContext, ROOTSerializable, read_value + +DATA = Path(__file__).parent.parent / "reference" / "root-io-spec" / "data" +UNFRAMED = DATA / "serialization" / "unframed-records.root" + + +@pytest.mark.parametrize( + "method", + [ + FileContext.type_by_name, + type(BOOTSTRAP_CONTEXT).type_by_name, + DynamicFileContext.type_by_name, + TKey.read_from, + ], +) +def test_return_type_is_object(method): + """Issue #135: a lookup can return an annotated builtin, not only a class, + and a key can read as a builtin, so neither is typed as ROOTSerializable""" + assert get_type_hints(method)["return"] is object + + +@pytest.mark.skipif(not UNFRAMED.exists(), reason="root-io-spec not checked out") +def test_lookup_of_tdatime_is_an_annotated_builtin(): + """root-io-spec serialization/unframed-records stores a TDatime as a record + of its own; its class name resolves to the annotated alias, which read_value + reads and which has no read method. The case.toml pins the payload at 318: + 0x7E6CC000, 2026-09-22 12:00:00 packed""" + with open_path(UNFRAMED) as reader: + (key,) = [k for k in reader.keylist().values() if k.fClassName == b"TDatime"] + buffer = reader.fetch.buffer(key) + readtype = buffer.file_context.type_by_name("TDatime") + assert readtype is TDatime + assert not isinstance(readtype, type) + payload = buffer[key.header.fKeylen :] + value, rest = read_value(readtype, payload) + assert value == 0x7E6CC000 + assert not rest + # mypy rejects this call, which is what #135 asks for + with pytest.raises(AttributeError): + readtype.read(payload) # type: ignore[attr-defined] + assert reader.fetch(key) == 0x7E6CC000 + + +@pytest.mark.skipif(not UNFRAMED.exists(), reason="root-io-spec not checked out") +@pytest.mark.parametrize( + ("name", "expected"), + [("TString", TString), ("TStringLong", TStringLong), ("string", TString)], +) +def test_lookup_of_strings(name: str, expected: object): + """The string classes resolve to their annotated aliases (#68), in the + bootstrap context and in a file's""" + with open_path(UNFRAMED) as reader: + key = next(iter(reader.keylist().values())) + context = reader.fetch.buffer(key).file_context + for ctx in (BOOTSTRAP_CONTEXT, context): + assert ctx.type_by_name(name) is expected + + +def test_lookup_of_a_class_narrows(): + """A caller that needs the class narrows the result""" + readtype = BOOTSTRAP_CONTEXT.type_by_name("TNamed") + assert isinstance(readtype, type) + assert issubclass(readtype, ROOTSerializable) + assert readtype is TNamed From cd63cf82a01b2652b7334c7d626c02388c2dc58e Mon Sep 17 00:00:00 2001 From: Samantha Abbott <52800387+sathabbott@users.noreply.github.com> Date: Mon, 5 Oct 2026 16:44:29 -0500 Subject: [PATCH 2/3] tests: drop the object annotations #145 added for string records #145 typed two string-record reads in test_root_string.py as object, because TKey.read_from claimed ROOTSerializable. It is typed object now, so the reads compare directly again. Assisted-by: claude-code:claude-opus-5-5 --- tests/test_root_string.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/test_root_string.py b/tests/test_root_string.py index cd0100d..ee503b7 100644 --- a/tests/test_root_string.py +++ b/tests/test_root_string.py @@ -214,11 +214,9 @@ def test_string_records(): serialization/stringlong (Conventions §5.1, §5.1.1)""" with open_path(DATA / "serialization" / "unframed-records.root") as reader: keylist = reader.keylist() - # TKey.read_from is annotated ROOTSerializable but reads these as bytes (#135) - tstring: object = reader.fetch(keylist.get_by_name(b"tstring")) - assert tstring == b"hello" - tstringlong: object = reader.fetch(keylist.get_by_name(b"tstringlong")) - assert tstringlong == b"a long string" + # A string record reads as bytes, so TKey.read_from is typed object (#135) + assert reader.fetch(keylist.get_by_name(b"tstring")) == b"hello" + assert reader.fetch(keylist.get_by_name(b"tstringlong")) == b"a long string" with open_path(DATA / "serialization" / "stringlong.root") as reader: keylist = reader.keylist() # Classes generated from the file's StreamerInfo have no static type From db3e3327cd6c1098009f05ec63f8452d94713e49 Mon Sep 17 00:00:00 2001 From: Samantha Abbott <52800387+sathabbott@users.noreply.github.com> Date: Mon, 5 Oct 2026 16:52:08 -0500 Subject: [PATCH 3/3] Say what Locator's type variable is for The comment on T_co only said why it is unbound. It is the type a locator returns (Locator[RPage], Locator[TTree]), unbound because some locators return a builtin. Wording from Nick's review of #147. Assisted-by: claude-code:claude-opus-5-5 --- src/rootfilespec/serializable.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/rootfilespec/serializable.py b/src/rootfilespec/serializable.py index 3ea1715..b0a857d 100644 --- a/src/rootfilespec/serializable.py +++ b/src/rootfilespec/serializable.py @@ -367,8 +367,10 @@ def update_members( DataFetcher = Callable[[int, int], ReadBuffer] -# Not bound to ROOTSerializable: a TKey locates a builtin when its class reads as -# one, such as a TString record read as bytes (#68, #135) +# The type a locator returns, so that ``Locator[RPage]`` or ``Locator[TTree]`` +# says what ``read_from`` gives. It is not bound to ROOTSerializable, because +# some locators return a builtin: a TKey of a TString record reads as bytes +# (#68, #135) T_co = TypeVar("T_co", covariant=True)