Remove Semigroup and Monoid instances for Either
Andrew Martin
andrew.thaddeus at gmail.com
Thu May 24 14:48:40 UTC 2018
There's a project at work that a colleague and I worked on where we both
assumed that the Monoid instance for Either would behave like the one I
proposed, and then it didn't. There's also someone on reddit (
https://www.reddit.com/r/haskell/comments/8lbzan/semigroup_maybe_too_strict/dzhwzjs/)
would wrote:
> There is a bug somewhere anyway as instances for Either () a and Maybe a
really should match, but I'm no longer sure that the bug is in the Maybe
instance rather than the Either () a one.
Those are my two data points. It's small, but I wanted to gauge what others
thought.
I'm familiar with the discussion around the Semigroup/Monoid instance for
Map. There was no general consensus on whether or not there should be a
gap, and I fall pretty strongly on the other side of the fence for reasons
that are listed elsewhere in that thread. In the case of the
Semigroup/Monoid instances for Either, I don't really care as much since
the current instances have such little utility that I've never even seen
anyone use them in the wild.
On Thu, May 24, 2018 at 10:28 AM, Herbert Valerio Riedel <hvriedel at gmail.com
> wrote:
> Andrew,
>
> On 2018-05-24 at 09:56:48 -0400, Andrew Martin wrote:
> > One would expect that the Semigroup instances for `Maybe a` and `Either
> ()
> > a` would have the same behaviors
>
> Given this expectation is used as motivation for the proposal, could you
> expand the rationale from which you derive this expectation? Are there
> data-points that give reason to believe that the current instance are
> actually problematic, as they result in code being written that behaves
> in an unexpected way that isn't detected right-away by programmers?
>
>
> > For now, all I'm proposing is removing the existing Semigroup and Monoid
> > instances for Either. I do not believe that new instances should be given
> > immidiately, since that would be the worst kind of breaking change: one
> > that the compiler does not error on.
>
> If the intent is to re-add different instance at a later time, then I
> strongly oppose this IMHO wrong reasoning -- either get rid of the
> instances forever, or switch over gap-less
>
> We had a very similar discussion a couple months ago; I'll just refer
> to the argument from then which applies here as well with minor
> modifications, see below
>
>
> -------------------- Start of forwarded message --------------------
> Subject: Re: Proposal: Remove Semigroup and Monoid instances for
> Data.Map, Data.IntMap, Data.HashMap
> From: Oleg Grenrus <oleg.grenrus at iki.fi>
> Message-ID: <ab7a0a16-1c64-1a9d-0733-7042ef2866b6 at iki.fi>
> Date: Wed, 14 Feb 2018 17:48:36 +0200
>
> I don't like a "no-instances" window. We can change the instances to
> correct
> ones directly. That's what semantic versioning is about, to be able to
> communicate semantic changes.
>
> We should go directly to
>
> Semigroup v => Semigroup (Map k v)
> Semigroup v => Monoid (Map k v)
>
> If `containers-0.6` or `containers-1.0` included some other (compile-time)
> breaking change: even better.
>
> I want to point out that a lot of code *will* break at compile-time with
> the
> change. In polymorphic setting `v` doesn't have `Semigroup v`
> attached. Many
> concrete types arent `Semigroup` (e.g. `Int`; here and later when I say
> isn't
> `Semigroup`, I mean that it isn't `Monoid` either).
>
> I tried to compile our `futurice-prelude` [1] and one small size internal
> app (40 modules) with patched containers: things break at compile time
>
> This change *won't get thru unnoticed*. See below for details.
>
> Also people (we and others) use `mempty` for `Map.empty`, but for `mempty`
> semantics won't change. We might need to use `Map.empty` if `v` isn't
> `Semigroup`,
> but that's catched by compiler. Note: that would mean to use `Map.empty`
> everywhere
> if there is no-instances window.
>
> I also conjecture, based on the results, that `Ord k => Monoid (Map k
> v)` for
> `union` is currently avoided in libs. Maybe because it does *the wrong
> thing*,
> so people don't use it. In our own 50kLOC codebase we have three
> `Map.unionWith` or `Map.unionsWith`, one of which is
>
> groupThings = Map.unionsWith (<>) . map f).
>
> I think `Map k String` or `Map k Text` things might be common in private
> codebases (not ours). Maybe not the most popular opinion, but I think they
> deserve to break.
>
>
> Something else: If we replace the instance, we have to leave it out of
> `containers` for older than `base-4.9`.
> - Either we provide `Semigroup` (and `Monoid` !) in `semigroups` (See +) or
> - just have `base >= 4.9` in that release of `containers`
> (that will be three GHCs soon).
>
>
> To conclude, The brekage on Hackage would be driven by things not having
> `Semigroup` instances, not semantics. There might be subtle breakages,
> but not
> something we as community cannot handle. So let's just make a change.
>
> Best regards, Oleg
>
> + At this point we want `Semigroup v` for `Monoid (Map k v)`, and because
> `semigroups` depend on `containers` (and not other way), we'd need to
> have
> `Monoid (Map k v)` also in `semigroups`.
>
> ---
>
> I did an experiment: compile our `futurice-prelude` [1] with amended
> `containers`, I changed only instances for `Map`.
>
>
> Removal of the instances breaks Cabal, so I could test far enough what
> happens:
>
> Distribution/Simple/Program/GHC.hs:546:12: error:
> • No instance for (Semigroup (Map Extension String))
> arising from a use of ‘gmempty’
> • In the expression: gmempty
> In an equation for ‘mempty’: mempty = gmempty
> In the instance declaration for ‘Monoid GhcOptions’
> |
> 546 | mempty = gmempty
> | ^^^^^^^
>
> This is one place where we want `union` semantics.
> OTOH in current development version of Cabal the map is of type
>
> Map Extension (Maybe Compiler.Flag),
>
> as we moving avoid from stringly-typed programming. And `Flag` doesn't have
> `Semigroup` instance.
>
>
> Second step: Let's see where Monoid (Map k v) is used. We cannot
> deprecate the
> instance, but we can se where it's used by a -fdefer-type-error trick
> (thanks Bartosz Nitka aka niteria [2])
> Note that this step actually has changes from third step already, so
> we don't see failures where `Semigroup v` context would have caused
> compilation
> failure.
>
> To my happy surprise the instance were used only once in Cabal-2.0.1.1
> (above),
> and then in `futurice-prelude` itself where it would also break with
> `Semigroup v`.
> So 231 packages at the bottom of dependency graph of lens+servant apps
> don't use `Monoid (Map k v)`. Interesting fact, I'd say.
>
>
> Third step: If I change instance to use `unionWith (<>)`, then the first
> failure in our production code is `reducers-3.12.2`
>
> src/Data/Semigroup/Reducer.hs:233:10: error:
> • Could not deduce (Semigroup v)
> arising from the superclasses of an instance declaration
> from the context: Ord k
> bound by the instance declaration
> at src/Data/Semigroup/Reducer.hs:233:10-42
> Possible fix:
> add (Semigroup v) to the context of the instance declaration
> • In the instance declaration for ‘Reducer (k, v) (Map k v)’
> |
> 233 | instance Ord k => Reducer (k, v) (Map k v) where
>
> and `servant-0.13`
>
> src/Servant/Server/Internal.hs:691:34: error:
> • No instance for (Data.Semigroup.Semigroup
> (Router' env RoutingApplication))
> arising from a use of ‘mempty’
> • In the first argument of ‘StaticRouter’, namely ‘mempty’
> In the expression: StaticRouter mempty mempty
> In an equation for ‘route’:
> route Proxy _ _ = StaticRouter mempty mempty
> |
> 691 | route Proxy _ _ = StaticRouter mempty mempty
>
> and `swagger2-2.2`. `SwaggerMonoid` differes for `Maybe`, but has default
> implementation in terms of `Monoid`.
>
> src/Data/Swagger/Internal/Utils.hs:116:10: error:
> • Could not deduce (Data.Semigroup.Semigroup v)
> arising from a use of
> ‘Data.Swagger.Internal.Utils.$dmswaggerMappend’
> from the context: Ord k
> bound by the instance declaration
> at src/Data/Swagger/Internal/Utils.hs:116:10-41
> Possible fix:
> add (Data.Semigroup.Semigroup v) to the context of
> the instance declaration
> • In the expression:
> Data.Swagger.Internal.Utils.$dmswaggerMappend @Map k v
> In an equation for ‘swaggerMappend’:
> swaggerMappend
> = Data.Swagger.Internal.Utils.$dmswaggerMappend @Map k v
> In the instance declaration for ‘SwaggerMonoid (Map k v)’
> |
> 116 | instance Ord k => SwaggerMonoid (Map k v)
>
> And then we get to `futurice-prelude` which also fails:
>
> src/Futurice/Prelude.hs:177:41: error:
> • Could not deduce (Semigroup v) arising from a use of ‘ifolded’
>
> Funny thing about that code, is that there we work with `Map k (Map l
> v)` and
> fold them. For other one we want `UnionWith` and for other any semantic
> should
> be ok (we have tests, they *will* break if I get things wrong).
>
> We also have a `Map` wrapper which also breaks loudly.
>
>
> Fourth step: fix above and contunue investigation where `Monoid (Map k
> v)` is
> used. I built one internal application which pulls in even more
> dependencies
> in addition to what `futurice-prelude` already depends upon (465 deps, e.g.
> `amazonka`).
>
> - Two of our internal libraries use `mempty` (second one a lot). Not
> breaking.
>
> - One package uses a `Map` wrapper used above (for the sake of experiment I
> passed the `Warning` constraint down)
>
> - The app itself uses `mempty` of `folded` on things which aren't
> `Semigroup`.
>
> - `yaml-0.8.28` breaks:
>
> Data/Yaml/Parser.hs:154:13: error:
> • Could not deduce (Data.Map.Internal.Warning YamlValue)
> arising from a use of ‘await’
>
> `YamlValue` doesn't have a `Semigroup` (or `Monoid`) instance.
>
> - `amazonka-core-1.5.0`: `mempty`: non silent breakage.
>
> src/Network/AWS/Data/XML.hs:252:49: error:
> • No instance for (containers-0.5.11.0:Data.Map.Internal.Warning
> Text)
> arising from a use of ‘mempty’
> • In the second argument of ‘Element’, namely ‘mempty’
> In the first argument of ‘NodeElement’, namely
> ‘(Element (name k) mempty [NodeContent v])’
> In the expression:
> NodeElement (Element (name k) mempty [NodeContent v])
> |
> 252 | node (k, v) = NodeElement (Element (name k) mempty
> [NodeContent v])
>
>
> [1]: https://github.com/futurice/haskell-futurice-prelude
> [2]: https://ghc.haskell.org/trac/ghc/ticket/12014#comment:6
>
>
> On 14.02.2018 01:50, Herbert Valerio Riedel wrote:
> > David,
> >
> >>> Alright, then let's do a little Gedankenexperiment; what if you release
> >>> a containers-0.9.0.0 (which drops the instances) and a
> >>> containers-1.0.0.0 (which 'adds back' the desired new instances) on the
> >>> same day!
> >>>
> >>> ...wouldn't this allow us to have the cake and eat it too? ;-)
> >> It would let us have some cake. Users would be able to test against
> >> 0.9, in theory. But they'd have to do it intentionally.
> > This is what I was trying to get at: that's always the case. It doesn't
> > matter whether you release it same day, a week later, or a year later. I
> > have to intentionally *not* skip the 0.9 release. In fact, I'd want to
> > skip the 0.9 release myself if possible, as supporting both,
> > containers-0.9 and containers-1.0 in the same consumer code is going to
> > be awkward enough for me not wanting to do that (or I'd have to
> > basically not use the instances at all); If I care about the new
> > instances, I'd rather specify `containers ^>= 1.0.0.0` and thus not
> > support containers-0.9.
> >
> > I would have proceeded to point out in my Gedankenexperiment, that in
> > order to use such a container-0.9 release, you'd have to force every
> > package that depends on `containers` to go through such a 0.9 phase in
> > lockstep, which in itself poses an ordeal in terms of package update
> > logistic; and then if you spread the time between the 0.9 and the 1.0
> > release apart, repeat this dance a 2nd time, with yet another new major
> > version bump, which requires yet another round of consumer-code reviews
> > (*because* a major version increment was signalled; and we do that as
> > API consumers are supposed to pay attention to the major version
> > increment signal...)
> >
> > So consequently, if anything, a container-0.9 release would be a kind of
> > an artificial pseudo-release anyway IMO. You could even just condense it
> > into a cabal package flag of a containers-1.0 release, which disables
> > the Monoid/Semigroup instances; then you don't even need a distinct
> > container-0.9 release at all, nor do you have to contaminate the API
> > progression with an artificial container-0.9 discontinuity.
> >
> >> And Stack-based projects would probably need some shenanigans to deal
> >> with a version of containers that doesn't come with GHC.
> > I'm not convinced we should let the weakest link hold us back. If
> > there's limitations in the tooling, we should rather address them, rather
> > than contort ourselves; c.f. the Genius Tailor
> > ( http://www.wealthculture.cn/infoShow.asp?nid=880&sort=Article%20List )
> >
> >> So I really think that avoiding these subtle bugs requires at least a
> >> full GHC release cycle.
> > I don't think waiting a full GHC release would be either necessary nor
> > effective at addressing your concerns, which I consider to be disputable
> > arguments to begin with.
> >
> > -- hvr
> -------------------- End of forwarded message --------------------
>
--
-Andrew Thaddeus Martin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.haskell.org/pipermail/libraries/attachments/20180524/df4994e0/attachment.html>
More information about the Libraries
mailing list