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

Proposal: Add PrettyAnn typeclass #222

Open
BinderDavid opened this issue Feb 4, 2022 · 4 comments · May be fixed by #256
Open

Proposal: Add PrettyAnn typeclass #222

BinderDavid opened this issue Feb 4, 2022 · 4 comments · May be fixed by #256

Comments

@BinderDavid
Copy link

As discussed in #102, there is currently no way to use the Pretty typeclass in combination with annotations. In that issue, several alternatives were discussed, which were either backwards incompatible or otherwise potentially computationally expensive.
But the first issue also mentioned the "obvious" version of the Pretty typeclass. Namely:

class Pretty a ann where
    pretty :: a -> Doc ann

But changing the definition of the typeclass would be obviously backwards incompatible.
So I propose the following change:

Add a PrettyAnn typeclass

Add the following typeclass to the library

class PrettyAnn a ann where
    prettyAnn :: a -> Doc ann

together with the obvious instances for primitives, strings, text etc. Just like for Pretty right now.

I see the following benefits:

  • Purely additive change, no problems with backwards incompatibility.
  • "Simple" solution. Neither fancy types nor other typeclasses are involved. Only MultiParamTypeclasses.
  • Performance characteristic is obvious: No implicit insertions of unAnnotate or similar functions which traverse the Doc.
  • This is already a solution users implement downstream in their applications. Why not provide a canonical way how to do it?

And the following downsides:

  • Increases API surface. Now there are two typeclasses for prettyprinting. The documentation could just explicitly state that the user should use Pretty for prettyprinting without annotations, and PrettyAnn for prettyprinting with annotations.
  • Does not lead to the "ideal" solution, since the Pretty class is left as a historical wart, instead of being a special case of PrettyAnn a Void
@Kleidukos
Copy link

Thank you for bringing this up, I think it's a good proposal if we want to introduce minimal disruption whilst allowing to retain annotations.

@sjakobi
Copy link
Collaborator

sjakobi commented Feb 7, 2022

@BinderDavid Could you clarify which instances you think are obvious?

I imagine that users would want to use concrete annotations for certain primitive types. Wouldn't this be inhibited by the instances that we could provide from prettyprinter?

@BinderDavid
Copy link
Author

BinderDavid commented Feb 7, 2022

My original idea was that for every instance of Pretty T that is currently defined in the library, the corresponding instance PrettyAnn T ann should also be provided. This means, of course, that no annotations are used in these instances for these types T, since the implementation is polymorphic in the annotations.

Whether this is desirable depends, of course, on the way the prettyprinter library is used. Personally, I always have newtype wrappers like newtype Var = MkVar String instead of type definitions type Var = String which can be used to generate the appropriate semantic annotations. I don't know whether the use case of generating concrete annotations for plain Strings/Numbers/Lists is prevalent?

The instance could also be declared Overlappable, so the user could provide an overlapping instance instance {-# OVERLAPPING #-} PrettyAnn T ConreteAnnot, no? I would have to check how Overlappable instances interact with MultiParam typeclasses. I haven't looked at the details in a while. (Edit: That probably doesn't make sense)

I wrote this proposal to see whether there is some general interest in adding such a typeclass. If there is some favorable reception of the idea, I could implement a PR which can be the basis of a discussion of details.

@mmhat mmhat linked a pull request Nov 4, 2024 that will close this issue
mmhat added a commit to mmhat/prettyprinter that referenced this issue Nov 4, 2024
@andreasabel
Copy link
Contributor

Me too.
In my semi-first steps with prettyprinter I just discovered that the Pretty class isn't usable if one wants any annotations at all... (I already had a bad feeling in my stomach...)

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 a pull request may close this issue.

4 participants