Unbiasing the Semigroup instance for Map

Change Proposals
#21

I’ve opened this PR for the time being: https://github.com/purescript/purescript-ordered-collections/pull/38

3 Likes
#22

@kritzcreek points out in https://github.com/purescript/purescript-ordered-collections/pull/38 that duplicating the API does have a disadvantage, in that it could be confusing for people searching Pursuit or asking their tooling to import Map-related functions for them that there are now two different Map APIs to choose from, and they have to learn what an “unbiased” Map is (since that isn’t really a concept which exists anywhere else). That’s quite persuasive to me: I think there are probably at least a few places where people are using Maps without making use of the Semigroup instance at all.

I’m now leaning more towards the approach of providing just the newtype wrappers so that people can get the Semigroup behaviour they want while this migration is ongoing, instead of duplicating the whole API.

3 Likes
#23

To follow up with this, here are the changes we would make if we went with the approach above described by Harry:

  • drop the Semigroup and Monoid instance for Data.Map
  • define a newtype in Data.Map called SemigroupMap. Its name makes it more obvious why it exists. Its Semigroup instance would be unbiased. It would have all the type class instances of Data.Map. However, it does not have any of the API of Data.Map (e.g. lookup, etc.). Thus, you would only use it when you need Data.Map to have a Semigroup instance.

This comes with the following advantages

  • Those who do rely on the left-bias nature of the current Map will not experience a “silent” change. Rather, they will be forced to update their code. In situations where they are using this behavior, they will have to coerce the Map k v to Map k (First v) and then wrap it in the SemigroupMap newtype to get the same behavior.
  • Those who want the flexibility of the unbiased Semigroup instance can have it now rather than later.
  • We don’t have two Map types that may confuse beginners or make it harder to import the correct function.
  • In v0.15.0, I believe we would deprecate the SemigroupMap newtype and add the unbiased Semigroup and Monoid instance back to Data.Map with a Warn that says something like “Instance differs from the instance defined in PS releases <= 0.13.x.” In v0.16.0, we would remove the SemigroupMap newtype and remove the Warn constraint on the Semigroup instance.
  • Those who jump two major releases would be notified of the change via the Warn constraint. Those who jump three major releases would not be notified, but that is likely very rare.
4 Likes
#24

We still have the option of adding a warning to the Semigroup (Map k v) instance when we add it back in 0.15 with this approach, right?

#25

Yeah. We could add it back in saying, “This instance differs from the implementation defined in the PureScript v0.13.x release or earlier” or something along those lines.

So, the “two-release-jump” people would still be notified. I’ve updated my response above to include that part.

#26

I might misunderstand the use of the Warn class, but couldn’t it be used in order to print a deprecation warning (if the team will go that direction for the next release)? I don’t see why it would be difficult for people upgrading to see the differences if there will be a deprecation warning on build or run.

#27

I’m sure people have been busy with holiday stuff in the past couple of weeks. If there is any thing else anyone would like to say, please do so soon.

@razcore-rad I’m not sure.

#28

This is another breaking change release that should be made in v0.15.0.

Per the my comment above:

In v0.15.0, I believe we would deprecate the SemigroupMap newtype and add the unbiased Semigroup and Monoid instance back to Data.Map with a Warn that says something like “Instance differs from the instance defined in PS releases <= 0.13.x.” In v0.16.0, we would remove the SemigroupMap newtype and remove the Warn constraint on the Semigroup instance.

I don’t think we should do what was originally described exactly. If we have a Warn on both the SemigroupMap and Map instances, then everyone will get compiler warnings for the rest of v0.15.0. In v0.15.0, I’d rather not having to see a new MonadZero-like warning for the rest of the release. I would rather we put a Warn on Map so that those who jump two breaking changes are still notified, and NOT put a Warn on SemigroupMap's instances.

In other words:

  • v0.14.0
    • remove Map's Semigroup and Monoid instance
    • add SemigroupMap with unbiased Semigroup and Monoid instance
  • v0.15.0
    • add unbiased Semigroup and Monoid instances back to Map with a Warn in case people jump two breaking changes
    • SemigroupMap's instances are untouched.
  • v0.16.0
    • remove Warn on Map's Semigroup and Monoid instances.
    • add Warn on SemigroupMap's Semigroup and Monoid instances.
    • add deprecation notice to SemigroupMap
  • v0.17.0
    • remove SemigroupMap
4 Likes
#29

Hi,

I observe a compiler warning in the program for the quoted snippet below:

component ::
  forall query output m. H.MonadAff m => Navigate m =>
  H.Component query TrainingInput output m
component =
  H.mkComponent

A custom warning occurred while solving type class constraints:

Data.Map's `Semigroup` instance is now unbiased and differs from the left-biased instance defined in PureScript releases <= 0.13.x.

How to resolve the warning properly?
I don’t see a way how these highlighted lines are related to Data.Map or Semigroup, because Halogen component doesn’t have Maps and MonadAff, Component, Navigate definitions neither.

So besides the warning itself, I would like to pay attention to the situation where compiler reports a problem, but developer cannot follow the hint. Intermediate lines/transitive symbols should be mentioned in the report between the highlighted line and the pragma warning.

#30

You don’t. That warning is there to ensure people don’t accidentally rely upon the old Semigroup instance. If you are using the new spago though, you can filter out this warning.

  # Optional section to further customise the build.
  build_opts:
    # Specify whether to censor warnings coming from the compiler
    # for files in the `.spago` directory`.
    censor_library_warnings:
      # Note: when using `by_prefix`, use the `>` for block-string:
      # see https://yaml-multiline.info/
      - by_prefix: >
        Data.Map's `Semigroup` instance
1 Like
#31

This is not good, because warning is a signal to a developer, that something goes wrong. Unconditional warning looks strange.
PureScript is positioned as type safe language. This kind of warning I would expect to experience in plain JS based library.

1 Like
#32

You don’t.

Actually, I’m wrong. You need to wrap your Map values in the SemigroupMap newtype. For example:

-- before
map1 <> map2

-- after
(SemigroupMap map1) <> (SemigroupMap map2)
#33

over and under from Data.Newtype come in handy here: