Skip to content

Improve error message formatting for tool interop#1285

Closed
epost wants to merge 2 commits intopurescript:masterfrom
epost:gcc-error-format
Closed

Improve error message formatting for tool interop#1285
epost wants to merge 2 commits intopurescript:masterfrom
epost:gcc-error-format

Conversation

@epost
Copy link
Copy Markdown
Contributor

@epost epost commented Jul 19, 2015

Improve editor interop by printing compiler errors in a format like that of gcc, ghc, javac, etc.

The new format allows editors like emacs to highlight errors and warnings correctly, and in such a way that the highlighting depends on the error level. Additionally, (emacs) features like 'go to error line' and 'go to next error' now work (more) correctly. See also #1098.

Changes:

  • The headings Error: and Warning: at the top of the error list are interpreted by emacs as proper errors, which causes problem with highlighting and with jumping to the error line, because it doesn't exist. Fix: replace Error: by Error found: and so on. The introduction of a space character fixes the problem.
  • PositionErrors are now formatted like /path/to/File.purs:3:5:warning:Some descriptive message, following gcc. This allows emacs to parse the source position and error loevel correctly.
  • The ending positions of the span are no longer printed. They are still available in the JSON format.

Before:

Error:
Error in module Foo:
Error at /Users/erik/dev/ex/examples-purescript/purescript-0.7-project/error-msg-formats/bla.purs line 3, column 1 - line 4, column 1:
  Orphan type declaration for f

After:

Error found:
Error in module Foo:
/Users/erik/dev/ex/examples-purescript/purescript-0.7-project/error-msg-formats/bla.purs:3:1:Error:
  Orphan type declaration for f

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented Jul 19, 2015

Looks good to me as a default. Later we can add JSON support too, perhaps, for other editors. I'd be interested, as a non-Emacs user, to see how this looks.

@epost
Copy link
Copy Markdown
Contributor Author

epost commented Jul 19, 2015

Cool! There's a pic in #1098. I think JSON support is already in there; didn't @hdgarrood add it (somewhat) recently?

@hdgarrood
Copy link
Copy Markdown
Contributor

I didn't, I guess you might be thinking of psc-publish, which produces JSON for Pursuit? I did add a bunch of JSON instances for types like Language.PureScript.Type and Language.PureScript.Kind while I was doing that.

@hdgarrood
Copy link
Copy Markdown
Contributor

I'm not sure about this, I think the current format is quite a bit easier to read. Also, does this mean we lose the information about where the problematic part of the code ends?

@epost
Copy link
Copy Markdown
Contributor Author

epost commented Jul 19, 2015

Hi Harry, thanks for your comments. A few thoughts:

  • Yes, this discards the end-of-span coordinate from the output. There may be a way in this format to include the end of the span in a standard way, but I'd have to research that. I did leave all the original code in place so it would be trivial to put it back later. Another option might be to have it in the JSON (if/when that lands) but not in the plaintext output.
  • How do you envision the end-of-span coordinates being used in their current format? E.g. do you expect the programmer reading the character position off the screen and navigating there in his editor manually? What does your editor (vi?) do with the output for example?
  • I checked three compilers: gcc, ghc, and javac. They all use this format, and emacs understands it out of the box. It seems like a fairly safe assumption that other compilers --and perhaps editors-- will do the same. Adhering to a common format like that seems like a clear win to me, but I don't think I could offer any arguments to change your mind on its readability, unfortunately. Would it help if I found with some more compilers/tools using this format?
  • I agree there are some improvements to be made, readability-wise, but I wanted to come up with a minimal but usable first (ever) PR first. One improvement might be to suppress the full path to to the source file in case it matches the working dir, and make it relative to that instead. I think that would improve readability quite a bit. The order and formatting could be improved too, but that seemed a bit trickier.
  • Where I got the JSON idea: there was some talk under Parse errors are formatted differently, which trips up editors trying to parse them. #1098 about adding JSON output, and I came across this SourcePos instance

@hdgarrood
Copy link
Copy Markdown
Contributor

E.g. do you expect the programmer reading the character position off the screen and navigating there in his editor manually?

Yes, I often do this. I use Vim; I don't know how Vim would react to the current output, as I haven't tried to set that up.

Adhering to a common format like that seems like a clear win to me

I agree that it is a clear win for people who have their editor parse the compiler output, but I also think it is a clear loss for people who read it themselves. I think the default should be optimized for human readability. Could the editor-friendly output be off by default, and turned on with a command line switch?

One improvement might be to suppress the full path to to the source file in case it matches the working dir, and make it relative to that instead

I like this suggestion very much.

@jdegoes
Copy link
Copy Markdown

jdegoes commented Jul 19, 2015

Could the editor-friendly output be off by default, and turned on with a command line switch?

