-
Notifications
You must be signed in to change notification settings - Fork 26
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
Drop Semigroup/Monoid instances for Map; add SemigroupMap #38
Drop Semigroup/Monoid instances for Map; add SemigroupMap #38
Conversation
I think it should be possible to duplicate the API without duplicating the implementations. Something like this: newtype Map k v = UnbiasedMap (Biased.Map k v)
deriving newtype instance [...]
lookup :: forall k v. Ord k => k -> Map k v -> Maybe v
lookup = coerce Biased.lookup |
Those names seem strange to me, and using different names would prevent being able to easily switch between them just by adjusting the imports. We really just need to fix this in the compiler. |
I think if we do this (I’m undecided personally) we ought to have a deprecation cycle first. |
I don’t think that having both Data.Map.Biased and Data.Map.Unbiased is much clearer than Data.Map and Data.Map.Unbiased, at least not enough to warrant the work implied by the deprecation of Data.Map.
I agree with Harry, this isn’t something we ought to workaround with suffixes. Thinking about usage a bit, if both Data.Map and Data.Map.Unbiased are imported in the same module at least one of them will have to be qualified to avoid conflicts. import Data.Map
import Data.Map.Unbiased as Unbiased import Data.Map as Biased
import Data.Map.Unbiased import Data.Map as Biased
import Data.Map.Unbiased as Unbiased In each case I don’t think prefixing the newtype constructor (UnbiasedMap instead of Map) helps much to disambiguate given the qualified imports, so could we drop the prefix from Harry suggestion?
This is indeed possible, and it would nicely help to highlight where Data.Map.Unbiased diverges from Data.Map. The compiler cannot coerce polytypes though, so they need to be instantiated manually with type annotations: lookup :: forall k v. Ord k => k -> Map k v -> Maybe v
lookup = coerce (Biased.lookup :: k -> Biased.Map k v -> Maybe v) |
Let's take a step back and think about what a
I feel strongly that it would be bad if we duplicated the entire API and ended up with incompatible autocompletes and imports. My preferred solutions at this point in descending order:
|
To me, not having the option of getting a Map type with the ideal Semigroup instance is much worse than any confusion that having two copies of the API would cause. Any autocompletion ought not to get too confused if one out of Data.Map and Data.Map.Unbiased is already imported (like, hopefully if I type |
Sounds like Option 2 then. If there's an ideal one, there's no reason to have two different modules.
If I'm typing |
There still is a reason to have two different modules: it allows us to provide the functionality we want to provide without breaking anything.
I don’t think “is this already a known thing in CS literature” should be that much of a factor here, especially since I imagine a fairly large chunk of PureScript users don’t concern themselves with CS literature. If people don’t know why there are two, they could read the docs on Pursuit, where the difference would be explained. You’re right that it could be a bit awkward if you have the wrong kind of Map for a function you want to use, although that should be addressable by an application of I also just want to check you’re aware that the idea is that we would only have two separate Map modules for a certain period of time; eventually we would just have Data.Map with the unbiased Semigroup instance. Does knowing that affect your preferred option? |
Well I'll just include StackOverflow in CS literature for this argument. Similarly Googling for various combinations of
So people go to Pursuit to try and understand which module to pick. I'd say the "large chunk of PureScript users that don't concern themselves with CS literature" also aren't going to be happy that they need to understand what a
I wouldn't want
I wasn't aware of that. If the end-goal is to end up with a single |
That's exactly the goal. The current instance semigroupMap :: Ord k => Semigroup (Map k v) where
append = union ... will be changed to the following instance semigroupMap :: (Ord k, Semigroup v) => Semigroup (Map k v) where
append = unionWith append The original behavior can still be achieved by using leftMap `union` rightMap == (c leftMap) <> (c rightMap) where
c :: forall k v. Map k v -> Map k (First v)
c = coerce |
I've updated the unbiased
|
CI now builds. A few questions I have:
|
I think testing the Semigroup instance like I did in purescript/purescript-foreign-object#19 should be enough. |
I think we should consider the points @kritzcreek raised a bit more. I can’t help but agree that everyone having to understand why there are two versions of every Map function is a fairly big disadvantage of this approach. I think I might also prefer just to remove the instance entirely and re-add it later. |
I'm not going to argue for/against dropping the instance below. Merely make a few comments.
If people upgrade from The disadvantage of dropping it altogether is that While core team members could agree not to create this repo personally, anyone in the community could create it. Eventually, someone will likely create it since those who need it will need to reimplement it in each repo/project in which they need it. Thus, "dropping the instance" wouldn't remove the "two |
Just thought I'd summarize what we have discussed so far. AFAICT, the plan we have in place for making such a change accounts for problematic issues, such as the "silent changes" in the library, people jumping two major releases, enabling One of the desires to unbias the The second motive, as claimed by a number of core contributors, is that unbiasing this instance makes it "more useful" in that one can make it left-biased, right-biased, or As Phil and Joe point out above, the current left-biased instance is not broken. And for better or worse, it has been around longer, so the chance that more of the ecosystem already depends on the instance being left-biased is higher. Lastly, if we didn't make this change, one could still get right-biased or Upon reflection, I realized we already have two Phil's example above complicates things because he's right. If someone defines a type class like what he shows above, one cannot use coercion to solve the problem. AFAICT, one would be forced to do one of the following things:
Phil's example clearly demonstrates his point. However, isn't this problem just as problematic for Let's hypothesize.
Finally, (and just as a reminder because I know these discussions can get out of hand quickly) we're not enemies. Thank you for participating in this discussion and making arguments that (hopefully) push us towards better conclusions on what to do in these situations. I hope that throughout this discussion I have understood you and shown respect to you and your reasoning. |
I'm not sure if this is convincing for you Joe, but my support for this change comes from the fact I specifically avoid using the current instance since I find it such a footgun. That's also why I originally suggested dropping the instance entirely in favour of newtyping to choose an instance, |
I'm curious to hear what people think about changing the |
I don’t think that instance is either more or less general than the current one, as there’s no situation in which the two instances can be made to behave in the same way (unlike for Map, where you do get the same behaviour if the values are wrapped in First). But yes, this is why I’ve never brought up any idea of “unbiasedness” because I have never found that compelling either. I really just want it because it’s more useful in practice. If we want to talk about general principles which make the proposed instance preferable to the current one, I’d suggest that implicitly chucking data away is a poor default. I don’t think lists can be thought of as maps from indices to values because you can’t “insert” at index n unless all indices <= n are already occupied. |
Here's another unsubstantiated data point of one:
So that's what's wrong with the current instance from my naïve standpoint. But more importantly than trying to find what's wrong about the old instance, I'm interested in what's right about it. We've established that it is law-abiding and the same as Haskell. I'm very curious about practical applications of the old instance in some applications or libraries (I have the same reservations about |
We're going to go ahead with this change. Most of the feedback when this was originally proposed was positive, and most of the pushback seems to have been around inadvertently breaking the behaviour of existing code by changing the meaning of the instance, but the proposal as it stands now sidesteps that by removing the instance. It also mirrors the choice we made for numbers where a newtype is required to direct the behaviour, since there's an absence of a clear best default instance. The fix for the breaking change this will impose is also pretty trivial, excluding situations like those that Phil outlined. I do see Phil's concern about not being able to do the kind of thing he suggested in his hypothetical example too, but it seems Cyril had some suggestions to work around that. I think the number of people affected by that situation is going to be a lot smaller than the number of feet being shot by the current instance in the long run. I am also definitely sympathetic to the argument that the core libraries should offer good stability, but I actually take that as an argument for doing this now rather than later - the longer we leave it, the worse it will be to change it. This change isn't just for aesthetic or ideological reasons - the fact there's a part of the core libraries that I think "oh yeah, avoid using that" says there is definitely a problem to me. (Although ironically, after all this discussion, I personally would probably never again forget that it behaves the way it does just now 😄). |
CI passes. Any other updates to make before merging this? |
Maybe worth mentioning in the changelog that |
Does the following work?
|
Yup! |
… (slot2 query3 output4))` error after purescript/purescript-ordered-collections#38
Based on feedback from the community, it seems like having two versions of
Map
, the current one unchanged (in that it's Semigroup instance prefers the first Map's values over the second Map's values when they both have the same key) and the new one added in this PR having itsSemigroup
instance unbiased, which can provide the same behavior as the biased Map but allows flexibility to handle that case differently.Based on Harry's comment, the unbiased version of Map should use
unionWith append
for its implementation ofappend
andunion
for its implementation ofalt
.I have the following questions:
Map
is moved toData.Map.Biased
, so it's even that much clearer?Map
with typeMap
" errors because they imported functions from both modules on accident. Should we also change the name of one or both of these types to make such errors easier? For example,MapU
orMapB
?Regarding documentation, where should we document the difference between the Biased and Unbiased Maps? I assume at both the Module level, the
Map
declaration and theSemigroup
instance documentation for completeness.And what specifically should the documentation say?