Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make functions correspond more closely to Data.Map behavior #50

Merged
merged 1 commit into from
Aug 24, 2019

Conversation

ali-abrar
Copy link
Collaborator

@ali-abrar ali-abrar commented Aug 22, 2019

This is a breaking change. The more "monoidal" behaviors that this
reverts could eventually be re-added as a separate module that more
thoroughly and systematically applies the monoid/semigroup operations.

The goal of Data.Map.Monoidal is to be as similar to Data.Map as
possible except for the Monoid/Semigroup instance that it replaces.

Thanks to @cgibbard and @bgamari for helping clarify this goal.

This is a breaking change. The more "monoidal" behaviors that this
reverts could eventuallly be re-added as a separate module that more
thoroughly and systematically applies the monoid/semigroup operations.

The goal of Data.Map.Monoidal is to be as similar to Data.Map as
possible except for the Monoid/Semigroup instance that it replaces.
@3noch
Copy link
Contributor

3noch commented Aug 23, 2019

Some ideas worth considering before we go all out on this revert. @bgamari and @ali-abrar and I are already in agreement that fromList in this MR is a footgun. Ideas:

  1. Don't export fromList, insert, and mapKeys since they can be easily reconstructed with fromListWith const, insertWith const, and mapKeysWith const. This avoids ambiguity and forces developers to be explicit about what kind of merging they want.
  2. If maintaining API parity with Data.Map is an extremely important design goal, then make a separate Compat module (better name please) that exports this API but leaves the ambiguous functions out by default.
  3. Flip that around and make them exported by the default module but expose another module (Safe?) that doesn't export ambiguous functions.

At the very least we need to have some clear documentation on both of these fronts:

  1. Library design direction. The fact that we went one direction and now plan to revert means there is not clear direction and we need that to be permanently fixed.
  2. Clearly document each function that could be ambiguous w.r.t. to merging keys.

@cgibbard
Copy link

cgibbard commented Aug 24, 2019

The clear direction (at least in my mind) is that this library is just Data.Map, etc. with different instances. We should be able to auto-generate it in some way, implementing everything in terms of coerce, which would be good, since then perhaps we'd get Data.Map.Merge etc. and there would be no question as to how any given thing worked: it works in exactly the same way Data.Map works, except that it corrects a historical accident and has the Semigroup and Monoid instances that Data.Map ought to have had all along.

The only really confusing thing is the name "Monoidal" possibly suggesting more than what was/is the case. There's nothing especially monoidal about these maps, it's just that they have a more general Monoid instance that uses a Semigroup on the values. The Maybe data type for example isn't called MonoidalMaybe, and really has very little to do with monoids in general, it just has an instance of this sort. I liked the name "AppendMap" in our other library prior to this one a bit more, but even that is insufficiently subtle.

However, I do think there is some need for additional libraries which more aggressively use a monoid structure on the values to emulate natural operations on functions k -> m when m is a monoid. That's just not what this library is (at least not versions 0.1 through 0.4), and changing it in a piecemeal way has broken a bunch of stuff and made an impossible upgrade situation where moving to 0.5 causes probably-incorrect changes to behaviour that nobody will be warned about (which has sadly affected reflex-dom for a couple versions since it required one of the other, more subtle changes to this library that landed in 0.5, due to the Data.Align split).

Now, we could filter certain functions out of this library relative to Data.Map, and at least there would be an upgrade path (though a possibly somewhat annoying one), but the confusion about them confuses me. Would we really advocate for the removal of insert and fromList from containers? I think Elliot perhaps would, but I'm not so sure about everyone else. These functions seem fine to me, so long as they're used conscientiously. Yes, they're not injective (though neither are insertWith (<>) etc.), and yes, insertWith and fromListWith are preferable when you have a meaningful way of combining the elements. But I don't know about just getting rid of them entirely. There are times when insert and fromList ought not to be used, but I don't know if I'd really agree that's "all of the time".

We can think more about that, but right now I just want to put things back to where they were roughly with 0.4 (except with the other new additions intact) and deprecate 0.5 of monoidal-containers and all the affected versions of our other libraries before more damage is done by the unexpected semantic changes. Then it won't be an emergency, and we can think more calmly about whether we want to drop certain functions and push that through.

Copy link
Owner

@bgamari bgamari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking this on!

@ali-abrar
Copy link
Collaborator Author

@3noch thanks for the input. I think @cgibbard is right that we should roll back first and then reconsider alternatives.

@ali-abrar ali-abrar merged commit a3d5441 into master Aug 24, 2019
@3noch
Copy link
Contributor

3noch commented Aug 24, 2019

@cgibbard You're right that I would advocate for removing insert et al from Data.Map as well. Since others don't seem to be as confused about this library as I am, I will offer a section to the README about what it is and what it isn't meant to be. Prior to 0.5, @bgamari, @ali-abrar, @alexfmpe, and I were all convinced that the "footgunness" of fromList was worth a change. I fear that many who come after us and haven't read this thread will have the same confusion.

3noch added a commit to 3noch/monoidal-containers that referenced this pull request Aug 24, 2019
@3noch 3noch mentioned this pull request Aug 24, 2019
@3noch
Copy link
Contributor

3noch commented Aug 24, 2019

Perhaps a better name than monoidal would have been unbiased.

@alexfmpe
Copy link

The clear direction (at least in my mind) is that this library is just Data.Map, etc. with different instances.

This causes another difference: the relationship between some functions changes. For instance, (<>) = union no longer holds if we just replace the semigroup/monoid instances.
This is way more tricky than I first thought. What @3noch and me had been tending towards was more of a semigroupish-lookup semantics. That is, whenever two lookups for the same key are available (e.g. union/intersection/etc), combine them with <>. Then we realized that caused some unexpected relationships between some functions and cascaded the semigroup behaviour.
An actual monoidalish-lookup would also require lookup :: Monoid v => k -> Map k v -> v to return mempty when a key had no associated value, just as a map-based multiset must default to 0 on a failed lookup.

@3noch
Copy link
Contributor

3noch commented Aug 26, 2019

Right. Which is why I think documenting the scope and purpose of this library (as in #51) is paramount.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants