search

ollef / Earley

Parsing all context-free grammars using Earley's algorithm in Haskell.
BSD 3-Clause "New" or "Revised" License
361 stars 24 forks source link

Behavior of Semigroup/Monoid instances for Prod? #39

Closed chowells79 closed 5 years ago

chowells79 commented 5 years ago

I noticed that the Monoid and Semigroup instances for Prod duplicate the Alternative instance. I found that unexpected. I was expecting them to lift the element instances over the Applicative instance, a la (<>) = liftA2 (<>). That's the behavior of those instances in many other parsing libraries.

Changing the instances now would be awkward, of course. People might not see the change and think that just because their grammars still compile, the code still is correct. I'm not really asking for a change. Just wondering why this design was chosen, and suggesting that a change is something you might consider if you ever make large-scale changes that would break things in other ways.

ollef commented 5 years ago

Yes, that behaviour seems more useful than the current one, and it's better to use an established standard behaviour when we can. I can't remember if there's any good reason for the current design, but I don't think so. It was probably just the most obvious monoid I could think of at the time.

If we were able to deprecate uses of a type class instance it would be quite reasonable to change them after a deprecation period on the old instances, but I don't think that's possible with GHC right now.

Since that's not possible, perhaps we could remove them in a release and reintroduce the new instances after some time?

What do you think? Does anyone else have any input?

sboosali commented 5 years ago

why not document it and bump the major version? it seems cleaner than a gap in the presence of an instance (but I agree that it being misleading as a non-type-error change is problematic).

On Wed, Nov 28, 2018, 00:05 Olle Fredriksson <notifications@github.com wrote:

Yes, that behaviour seems more useful than the current one, and it's better to use an established standard behaviour when we can. I can't remember if there's any good reason for the current design, but I don't think so. It was probably just the most obvious monoid I could think of at the time.

If we were able to deprecate uses of a type class instance it would be quite reasonable to change them after a deprecation period on the old instances, but I don't think that's possible with GHC right now.

Since that's not possible, perhaps we could remove them in a release and reintroduce the new instances after some time?

What do you think? Does anyone else have any input?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ollef/Earley/issues/39#issuecomment-442355474, or mute the thread https://github.com/notifications/unsubscribe-auth/ACNoMbKJEWixdJ14i1T2ZdEVBEMZ_qlvks5uzkPmgaJpZM4Y2sZc .

ollef commented 5 years ago

Yeah, it's to avoid breaking people's code without compile errors.

I'm not sure how careful we need to be here. Are there even any users using the instances in question, and if they are, are they returning results that have Monoid or Semigroup instances?

phadej commented 5 years ago
chowells79 commented 5 years ago

I would hazard a guess that few people are using those instances, but it's really hard to know. I've got no great insights to add regarding an upgrade path.