formatter margins and color handling: revert #244

[gasche]

The logic in #244 is wrong:

• setting the margins is not the responsibility of the color-handling code
• because setup_colors is called at inpredictable times (on the first error/warning),
  the logic makes it very difficult to correctly set margins for {str,err}_formatter

The patch was originally proposed in the caml-list discussion
https://sympa.inria.fr/sympa/arc/caml-list/2015-09/msg00164.html
but it does not convincingly solve the problem:

• there is no reason to use std_formatter
  rather than err_formatter as a reference, and
• the user can set the both margins themselves anyway.

We have an API in Location to access and configure error/warning
formatters, but it is not accessible from the toplevel. Changing the
margins without using this API is fragile, for example, utop or
expect-tests change the formatter from the defaut of err_formatter.

[gasche]

Note: in #244, @c-cube added some tests to check that the behavior was as intended. The tests are not removed in the present PR, and yet they keep passing. I take this as further support for the idea that the implementation change was not the right one, and can be reasonably reverted.

[Armael]

Could you perhaps say a bit more about the story behind the present PR?
How did you notice that it is needed (or that #244 is wrong)?

FWIW doing what you propose looks good to me, just by following local reviewing heuristics (indeed, updating only std_formatter, and doing that in the color handling code does seem fishy). However I have trouble seeing the bigger picture.

[gasche]

Ah indeed, I told my exciting story in this message on #244.

I must say that I also have a bit of trouble seeing the big picture, and I think we are not alone. The caml-list discussion is obscure and hard to follow, @c-cube did the change on suggestion, the PR discussion was inconclusive, a nicer implementation was proposed in the thread and appears to have been ignored. We did not do a very good job here and should probably not have merged this PR without understanding the context more.

What I can tell for sure is:

• I found the behavior problematic to do other unrelated things (see the comment linked above).
• The code looks very odd (setting margins is not Color's responsibility).
• The supposedly-regression test that was written in fact does not require the code.
• In any case, the compatibility issue that the code was supposed to fix is not very problematic (people cannot set the margins of the toplevel, which is not something we have ever tried to allow in a robust way, and is broken in many contexts), so there is little risk if we have a regression.

[gasche]

(Rest assured that our standards are much higher these days, we organize weeks-long consultations to settle burning indentation questions.)

[Armael]

Aha, I think I know why it works now.

If you look at @brabalan's nicer patch, you can see that an error message used to go through an intermediate Buffer-based formatter, then was turned into a string. This is because the old error type stored messages as strings. So to get correct margins in the error string, you needed to get correct margins in the intermediate buffer formatter, and thus obtain those from some user-modifiable formatter (and this is what the patch is doing).

Now, after my refactoring of Location (wohoo), error no longer stores a string but a "suspended message" (formatter -> unit), removing the indirections. So I believe that it should indeed "just work" now, provided the user configures the margins of the formatter that is actually used for printing (knowing that is still a bit fragile if I understand correctly, e.g. because we don't expose formatter_for_warnings as you point out).

[Armael]

Good cleanup.

[Armael]

If you agree with my analysis, then it would be worth editing your Changes/commit explanation to make clear that we can now simply remove the code because some cleanup has been done in the meantime, but that it was not the case originally.

[c-cube]

That makes a lot of sense, thanks for the explanation @Armael . Glad we can just throw this away now.

[gasche]

Excellent call, I'll update the Changes entry.

[gasche]

Note: even if we did re-adopt the approach of going through an intermediary formatter in the future, we should still try to setup this formatter correctly (temporarily setting the same geometry as the final/destination formatter) instead of changing geometries globally.

[gasche]

@Armael I rephrased the (commit message and) Changes entry. Let me know what you think.

[Armael]

@gasche your commit message seems to suggest that setting err_formatter is the right thing to do in all situations. However, it is worth noting that it is not the case at least for the toplevel (I just tested this). In other words, error messages in the toplevel are printed on std_formatter, and thus changing the margins of the toplevel output still requires updating the margins of std_formatter. (the culprit)

(I can't say whether that's a good or a bad thing...)

[gasche]

I thought that warnings would go to err_formatter (it is the default value of Location.formatter_for_warnings), but in fact Toploop.loop sets formatter_for_warnings to its own ppf parameter, which is std_formatter as you show. (Compiler dumps, which I'm looking at right now, do go to err_formatter, and so where also affected by Color.setup () before the present PR.)

I reformulated the commit and Changes messages again.

[Armael]

Perfect. (ready to merge whenever CI passes)

[gasche]

Thanks!