Repository navigation
pxf: bind-time validation covers the import closure (#34) - #39
Merged
Merged
Conversation
ValidateFile and ValidateDescriptor walked the declaring file only, where draft -01 § Scope of Bind-Time Checks says the schema a check covers is that file together with its transitive imports, and that a tool MUST NOT report a schema conformant having checked only the declaring file. A field named null or a misplaced (pxf.key) in an imported .proto bound without a violation and was silently unhonoured when the decoder reached it. The walk now covers the closure, each path once (the diamond A → B, C → D reports D once); Violation::file names the declaring file, and the result sorts by file, then element, stable. google/protobuf/* files are skipped: they carry no PXF annotation and no reserved name, and descriptor.proto — in the closure of every annotated schema through pxf/annotations.proto — measured 1.1 µs to walk against a 3.9 µs decode of the bench document. With the skip the closure walk costs ~160 ns. The assumption is pinned by PxfSchema.GoogleProtobufFilesAreConformant, which validates each of those files directly. No cache keyed by descriptor pointer, as the reference has: a freed DescriptorPool can hand a new file the same address, which this repository's tests do on every fixture. Claude-Session: https://claude.ai/code/session_019rmcvyuzWRwq4e13b1KmB4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #34.
What
ValidateFile/ValidateDescriptorwalked the declaring file only; draft-01§ Scope of Bind-Time Checks requires the transitive import closure and forbids reporting a schema conformant on the declaring file alone. They now walk the closure, each path once (the diamond A → B, C → D reports D once),Violation::filenames the declaring file, and results sort by file then element (stable, so a field's reserved-name and(pxf.key)violations keep walk order). The per-decode gate inUnmarshaltherefore refuses a schema whose import is non-conformant, whether or not the document reaches the imported type.Cost, measured rather than inferred. descriptor.proto is in the closure of every annotated schema (through
pxf/annotations.proto) and cannot carry a PXF violation. Walking it cost 1.1 µs per call against a 3.9 µsbench_pxfdecode of the 652-byte bench document — a quarter of a small decode, though a small fraction of a real one.google/protobuf/*is therefore skipped in the walk, and the assumption is pinned byPxfSchema.GoogleProtobufFilesAreConformant, which validates descriptor.proto and five well-known types directly (no reserved names, no(pxf.key)) and fails the day that stops being true. With the skip:ValidateFilebench_pxfunmarshal on the spec's bench fixture (652 bytes; imports timestamp and duration, both skipped) moves from 3892–3902 ns/op (three runs before) to 3998–4042 ns/op (two runs after): about +3%, consistent with the ~100 ns the closure bookkeeping (path list, dependency loop) adds per decode. On a small document that is a visible fraction; on a real document it is a fixed ~100 ns. Callers decoding one schema repeatedly can setskip_validateafter validating once.I did not add the descriptor-keyed cache the reference has: a freed
DescriptorPoolcan hand a new file descriptor the same address, and this repository's tests compile a fresh pool per fixture, so such a cache would return a stale result for a different schema.UnmarshalOptions::skip_validateremains the escape hatch for callers that validated once.Evidence
PxfSchema.ImportedFileViolationsReportedWithDeclaringFile: A imports B; B'snullfield and misplaced(pxf.key)are reported withfile == "b_closure.proto", through bothValidateFileandValidateDescriptor, andUnmarshalof an empty document against A's root is refused naming B.PxfSchema.DiamondImportReportedOnceAndSortedByFile: D reported once; order a, b, d.PxfSchema.AnnotationsClosureIsConformant,PxfSchema.GoogleProtobufFilesAreConformant.ctest --test-dir build→ 100% tests passed out of 297.clang-format --dry-run -Werrorclean.Note: this stacks on nothing; #38 (#32) is independent.
https://claude.ai/code/session_019rmcvyuzWRwq4e13b1KmB4