rpc: serialize user message styles

[Alizter]

We serialise user message styles in RPC allowing Ansi formatted error codes to be serialised faithfully. We also take the time to fix the differences between RPC messages and regular dune ones that were observed in #8411.

• fixes Extend diagnostics RPC with display markup #6921
• RPC version compliant
• changlog

[Alizter]

This generalises the previous sexp_pp function by allowing for the Conv of tags, and adds it to the API for use elsewhere.

[Alizter]

I actually had to introduce two versions of this function. The new version serialises tags as (Tag (pair tag pp)) which is different to how it was done previously as (Tag pp). Therefore I have included the old version too and made sure to version bump both diagnostics and the diagnostic poll.

[Alizter]

Jobs also have a unit Pp.t description, but I will not touch it in this PR.

[Alizter]

We add an extra function allowing for an opt in to the stylised message. This keeps our API version compatible. The old version is just the new version with the style tags removed.

[Alizter]

Since we have a faithful reconstruction of the styles, we get rid of the ad-hoc message styling we had before and simply use Stdune.User_message.make to get more agreement. There are still some pain points however:

• User_message concatenates hints etc. during construction which we cannot remove. However since the style is also serialised this doesn't appear different to the user. Only the pp, which in the tests can be inspected, is slightly different.

[Alizter]

It's still possible for some messages to lose the Error: prefix. I have not yet been able to reproduce this in a test. I am not exactly sure what is causing this. I think it has something to do with how we use ocamlc-loc to parse error messages into compound user messages and then turn them into user messages. But I haven't been able to confirm.

[pmwhite]

Sorry for the delay in starting to review this! I just started looking at it today. One thing that I don't understand is why we need to introduce new User_message and Ansi_color modules if Stdune already has sub-modules that look very similar to the ones introduced in this feature. Maybe this is related to what you mentioned about the compiler not letting you use polymorphic variants; I didn't follow that part - could you elaborate?

[Alizter]

@pmwhite That's related but not exactly why we have to mirror the types in the dune_rpc library. We need to keep the dependencies of that library small so exposing Stdune in the API would require consumers to also pull in the Stdune library. And this is not desirable IIUC. cc @rgrinberg

My hope with the poly variants was that we could mirror the API but share the implementation but that didn't work out so I ended up mirroring everything.

[pmwhite]

I see, I wasn't aware of that constraint. The library does depend on stdune, but it doesn't mention it in the interface, which I assume is useful so that users of the API can more easily use their own standard library, like base or something. How come mirroring the API and sharing the implementation doesn't work? I would have expected that to work out just fine.

[Alizter]

@pmwhite When I tried it, I had to do something like this:

let to_user_message_style = function
  | `Fg_8_bit_color rgb -> RGB8.to_rgb8 rgb
...
  | x -> x

Now you would have thought that the final clause would go through since the type is the same structurally, however the compiler rejected this. This meant I had to write out each case anyway. Doing so meant there was little point keeping the poly variants around.

The reason we have poly variants in Stdune is because we add extra constructors when parsing escape codes. Here we do no such magic therefore we might as well use an ordinary variant.

[rgrinberg]

Now you would have thought that the final clause would go through since the type is the same structurally, however the compiler rejected this.

I think you just needed to add a type annotation.

[rgrinberg]

I see, I wasn't aware of that constraint. The library does depend on stdune, but it doesn't mention it in the interface, which I assume is useful so that users of the API can more easily use their own standard library, like base or something

That's right. In particular, stdune is completely unstable so depending on it in code is going to produce churn for users of this API. Although the dune rpc api isn't completely stable, we already have production users so we do our best to maintain backwards compatibility.

[pmwhite]

@pmwhite When I tried it, I had to do something like this:

let to_user_message_style = function
  | `Fg_8_bit_color rgb -> RGB8.to_rgb8 rgb
...
  | x -> x

Is the function necessary? Could we instead expose the equality to Stdune.User_message.Style.t in private/exported_types.mli, since all we care about is not having Stdune show up in the public dune-rpc interface? Most of the value I've been imagining sharing the types would have is that then we wouldn't have to convert back and forth between the types. (maybe I'm missing a reason why we can't share the types?)

[rgrinberg]

Could you introduce Conv.enum to make this all a bit more compact?

[Alizter]

Conv.enum as it exists is not sufficient since I cannot handle constructor arguments.

[Alizter]

@pmwhite I thought a bit harder about this and you are totally right, we can share much of the implementation. I've gone ahead and removed all the conversion functions and it has simplified things considerably.

[Alizter]

I've added more functions to RGB8 and RGB24 in the dune_rpc API, allowing consumers to produce and consume those values easily.

[rgrinberg]

I don't think these functions are very useful. What about accessors like red, green, blue?

[Alizter]

It's not possible to do split into red, green and blue like we do for 24-bit color since not every terminal encodes 8-bit colors in the same manner. The only thing that doesn't change is that there are 256 colors.

[Alizter]

The only time this can happen is if Dune runs a program with 8-bit color output and we wish to preserve that color. At that point we are at the mercy of the client on how we quantize those values.

[rgrinberg]

Ditto

[Alizter]

If I'm making a web client for viewing RPC messages, this might be a useful value to have since colors are hex values usually. (As an example of how this might be useful).

[rgrinberg]

Okay, this looks ready to me. I'll give @pmwhite another chance to comment.

cc @ddickstein who was interested in this.

[pmwhite]

Yeah, looks good to me, too.