chore(cabal-install-solver): add comments and improve readability

Author: andreabedini

cabal-install-solver/src/Distribution/Solver/Modular/Builder.hs

@@ -62,41 +62,48 @@ type LinkingState = M.Map (PN, I) [PackagePath]
 -- We also adjust the map of overall goals, and keep track of the
 -- reverse dependencies of each of the goals.
 extendOpen :: QPN -> [FlaggedDep QPN] -> BuildState -> BuildState
-extendOpen qpn' gs s@(BS { rdeps = gs', open = o' }) = go gs' o' gs
+extendOpen qpn deps buildState@(BS { rdeps = rdeps0, open = goals0 }) = go rdeps0 goals0 deps
   where
     go :: RevDepMap -> [OpenGoal] -> [FlaggedDep QPN] -> BuildState
-    go g o []                                             = s { rdeps = g, open = o }
-    go g o ((Flagged fn@(FN qpn _) fInfo t f)  : ngs) =
-        go g (FlagGoal fn fInfo t f (flagGR qpn) : o) ngs
-      -- Note: for 'Flagged' goals, we always insert, so later additions win.
-      -- This is important, because in general, if a goal is inserted twice,
-      -- the later addition will have better dependency information.
-    go g o ((Stanza sn@(SN qpn _) t)           : ngs) =
-        go g (StanzaGoal sn t (flagGR qpn) : o) ngs
-    go g o ((Simple (LDep dr (Dep (PkgComponent qpn _) _)) c) : ngs)
-      | qpn == qpn'       =
-            -- We currently only add a self-dependency to the graph if it is
-            -- between a package and its setup script. The edge creates a cycle
-            -- and causes the solver to backtrack and choose a different
-            -- instance for the setup script. We may need to track other
-            -- self-dependencies once we implement component-based solving.
+    go rdeps goals [] =
+      buildState { rdeps = rdeps, open = goals }
+
+    go rdeps goals ((Flagged fn@(FN qpn' _) fInfo t f) : fdeps) =
+      go rdeps (FlagGoal fn fInfo t f (flagGR qpn') : goals) fdeps
+
+    -- Note: for 'Flagged' goals, we always insert, so later additions win.
+    -- This is important, because in general, if a goal is inserted twice,
+    -- the later addition will have better dependency information.
+    go rdeps goals ((Stanza sn@(SN qpn' _) t) : fdeps) =
+        go rdeps (StanzaGoal sn t (flagGR qpn') : goals) fdeps
+
+    go rdeps goals ((Simple (LDep dr (Dep (PkgComponent qpn' _) _)) c) : fdeps)
+      | qpn' == qpn =
+          -- We currently only add a self-dependency to the graph if it is
+          -- between a package and its setup script. The edge creates a cycle
+          -- and causes the solver to backtrack and choose a different
+          -- instance for the setup script. We may need to track other
+          -- self-dependencies once we implement component-based solving.
           case c of
-            ComponentSetup -> go (M.adjust (addIfAbsent (ComponentSetup, qpn')) qpn g) o ngs
-            _              -> go                                                    g  o ngs
-      | qpn `M.member` g  = go (M.adjust (addIfAbsent (c, qpn')) qpn g)   o  ngs
-      | otherwise         = go (M.insert qpn [(c, qpn')]  g) (PkgGoal qpn (DependencyGoal dr) : o) ngs
-          -- code above is correct; insert/adjust have different arg order
-    go g o ((Simple (LDep _dr (Ext _ext )) _)  : ngs) = go g o ngs
-    go g o ((Simple (LDep _dr (Lang _lang))_)  : ngs) = go g o ngs
-    go g o ((Simple (LDep _dr (Pkg _pn _vr))_) : ngs) = go g o ngs
+            ComponentSetup -> go (M.adjust (addIfAbsent (ComponentSetup, qpn)) qpn' rdeps) goals fdeps
+            _              -> go rdeps  goals fdeps
+      | qpn' `M.member` rdeps =
+          go (M.adjust (addIfAbsent (c, qpn)) qpn' rdeps) goals fdeps
+      | otherwise =
+          -- Note: insert/adjust have different arg order
+          go (M.insert qpn' [(c, qpn)] rdeps) (PkgGoal qpn' (DependencyGoal dr) : goals) fdeps
+
+    go rdeps o ((Simple (LDep _dr (Ext _ext )) _c) : goals) = go rdeps o goals
+    go rdeps o ((Simple (LDep _dr (Lang _lang)) _c) : goals) = go rdeps o goals
+    go rdeps o ((Simple (LDep _dr (Pkg _pn _vr)) _c) : goals) = go rdeps o goals
 
     addIfAbsent :: Eq a => a -> [a] -> [a]
     addIfAbsent x xs = if x `elem` xs then xs else x : xs
 
-    -- GoalReason for a flag or stanza. Each flag/stanza is introduced only by
-    -- its containing package.
-    flagGR :: qpn -> GoalReason qpn
-    flagGR qpn = DependencyGoal (DependencyReason qpn M.empty S.empty)
+-- GoalReason for a flag or stanza. Each flag/stanza is introduced only by
+-- its containing package.
+flagGR :: qpn -> GoalReason qpn
+flagGR qpn = DependencyGoal (DependencyReason qpn M.empty S.empty)
 
 -- | Given the current scope, qualify all the package names in the given set of
 -- dependencies and then extend the set of open goals accordingly.
@@ -127,12 +134,14 @@ build = ana go
     go :: Linker BuildState -> TreeF () QGoalReason (Linker BuildState)
     go s = addLinking (linkingState s) $ addChildren (buildState s)
 
+-- | Add children to the tree based on the current build state.
 addChildren :: BuildState -> TreeF () QGoalReason BuildState
 
 -- If we have a choice between many goals, we just record the choice in
 -- the tree. We select each open goal in turn, and before we descend, remove
 -- it from the queue of open goals.
 addChildren bs@(BS { rdeps = rdm, open = gs, next = Goals })
+  -- No goals left. We have done.
   | L.null gs = DoneF rdm ()
   | otherwise = GoalChoiceF rdm $ P.fromList
                                 $ L.map (\ (g, gs') -> (close g, bs { next = OneGoal g, open = gs' }))
@@ -254,16 +263,17 @@ buildTree idx igs =
     build Linker {
         buildState = BS {
             index = idx
-          , rdeps = M.fromList (L.map (\ qpn -> (qpn, []))              qpns)
-          , open  = L.map topLevelGoal qpns
+          , rdeps = M.fromList [(qpn, []) | qpn <- qpns]
+          , open  = [ PkgGoal qpn UserGoal | qpn <- qpns ]
           , next  = Goals
           }
       , linkingState = M.empty
       }
   where
-    topLevelGoal qpn = PkgGoal qpn UserGoal
+    -- The package names are interpreted as top-level goals in the host stage.
+    path = PackagePath Stage.Host QualToplevel
+    qpns = [ Q path pn | pn <- igs ]
 
-    qpns = L.map (Q (PackagePath Stage.Host QualToplevel)) igs
 
 {-------------------------------------------------------------------------------
   Goals

cabal-install-solver/src/Distribution/Solver/Modular/Dependency.hs

@@ -90,14 +90,36 @@ type FlaggedDeps qpn = [FlaggedDep qpn]
 
 -- | Flagged dependencies can either be plain dependency constraints,
 -- or flag-dependent dependency trees.
+--
+-- Note: this is a recursive data structure representing a tree of dependencies.
+--
+-- Note 2: why LDep contains its own DependencyReason? I am thinking it should
+-- be external to this type. Basically you traverse the tree and the flag and
+-- stanza choices are the DepedencyReason?
 data FlaggedDep qpn
   = -- | Dependencies which are conditional on a flag choice.
-    Flagged (FN qpn) FInfo (TrueFlaggedDeps qpn) (FalseFlaggedDeps qpn)
-  | -- | Dependencies which are conditional on whether or not a stanza
+    Flagged
+      (FN qpn)
+      -- ^ The qualified flag name.
+      FInfo
+      -- ^ The flag information.
+      (FlaggedDeps qpn)
+      -- ^ Extra dependencies when the flag is true.
+      (FlaggedDeps qpn)
+      -- ^ Extra dependencies when the flag is false.
+  | -- | Dependencies which are conditional on whether or not a stanza.
     -- (e.g., a test suite or benchmark) is enabled.
-    Stanza (SN qpn) (TrueFlaggedDeps qpn)
-  | -- | Dependencies which are always enabled, for the component 'comp'.
-    Simple (LDep qpn) Component
+    Stanza
+      (SN qpn)
+      -- ^ The qualified stanza name.
+      (FlaggedDeps qpn)
+      -- ^ Extra dependencies when stanza is enabled.
+  | -- | Dependencies which are always enabled.
+    Simple
+      (LDep qpn)
+      -- ^ The dependency.
+      Component
+      -- ^ The component of `qpn` introducing the dependency.
   deriving Show
 
 -- | Conservatively flatten out flagged dependencies
@@ -111,16 +133,18 @@ flattenFlaggedDeps = concatMap aux
     aux (Stanza _ t) = flattenFlaggedDeps t
     aux (Simple d c) = [(d, c)]
 
-type TrueFlaggedDeps qpn = FlaggedDeps qpn
-type FalseFlaggedDeps qpn = FlaggedDeps qpn
-
 -- | A 'Dep' labeled with the reason it was introduced.
 --
 -- 'LDep' intentionally has no 'Functor' instance because the type variable
 -- is used both to record the dependencies as well as who's doing the
 -- depending; having a 'Functor' instance makes bugs where we don't distinguish
 -- these two far too likely. (By rights 'LDep' ought to have two type variables.)
-data LDep qpn = LDep (DependencyReason qpn) (Dep qpn)
+data LDep qpn
+  = LDep
+    (DependencyReason qpn)
+    -- ^ The reason the dependency was introduced.
+    (Dep qpn)
+    -- ^ The dependency itself.
   deriving Show
 
 -- | A dependency (constraint) associates a package name with a constrained
@@ -139,21 +163,35 @@ data Dep qpn
 
 -- | An exposed component within a package. This type is used to represent
 -- build-depends and build-tool-depends dependencies.
-data PkgComponent qpn = PkgComponent qpn ExposedComponent
+data PkgComponent qpn
+  = PkgComponent
+      qpn
+      -- ^ The qualified name of the package.
+      ExposedComponent
+      -- ^ The component exposed by the package.
   deriving (Eq, Ord, Functor, Show)
 
 -- | A component that can be depended upon by another package, i.e., a library
 -- or an executable.
 data ExposedComponent
-  = ExposedLib LibraryName
-  | ExposedExe UnqualComponentName
+  = -- | A library component
+    ExposedLib LibraryName
+  | -- | An executable component
+    ExposedExe UnqualComponentName
   deriving (Eq, Ord, Show)
 
 -- | The reason that a dependency is active. It identifies the package and any
 -- flag and stanza choices that introduced the dependency. It contains
 -- everything needed for creating ConflictSets or describing conflicts in solver
 -- log messages.
-data DependencyReason qpn = DependencyReason qpn (Map Flag FlagValue) (S.Set Stanza)
+data DependencyReason qpn
+  = DependencyReason
+      qpn
+      -- ^ The qualified name of the dependent package.
+      (Map Flag FlagValue)
+      -- ^ The flag choices that introduced the dependency.
+      (S.Set Stanza)
+      -- ^ The stanza choices that introduced the dependency.
   deriving (Functor, Eq, Show)
 
 -- | Print the reason that a dependency was introduced.

cabal-install-solver/src/Distribution/Solver/Modular/Index.hs

@@ -31,10 +31,15 @@ type Index = Map PN (Map I PInfo)
 -- globally, for reasons external to the solver. We currently use this
 -- for shadowing which essentially is a GHC limitation, and for
 -- installed packages that are broken.
-data PInfo = PInfo (FlaggedDeps PN)
-                   (Map ExposedComponent ComponentInfo)
-                   FlagInfo
-                   (Maybe FailReason)
+data PInfo = PInfo
+  (FlaggedDeps PN)
+  -- ^ The package dependencies, whether they are conditional on a flag, a
+  -- stanza or always active.
+  (Map ExposedComponent ComponentInfo)
+  -- ^ Info associated with each library and executable component.
+  FlagInfo
+  --
+  (Maybe FailReason)
 
 -- | Info associated with each library and executable in a package instance.
 data ComponentInfo = ComponentInfo {

cabal-install-solver/src/Distribution/Solver/Modular/Tree.hs

@@ -49,19 +49,52 @@ type Weight = Double
 --
 -- TODO: The weight type should be changed from [Double] to Double to avoid
 -- giving too much weight to preferences that are applied later.
-data Tree d c =
-    -- | Choose a version for a package (or choose to link)
-    PChoice QPN RevDepMap c (WeightedPSQ [Weight] POption (Tree d c))
+--
+-- Note: this the tree *of possible choices*, which is used to explore all
+-- possible solutions to a given problem. It does not describe a single solution.
+data Tree d c
+  = -- | Choose a version for a package (or choose to link)
+    PChoice
+      QPN
+      -- ^ The package to choose an instance for
+      RevDepMap
+      -- ^ The reverse dependency map (FIXME ?)
+      c
+      -- ^ Additional data for the choice node
+      (WeightedPSQ [Weight] POption (Tree d c))
+      -- ^ Weighted list of possible options (`POption`) paired with the subsequent search tree.
 
-    -- | Choose a value for a flag
-    --
-    -- The Bool is the default value.
-  | FChoice QFN RevDepMap c WeakOrTrivial FlagType Bool (WeightedPSQ [Weight] Bool (Tree d c))
+  | -- | Choose a value for a flag.
+    FChoice
+      QFN
+      -- ^ The flag to choose a value for.
+      RevDepMap
+      -- ^ The reverse dependency map (FIXME ?).
+      c
+      -- ^ Additional data for the choice node.
+      WeakOrTrivial
+      -- ^ Whether the choice should be deferred.
+      FlagType
+      -- ^ Whether the flag is manual or automatic.
+      Bool
+      -- ^ The flag default value
+      (WeightedPSQ [Weight] Bool (Tree d c))
+      -- ^ Weighted list of possible options paired with the subsequent search tree.
 
-    -- | Choose whether or not to enable a stanza
-  | SChoice QSN RevDepMap c WeakOrTrivial (WeightedPSQ [Weight] Bool (Tree d c))
+  | -- | Choose whether or not to enable a stanza.
+    SChoice
+      QSN
+      -- ^ The stanza to choose to enable or disable.
+      RevDepMap
+      -- ^ The reverse dependency map (FIXME ?).
+      c
+      -- ^ Additional data for the choice node.
+      WeakOrTrivial
+      -- ^ Whether the choice should be deferred.
+      (WeightedPSQ [Weight] Bool (Tree d c))
+      -- ^ Weighted list of possible options paired with the subsequent search tree.
 
-    -- | Choose which choice to make next
+  | -- | Choose which choice to make next
     --
     -- Invariants:
     --
@@ -72,13 +105,25 @@ data Tree d c =
     --   invariant that the 'QGoalReason' cached in the 'PChoice', 'FChoice'
     --   or 'SChoice' directly below a 'GoalChoice' node must equal the reason
     --   recorded on that 'GoalChoice' node.
-  | GoalChoice RevDepMap (PSQ (Goal QPN) (Tree d c))
+    GoalChoice
+      RevDepMap
+      -- ^ The reverse dependency map (FIXME ?).
+      (PSQ (Goal QPN) (Tree d c))
+      -- ^ Priority search queue associating a goal with the search tree.
 
-    -- | We're done -- we found a solution!
-  | Done RevDepMap d
+  | -- | We're done -- we found a solution!
+    Done
+      RevDepMap
+      -- ^ The reverse dependency map (FIXME ?).
+      d
+      -- ^ The solution.
 
-    -- | We failed to find a solution in this path through the tree
-  | Fail ConflictSet FailReason
+  | -- | We failed to find a solution in this path through the tree
+    Fail
+      ConflictSet
+      -- ^ The conflict set.
+      FailReason
+      -- ^ The reason for failure.
 
 -- | A package option is a package instance with an optional linking annotation
 --
@@ -96,7 +141,12 @@ data Tree d c =
 -- dependencies must also be the exact same).
 --
 -- See <http://www.well-typed.com/blog/2015/03/qualified-goals/> for details.
-data POption = POption I (Maybe PackagePath)
+data POption
+  = POption
+      I
+      -- ^ The choosen package instance.
+      (Maybe PackagePath)
+      -- ^ The package this choice is linked to (if any).
   deriving (Eq, Show)
 
 data FailReason = UnsupportedExtension Extension
@@ -132,7 +182,14 @@ data FailReason = UnsupportedExtension Extension
   deriving (Eq, Show)
 
 -- | Information about a dependency involved in a conflict, for error messages.
-data ConflictingDep = ConflictingDep (DependencyReason QPN) (PkgComponent QPN) CI
+data ConflictingDep
+  = ConflictingDep
+      (DependencyReason QPN)
+      -- ^ The reason for the dependency.
+      (PkgComponent QPN)
+      -- ^ The component of the package that caused the conflict.
+      CI
+      -- ^ The constrained instance.
   deriving (Eq, Show)
 
 -- | Functor for the tree type. 'a' is the type of nodes' children. 'd' and 'c'