This is somewhat subjective. Having been writing software since the late 80's, the "editor-friendly" style is very easy for me to read, in fact I have great difficulty parsing the current PureScript error messages.

But as long as you can change the mode from the command-line / build tool, works for me.

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented Jul 23, 2015

I agree that we should try to keep the end position of the range. Maybe this should be a format string as a command line argument or something?

@jdegoes
Copy link
Copy Markdown

jdegoes commented Jul 23, 2015

There is already a convention for editors to specify the range in the error / warning message: it's generally, <start_row>:<start_col>:<end_row>:<end_col>. Sometimes the middle colon is a dash or something else, see below. Many editors can extract this information already.

    public static final ErrorWarningPattern GNU_STYLE_ROW_COL_WITH_END = BUILDER.build("${FILE}:${ROW}${REGEXP=[:\\.]}${COL}-${END_ROW}${REGEXP=[:\\.]}${END_COL}: ${MESSAGE}");
    public static final ErrorWarningPattern GNU_STYLE_ROW_COL   = BUILDER.build("${FILE}:${ROW}${REGEXP=[:\\.]}${COL}: ${MESSAGE}");
    public static final ErrorWarningPattern GNU_STYLE_ROW       = BUILDER.build("${FILE}:${ROW}: ${MESSAGE}");
    public static final ErrorWarningPattern GNU_STYLE_ROW_COL_WITH_END_E = BUILDER.build("${FILE}:${ROW}${REGEXP=[:\\.]}${COL}-${END_ROW}${REGEXP=[:\\.]}${END_COL}: Error: ${MESSAGE}");
    public static final ErrorWarningPattern GNU_STYLE_ROW_COL_E = BUILDER.build("${FILE}:${ROW}${REGEXP=[:\\.]}${COL}: Error: ${MESSAGE}");
    public static final ErrorWarningPattern GNU_STYLE_ROW_E     = BUILDER.build("${FILE}:${ROW}: Error: ${MESSAGE}");
    public static final ErrorWarningPattern GNU_STYLE_ROW_COL_WITH_END_W = BUILDER.build("${FILE}:${ROW}:${COL}-${END_ROW}:${END_COL}: Warning: ${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern GNU_STYLE_ROW_COL_W = BUILDER.build("${FILE}:${ROW}:${COL}: Warning: ${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern GNU_STYLE_ROW_W     = BUILDER.build("${FILE}:${ROW}: Warning: ${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern ABS_FORTRAN_77_E    = BUILDER.build("Error on line ${ROW} of ${FILE}: ${MESSAGE}");
    public static final ErrorWarningPattern ABS_FORTRAN_77_W    = BUILDER.build("warning on line ${ROW} of ${FILE}: ${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern D_ERROR             = BUILDER.build("${FILE}(${ROW}): ${MESSAGE}");
    public static final ErrorWarningPattern D_ERROR_MULTILINE   = BUILDER.build("${FILE}(${ROW}): called with argument types:${NEWLINE}\\t${MESSAGE}${NEWLINE}${MESSAGE}${NEWLINE}\\t${MESSAGE}${NEWLINE}${MESSAGE}${NEWLINE}\\t${MESSAGE}");
    public static final ErrorWarningPattern DELPHI_ERROR        = BUILDER.build("${FILE}(${ROW}) Error: ${MESSAGE}");
    public static final ErrorWarningPattern DELPHI_WARNING      = BUILDER.build("${FILE}(${ROW}) Warning: ${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern ANT_JAVAC_ERROR     = BUILDER.build("[javac] ${FILE}:${ROW}: ${MESSAGE}");
    public static final ErrorWarningPattern ANT_JAVAC_WARNING   = BUILDER.build("[javac] ${FILE}:${ROW}: warning: ${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern FAN_LANGUAGE        = BUILDER.build("${FILE}(${ROW},${COL}): ${MESSAGE}");
    public static final ErrorWarningPattern ANT_JIKES_ERROR     = BUILDER.build("[jikes] ${FILE}:${ROW}:${COL}:${END_ROW}:${END_COL}: Semantic:${MESSAGE}");
    public static final ErrorWarningPattern ANT_JIKES_WARN1     = BUILDER.build("[jikes] ${FILE}:${ROW}:${COL}:${END_ROW}:${END_COL}: Warning:${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern ANT_JIKES_WARN2     = BUILDER.build("[jikes] ${FILE}:${ROW}:${COL}:${END_ROW}:${END_COL}: Caution:${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern JAVAC_ERROR         = BUILDER.build("${NUMBER}. ERROR in ${FILE} (at line ${ROW})${NEWLINE}${ANYTHING}${NEWLINE}${COL_INDICATOR}${NEWLINE}${MESSAGE}");
    public static final ErrorWarningPattern JAVAC_WARNING       = BUILDER.build("${NUMBER}. WARNING in ${FILE} (at line ${ROW})${NEWLINE}${ANYTHING}${NEWLINE}${COL_INDICATOR}${NEWLINE}${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern MICROSOFT_CPP_ERROR = BUILDER.build("${FILE}(${ROW}) : error ${REGEXP=[A-Z]+}${NUMBER}: ${MESSAGE}");
    public static final ErrorWarningPattern MICROSOFT_CPP_WARN  = BUILDER.build("${FILE}(${ROW}) : warning ${REGEXP=[A-Z]+}${NUMBER}: ${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern MICROSOFT_CS_ERROR  = BUILDER.build("${FILE}(${ROW}) error ${REGEXP=[A-Z]+}${NUMBER}: ${MESSAGE}");
    public static final ErrorWarningPattern MICROSOFT_CS_WARN   = BUILDER.build("${FILE}(${ROW}) warning ${REGEXP=[A-Z]+}${NUMBER}: ${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern PERL_ERROR1         = BUILDER.build("${MESSAGE} at ${FILE} line ${ROW}");
    public static final ErrorWarningPattern PERL_ERROR2         = BUILDER.build("${MESSAGE} in file ${FILE} at line ${ROW}");
    public static final ErrorWarningPattern PHP_ERROR1          = BUILDER.build("Parse error: syntax error, ${MESSAGE} in ${FILE} on line ${ROW}");
    public static final ErrorWarningPattern PHP_ERROR2          = BUILDER.build("Fatal error: ${MESSAGE} in ${FILE} on line ${ROW}");
    public static final ErrorWarningPattern GPL_XML_ERROR       = BUILDER.build("Error: ${MESSAGE}${NEWLINE}${MESSAGE} at line ${ROW} char ${COL} of file://${FILE}");
    public static final ErrorWarningPattern GPL_XML_WARNING     = BUILDER.build("Warning: ${MESSAGE}${NEWLINE}${MESSAGE} at line ${ROW} char ${COL} of file://${FILE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern RUBY_SYNTAX_ERROR   = BUILDER.build("${FILE}:${ROW}: syntax error, ${MESSAGE}");
    public static final ErrorWarningPattern CAML_ERROR          = BUILDER.build("File \"${FILE}\", line ${ROW}, characters ${COL}-${END_COL}:${NEWLINE}Error: ${MESSAGE}");
    public static final ErrorWarningPattern CAML_ERROR_2        = BUILDER.build("File \"${FILE}\", line ${ROW}, characters ${COL}-${END_COL}:${NEWLINE}Parse error: ${MESSAGE}");
    public static final ErrorWarningPattern CAML_WARNING        = BUILDER.build("File \"${FILE}\", line ${ROW}, characters ${COL}-${END_COL}:${NEWLINE}Warning: ${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern CAML_WARNING_2      = BUILDER.build("File \"${FILE}\", line ${ROW}, characters ${COL}-${END_COL}:${NEWLINE}Warning ${REGEXP=[A-Z]}: ${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern PYTHON_ERROR1       = BUILDER.build("  File \"${FILE}\", line ${ROW}${NEWLINE}${ANYTHING}${NEWLINE}  ${COL_INDICATOR}${NEWLINE}${REGEXP=\\w+Error}: ${MESSAGE}");
    public static final ErrorWarningPattern PYTHON_ERROR2       = BUILDER.build("  File \"${FILE}\", line ${ROW}${NEWLINE}${ANYTHING}${NEWLINE}${REGEXP=\\w+Error}: ${MESSAGE}");
    public static final ErrorWarningPattern PYTHON_ERROR3       = BUILDER.build("  File \"${FILE}\", line ${ROW}${NEWLINE}${REGEXP=\\w+Error}: ${MESSAGE}");
    public static final ErrorWarningPattern PYTHON_ERROR4       = BUILDER.build("  File \"${FILE}\", line ${ROW}, in ${ANYTHING}${NEWLINE}${ANYTHING}${NEWLINE}  ${COL_INDICATOR}${NEWLINE}${REGEXP=\\w+Error}: ${MESSAGE}");
    public static final ErrorWarningPattern PYTHON_ERROR5       = BUILDER.build("  File \"${FILE}\", line ${ROW}, in ${ANYTHING}${NEWLINE}${ANYTHING}${NEWLINE}${REGEXP=\\w+Error}: ${MESSAGE}");
    public static final ErrorWarningPattern PYTHON_ERROR6       = BUILDER.build("  File \"${FILE}\", line ${ROW}, in ${ANYTHING}${NEWLINE}${REGEXP=\\w+Error}: ${MESSAGE}");
    public static final ErrorWarningPattern PYTHON_ERROR7       = BUILDER.build("  SyntaxError: ${MESSAGE} (${FILE}, line ${ROW})${NEWLINE}${ANYTHING}${NEWLINE}  ${COL_INDICATOR}");
    public static final ErrorWarningPattern PYTHON_ERROR8       = BUILDER.build("  SyntaxError: ${MESSAGE} (${FILE}, line ${ROW})");
    public static final ErrorWarningPattern PYTHON_INDENT_ERROR = BUILDER.build("  IndentationError: ${MESSAGE} (${FILE}, line ${ROW})");
    public static final ErrorWarningPattern PMD_PATTERN         = BUILDER.build("${FILE}:${ROW}${TAB}${MESSAGE}", Classifications.WARNING_CLASSIFICATION);
    public static final ErrorWarningPattern PHP_WIN_ERROR       = BUILDER.build("PHP Parse error:${SPACES}syntax error, ${MESSAGE} in ${FILE} on line ${ROW}");
    public static final ErrorWarningPattern LUA_ERROR           = BUILDER.build("${IGNORE_FILE}: ${FILE}:${ROW}: ${MESSAGE}");
    public static final ErrorWarningPattern OPEN_PASCAL_ERROR   = BUILDER.build("${FILE}(${ROW},${COL}) Error: ${MESSAGE}");
    public static final ErrorWarningPattern OPEN_PASCAL_WARNING = BUILDER.build("${FILE}(${ROW},${COL}) Warning: ${MESSAGE}", Classifications.WARNING_CLASSIFICATION);

@epost
Copy link
Copy Markdown
Contributor Author

epost commented Jul 27, 2015

@jdegoes Interesting, what's the source of your list? Here are some of the regexes that emacs supports out of the box; the colon-separated format seems to be a GNU thing.

In the meantime, as I'm not sure how to sensibly move this forward, I'll be using https://gist.github.com/epost/6db5fac97956d0050137#file-purescript-support-el-L45 to parse the existing message format from within emacs compilation-mode instead.

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented Aug 8, 2015

Can we make the format controllable via a command line option then?

@paf31 paf31 removed this from the 0.8.0 milestone Aug 9, 2015
@nwolverson
Copy link
Copy Markdown
Contributor

Parsing the current output in https://github.com/nwolverson/atom-linter-purescript - variable number of lines and lack of a "headline" error makes the experience less than ideal. But definitely want both start/end for row and column.

@jdegoes
Copy link
Copy Markdown

jdegoes commented Aug 22, 2015

@jdegoes Interesting, what's the source of your list? Here are some of the regexes that emacs supports out of the box; the colon-separated format seems to be a GNU thing.

I created it many years ago when writing a text editor that needed to extract out errors from lots of different tools and compilers. The regexes above can parse most errors and warnings, with start and optionally end columns for those format that support them.

@hdgarrood
Copy link
Copy Markdown
Contributor

Am I correct in thinking that the new --json-errors flag obviates this pull request?

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented Dec 22, 2015

Yes, I think so, although there is quite likely work still to do.

@epost
Copy link
Copy Markdown
Contributor Author

epost commented Dec 22, 2015

It could, but I think it'd still be a shame not to settle on a sensible
textual format. Json seems a bit like overkill for the use case I had in
mind.
On 22 Dec 2015 17:41, "Phil Freeman" notifications@github.com wrote:

Yes, I think so, although there is quite likely work still to do.


Reply to this email directly or view it on GitHub
#1285 (comment)
.

@hdgarrood
Copy link
Copy Markdown
Contributor

I realise that this viewpoint goes against the grain somewhat, but I don't think we should be supporting or even encouraging editors to be trying to parse the default textual output. It seems to me that by trying to support both, we'll inevitably up with a format that's not particularly good at either (ie, reading by humans, reading by editors), and it also means we won't be able to make changes to the text format without breaking everyone's editor interop.

@damncabbage
Copy link
Copy Markdown
Contributor

Agreed with @hdgarrood.

If at all possible, I'd love for the default output to be for humans, and --json-errors (or others) being for tools. It gives us the flexibility to change the human-readable error format as we find ways to make it more friendly (see Elm for an extremely friendly take on this), while locking the API for tools with JSON. As it is, editor integration seems to require a substantial amount of effort, I'm a bit leery of making what I'm seeing as a shortcut tweak for an integration bonus.

(@jdegoes' list shows us that formats that people are previously used to are all over the place. As a number of us are starting with experience with tools that aren't gcc, ghc or javac, I'm doubly leery of biasing output in that direction.)

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented Feb 27, 2016

I think this has been superceded by --json-errors now, correct?

@paf31 paf31 closed this Mar 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants