refactor(core): retire MultiFocus.representableAt — the repr0 index was unobservable - #123
Merged
Merged
Conversation
…ervable `representableAt[F, A](F)(repr0)` discarded its index (`val _ = repr0`) and rebuilt pointwise via `F.tabulate`, so it built exactly the optic `MultiFocus.representable` builds, plus a parameter neither the factory body nor any operation on the built optic could reach: the Grate encoding's focus is the whole bundle `F.Representation => A` with `X = Unit`, and the write is positional. Two calls with different indices were the same optic. Decision — retire it (option b of the three considered): - (a) give `repr0` a runtime meaning: there is nowhere to put it. `X = Unit`, and the public `X` is existential, so a `.lead` accessor would need a field on the optic class plus a runtime witness match (the `Function1BroadcastOptic` shape) to recover it — sugar over `.at(repr0)`, which is the same read without optics-side state. The lead VALUE was already deleted as dead code: `MultiFocusLeadPosition` was dropped by 12b8279 for having zero readers, and the fold spike records "the lead value is empirically dead through `.modify`". A lead-sampling WRITE is not a missing feature but an unlawful one: `from` must tabulate positionally, and sampling one index rebuilds a constant container — the collapse the sibling `fix/grate-positional-composition` change set fixes. - (b) retire: position is a read-time argument, never a property of the optic, and `Representable` has no canonical index to hand a constructor (`Function1[Boolean, *]` has no privileged `Boolean`; `Function1[Nothing, *]` none at all). `.at(i)` subsumes the construction-time index, and works on composed optics too. - (c) keep it vestigial but pinned: that would freeze a lie in a published signature. The line's policy is to remove dead public surface (0.10's pruning, 0.13's `widenB`) — MiMa is off (`tlMimaPreviousVersions := Set.empty`) and each 0.x break is documented in `mima.sbt`. Source- and binary-breaking for direct callers; the migration is dropping the argument and reading a position with the `.at(i)` extension. Recorded in `mima.sbt` (newest break-list entry), `CHANGELOG.md` (Unreleased → Removed) and in both doc pages, with a tombstone on `representable`'s scaladoc. Specs: - `MultiFocusFunction1Spec`: the absorbed-Grate block is rewired onto `representable` and pinned against the instance's own `index` / `map` for two `Representable`s — the second permuted (index order ≠ field order), so "index 0" and "first field" are different questions, and `modify` / `replace` must stay pointwise (a lead-sampling write would stop matching the instance's `map`). - `UnlawfulFixturesSpec`: negative fixture — a hand-written lead-sampling twin of the factory fails `MultiFocusLaws.modifyIdentity`, while the shipped `representable` passes the same law on the same carrier and input; the twin also fails the pointwise-`map` pin the core spec holds `representable` to. The carrier is a structural container: the law compares with `==`, which is reference equality on a function, so a function-typed `S` would fail it whatever the rebuild shape. Verified on JDK 25 (Temurin 25.0.4.1): `sbt scalafmtAll test` green across the root aggregate (237 examples, 0 failures; 2 pre-existing skips), focused `core/testOnly dev.constructive.eo.MultiFocusFunction1Spec` (5 examples, 313 expectations) and `tests/testOnly dev.constructive.eo.UnlawfulFixturesSpec` (4 examples, 6 expectations), `sbt scalafmtCheckAll`, `sbt docs/mdoc` (0 errors) and `sbt docs/laikaSite`.
Contributor
|
🚀 Cloudflare Pages preview for https://12c7cc31.cats-eo-docs.pages.dev Branch alias: https://refactor-multifocus-retire-r.cats-eo-docs.pages.dev Built from commit |
CI's `sbt scalafixAll --check` step flagged both specs: with the brace form
`cats.{Functor, Representable}` the ASCII sort puts
`cats.instances.function.given` first (the pre-change `cats.Representable`
sorted before it, which is why the single-name import read fine). Applied with
`sbt scalafixAll`; no semantic change.
Verified on JDK 25: the CI gate set locally — `scalafmtCheckAll
scalafmtSbtCheck benchmarks/scalafmtCheck`, `scalafixAll --check`,
`githubWorkflowCheck`, `mimaReportBinaryIssues` — plus the root-aggregate
`sbt test` (all modules 0 failures) and the two focused specs (5 examples /
313 expectations, 4 examples / 6 expectations).
Contributor
Benchmark A/BAllocation (B/op) — authoritative
442 more benchmarks
Timing (ns/op) — directional only, same-VM but shared runner
base_sha: |
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.
Summary
MultiFocus.representableAt[F, A](F)(repr0)discarded its index (val _ = repr0) and rebuiltpointwise through
F.tabulate, so it built exactly the opticMultiFocus.representablebuilds —plus a parameter neither the factory body nor any operation on the built optic could reach: the
Grate encoding's focus is the whole bundle
F.Representation => AwithX = Unit. Two calls withdifferent indices were the same optic.
Decision: retire it (option b). Remove the method, record the break, migrate callers to
MultiFocus.representable+.at(i). Removed rather than deprecated, matching this line'sdead-surface policy: MiMa is off (
tlMimaPreviousVersions := Set.emptyinmima.sbt) and every 0.xbreak is listed there and documented in the changelog (
Affine.apply, theJsonFieldsPrismaliases,ConfluentWire.resolvingRecord,AvroCodec#decodeUnsafeall left the same way).API-compatibility consequence (published artifact)
Source- and binary-breaking for direct callers of cats-eo 0.18.x. MiMa is disabled on this line,
so nothing fails at build time; the break is recorded in
mima.sbt's newest-first list (entry0.19)and in
CHANGELOG.mdunder[Unreleased] → Removed. Migration is one line:and where the index was wanted for a read, position is now a call-time argument:
(
mima.sbtand the scaladoc tombstone label the break0.19, assuming it ships as a minor like everybreak so far; if it goes out as
0.18.1, those two labels are a one-line edit.)Why not (a) — give the index a runtime meaning
X = Unit, and the publicXis existential, so a.leadaccessor would need a stored field on the optic class plus a runtime witness match to recover it
(the shape
Function1BroadcastOpticneeds for composition in81a53d3d) — sugar over.at(repr0),which is the same read with no optics-side state.
MultiFocusLeadPositionwas dropped in12b8279for having zero readers, and the fold spike records "the lead value is empirically deadthrough
.modify— every shipped path discards it".site/docs/multifocus.md§Historical landmarksalready tells that story; this PR removes the last remnant of it.
frommust tabulate positionally;sampling one index rebuilds a constant container. That is exactly the collapse the sibling change
set
fix/grate-positional-compositionfixes — and it is now pinned negatively here.Grate.at, deleted with the carrierin the Grate fold; its representative-focus slot does not exist in this encoding.
Why not (c) — keep it vestigial but pinned
That freezes a lie into a published signature ("pass your index here" for a parameter that changes
nothing). The useful half of (c) is available without it:
UnlawfulFixturesSpecnow proves theshipped pointwise pin discriminates a lead-shaped write. A future
.lead— if ever wanted — needs anew witness type and a new constructor anyway, so it does not need this parameter to survive.
Changes
core/data/MultiFocus.scala—representableAtdeleted;representable's scaladoc states thecontract explicitly (position is a read-time argument;
Representablehas no canonical index) witha one-line tombstone for the removed name.
core/MultiFocusFunction1Spec.scala— the absorbed-Grate block rewired ontorepresentableandstrengthened: reads pinned against the instance's own
indexfor twoRepresentables, the secondpermuted (index order ≠ field order, so "index 0" is the third field), and
modify/replacepinned against the instance's own
map/tabulate.tests/UnlawfulFixturesSpec.scala— negative fixture: a hand-written lead-sampling twin of thefactory fails
MultiFocusLaws.modifyIdentityand the pointwise-mappin, while the shippedrepresentablepasses both on the same carrier and input.mima.sbt,CHANGELOG.md,site/docs/multifocus.md,site/docs/optics.md— break recorded, theremoved name dropped from prose and the constructor list.
Verification
JDK 25 (Temurin 25.0.4.1 — the JDK this box supports; its default
javais a broken JDK 27):sbt scalafmtAll testFailed 0, Errors 0(1 / 18 / 10 / 11 / 46 / 56 / 268 / 130 / 424 examples; last module 237 = 235 passed + 2 pre-existing skips)sbt 'core/testOnly dev.constructive.eo.MultiFocusFunction1Spec'sbt 'tests/testOnly dev.constructive.eo.UnlawfulFixturesSpec'sbt scalafmtCheckAllsbt docs/mdocsbt docs/laikaSiteNon-vacuity: the fixture's lead-sampling optic returns
falsefromMultiFocusLaws.modifyIdentityand differs from its carrier instance's
map, while the shippedrepresentablereturnstrueandagrees — same law, same carrier, same input.
Rebase: based on
main(188f10bd).fix/grate-positional-composition(81a53d3d) is present inthe authoring clone's object store, so the rebase was verified by cherry-picking this commit onto it:
clean (one 3-way merge, no conflicts), and on the merged state
MultiFocusFunction1Specis 7 examples /329 expectations green,
UnlawfulFixturesSpec4 / 6 green, fullsbt testgreen.One cross-change-set follow-up
81a53d3dadds prose naming the removed factory as a live Grate factory; after both land three spotsgo stale —
site/tools/gen-qa-report.py:198(generates the QA page caption atsite/docs/quality-assurance.md:72,100) and the header comment attests/…/GrateShapeSpec.scala:10. They should readMultiFocus.tuple/representable/apply(plus a
.at(i)mention if wanted). Deliberately untouched here: those lines do not exist onmain,so editing them would break this PR's rebase.