haskell / pretty

Haskell Pretty-printer library
Other
69 stars 30 forks source link

Use `Semigroup((<>))` and define Monoid/Semigroup instances #30

Open hvr opened 8 years ago

hvr commented 8 years ago

Ideally this needs to happen in time for GHC 8.0 as pretty is bundled with GHC 8

See https://github.com/ekmett/ansi-wl-pprint/commit/dd40c6138e1a4ca948d8045182d25f30a8682c4d for how the (<>) part can be handled

The Semigroup part is a bit tricky but doable as well. I'll probably prepare a PR as that's easier than to explain how to do it =)

/cc @ekmett

hvr commented 8 years ago

damn... I stumbled over https://mail.haskell.org/pipermail/libraries/2011-November/017066.html :-/

PS: pretty currently defines the following operator fixities:

infixl 6 <>
infixl 6 <+>
infixl 5 $$, $+$

While (<>) from Monoid/Semigroups is defined as infixr 6 <>; ansi-wl-pprint didn't have this problem

dterei commented 8 years ago

Yeah, this is an old issue that's never been resolved. I believe I took over pretty after this all had occurred.

It seems our options are: 1) Do nothing. <> stays it's own symbol in pretty that conflicts with Monoid and requires namespacing to resolve. 2) Switch <> and <+> to infixr 6. As pointed out in the mailing list, this changes the meaning of some code due to a <> empty <+> b changing since empty is the identity element of both <> and <+>. 3) Switch <> to infixr 6 and <+> to infixr 5, some code can still break, but arguably code relying on unintuitive semantics (since somewhat odd <> and <+> have same precedence when both treat empty as identity). Issue here is <+> would now have same precedence as ++. I think this is probably not a big deal, but don't have a great intuition here.

I'm leaning towards (3) myself. It would have been nice if this had all been done at the same time as the original Data.Monoid.<> addition, as have Data.Monoid.<> be infixr 7 would be better.

(1) seems annoying long-term, and (3) a slight improvement over (2).

Thoughts?

hvr commented 8 years ago

Btw, @ekmett tells me there that changing the fixity from left to right changes the asymptotics badly.

Inside GHC we're considering renaming the current left-assoc operators to something else to avoid the nameclash (and confusion) with the Semigroup-<>. Changing left to right w/o renaming would require to go over a lot of <>-use-sites and give each use-site some thought, rather than simply rename the occurences...

More generally, I'm considering asking on libraries@ for opinions for a left-assoc alias infix-op of (<>) (maybe named (><)?).

dterei commented 8 years ago

Interesting. Is this asymptotic issue explained somewhere? The previous mailing list discussion and benchmarking people had done seemed to suggest pretty had improved behavior (just slightly) with right associative.

If that's the case, then yes, sadly it seems you want to retain the pretty <> as a distinct operator from Data.Monoid.<>, which means either just living with the name clash or renaming.

Please let me know how GHC resolves this and if a left-assoc of <> emerges. Until then, it seems the status quo is the best option.

hvr commented 8 years ago

Here's the GHC ticket btw: https://ghc.haskell.org/trac/ghc/ticket/11149

ekmett commented 8 years ago

I'm mostly half remembering it from a comment in one of the pretty printing libraries' codebases where it talked about how using the other associativity cost it an O(n^2) factor. I can't find it offhand though.

-Edward

On Wed, Dec 2, 2015 at 4:04 AM, David Terei notifications@github.com wrote:

Interesting. Is this asymptotic issue explained somewhere? The previous mailing list discussion and benchmarking people had done seemed to suggest pretty had improved behavior (just slightly) with right associative.

If that's the case, then yes, sadly it seems you want to retain the pretty <> as a distinct operator from Data.Monoid.<>, which means either just living with the name clash or renaming.

Please let me know how GHC resolves this and if a left-assoc of <> emerges. Until then, it seems the status quo is the best option.

— Reply to this email directly or view it on GitHub https://github.com/haskell/pretty/issues/30#issuecomment-161227507.

hvr commented 8 years ago

@dterei I've started writing up https://ghc.haskell.org/wiki/Proposal/LeftAssocSemigroupOp and I plan to put this up for discussion in a couple of days... any comments?

Shall I mention the

Switch <> to infixr 6 and <+> to infixr 5

suggestion? (of which I'm not sure what the consequences are, and ultimately it doesn't really address the subtly clashing definitions among pretty-printing libs which annoyingly don't converge on the same vocabulary in terms of associativity of <>/<+>)

dterei commented 8 years ago

Looks great! Thanks for writing up. Yes, it seems worth while mentioning the <> infixr 6 and <+> infirx 5 idea, although the levels should actually be:

> infixr 7 <>
> infixr 6 <+>
> infixr 5 $$, $+$

To avoid conflict between <+> and ++.

dterei commented 8 years ago

@hvr what ever happened with this?

hvr commented 8 years ago

@dterei still on my TODO list; hasn't been forgotten! if you want to accelerate this, please take a last look at the wiki page, and edit as you see fit! :-)

In any case, I'll try to publish this to ghc-devs/libraries@ within the next 7 days


EDIT/PS: For the record, here's the mailing list thread with the proposal I published back in 2016: https://mail.haskell.org/pipermail/libraries/2016-June/027074.html

AndreasLoow commented 6 years ago

Just wanted to mention that this problem just got worse because in GHC 8.4 <> is now part of Prelude.