Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#10021 closed feature request (fixed)

Add "Error:" prefix for compile-time error messages

Reported by: k-bx Owned by: k-bx
Priority: normal Milestone: 8.0.1
Component: Compiler Version: 7.8.4
Keywords: Cc: sean.leather@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description (last modified by spl)

We propose adding the prefix Error: to compile-time error messages produced by GHC.

An example of a warning message (see Warning: prefix):

➜ cat test.hs
main = putStrLn "Hello!"
➜ ghc --make -fforce-recomp -Wall ./test.hs
[1 of 1] Compiling Main             ( test.hs, test.o )

test.hs:1:1: Warning:
    Top-level binding with no type signature: main :: IO ()
Linking test ...

An example of a current error message:

➜ cat test2.hs
main = foo
➜ ghc --make -fforce-recomp -Wall ./test2.hs
[1 of 1] Compiling Main             ( test2.hs, test2.o )

test2.hs:1:8: Not in scope: ‘foo’

The proposal is to change the error message to become:

➜ cat test2.hs
main = foo
➜ ghc --make -fforce-recomp -Wall ./test2.hs
[1 of 1] Compiling Main             ( test2.hs, test2.o )

test2.hs:1:8: Error: Not in scope: ‘foo’

This change affects only the error messages produced by GHC when compiling. It does not change runtime error reporting, e.g. with the error function.

Motivation

We wish to make compile-time error messages easier for humans and computers to identify. For example, when running a long parallel build, it can be easy to miss an error message (see #9219 for an example). With a known string such as Error:, we can use search functionality in a terminal window or text editor (when viewing the build log) or use a command-line tool such as grep (when streaming and filtering the build log) to quickly find errors.

As a secondary motivation, adding Error: for error messages brings a nice symmetry to the current use of Warning: for warning messages.

Disadvantages

The output of many regression tests will need to be updated for the new error message format.

Additional consideration

@hvr suggested that during work on this bug we could make GHC output more consistent with other compilers (GCC and clang) by using lowercase error: and warning: prefixes. This might be potentially useful for existing tools that work on top of compiler output for finding errors/warning, and GHC's output will become "more valid" for them.

Change History (20)

comment:1 Changed 3 years ago by simonpj

You don't motivate the change. Could you update the Description to explain why this would be a good change to make.

Are there any disadvantages? Presuambly ever should_fail test in GHC's regression suite, and every other library's regression suite, would need to be updated, which would be a nuisance.

If there's a consensus in favour, I'm happy for this to happen. It should also be easy to implement.

Simon

comment:2 Changed 3 years ago by k-bx

Description: modified (diff)

comment:3 Changed 3 years ago by k-bx

@simonpj added motivation and disadvantages sections. Waiting for more input on mailing list for now and started digging into ghc ecosystem (e.g. was fighting few sh validate errors).

comment:4 Changed 3 years ago by simonpj

Description: modified (diff)

comment:5 Changed 3 years ago by hvr

I'd like to propose to change the case of Error and Warning to lower case to match the convention as used by the most popular C/C++-compilers these days.

For reference, this is what gcc currently does:

$ gcc -Wall  -c foo.c
foo.c: In function ‘main’:
foo.c:2:1: warning: control reaches end of non-void function [-Wreturn-type]
 int main() { }
 ^
$ gcc -Wall -Werror -c foo.c
foo.c: In function ‘main’:
foo.c:2:1: error: control reaches end of non-void function [-Werror=return-type]
 int main() { }
 ^
cc1: all warnings being treated as errors

and here's what clang does:

$ clang-3.5 -Wall  -c foo.c
foo.c:2:18: warning: unused variable 'x' [-Wunused-variable]
int main() { int x = 0; }
                 ^
1 warning generated.
$ clang-3.5 -Wall -Werror -c foo.c
foo.c:2:18: error: unused variable 'x' [-Werror,-Wunused-variable]
int main() { int x = 0; }
                 ^
1 error generated.

comment:6 Changed 3 years ago by k-bx

Description: modified (diff)
Summary: Add "Error:" prefix for error-messagesAdd "Error:" prefix for compile-time error messages

comment:7 Changed 3 years ago by k-bx

@hvr while Emacs compilation highliter (the one that lets you navigate through errors after M-x compile) is smart enough to understand current GHC output, I would say that lower-casing might be useful for other similar tools that work on top of compiler's output and are hard-coded for GCC/clang lower-cased "error" and "warning", so +1 from me on this.

comment:8 Changed 3 years ago by spl

Cc: sean.leather@… added
Description: modified (diff)

I modified the ticket description in an attempt to be more clear about the proposal and to expand the motivation.

P.S. I'm heavily in favor of this.

comment:9 in reply to:  5 Changed 3 years ago by spl

Description: modified (diff)

comment:10 Changed 3 years ago by k-bx

One more thing to consider. Current severities are:

data Severity
  = SevOutput
  | SevDump
  | SevInteractive
  | SevInfo
  | SevWarning
  | SevError
  | SevFatal

For severity SevFatal, would it make sense to output fatal: <msg>, stick to error: <msg>, or just keep it as it is right now?

comment:11 Changed 3 years ago by simonpj

Yes, it's horrible that these seven (seven!) levels of severity are declared in a data type with no comment whatsoever to indicate what the author had in mind. One can guess from the names but it still seems like a huge omission. I have no opinion about fatal vs error for SevFatal. I'm inclined to the former, since it reflects the underlying truth.

comment:12 Changed 3 years ago by spl

Having not looked at the code, I'm not even sure how output, dump, and interactive (to say the least) are cases of severity.

comment:13 Changed 3 years ago by k-bx

Owner: set to k-bx

comment:14 Changed 3 years ago by k-bx

Just wanted to update a little on my work. I had one week off and will have one more now, so things are slower than I wanted, otherwise here are how things going:

I wrote two scripts to help me working on this issue. First is turtle-replace-stdout.hs, second is turtle-review.hs. Here they are: https://gist.github.com/k-bx/fe7027c919980eb9b7f0

When you run first one, it will replace (almost) all failed test's actual output with expected one. Afterwards, you should run second script, which will review trivial cases and git-add them. Finally, the ones that are left were reviewed manually.

Currently status is that few tests still fail upon validate, and few will probably become failing after rebase. Anyway, will finish the work quickly after I come back from Norway.

comment:15 in reply to:  14 Changed 3 years ago by thomie

I wrote two scripts to help me working on this issue. First is turtle-replace-stdout.hs, second is turtle-review.hs. Here they are: https://gist.github.com/k-bx/fe7027c919980eb9b7f0

I haven't look at your script in detail, but are you aware of make accept?

comment:16 Changed 3 years ago by k-bx

@thomie thanks, haven't been aware of it at all. Will check it out!

comment:17 Changed 3 years ago by thomie

If you're going to run that on the full testsuite, make sure you install these packages with the inplace compiler. Otherwise some tests get skipped.

comment:18 Changed 3 years ago by Austin Seipp <austin@…>

In 7febc2bb86b238713cfb9f52dff32039464dfe66/ghc:

Add "error:" prefix to error-messages

Add "error:" prefix to error-messages, also lowercase "Warning:"
message to match GCC behavior closer.

Reviewed By: thomie, austin

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

GHC Trac Issues: #10021

comment:19 Changed 3 years ago by k-bx

Resolution: fixed
Status: newclosed

comment:20 Changed 2 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

Note: See TracTickets for help on using tickets.