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
Post by Chris WongDavid,
Post by David FeuerPost by Herbert Valerio RiedelAlright, 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.
Post by David FeuerAnd 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 )
Post by David FeuerSo 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
_______________________________________________
Libraries mailing list
http://mail.haskell.org/cgi-bin/mailman/listinfo/libraries