refactor: one read-terminal normalization point + executable fold laws - #276
Merged
Conversation
…law suites Item 3 — the eager read terminals (toList / toListIndexed / count / exists) now route through a single private visitFocuses normalization: a null root focuses nothing on every optic shape, a Lens (which includes every Iso-rooted telescope) has exactly one focus whose value may be null, an Affine has zero or one, and a composed traversal walks its visitor. The per-terminal Lens/Affine instanceof fast paths this replaces were each a hand-maintained copy of the null table — the two shipped divergences (#274's exists NPE, toListIndexed's [Indexed[0, null]] vs []) were both one terminal's copy drifting. read/find keep their fast paths: they pull a lazy head, and their shortcuts carry real dispatch savings on the codegen-holder hot path. toList now returns an unmodifiable list on every shape (previously mutable on the composed-traversal branch only). Item 1 — the laws become executable: - internal: FoldLaws.assertFoldLaws pins Fold's documented contract ("both primitives enumerate the same focuses in the same order" + short-circuit reporting) and OpticLawsTest applies it to every optic — all single-focus shapes (null and miss cases included), all four container traversals (null/empty included), eachOptional (null/empty/present), composed and filtered traversals. - core: ReadTerminalConsistencyTest asserts the cross-terminal laws (count == toList.size == toListIndexed.size; exists == count > 0; indexed mirrors toList positionally; find-present implies exists) over the full shape × input matrix — Iso roots, lens paths, traversals, affines, filters, and the split container form, each against null roots, null focuses, null containers, and empties. This suite would have caught both prior divergences before they shipped. Item 2, downsized honestly — the inspection proposed deleting firstHopName and deriving it from the trail, but the generated <X>FieldOptics holder constants are bare Telescope.lens(Accessor) values whose stored name is load-bearing with an empty trail, and seeding the trail from lens() creates a Focus-vs-Traverse context knot only the caller can resolve (the codegen emitters append the correct node kind today). The field stays; the drift hazard is closed instead by a consistency pin: stored firstHopName must match the trail's first Focus/Traverse node (PropertyNames-normalized for bean paths) across record paths, container steps, filters, and fieldByName.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #276 +/- ##
============================================
+ Coverage 79.18% 79.74% +0.55%
- Complexity 2033 2041 +8
============================================
Files 88 88
Lines 6477 6471 -6
Branches 1330 1324 -6
============================================
+ Hits 5129 5160 +31
+ Misses 792 769 -23
+ Partials 556 542 -14 ☔ View full report in Codecov by Harness. |
…h pin, honest labels Review confirmed the normalization semantically equivalent by running the new suite against main's Telescope; the polish closes its findings: a genuine .as(...) Affine fixture (whenPresent composes into a Traversal, so the matrix's Affine branch was untested), a bean-path first-hop pin (the comment claimed bean normalization was covered; now it is), honest DisplayNames, assertFalse in FoldLaws, the Iso-null omission documented, two inline FQNs imported, and toList's unmodifiable guarantee stated in its javadoc.
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
1 participant
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.
What
Items 1–3 of the design round (the facade/lattice trio — they interlock):
One normalization point (item 3).
toList/toListIndexed/count/existsroute through a single privatevisitFocuses: null root focuses nothing, Lens (⊇ every Iso root) has one focus, Affine zero-or-one, traversals walk. The per-terminal fast paths this replaces were each a hand-copy of the null table — both shipped divergences (#274, thetoListIndexedbug) were one copy drifting.read/findkeep their fast paths (lazy head + real dispatch savings on the codegen-holder hot path). One deliberate contract tightening:toListis now unmodifiable on every shape (previously mutable on the composed branch only).Executable laws (item 1).
FoldLaws.assertFoldLawspins the Fold contract per optic — applied to every single-focus shape, all four container traversals,eachOptional, and composed/filtered shapes, nulls and empties included. Core-side,ReadTerminalConsistencyTestasserts the cross-terminal laws over the full shape × input matrix. This suite catches the entire bug class the last two campaigns fixed one instance at a time.firstHopName— downsized honestly (item 2). The inspection proposed deleting the field and deriving from the trail. The deletion test failed on real evidence: the generated<X>FieldOpticsholder constants are bareTelescope.lens(Accessor)values whose stored name is load-bearing with an empty trail, and seeding trails fromlens()creates a Focus-vs-Traverse context knot only the emitters can resolve. Kept the field; closed the actual hazard (silent drift) with a stored-vs-trail consistency pin across every nav shape.Full
:core+:internalsuites green.