compiler messages: a semantic tag for quoting inline code

[Octachron]

This PR proposes to replace the various quotation styles ("...",`...',and '...') used through the compiler messages by a single inline_code tag. It is an alternative to #10321 which is more focused on the uniformity of quotes.

Moreover, following #10321 idea, this new inline_code tag is used in many (all?) locations that are inlining code or type expressions inside English sentences.

Note that currently, warning messages are not covered because warning messages are purely textual only and don't support tag, my current plan is to fix this point when introducing serializable Format messages.

In this PR, this inline_code is currently styled as bold.

[gasche]

This looks okay overall and I would be happy to move forward on this.

Some remarks:

• you don't print anything anymore on dumb terminals, I would have semi-expected to keep a visible printing style (for example "two backticks")
• I'm not fond of inline_code (or the derived inline_tag etc. locally defined) as a %a-printer name, I would find pp_inline_code more natural. But I am willing to stick to your choice if you are convinced that it is better.
• I wonder if we wouldn't have more readable format strings if we chose a short-enough tag name and wrote it explicitly, "...@[<code>%s].." ... foo instead of "...%a..." ... Style.pp_inline_code foo. But your approach protects against typos in the tag name, so there is that. (Maybe we could use the "inline tag" approach in the cases where the code fragment is literally included in the format string and not a variable.) I don't have a strong opinion and your current approach is okay with me.

[gasche]

Should we move the parts of the Color module that are really about Style to a new submodule Misc.Style, instead of hinting at the better API in user code only?

[Octachron]

I would rather propose to move the whole Misc.Color submodule to a new Style module outside of Misc.

[Octachron]

After rereading the PR, I did end up splitting Misc.Color into Misc.Color and Misc.Style.

[gasche]

Suggested change

	Style.inline_code (program ^" script-file")
	Style.inline_code (program ^ " script-file")

[Octachron]

• I don't think that the pp_ prefix brings much in term of readability if we keep the qualified path.
• Using %a functions rather than tags also make it easier to potentially upgrade to non-string tags.

[gasche]

Coming back to this PR: I am fine with your answer to my two minor remarks, but I still believe that we should have some visible delimiters in dumb terminals to preserve readability of the error messages. (In particular, messages recorded in the testsuite.)

I went through a part of the testsuite changes. In all cases involving keywords and most cases involving user variables (except for one-letter variables) I would prefer to see visible markup. On the other hand, for literals (numbers, (), etc.) it feels less important. In first approximation I would prefer to have the visible markup everywhere. (In a distant future one might consider having a literal_code tag with slightly different defaults, but it's not worth it right now.)

I don't have a strong opinion on which delimiters should be used. Looking at existing examples, it looks like most of them already use either `foo' or 'foo', and some use "foo" (which mostly looks ugly except for names of external primitives). If we don't have a strong consensus one way or another, my impression is that a quote-based syntax is the smaller change and should be our default safe bet: quote-quote, backquote-quote or backquote-backquote all look fine to me, people may not even notice the change compared to the current dumb printing. Using ocamldoc syntax would be a larger visual change, maybe we could delay this to a separate discussion.

[gasche]

Mixed feelings about this particular change. I liked to see a string literal just as in the source. But then if we keep a string literal and quote it as code on dumb terminals this may get very heavy very fast. So maybe let's look back at this example with visible code markup.

[gasche]

This is an example that I think becomes substantially less readable without visible code markers.

[gasche]

(With the exception of the extension name, the other parts of the diff that I commented upon do not require a specific change, I just decided to highlight them as representative examples of the dumb-terminal printing changes to help the discussion.)

[gasche]

I still have feelings about it, but I like the new "-using output better (because it is lighter on the eye). I wonder why you are not using the even-lighter ', but I said that I would be okay with any combination of ' ` " that you like and I should stick to that.

From my perspective, the PR is now in an okay shape and I could consider approving. Is there something else we want to do before that? (I considered suggesting "cleaning up the git history", but I actually think that the git history makes sense and we should leave it as-is. There is very little back-and-forth on the code itself, this is mostly about exploring several visual options.)

[Octachron]

The advantage of " is that it does not clash with any OCaml syntax contrarily to ''a' or ``X`.

There is a potential conflict with @shindere's PR on cmo identifier (#12031), so I would propose to wait for at least his PR to be merged.

[gasche]

Good point. I had in mind that it clashes with string literals (so they all clash with something), but clashing with well-bracketed uses is better than clashing with left-only uses.

[Octachron]

I have pushed two small fixes found while looking at the tests one last time. I will propose to merge this PR as soon as the tests are green to avoid waiting for collisions with other PRs.

[dbuenzli]

I had in mind that it clashes with string literals

I recently started working with 5.2 and personally I'm getting very confused by theses kind of messages. I think " is really a poor and confusing choice:

2048 |         let last_host = Http.Headers.get request "Host" in
                                                ^^^^^^^
Error: This expression has type "Http.Request.t" = "Request.t"
       but an expression was expected of type "String_map.key" = "string"

My reaction is: Why is the compiler showing me string equalities ?

For reference this is the message in 4.14.2:

2048 |         let last_host = Http.Headers.get request "Host" in
                                                ^^^^^^^
Error: This expression has type Http.Request.t = Request.t
       but an expression was expected of type String_map.key = string

I guess I should rather support color in my build system but I doubt a bit the whole quoting business in the context of these error messages. You are showing me some kind of foreign, made up syntax, while you could show me direct source-like OCaml syntax like 4.14.2 did.

I waited for a few days but my brain just refuses to adjust, it's a regression in my opinion.

[dbuenzli]

Note in the discussion about quotes @xavierleroy already mentioned that identifiers do not need to be quoted in error mesages and it's just nice if they are highlighted when you have text styling.

[dbuenzli]

Also it seems that is not very consistent and the Hint: below becomes totally ambiguous…

        ... In module "Fmt":
       Values do not match:
         val with_newline' :
           nl:string ->
           Format.formatter -> (Format.formatter -> unit -> 'a) -> 'a
       is not included in
         val with_newline' :
           nl:string -> Format.formatter -> (Format.formatter -> 'a) -> 'a
       The type
         "nl:string ->
         Format.formatter -> (Format.formatter -> unit -> 'a) -> 'a"
       is not compatible with the type
         "nl:string -> Format.formatter -> (Format.formatter -> 'b) -> 'b"
       Hint: Did you forget to provide "()" as argument?

[Octachron]

@dbuenzli , I agree that there is probably some tuning required there, I would try to come back to the issue in September.