[Diagnostics] Updated diagnostic printing style #30027

owenv · 2020-02-24T01:17:04Z

This PR adds a new diag printing style behind the -enable-experimental-diagnostic-formatting frontend flag. A few notes:

This style supports -no-color-diagnostics, but mostly just for testing purposes. When this is supported in the driver, I think it would be better just to fallback to the LLVM style if color is unsupported or disabled.
This relies on a lot of expensive line-breaking SourceManager operations. Perf seems ok so far, but I intend to investigate alternatives before this becomes an officially supported feature.
Until [DiagnosticVerifier] Make DiagnosticVerifier a DiagnosticConsumer #28313 lands, enabling this feature will break the verifier.
I assume vim/emacs can't parse this format out of the box, but I haven't tested it yet.
I'm not a designer so some of the color/layout decisions here are probably terrible :)

The code/tests aren't the easiest to parse, so here's an example of a program:

func foo(a: Int, b: Int) {
  a + b
}

foo(b: 1, a: 2)


func baz() {
  bar(a: "hello, world!")
}

struct Foo: Codable {
  var x: Int
  var x: Int
}

func bar(a: Int) {}
func bar(a: Float) {}


func bazz() throws {

}
bazz()

struct A {}
extension A {
  let x: Int = { 42 }
}

let abc = "👍

Which produces diags that look like this:

owenv · 2020-02-24T01:18:54Z

I've only tested this on macOS so far, let's get some preliminary results on other platforms.

@swift-ci please test

LucianoPAlmeida · 2020-02-24T01:44:35Z

lib/Frontend/PrintingDiagnosticConsumer.cpp

