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