Opened 3 years ago

Last modified 2 weeks ago

#8809 new feature request

Prettier error messages?

Reported by: joelteon Owned by:
Priority: normal Milestone:
Component: Compiler Version: 7.9
Keywords: Cc: bgamari, adamgundry, alanz, gridaphobe, mgsloan, niteria, sweirich, RyanGlScott, Phyx-, dalaing, michalt
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking: #13122
Related Tickets: #8809,#10073,#10179,#12906 Differential Rev(s):
Wiki Page:

Description

clang has very nice-looking error messages.

pretty.c:6:7: warning: incompatible pointer to integer conversion passing 'char [14]' to parameter of type 'int' [-Wint-conversion]
  foo("Hello, world!");
      ^~~~~~~~~~~~~~~
pretty.c:1:14: note: passing argument to parameter 'i' here
void foo(int i) {
             ^
1 warning generated.

ghc's error messages are not so good.

ugly.hs:7:18:
    Couldn't match expected type ‘()’ with actual type ‘[Char]’
    In the first argument of ‘f’, namely ‘"Hello, world!"’
    In the second argument of ‘($)’, namely ‘f "Hello, world!"’
    In the expression: print $ f "Hello, world!"

In my opinion, there are three independent improvements that could be made to GHC error messages and warnings: color, context and whitespace. Currently they're blobs of text.

Consider all three applied to error messages:

ugly.hs:7:18: error: Argument to 'f' is type '[Char]' but expected 'Int'
main = print $ f "Hello, world!"
                 ^~~~~~~~~~~~~~~

ugly.hs:3:1: note: type of 'f' is given here:
f :: () -> IO ()
     ^~

or

ugly.hs: note: type of 'f' is inferred:
f :: forall m. Monad m => () -> m ()
                          ^~

In my opinion, context and whitespace are more important that color. Even without color, compare this error message to the one shown above:

ugly.hs:7:18: error: Argument to 'f' is type '[Char]' but expected 'Int'
main = print $ f "Hello, world!"
                 ^~~~~~~~~~~~~~~

ugly.hs:3:1: note: type of 'f' is given here:
f :: () -> IO ()
     ^~

In my opinion this is much easier to visually process than GHC's current messages.

Change History (69)

comment:1 Changed 3 years ago by schyler

Improving the prettiness of GHC errors is a great way to make it friendlier for people to pick up Haskell. +1

Last edited 3 years ago by schyler (previous) (diff)

comment:2 Changed 3 years ago by goldfire

I'm afraid I don't quite see what you're getting at. The original post says, "[This modified version] is much easier to visually process than GHC's current messages." My question is: Why, precisely? I don't mean to be defensive or dismissive, but trying to generate grounds for a meaningful conversation. For example, here are a few things that you might be getting at:

  • Having different colors/font weights (i.e. boldness) makes the error messages more visually interesting and therefore easier to pay attention to and read.
  • Having blank lines in the middle of single error messages makes them less imposing.
  • Using position marker in a line below some code is easier to follow than an ever-growing context.
  • In the example, the type of f is given explicitly, so the context in which the error was made is more apparent.

Short of re-engineering the entire way that GHC handles error messages, it would certainly be hard to produce exactly the output that you are requesting. But, it may be possible to address bulletpoints like my suggested ones above piecemeal and nudge ourselves in the direction of better errors.

It's also worth pointing out that each of the bulletpoints above has reasons "against", such as:

  • Not every terminal supports these extra modes. In particular, GHC has already had some trouble getting "smart" quotes working in all possible environments (or, indeed, figuring out when to fall back onto dumb quotes).
  • An automated processor of error messages (that is, an IDE built around GHC) could easily get confused around the blank lines. In fact, I believe I've run into this exact problem when running clang from emacs -- the extra "context-setting" output gets interpreted as fresh warnings.
  • It's unclear to me, personally, if having the position marker on a separate line is necessarily better than the current output.
  • The user of a DSL in Haskell is generally unaware of the full, general type of a function they are using. Perhaps including the full type in the error message would make it scarier, not friendlier.

In any case, I'm curious to hear more about the specific things GHC can do to improve. I think we all want "better" error messages, but we need to agree on a definition of "better" first. And, the changes should probably be incremental, unless we have an eager volunteer to examine the whole error-message generation mechanism holistically. There is quite a bit of code dedicated to error messages, so this is not a task to be taken on lightly!

comment:3 Changed 20 months ago by bgamari

Edsko and I were thinking about this a bit in light of the recent discussion on Reddit. He had what I thought was a rather nice idea:

Putting aside the specific proposal made in this ticket, it seems like generally what we need is a more semantically-rich representation for our error messages. This need not be a giant AST encoding every possible error that might arise. Our current approach of encoding messages in SDoc works fairly well. What it lacks is the ability to denote meaningful references to various parts of the program (e.g. types, expressions, constraints).

A moderately painless(?) way to fix this would be to index Doc (and SDoc) on a type which could then be embedded in the document. To put it concretely,

newtype SDoc a = SDoc { runSDoc :: SDocContext -> Doc a }

data Doc a = Embed a
           | Empty
           | NilAbove Doc
           .
           .
           .

The Embed constructor could then be used to embed various compiler-phase specific atoms into the document. For instance, the type-checker might emit errors in the form of SDoc TcDoc where,

data TcDoc = TcExprDoc CoreExpr
           | TypeDoc TcType
           | InstancesDoc ClsInstLookupResult
           .
           .
           .

Consumers of error messages could then use these annotations as they like. Most of the existing consumers would likely expose a function which would take a function to project the phase-specific data back to a plain SDoc. For instance,

showSDoc' :: DynFlags -> (a -> SDoc Void) -> SDoc a -> String

and we could avoid breaking existing users of showSDoc by defining it as,

showSDoc :: Outputable a => DynFlags -> SDoc a -> String
showSDoc dflags = showSDoc' dflags ppr

Other uses (say, tooling using the GHC API) might choose to instead use a richer presentation of the data embedded in the document. These users will still be limited by the fact that the error representation is still ultimately a pretty-printer document, but at least now we aren't forcing them to parse a formatted error message to extract these key details. Moreover, we might be able to expose more context in this embedded data than we show in the current messages.

One of the nice properties of this approach is that it allows a somewhat gradual transition. Adding the infrastructure to enable this sort of embedding requires only minor changes to existing code (e.g. adding the index to SDoc). Moreover, I have a sneaking suspicion that it would allow us to clean up the current story around Names in Outputable.

Last edited 20 months ago by bgamari (previous) (diff)

comment:4 Changed 20 months ago by bgamari

Cc: bgamari added

comment:5 Changed 20 months ago by goldfire

It's come to my attention that my comment:2 may have shut down the conversation here. That was the opposite of my intent! I'd love to figure out how to break down the problem of difficult-to-work-with error messages into its pieces so that we can debate them (and hopefully implement improvements) sensibly.

I should also be clear on one particular point: the biggest barrier to getting this done is the love from someone(s) to see it all through. This would be a valuable service, indeed.

comment:6 Changed 20 months ago by diatchki

It would be nice if we could refactor GHC so that error messages are kept in some sort of structured format with all information that might be relevant. Then, when printed we could have flags to specify how to render the errors (e.g., "machine form", which would be good for external tools, such as IDEs; or "human form", which could have the nice formatting in the example).

I just saw a post about error messages in Elm, which looked pretty, and might give us ideas about formatting: http://elm-lang.org/blog/compiler-errors-for-humans

comment:7 in reply to:  6 Changed 20 months ago by bgamari

Replying to diatchki:

It would be nice if we could refactor GHC so that error messages are kept in some sort of structured format with all information that might be relevant. Then, when printed we could have flags to specify how to render the errors (e.g., "machine form", which would be good for external tools, such as IDEs; or "human form", which could have the nice formatting in the example).

Indeed this would be nice, however placing all of the information necessary for an error comes at a cost. I think Simon PJ articulates this fairly well in this comment on the Reddit post mentioned by goldfire (reproduced here for archival sake),

Building error messages from strings (or in GHC's case SDocs) is pretty lame because you can write them but not analyse them. The "obvious" alternative is to use a huge algebraic data type with one constructor for each error message that GHC can produce. Then you generate the constructor in one place, and render it into a string somewhere else, perhaps in more than one way. I am not optimistic about this, because it puts a big central road-block in the way of generating error messages, and splits the work into two different places (the renderer and the generator). That's an advantage in some ways, but there are so darn MANY different error messages that it feels far too centralised and brittle to me.

Idris does something in the middle. As I understand David Cristiansen, they have an abstract type a bit like SDoc, but it is much richer than GHC's SDoc. They can certainly do colour (and SDocs should too). And you can attach auxilary info to the SDoc so that when rendered in a web browser you get popup hints. This would all be very feasible in GHC, if someone did the work. Another big issue is having enough information to hand when you are generating the message in the first place. Attaching provenance information to type constraints is a huge help (as the Elm article suggests) which GHC does, but not well enough. For example Lennart Augustsson gave a talk at the Haskell Implementors workshop last year with some simple suggestions that work really well in his Mu compiler. Jurriaan Hage and his colleages at Utrecht have a lot of experience of this kind of thing with Helium. GHC is better placed to do this now than it has ever been before, because the type inference engine is increasingly based on solving constraints. Almost all type errors are generated in a single module, TcErrors, if you are interested to look there. I'm keen to make sure that running GHC in batch mode sending output to a text file or dumb terminal gives something useful. I don't want to require a snazzy IDE or emacs mode. But I'd love to be able to exploit one if it was available.

The proposal I lay out in comment:3 was an attempt to find a way to implement the alternative that Simon describes above while minimizing the impact of the change.

Last edited 20 months ago by bgamari (previous) (diff)

comment:8 Changed 20 months ago by adamgundry

Cc: adamgundry added

comment:9 in reply to:  3 Changed 20 months ago by goldfire

Replying to bgamari:

Edsko and I were thinking about this a bit in light of the recent discussion on Reddit. He had what I thought was a rather nice idea: ...

I think the idea of embedding richer info into SDoc is a good one. In particular, I like the idea that this enables a gradual transition. For example, we could have some large ADT defined in TcErrors that represents all of the errors that the module produces (but not other modules). Then some of the downside of the big-ADT approach that Simon is worried about is reduced. And then we could do another module... and so on.

However, I think indexing SDoc is going to lead to trouble. We won't be able to have lists of errors that originated in disparate parts of the compiler. And we won't be able to embed multiple types of information in the same error message. Instead, what if we just use dynamic typing here? (gasp!) By this, I mean something like

data Doc = forall a. Typeable a => Embed a
         | Empty
         | ...

When pulling out embedded bits, we just use dynamic checks to get the types. Although this seems somewhat un-Haskellish, I think it's forced by the very-dynamic nature of an error message. During parsing, a consumer can discover what type of embedded information should be at a certain spot, and then do the dynamic check. This seems like just the sort of thing that dynamic typing excels at.

comment:10 Changed 20 months ago by bgamari

goldfire, indeed the an ADT-per-compiler-phase is what I was thinking (and I have the beginnings of a branch looking at TcErrors in particular. In my case though, I was thinking of at least starting by merely annotating a few semantically-important elements of the message (e.g. Names, Types, TyVars, etc.). This would enable, for instance, IDEs to link to the definition span of a symbol, print an expanded representation of a type, etc.

That being said, there is no reason why one couldn't go further with this same approach and encode the entire error as a value in some ADT. This certainly offers further advantages, although also implies a bit more work (which is why I'm starting with the atoms listed above).

As far as the indexing issue goes, I was thinking we would give Doc a Monad instance. This would allow a number of quite convenient patterns. For instance, have msgs :: Doc TcErrDoc containing some errors you'd like to print: If you have pprTcErrDoc :: TcErrDoc -> Doc Void, you could trivially flatten the document with msgs >>= pprTcErrDoc.

Further if you want to combine a Doc TcErrDoc with a Doc ParserErrDoc, you'd simply lift them both into an ADT data GhcErrDoc = TcErrDoc TcErrDoc | ParserErrDoc ParserErrDoc with Applicative. Alternatively, if you'd rather keep the universe of error types open, you could opt to lift them into a universally quantified newtype, roughly like you suggest.

Last edited 20 months ago by bgamari (previous) (diff)

comment:11 Changed 20 months ago by bgamari

I should note that adding an index and Monad instance to Doc isn't entirely trivial. I believe it is possible (and have a patch with much of the work) but I haven't yet proven to myself that it will preserve the invariants that the Hughes pretty-printer expects.

There are a few implementations of annotated pretty-printers of various flavors on Hackage, but they either provide only Functor (e.g. pretty, annotated-wl-pprint), or are of the Wadler-Leijen variety (e.g. wl-pprint-extras).

My current approach treats the "Pure" constructor like text, adding a PureBeside a Bool (Doc a) constructor to Doc. This, however, makes it impossible (I believe) to correctly implement some combinators which expect to know the width of the string (e.g. sep).

I believe it might be easier to add Monad in a Leijen-style printer, where the width is only necessary on rendering. However, I'm afraid I'm not familiar enough with pretty-printers to know the trade-offs involved here. What are the reasons against doing this?

Last edited 20 months ago by bgamari (previous) (diff)

comment:12 Changed 20 months ago by simonpj

Check out

Any other useful links?

I'm all in favour of this kind of thinking but it needs careful thinking through.

Simon

comment:13 Changed 18 months ago by bgamari

David Christiansen's talk at HIW 2015 is also relevant here,

comment:14 Changed 18 months ago by alanz

Cc: alanz added

comment:15 Changed 17 months ago by thomie

comment:16 Changed 17 months ago by gridaphobe

Cc: gridaphobe added

comment:17 Changed 16 months ago by mgsloan

Cc: mgsloan added

comment:18 Changed 11 months ago by niteria

Cc: niteria added

comment:19 Changed 11 months ago by niteria

I've run into this while trying to hack together something like http://gcc.godbolt.org/, but for Haskell, with Core, Stg, Cmm and Asm output. With -g all the location data is already nicely tracked all the way to Asm and the only piece missing is a way to pretty print with some annotations about the ranges.

I took a stab at adding a parameter to Doc, but I haven't gone through with it because some other types use it transitively and I couldn't decide between SDoc a and SDoc Void there.

pretty appears to be the same library GHC uses and as pointed out by bgamari already supports annotations. Does anyone know if they diverged in functionality/semantics? If not, would it be a useful step to migrate GHC's Doc to annotated version?

I think an argument against it is that it doesn't have a Monad instance, but I don't immediately see the benefit of having it.

comment:20 in reply to:  19 Changed 11 months ago by thomie

Replying to niteria:

Does anyone know if they diverged in functionality/semantics? If not, would it be a useful step to migrate GHC's Doc to annotated version?

In #10735, I made GHC's copy of pretty pretty much the same as pretty-1.2.0, except GHC's copy uses FastString instead of String.

pretty-1.2.1 has 2 more commits, but I didn't apply them because of performance worries (compiler/utils/Pretty.hs is incredibly performance sensitive!), see ticket:10735#comment:23.

Right after pretty-1.2.1, those annotations were added.

comment:21 Changed 11 months ago by bgamari

I've run into this while trying to hack together something like ​http://gcc.godbolt.org/, but for Haskell,

Ahh, interesting.

​pretty appears to be the same library GHC uses and as pointed out by bgamari already supports annotations. Does anyone know if they diverged in functionality/semantics? If not, would it be a useful step to migrate GHC's Doc to annotated version?

There were a few deviations, although I think most of them have been patched up by thomie (see #10735).

I think an argument against it is that it doesn't have a Monad instance, but I don't immediately see the benefit of having it.

I think you ultimately need a Monad instance since this allows you to project your annotations back to the output currently producted by GHC. AFAICT the Hughes pretty-printer doesn't admit an efficient, correct monadic bind.

I think it might be interesting to try patching in another pretty printing library (e.g. Leijen) that does support monadic semantics and examine the effect on pretty-printing output and performance. After the pretty-printing issue is solved adding annotations should be largely a mechanical process (I've already identified several areas where annotations would be appropriate in my play branch).

Last edited 11 months ago by bgamari (previous) (diff)

comment:23 Changed 3 months ago by Rufflewind

I'd like to make an incremental attempt towards improving the aesthetics. Specifically, I wish to:

  • Add colors in a style similar to Clang/GCC.
  • Allow the color to be turned on and off via -fdiagnostics-color=(always|auto|never).
  • Implement auto detection of color support (auto).
  • Display one line of code extracted from the original source code, along with a marker indicating the location of the error.

So the final result should look like this, but with the usual white-on-black terminal colors:

/home/rf/revad.hs:26:1: warning: [-Wunused-top-binds]
    Defined but not used: ‘add’
add = Func $ \ (x, y) ->
^~~

Here is an attempt at implementing the first two objectives: https://github.com/ghc/ghc/compare/master...Rufflewind:diagnostics-color

Edit: Scrap idea about unbolding quoted text for now.

Last edited 3 months ago by Rufflewind (previous) (diff)

comment:24 Changed 3 months ago by Rufflewind

I would like to use terminfo to detect color support, but that would require pulling in terminfo as a dependency (it is already a boot package fortunately). Is this a good idea? If so, how do I even add a dependency to this build system? (Specifically, for main/DynFlags.hs)

comment:25 Changed 3 months ago by rwbarton

I don't know whether or not it is a good idea, but it should just be a matter of editing compiler/ghc.cabal.in.

comment:26 Changed 3 months ago by bgamari

Right, as rwbarton said, this is just a matter of editing the cabal file, which is generated by autoconf from compiler/ghc.cabal.in. Note that you will need to rerun ./boot (or just edit compiler/ghc.cabal manually`) after doing this.

It sounds like you have a reasonable start, Rufflewind. I would love it if someone pick up the proposal that I began laying out in comment:3. I think this would allow us to avoid baking too many formatting decisions into the compiler (which strikes me as something that we would regret) and instead defer them to the consumer of the message (which may very well be ghc's own compiler driver).

comment:27 Changed 3 months ago by Rufflewind

Display one line of code extracted from the original source code, along with a marker indicating the location of the error.

So this (“caret diagnostic” is what Clang calls them) is the one I'm a bit stuck on. In order to print a line in the original source file, I would need to read a file, i.e. perform IO. This is not possible with ErrUtils.mkLocMessageAnn, AFAICT.

I thought of a workaround, but it would require a small tweak to Doc: I need to add an atom for CaretDiagnostic containing the RealSrcInfo, and then read the original source file in printDoc. However, I feel that this is a bit hacky.

IMO, there should really be two layers in the system:

  • A Message type stores each individual diagnostic message unit along with an associated SrcLoc and any other useful information in their original format (e.g. fragments of the AST).
  • Doc should remain as a simple, textual output device with fancy formatting (no support for embedded ASTs). (It does need to understand colors and bold though, as their widths must be treated as zero.)

The rendering function [Message] -> IO Doc would support IO, and this is where the caret diagnostics can be constructed, and it would also provide all the necessary information for Doc deduce alignment correctly. Hence, IMO the debate over how to represent ASTs should focus on the Message type, not Doc.

That being said, even if this idea does work, I'm not sure I have the time to work on a major refactor right now, so I might go with the hacky solution for the time being.

comment:28 Changed 3 months ago by Rufflewind

The changes have been staged to Phabricator, in case anyone wants to review:

comment:29 Changed 3 months ago by sweirich

Cc: sweirich added

comment:30 Changed 3 months ago by RyanGlScott

Cc: RyanGlScott added

comment:31 Changed 3 months ago by bgamari

For the record, some time ago I actually tried quickly prototyping the ideas laid out in comment:3. The branch can be found here. The only slightly annoying issue with the approach is that it requires adding an index to all mentions of Doc, leading to a rather difficult to rebase branch. That being said, I really don't think there's a whole lot of work here if we agree that this is the right approach. If someone was motivated, I'm rather confident that we could get the painful refactoring changes into the tree within the space of a weekend.

Of course, the dynamically-typed alternative proposed by Richard in comment:9 is also still on the table. However, I suspect we'd rather want Doc to look like,

data Doc = forall a. (Typeable a, Outputable a) => Embed a
         | Empty
         | ...

to ensure that the consumer can always, at very least, output a human readable representation of the payload.

It's not clear which of these options is preferrable; I'll admit that the dynamically-typed option feels a bit like we are trading precision and long-term maintainability for some temporary convenience, but perhaps this is too pessimistic a view.

comment:32 Changed 3 months ago by Phyx-

Cc: Phyx- added

This looks nice, I don't know what the plans are, but I'd love for us to stop diverging on Windows support for stuff like this (VT100 escape sequences are supported fine on Windows 10 builds 10586 and later, or any console with ANSICON, ConEMU or xterm such as the standard MSYS2 console). I can help with the detection if you don't know how. I'll also be pushing an update to the Win32 package which adds a function isVT100ConsoleSupported. The definition can be in-lined and used.

comment:33 in reply to:  32 Changed 3 months ago by Rufflewind

Replying to Phyx-:

This looks nice, I don't know what the plans are, but I'd love for us to stop diverging on Windows support for stuff like this (VT100 escape sequences are supported fine on Windows 10 builds 10586 and later, or any console with ANSICON, ConEMU or xterm such as the standard MSYS2 console). I can help with the detection if you don't know how. I'll also be pushing an update to the Win32 package which adds a function isVT100ConsoleSupported. The definition can be in-lined and used.

I omitted support for Windows because I wanted to do it via SetConsoleTextAttribute, which would require further changes to Doc and its friends. I didn't realize it was still possible to ENABLE_VIRTUAL_TERMINAL_PROCESSING though − I had assumed it was a legacy feature that MS wanted to go away. That being said, the change in 10586 was actually a mistake. It is still necessary to manually ENABLE_VIRTUAL_TERMINAL_PROCESSING.

Actually, for Win32 it would be really nice to have GetConsoleMode. Then it'd be possible to color detection on Windows (heavily inspired by LLVM):

DWORD mode = 0;
HANDLE handle = GetStdHandle(STD_ERROR_HANDLE);
if (handle == INVALID_HANDLE_VALUE) {
    return 0;
}
if (GetConsoleMode, &mode) == 0) {
    return 0;
}
return 1;

comment:34 Changed 3 months ago by Phyx-

Yeah, you might still be required to manually turn on ENABLE_VIRTUAL_TERMINAL_PROCESSING and possibly DISABLE_NEWLINE_AUTO_RETURN If you get into modifying GHCi to have fancy stuff too.

That build is still significant since it's the one that brought native support in conhost for VT100 escape sequences. But yeah checking the flag on the buffer is what should be done, but because there's a large number of people using ANSICON we should check that too. VT100 support used to be only in the dos driver ansi.sys which is what is deprecated. You couldn't use it in Windows but the docs all referred to it.

But I'm happy to hear you plan on some Windows love too :). I wouldn't bother going down the road of SetConsoleTextAttribute. Unless we want to support the Pre Windows 10 era for this. That said, the design in comment:9 is the perfect way to make this easy to do. You just have a custom renderer then that interprets the Doc.

Yeah the patch will add GetConsoleMode and SetConsoleMode, but while they'll be usable for user program, the bootstrap process of GHC doesn't allow you to use libraries not available on the bootstrapping compiler. So you'd have to duplicate the code to inside GHC until a few releases down the road where we can undo the duplication.

You'll end up with something like:

isVT100ConsoleSupported :: IO Bool
isVT100ConsoleSupported
 = do isANSIcon <- fmap (maybe False ((/=""     ) . map toLower)) $ lookupEnv "ANSICON"
      isConEmu  <- fmap (maybe False ((=="on"   ) . map toLower)) $ lookupEnv "ConEmuANSI"
      isTerm    <- fmap (maybe False ((=="xterm") . map toLower)) $ lookupEnv "TERM"
      hOut <- getStdHandle sTD_OUTPUT_HANDLE
      outMode <- with 64 $ \ c_mask -> do
         failIfFalse_ "GetConsoleMode" $ c_GetConsoleMode hOut c_mask
         peek c_mask
      return $ or [ isANSIcon
                  , isConEmu
                  , isTerm
                  , let flag = outMode .&. eNABLE_VIRTUAL_TERMINAL_PROCESSING
                    in flag == eNABLE_VIRTUAL_TERMINAL_PROCESSING
                  ]

comment:35 Changed 3 months ago by simonpj

I'm delighted to see activity on the "better error messages" front. Thank you!

There are two separate lines of thought here:

  1. Using colour etc in SDoc, in a way that is portable across platforms. This seems to be non-trivial (comment:33 and comment:34), but it's non-controversial and it'd be great to have. I think Phab:D2716 and Phab:D2717 are about this.
  1. Some infrastructure to allow more info to be displayed in error messages. That's what Phab:D2718 is about.

I suggest we split (2) into a separate ticket. On (1) let's just do it.

On the (2) front, I confess that I'm not keen on the approach shown in Phab:D2718, which allows an SDoc to contain an I/O action. It smells wrong in principle to me; it would fork GHC away from our goal of using the Hackage pretty library (#10735); and I think we can do better.

I would love to make progress on adopting David Christiansen's idea for pretty printing, which he calls "A pretty printer that says what it means":

That's what Ben is alluding to in comment:31.

comment:36 Changed 3 months ago by Ben Gamari <ben@…>

In f1fc8cbf/ghc:

Make diagnostics slightly more colorful

This is a preliminary commit to add colors to diagnostics (warning and
error messages).  The aesthetic changes are:

  - 'warning', 'error', and 'fatal' are all colored magenta, red, and
    red respectively.
  - The warning annotation [-Wsomething] shares the same color.
  - Warnings and errors are also bolded (this is consistent with what
    other compilers do).

A new flag has been added to control the behavior:

    -fdiagnostics-color=(always|auto|never)

This flag is 'auto' by default.  However, auto-detection is not
implemented yet, so it effectively it defaults to off.

Test Plan: validate

Reviewers: austin, bgamari

Reviewed By: bgamari

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D2716

GHC Trac Issues: #8809

comment:37 Changed 3 months ago by Ben Gamari <ben@…>

In 52222f9b/ghc:

Detect color support

Test Plan: validate

Reviewers: erikd, Phyx, austin, bgamari

Reviewed By: bgamari

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D2717

GHC Trac Issues: #8809

comment:38 Changed 3 months ago by dobenour

I don't guess that the extremely hackish approach of using unsafeInterleaveIO would work here? The IO action (reading from a file) clearly need not be performed if its result is not used, and we can force it to happen by using deepSeq at the appropriate point (to make sure the SDoc is fully evaluated).

That would avoid needing the SDoc to contain an IO action at the expense of using unsafe code.

comment:39 in reply to:  38 Changed 3 months ago by Rufflewind

Replying to dobenour:

I don't guess that the extremely hackish approach of using unsafeInterleaveIO would work here? The IO action (reading from a file) clearly need not be performed if its result is not used, and we can force it to happen by using deepSeq at the appropriate point (to make sure the SDoc is fully evaluated).

That would avoid needing the SDoc to contain an IO action at the expense of using unsafe code.

I found a cleaner workaround for the problem, so IO actions in SDocs are no longer necessary. https://phabricator.haskell.org/D2718

comment:40 Changed 3 months ago by RyanGlScott

comment:41 Changed 3 months ago by Tamar Christina <tamar@…>

In 847d2293/ghc:

Color output is wreaking havoc on test results

Summary:
D2716 introduced colors into the output of GHC.
These color ourputs are done using escape characters output
to the terminal.

These however are wreaking havoc on the testsuite output as now
no stderr with a warning or error will match anymore.

Instead of accepting the new codes as expected values instead I
turn them off. So the testsuite is consistent on platforms/terminals we
don't support colors on.

Test Plan:
any test that outputs colors. e.g.

make test TEST=T9576

Reviewers: austin, Rufflewind, bgamari

Subscribers: thomie, #ghc_windows_task_force

Differential Revision: https://phabricator.haskell.org/D2787

GHC Trac Issues: #8809

comment:42 Changed 2 months ago by Rufflewind

Here's a sketch of what the caret diagnostics look like now:

testsuite/tests/warnings/should_fail/CaretDiagnostics1.hs:8:31: error:
    • Couldn't match expected type ‘[Char]’ with actual type ‘()’
    • In the second argument of ‘(+)’, namely ‘()’
      In the first argument of ‘pure’, namely
        ‘("this is not an IO" + ())’
      In a stmt of a 'do' block: pure ("this is not an IO" + ())

  pure ("this is not an IO" + (            ))
                              ^^^^^^^^^^^^^^

testsuite/tests/warnings/should_fail/CaretDiagnostics1.hs:13:7: error:
    • Couldn't match expected type ...

There are some minor tweaks to the style I mentioned previously: the underlining is now the same color as the error type (red = error, magenta = warning) and the underlined part of the code is also colored and bolded. It also uses exclusively carets (no more tildes). There is also an empty line separating the error message itself from the caret diagnostic.

I could go even further to add a margin, kind of like Rust:

testsuite/tests/warnings/should_fail/CaretDiagnostics1.hs:8:31: error:
    • Couldn't match expected type ‘[Char]’ with actual type ‘()’
    • In the second argument of ‘(+)’, namely ‘()’
      In the first argument of ‘pure’, namely
        ‘("this is not an IO" + ())’
      In a stmt of a 'do' block: pure ("this is not an IO" + ())
  |
8 |   pure ("this is not an IO" + (            ))
  |                               ^^^^^^^^^^^^^^

testsuite/tests/warnings/should_fail/CaretDiagnostics1.hs:13:7: error:
    • Couldn't match expected type ...

but I'm concerned that (a) it might add more noise to the diagnostics; (b) it loses the nice property that if your source code is ≤ 80 characters, the error message is guaranteed to be also ≤ 80 characters (otherwise, if the user's terminal is too small, the messages will wrap and become misaligned).

comment:43 Changed 2 months ago by joehillen

+1 for margin. I don't see the need for concern about line length; no other compiler seems to care about that.

I really hope the "In the first/second argument of" can someday be dropped. It's useless noise once you have carets, and it's always been difficult to grok.

comment:44 Changed 2 months ago by Ben Gamari <ben@…>

In 158530a5/ghc:

Add caret diagnostics

This is controlled by -f[no-]diagnostics-show-caret.

Example of what it looks like:
```
    |
 42 |     x = 1 + ()
    |         ^^^^^^
```
This is appended to each diagnostic message.

Test Plan:
testsuite/tests/warnings/should_fail/CaretDiagnostics1
testsuite/tests/warnings/should_fail/CaretDiagnostics2

Reviewers: simonpj, austin, bgamari

Reviewed By: simonpj, bgamari

Subscribers: joehillen, mpickering, Phyx, simonpj, alanz, thomie

Differential Revision: https://phabricator.haskell.org/D2718

GHC Trac Issues: #8809

comment:45 Changed 2 months ago by bgamari

One step closer to prettiness!

However, I still think having more semantically rich messages would be great for tooling authors.

comment:46 in reply to:  45 Changed 2 months ago by goldfire

Replying to bgamari:

However, I still think having more semantically rich messages would be great for tooling authors.

I have a student doing an independent study with me next semester who has expressed an interest in doing this. We've only really started the conversations on what she will work on, but at the moment, refactoring SDoc along the lines of comment:3 seems like the leading candidate.

So there is reason to be hopeful about this!

comment:47 Changed 5 weeks ago by mpickering

I'm a bit confused about the status of this ticket. The original poster suggests adding carets to error messages, this has now been done but the discussion in the ticket has moved all over the place. I want to bring order to the chaos!

I think a new ticket which focuses just on the issue of adding semantic information to error messages should be created. There should also be (dare I say it...) a wikipage which outlines the suggested designs on this ticket.

Richard, great that your student wants to work on this. Is that still the plan? Could she perhaps take care of creating the new ticket and the wikipage if she decides to take ownership of this. I entered this ticket with the intention to do so but don't want to step on her toes.

When I last looked at this it seemed that the main problem was #10735 which talks about the problems with pretty which have so far put everyone off working on this (me included). The crux of the matter is that pretty isn't very efficient and we use it in the code generator. The solution here seems to be to use a different (simpler) pretty printing library for code generation as this shouldn't require complicated layout instructions such as hanging or nesting.

comment:48 Changed 5 weeks ago by bgamari

mpickering, I agree that the issue of semantics messages should be moved to another ticket. Richard, perhaps you could have your student do this when the semester starts up?

The solution here seems to be to use a different (simpler) pretty printing library for code generation as this shouldn't require complicated layout instructions such as hanging or nesting.

As I've expressed in the past, I'm not at all sure that the solution to #10735 is to invent another pretty-printer. As far as I know there is no reason why the semantics implemented by pretty can't be implemented with the exact same computation effort as a simpler printer in the case of infinite ribbon width. I know that I have found the ability to spit out SDocs in assembler output to be quite useful in the past and I would hate to lose it, especially if doing so meant introducing a redundant pretty-printing implementation which could be avoided with a bit of refactoring.

Of course, if I'm wrong and there is a fundamental reason why pretty incurs an unavoidable overhead then I'm happy to concede this point.

comment:49 Changed 5 weeks ago by goldfire

Yes, it is still the plan for my student to work on this. But I would not expect much progress soon, as this student is still in the process of learning Haskell. She and I will meet next week to frame our work for the semester, but I doubt she'll be ready to knowledgeably make a wiki page, etc., until Feb at the earliest.

comment:50 in reply to:  48 Changed 5 weeks ago by mpickering

Replying to bgamari:

mpickering, I agree that the issue of semantics messages should be moved to another ticket. Richard, perhaps you could have your student do this when the semester starts up?

The solution here seems to be to use a different (simpler) pretty printing library for code generation as this shouldn't require complicated layout instructions such as hanging or nesting.

As I've expressed in the past, I'm not at all sure that the solution to #10735 is to invent another pretty-printer. As far as I know there is no reason why the semantics implemented by pretty can't be implemented with the exact same computation effort as a simpler printer in the case of infinite ribbon width. I know that I have found the ability to spit out SDocs in assembler output to be quite useful in the past and I would hate to lose it, especially if doing so meant introducing a redundant pretty-printing implementation which could be avoided with a bit of refactoring.

Of course, if I'm wrong and there is a fundamental reason why pretty incurs an unavoidable overhead then I'm happy to concede this point.

To put this in context, no one is still sure where the problem in pretty lies and this isn't for lack of trying. At least @thomie, @erikd and @niteria (three very fine hackers) have tried to find out where the problems are but haven't managed to find a fix. We don't want to move ahead with this until #10735 is resolved as doing so would cause pretty to diverge even further.

This being said, I consider this ticket a high priority for 8.4 as it finally will provide better support for tooling. This is why I don't want it to be blocked on a very difficult ticket when there is a simple albeit suboptimal solution on the table. The benefit of implementing this outweighs the cost of the loss of convenience in the code generation in my opinion.

comment:51 Changed 5 weeks ago by mpickering

Blocking: 13122 added

comment:52 Changed 5 weeks ago by Rufflewind

TBH, I think SDoc is too low level to embed rich semantic information like expressions, locations etc. It should stick to what it does best: laying out dumb text data. Instead, I think it'd be nicer to have a simple high-level data type for diagnostics, which could look like:

data Error = Error Severity (Maybe ErrorID) ErrorMessage [(Location, Maybe Expression)]
type ErrorID = String -- i.e. flag that disables the warning
                      -- it'd be nice to have them for errors too
type ErrorMessage = [ErrMsgSegment]
data ErrMsgSegment = EDoc SDoc | ELink Int -- link to some expression of interest

… something along those lines. There's plenty of room to bikeshed on the details, but it'd be great to keep it as simple as possible so as to minimize the effort required by downstream to interpret the diagnostics. In contrast, an SDoc does not give the consumer a lot of flexibility in the display of an error.

This approach allows for a gradual transition: simply edit the legacy error-emitting framework to emit an Error severity Nothing (EDoc msg) [(loc, Nothing)], which is the kind of “dumb” error that we have today. To take advantage of the new system, we just bypass the legacy framework and create Errors directly.

comment:53 Changed 5 weeks ago by goldfire

I'm a little lost now. It looks like pretty already has the annotations proposed in comment:3. So is #10735 really all that's left to allow annotations within SDocs? And I remember somewhere that color would be impossible without a design like comment:3... and yet we have color now.

Re comment:52: I like this idea a good deal, and I'll admit it's where I started my thinking about this problem, but then comment:3 seemed to go in a different (but quite worthwhile) direction.

So I guess the direct question is: what should my student and I work on? Adding annotations to SDocs seems redundant. Tackling #10735 will likely be beyond her level of experience. So I'm tempted to steer her toward comment:52... but I'd love some feedback before we start getting in too deep in one particular direction.

comment:54 Changed 5 weeks ago by mpickering

Phil makes a good point about where the semantic information should be embedded. In fact, this semantic information is already there in two cases, which flag the warning was caused by and the position of the error (see TcRnMonad.add_warn_at). In fact, looking at ErrMsg, there is already a lot of semantic information present there as well. These errors are then caught by ioMsgMaybe in HscMain and reported by throwErrors where the information is somehow lost but I have to go!

comment:55 Changed 5 weeks ago by mpickering

It seems that the Show instance for ErrMsg needs to be investigated as well as functions like throwOneError which bypass all this machinery. This is starting to seem infinitely more tractable now.

comment:56 Changed 5 weeks ago by bgamari

Phil, I'm not sure I understand your ELink constructor. Is the Int here a stand-in for any old type, or is this really an Int? If the latter, what does it represent?

Not to discourage discussion of other options, but I'd just like to remind everyone that Idris has set some very nice precedent for the semantics-details-in-pretty-printer approach. David Christiansen summarizes this nicely in his HIW 2015 talk, https://www.youtube.com/watch?v=m7BBCcIDXSg. I'd encourage anyone interested in this ticket to watch this talk.

In general I worry that by framing the problem solely in terms of "error messages" as done in comment:52 we miss out on the richness that the Idris folks enjoy. What enables their nice presentation is the fact that the semantically rich objects and the pretty-printer documents are one and the same. This means that any time your error pprs a type/expression/source span/etc. you automatically get a rich representation for free. This is something that I believe would be non-trivial to reproduce in the approach of comment:52. Recall that in GHC we typically build up error messages compositionally from a variety of often nested pieces; it seems to me that by distinguishing "error messages" from SDoc we give up the ability to do this (unless, of course, we refactor our error message building blocks in terms of ErrMsg, in which case why didn't we just use SDoc to begin with?).

comment:57 in reply to:  53 Changed 5 weeks ago by bgamari

Replying to goldfire:

I'm a little lost now. It looks like pretty already has the annotations proposed in comment:3. So is #10735 really all that's left to allow annotations within SDocs?

That is pretty much right.

And I remember somewhere that color would be impossible without a design like comment:3... and yet we have color now.

Well this isn't quite true. Yes, we have color, but I think part of the initial suggestion was to make much greater use of color throughout the message. For instance, we might render the type Int in one color and String in another, or perhaps render all types in one color and all expressions in another. For this we need more rich semantic content within the message so we don't need to bury the details of error message styling deep inside the compiler.

Last edited 5 weeks ago by bgamari (previous) (diff)

comment:58 Changed 5 weeks ago by alanz

And if the semantically rich errors can be accessed via the GHC API it makes much better options available to tool developers.

comment:59 Changed 5 weeks ago by mpickering

Here is how I see this ticket. There are two suggestions:

  1. Make it possible to output error messages as JSON (what I want and easy with current infrastructure).
  2. Add more semantic information to error messages (ie attaching the actually expressions to SDocs, what Ben is mainly talking about, idris style).

Ben in his last comment is saying that if 2. is implemented then 1. comes for free (I think). The main advantage of 2. is that you can build interfaces which use the additional localised information to provide different information to users, ie highlighting, hovers etc. This seems right to me but implementing 2. is a significant amount of work compared to 1. and I can't imagine the exact design off the top of my head.

My essential point is that 1. should not be blocked by 2. Once 2. is implemented then 1. can be reimplemented in terms of 2. There seems to be suggestions of how to resolve the pretty issue but even after that there is still an extremely large amount of work to embed the right information in the SDoc.

comment:60 Changed 5 weeks ago by bgamari

alanz, indeed much of the motivation for providing these annotations to begin with is to enable tooling like Idris'.

mpickering, your assessment is pretty much dead on. I agree that we can and should move ahead with (1) independently from (2). ErrMsg already includes the information that we would need to do this; all that needs to be done is to ensure that we don't lose it and potentially add some output format for tooling consumers (e.g. JSON).

However, I think we should refrain from trying to solve the weakened form of (2) that comment:52 tackles. This approach appears to introduce a lot of accidental complexity for what is easily accomplished using an existing annotated pretty-printing library. The only thing holding us back from moving ahead here is #10735.

As far as I can tell, the problem in #10735 is that both the Wadler-Leijen and Hughes pretty-printers can become quadratic in some cases. This hurts a great deal in the case of GHC, where we use the pretty-printer to emit massive quantities of code. Mpickering has advocated that we work around the issue by simply eliminating the use of the pretty-printer for code generation. I would prefer that we not do this if possible. Ultimately I believe that the root cause of the non-linearity is back-tracking due to the document exceeding the ribbon width. However, when we are producing code we configure pretty with an infinite ribbon width, meaning that it should never be necessary to backtrack. Unfortunately the pretty implementation doesn't take advantage of this fact. I argue that this should be fixed.

The other related issue is the inability to make use of FastString with the upstream pretty since the text combinator has type String -> Doc. This is actually a very poor interface as text needs the length of the string, which is an O(n) operation. Really it should at least allow usage of a more efficient string representation. niteria and I discussed this a few months ago and agreed that this could be resolved by simply generalizing text over its string type with a typeclass providing an unpack and length function.

goldfire, does any of the above sound like a reasonable task for your student?

comment:61 in reply to:  56 Changed 5 weeks ago by Rufflewind

Replying to bgamari:

Phil, I'm not sure I understand your ELink constructor. Is the Int here a stand-in for any old type, or is this really an Int? If the latter, what does it represent?

It's meant to be an index that points to the relevant item inside the [(Location, Maybe Expression)]. It's not a very good representation for links, but that was just a sketch of the general idea.

What enables their nice presentation is the fact that the semantically rich objects and the pretty-printer documents are one and the same.

If my understanding of David Christiansen's talk is right, then I don't agree that conflating pretty-printer documents and semantically rich objects is a good idea, because you are throwing away structural information in the process while also committing to a specific representation of the error message. In effect, the burden is shifted towards the consumers, because they have sift through the Doc to find the parts the want (what if they want to rearrange pieces of the error message?). Moreover, if GHC decides to make a superficial change in how the errors are laid out, then it will impact downstream consumers.

If I may draw upon an analogy, this is kind of like if a web API decided to, instead of emitting data using simple JSON, emitted full-blown HTML files that are many times more difficult to parse while also requiring you do sift through the HTML elements to find the parts you want. Meanwhile, you'd worry what would happen to the structure if the web API gets upgraded.

I'm all for semantically rich error messages. Error data type I sketched earlier could be tweaked to allow for the kinds of things that David Christiansen talks about. But by keeping the structural information it would provide a more informative interface for consumers downstream. In other words, I would much prefer:

Error loc (TypeMismatch expr1 expr2)

than

docLoc loc <+> text "Type mismatch:" <+> docExpr expr1 <+> "vs" <+> docExpr expr2

comment:62 Changed 5 weeks ago by Iceland_jack

Ideas from ignorance: Apply technique from Trees That Grow + index something by the type it generates or by a sublanguage (more efficient sublanguage of pretty, error messages in GHC syntax tree)

Last edited 5 weeks ago by Iceland_jack (previous) (diff)

comment:63 Changed 5 weeks ago by goldfire

The previous string of comments are very very helpful. I have a good understanding of the issues here. I also don't have any strong opinions about good vs bad design, so I'm happy to let you folks debate this.

As for my student: this student is fairly new to me (and to Haskell), and we have yet to have our first meeting in the spring semester. My guess is that she will be able to work well with a rather-concrete specification -- but it seems we are moving in that direction (modulo challenges with #10735).

comment:64 Changed 4 weeks ago by bgamari

For the record, yesterday dalaing_ stopped by #ghc and expressed interest in picking the pretty refactoring necessary for this ticket (comment:60). I'll try to make sure he subscribes to this ticket.

comment:65 Changed 4 weeks ago by goldfire

Bad news and good news.

Bad news first: my student who said she was keen on this has changed direction and will not be working on this in the near future. Good news: she's spending the semester learning more type theory, and I'm hoping to get her to tackle something like implication constraints or visible type application in patterns or something more type-y. Even better news: where there was originally one student, there are now three. So the future bodes well.

In any case, I have no intention of myself or anyone in my immediate vicinity to work on this ticket.

comment:66 Changed 4 weeks ago by dalaing

Cc: dalaing added

If/when I start working on pretty, should I be tracking that work here, or in #10735?

comment:67 in reply to:  65 Changed 4 weeks ago by Iceland_jack

Replying to goldfire:

… something like implication constraints or visible type application in patterns …

<3!

comment:68 Changed 4 weeks ago by bgamari

dalaing, I think #10735 is probably more appropriate.

comment:69 Changed 2 weeks ago by michalt

Cc: michalt added
Note: See TracTickets for help on using tickets.