+  }
+
+  // Describe a fix-it out-of-line.
+  static void describeFixIt(SourceManager &SM, DiagnosticInfo::FixIt fixIt,


This is nice!

owenv · 2020-02-24T03:12:06Z

@swift-ci please test Windows platform

owenv · 2020-02-24T15:40:16Z

@swift-ci please test Windows platform

owenv · 2020-02-24T15:53:02Z

This is a big change, so here's a rough overview of how everything fits together.

The PrintingDiagnosticConsumer entry point remains handleDiagnostic Depending on whether experimental formatting is enabled, it either uses the existing LLVM-style formatting, or creates an AnnotatedSourceSnippet which groups a diagnostic and any notes which follow it.
AnnotatedSourceSnippet manages a group of AnnotatedFileExcerpts. Often this will only be one unless the diagnostic and notes span multiple files/compiled modules. It also tracks any parts of the diagnostic with unknown source locations.
Each AnnotatedFileExcerpt has one or more AnnotatedLines, which collect the messages, highlights, and inline fix-its for a single line in the source file.

Printing each message is a two stage process. First, as the consumer receives diags and associated notes, it builds up an AnnotatedSourceSnippet using the appropriate add* methods. Then, once the snippet is complete, render is called to perform the actual printing to an output stream.

xedin · 2020-02-24T18:27:00Z

Thank you, @owenv! I'll try to take a look asap

xwu · 2020-02-27T01:11:27Z

lib/Frontend/PrintingDiagnosticConsumer.cpp

+      if (shouldDelete != Deleted) {
+        Out.resetColor();
+        if (shouldDelete) {
+          Out.changeColor(ColoredStream::Colors::RED);


For accessibility reasons, proposed insertions and deletions shouldn't be distinguished (at least solely) by the colors red and green. This is because red-green colorblindness is the most common form of the condition.

Thanks for bringing this up. I've started to address this by adding a textual description of the fix-it to the end of all messages with fix-its, instead of just notes (e.g. error: argument 'a' must precede argument 'b' [remove ', a: 2' and insert 'a: 2, ']).

I think there's more that can and should be done here, but I haven't come up with a design I'm happy with yet, so I'll leave this unresolved for now.

I've now modified the formatting to put a + below inserted characters and a - below deleted characters. The message now looks like this:

Combined with the change above, I think this is more accessible than the first attempt and mostly addresses your second comment as well.

xwu · 2020-02-27T01:13:01Z

lib/Frontend/PrintingDiagnosticConsumer.cpp

+                              /*background*/ false);
+              ++i;
+            } else {
+              Out << fixIt.Text[i];


It may be helpful to have some other way of indicating that a certain span of text is a proposed fix-it insertion. At present, my first glance impression was that there was some syntax coloring going on, which doesn't immediately make it clear that the text isn't already in the source code.

This new style directly annotates small snippets of code with error messages, highlights and fix-its. It also uses color more effectively to highlight important segments.

…ind separate frontend flags

…ush method

…ceholders

…cription of fix its

…ebase

…ectly when rendering highlights and messages

…ficiency

…eletions in the highlight line

…n-ASCII anchored messages

owenv · 2020-02-27T23:49:24Z

Kicking off another round of tests to verify the recent changes

@swift-ci please smoke test

…ering

…arators

xedin

Looks great! Thank you, @owenv! You can also split some of the changes, e.g. renaming of the language option for educational notes, into a separate PR so it could be merged sooner.

owenv · 2020-03-02T22:50:08Z

@xedin Thanks for looking over this! In that case, should I wait to merge this PR? Since this is frontend-only right now, I was planning on improving it on master until I'm ready to add driver support or make it public.

xedin · 2020-03-02T22:51:37Z

Since this is under a flag and doesn't affect anything else I think you can go ahead and merge it.

owenv · 2020-03-02T22:58:04Z

@xedin sounds good, I'll just make sure this is still passing tests on all three platforms then.

owenv · 2020-03-02T22:58:12Z

@swift-ci please test

owenv · 2020-03-02T22:58:24Z

@swift-ci please test Windows platform

LucianoPAlmeida reviewed Feb 24, 2020

View reviewed changes

owenv requested review from hborla and xedin February 24, 2020 15:53

xwu reviewed Feb 27, 2020

View reviewed changes

owenv added 10 commits February 27, 2020 08:35

[Diagnostics] Experimental diagnostic printing updates

7b5ecad

This new style directly annotates small snippets of code with error messages, highlights and fix-its. It also uses color more effectively to highlight important segments.

[Diagnostics] Stage educational notes and experimental formatting beh…

4dad89e

…ind separate frontend flags

[Diagnostics] Refactor expensive line lookups in diag formatting

d01dc92

[Diagnostics] minor experimental formatting style refactor

5e6ae98

[Diagnostics] Remove dead formatting code

183a4d2

[Diagnostics] Refactor some PrintingDiagnosticConsumer code into a fl…

ad3fa79

…ush method

[Diag-Experimental-Formatting] Custom formatting for Xcode editor pla…

d87e913

…ceholders

[Diagnostics] Make the experimental formatting tests less fragile

d2db08f

[Diag-Experimental-Formatting] Better and more consistent textual des…

1d9c00a

…cription of fix its

[Diag-Experimental-Formatting] Update some flag usage in tests post r…

3382965

…ebase

owenv force-pushed the pretty_print_errors branch from c52c41b to 3382965 Compare February 27, 2020 17:09

owenv added 6 commits February 27, 2020 09:27

[Diags-Experimental-Formatting] Handle lines with tab characters corr…

29b63c0

…ectly when rendering highlights and messages

[Diag-Experimental-Formatting] Refactor byte-to-column mapping for ef…

bde8f32

…ficiency

[Diag-Experimental-Formatting] Fix line number indent calculation

4fc9af8

[Diag-Experimental-Formatting] Include indicators of insertions and d…

5ee88d0

…eletions in the highlight line

[Diag-Experimental-Formatting] Change color of indicator arrow for no…

c03ffd8

…n-ASCII anchored messages

[Diag-Experimental-Formatting] Document some recent changes

8566c15

owenv added 3 commits February 28, 2020 10:06

[Diag-experimental-formatting] Make tests less sensitive to line numb…

3d38dfd

…ering

[Diag-Experimental-Formatting] Update tests to allow windows path sep…

00d43d3

…arators

[Diag-Experimental-Formatting] Bug fixes for the integrated REPL

45a5883

xedin approved these changes Mar 2, 2020

View reviewed changes

owenv merged commit 69b513a into swiftlang:master Mar 3, 2020

[Diagnostics] Updated diagnostic printing style #30027

[Diagnostics] Updated diagnostic printing style #30027

Uh oh!

Conversation

owenv commented Feb 24, 2020

Uh oh!

owenv commented Feb 24, 2020

Uh oh!

LucianoPAlmeida Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

owenv commented Feb 24, 2020

Uh oh!

owenv commented Feb 24, 2020

Uh oh!

owenv commented Feb 24, 2020

Uh oh!

xedin commented Feb 24, 2020

Uh oh!

xwu Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

owenv Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

owenv Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

xwu Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

owenv commented Feb 27, 2020

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

owenv commented Mar 2, 2020

Uh oh!

xedin commented Mar 2, 2020

Uh oh!

owenv commented Mar 2, 2020

Uh oh!

owenv commented Mar 2, 2020

Uh oh!

owenv commented Mar 2, 2020

Uh oh!

Uh oh!