Consider if other order of combination is more appropriate for the monoids on config types

[mgsloan]

Many of the fields use configField l <|> configField r. This means the value on the left overrides the value on the right. I noticed that this isn't uniformly followed. For example,

, configMonoidGhcOptions = Map.unionWith (++) (configMonoidGhcOptions l) (configMonoidGhcOptions r)

I would expect the order to be the opposite here, since ghc-options later in the list override earlier options. These definitions should be considered more carefully.

For now, in working on https://github.com/commercialhaskell/stack/issues/863, I am sticking with the same semantics as before. So, if this gets changed, that should also get modified.

[sjakobi]

I've been wondering why we don't use newtype wrappers like First for the fields and derive the Monoid instances using generic-deriving. Of course we'd need a few custom wrappers for the Maps and VersionRanges.

The upside would be reduced code size and obvious semantics. On the downside I only see some boilerplate during construction and access – not much of a downside IMO because it means coming across the semantics once more.

Am I missing something?

[mgsloan]

That makes sense to me! I think maybe it's explicit so that we can easily add more complicated monoid logic. It could even be good to enforce that the monoid behaves in a more isolated fashion, where each field can be considered independently

[sjakobi]

Here's a tiny sample converting UrlsMonoid to the deriving approach: https://github.com/sjakobi/stack/commit/f13e6895a1911fff64a66c37b5038a606bc58ac5

[mgsloan]

Nice, works well for that!

[sjakobi]

Working on #2095 I came across some more funky Monoid instances:

DepError:

instance Monoid DepError where
    mempty = DepError Nothing Map.empty
    mappend (DepError a x) (DepError b y) = DepError
        (maybe a Just b)
        (Map.unionWith C.intersectVersionRanges x y)

maybe a Just b == b <|> a – but I'm somewhat unsure that's intended…

SetupInfo:

    mappend l r =
        SetupInfo
        { siSevenzExe = siSevenzExe r <|> siSevenzExe l
        , siSevenzDll = siSevenzDll r <|> siSevenzDll l
        , siMsys2 = siMsys2 r <> siMsys2 l
        , siGHCs = Map.unionWith (<>) (siGHCs r) (siGHCs l)
        , siGHCJSs = Map.unionWith (<>) (siGHCJSs r) (siGHCJSs l)
        , siStack = Map.unionWith (<>) (siStack l) (siStack r) }

Here I'm just surprised that all fields except siStack are right-biased.

Maybe SetupInfo should get its own proper SetupInfoMonoid too or the type could be simplified somehow? I don't quite understand it yet.

[mgsloan]

For the DepError monoid, the particular use of DepError there means that if that field is a Just, they will be the same version (the available package version). So, that's fine.

The SetupInfo monoid is used here. Looks like stack's yaml is first in the list. So, this monoid currently prefers stack's setup-info over custom. This seems like a reasonable default - it makes it so that resolvers and lts snapshots can refer to very concrete GHC versions. If we allow setup-info to override these, then different ghcs could be fetched instead of official versions. On the other hand, that is more flexible.

I'm in favor of flipping the monoid around for siStack. Or was that a conscious decision? Thoughts, @borsboom ?

[sjakobi]

For the DepError monoid, the particular use of DepError there means that if that field is a Just, they will be the same version (the available package version). So, that's fine.

Thanks for the explanation! Would you mind if I change the instance to highlight that invariant?

    mappend (DepError a x) (DepError b y) = DepError
        (assert (a == b) a)
        (Map.unionWith C.intersectVersionRanges x y)

[mgsloan]

Sure! Now we'll see if my reasoning holds at runtime :)