Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve compiler's diagnostic messages #6931

Merged
merged 21 commits into from
Jun 8, 2023

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Jun 2, 2023

Pull Request Description

Improve compiler's diagnostic messages. Inspired by gcc.

Important Notes

  • Heuristic for disabling the colored output:
    • NO_COLOR env var is defined - https://no-color.org/
    • The output of the Compiler is redirected
    • JVM's output is not interactive (System.console() == null)
    • TERM env var does not contain "color"

Example of messages:
image

Some of our diagnostics are multiline, and gcc does not have any multiline reports. For multiline diagnostics, there is no underline:
image

image

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • Ensure that error reporting to IDE has not changed
  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.

@Akirathan Akirathan self-assigned this Jun 2, 2023
@Akirathan Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 2, 2023
@Akirathan Akirathan marked this pull request as ready for review June 5, 2023 07:47
@Akirathan
Copy link
Member Author

@JaroslavTulach Suggested that the reported file names should be navigable from our IDEs (NetBeans, VSCode, IJ, ...) with some kind of Ctrl + click shortcut.

Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, please check out the library I mentioned to potentially avoid re-inventing the wheel.
Also, we might want to have a non-coloring version for CI

@radeusgd
Copy link
Member

radeusgd commented Jun 5, 2023

@JaroslavTulach Suggested that the reported file names should be navigable from our IDEs (NetBeans, VSCode, IJ, ...) with some kind of Ctrl + click shortcut.

Indeed, I think that maybe we should provide an absolute path? Or at least a bit more than just filename?

I know that technically we have the module path above the set of errors, but I always found it tedious to have to find the top one (if there are many errors it may be quite a few lines above) just to identify if that Table.enso comes from Database or Table. So having a full(er) path could be helpful IMO (although I'm slightly worried if we don't make the output too noisy).

@Akirathan
Copy link
Member Author

After the latest update, the format of diagnostics is 100% aligned with the output from GCC. Some examples:
image

image

Comment on lines 1054 to 1057
for (lineNum <- section.getStartLine to section.getEndLine) {
str ++= oneLineFromSource(lineNum)
str ++= "\n"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I read correctly that this will print all lines from a section?

If I understand correctly, a section may have hundreds of lines (if it is a definition of a type). Printing so much for a single error seems faaar too much.

I recall there were plans to limit the output, no? I think we should print 2-3 lines max and then can have some kind of ellipsis ... and N more lines for example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Implemented in d1ab377

The ellipsis looks like this:
image

Pretty readable, I guess. The maximum number of lines to be printed from the source in case of multiline diagnostics is hard-coded in a variable.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what to think about ANSI escape codes leaking to the CI output.

On one hand, I'd appreciate the output to be colored there (does it work though? did you check how it looks in the CI?)

But it does not seem good when the escape codes are printed raw into CI-related messages like here:
image

Maybe we should detect the ANSI support better and revert back to plain text in environments like the CI?

@radeusgd
Copy link
Member

radeusgd commented Jun 6, 2023

But it does not seem good when the escape codes are printed raw into CI-related messages like here: image

One more thing I noticed on this screenshot - look at the filename - it is null.

I guess it makes sense that in some synthetic tests the source section does not report a filename. But I think we should handle it better - maybe instead of null let's have <Unknown source> displayed there? Seeing null looks just like someone forgot to handle a special case 😅

@Akirathan Akirathan removed the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 8, 2023
@Akirathan Akirathan merged commit 372bc8f into develop Jun 8, 2023
@Akirathan Akirathan deleted the wip/akirathan/pretty-compiler-errors branch June 8, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants