refactor(cabal-install): generalise GenericInstallPlan to arbitrary node keys#12092
refactor(cabal-install): generalise GenericInstallPlan to arbitrary node keys#12092andreabedini wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors cabal-install’s install-plan graph abstractions to stop hardcoding UnitId as the node key, instead threading a shared Key type through GenericInstallPlan operations via a new IsGraph constraint. This is a behaviour-preserving abstraction tightening that keeps the current UnitId-keyed instantiation working while enabling future work (e.g. stage-qualified keys for cross compilation).
Changes:
- Replace the
IsUnitsynonym withIsGraphand generalise key-dependent APIs (depends,Processing,BuildOutcomes,lookup, closures, execution). - Update
ProjectBuildingcode to usenodeKey/generic keys instead ofinstalledUnitId/UnitId-specific types. - Adjust unit tests to use
IsGraphand add key-type constraints where required for pretty-printing/debug output.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| cabal-install/src/Distribution/Client/InstallPlan.hs | Introduces IsGraph and generalises plan operations/state from UnitId to an arbitrary shared node key. |
| cabal-install/src/Distribution/Client/ProjectBuilding.hs | Updates dependency-fold and build-status lookup to use generic node keys (nodeKey) and key-parameterised maps. |
| cabal-install/tests/UnitTests/Distribution/Client/InstallPlan.hs | Updates generators/tests to work with IsGraph and key pretty-printing/show constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ode keys
The install-plan machinery in Distribution.Client.InstallPlan was written to
be polymorphic in its two node types (`GenericPlanPackage ipkg srcpkg`), yet
it hardcoded the *key* of those nodes to `UnitId` everywhere: the `IsUnit`
synonym pinned `Key a ~ UnitId`, `Processing` held `Set UnitId`, and
`BuildOutcomes` was a `Map UnitId`. That is an over-specialisation — nothing
in the plan logic actually depends on the key being a `UnitId`; it only needs
the two node types to share a common key that is an `IsNode` key.
This replaces the `UnitId`-specific `IsUnit` constraint with
type IsGraph ipkg srcpkg = (IsNode ipkg, IsNode srcpkg, Key ipkg ~ Key srcpkg)
and threads the shared key type (`Key ipkg`) through the operations that
previously mentioned `UnitId` directly, so `Processing` and `BuildOutcomes`
become parameterised over the key. The few functions that print or report on
keys gain `Pretty`/`Show (Key ipkg)` constraints accordingly. Helpers become
more honest about what they need — e.g. `depends :: IsNode a => a -> [Key a]`
rather than `depends :: IsUnit a => a -> [UnitId]`.
This is a behaviour-preserving refactor. The sole instantiation on master,
`type PlanPackage = GenericPlanPackage InstalledPackageInfo (ConfiguredPackage
UnresolvedPkgLoc)`, remains `UnitId`-keyed, so there is no functional change.
The motivation is cross-compilation (haskell#11179), where install plans are keyed by
a stage-qualified key rather than a bare `UnitId`; extracting this
generalisation on its own removes the `UnitId` assumption ahead of that work
and keeps the eventual core change smaller.
31a8412 to
07c2b95
Compare
|
Great to see PRs split and refactors done in separate PRs. Could you add a PR template and a changelog? Is there UndecideableInstance unavoidable? |
| {-# LANGUAGE ConstraintKinds #-} | ||
| {-# LANGUAGE FlexibleContexts #-} | ||
| {-# LANGUAGE ScopedTypeVariables #-} | ||
| {-# LANGUAGE TupleSections #-} |
There was a problem hiding this comment.
It seems some of these extensions are not requires. May suggest adding stylish-haskell or something like that to the AI workflow?
Summary
Distribution.Client.InstallPlanwas already polymorphic in its two node types(
GenericPlanPackage ipkg srcpkg), yet it hardcoded the key of those nodes toUnitIdthroughout: theIsUnitsynonym pinnedKey a ~ UnitId,Processingheld
Set UnitId, andBuildOutcomeswas aMap UnitId.This replaces the
UnitId-specificIsUnitconstraint withand threads the shared key type (
Key ipkg) through the operations thatpreviously mentioned
UnitIddirectly, soProcessingandBuildOutcomesbecome parameterised over the key. Functions that print or report on keys gain
Pretty/Show (Key ipkg)constraints accordingly, and helpers become morehonest about what they need — e.g.
depends :: IsNode a => a -> [Key a]insteadof
depends :: IsUnit a => a -> [UnitId].Why
Nothing in the plan logic actually depends on the key being a
UnitId; it onlyneeds the two node types to share a common
IsNodekey. Removing thatover-specialisation is a small tightening of the abstraction on its own.
The concrete motivation is cross-compilation (#11179), where install plans are
keyed by a stage-qualified key rather than a bare
UnitId. This is apreparatory refactor extracted from that PR — landing it separately shrinks
the eventual core change and keeps it reviewable.
Behaviour
Behaviour-preserving. The sole instantiation on
master—type PlanPackage = GenericPlanPackage InstalledPackageInfo (ConfiguredPackage UnresolvedPkgLoc)— remains
UnitId-keyed, so there is no functional change. No changelog entry(internal refactor, not user-facing).
QA notes
cabal build allis green.cabal-install/InstallPlan/unit tests pass.-Werrorvalidate build (cabal.validate.project) is clean; hlint reports no hints.