feat: Render diagnostics #552

mark-koch · 2024-10-09T13:41:48Z

Closes #537

codecov-commenter · 2024-10-09T13:44:30Z

Codecov Report

Attention: Patch coverage is 98.37398% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.09%. Comparing base (944bef0) to head (6dfc336).
Report is 1 commits behind head on feat/diagnostics.

Files with missing lines	Patch %	Lines
guppylang/span.py	88.23%	2 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                  @@
##           feat/diagnostics     #552      +/-   ##
====================================================
+ Coverage             90.22%   91.09%   +0.86%     
====================================================
  Files                    61       61              
  Lines                  6376     6482     +106     
====================================================
+ Hits                   5753     5905     +152     
+ Misses                  623      577      -46

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ering

acl-cqc

This is an absolutely beautiful PR, both the error messages and the care that's gone into it. A few thoughts, but I'm happy to leave you to decide what to do about them. (Do think about the long-message after a highlight reaching to nearly the end of the line, though.)

acl-cqc · 2024-10-10T11:31:21Z

guppylang/diagnostic.py

            raise InternalGuppyError("Diagnostic: Span label provided without span")

+    @property
+    def rendered_title(self) -> str:


I note that Diagnostic is basically SubDiagnostic plus title/rendered_title - is it plausible to combine them (make Diagnostic inherit subdiagnostic)?

acl-cqc · 2024-10-10T11:35:16Z

guppylang/diagnostic.py

+        diagnostic.
+        """
+        values = {f.name: getattr(self, f.name) for f in fields(self)}
+        return s.format(**values) if s is not None else None


nit: this builds values (with lots of getattr) even when it's not needed...(not a big deal for diagnostic code though)

I'm wondering about this Optional[str] -> Optional[str] though. As a small/first step, you could add a couple of @overloads (str -> str and None -> None) - then I think the assert in rendered_title would disappear. (I'm not certain how thoroughly overloads are actually type-checked though...)

But, I wonder about making render just str -> str. Then at the callsites you do return self.long_message and self._render(self.long_message). There are not that many callsites, and it gets the right type signatures everywhere without needing overload or assert.

I like the overloading idea. Mypy also seems to be happy with it 👍

acl-cqc · 2024-10-10T11:36:50Z

guppylang/diagnostic.py

+    #: Maximum length of span labels after which we insert a newline
+    MAX_LABEL_LINE_LEN: Final[int] = 60
+
+    #: Maximum length of span labels after which we insert a newline


Doc/comment here is identical to previous, should it be?

acl-cqc · 2024-10-10T11:48:08Z

guppylang/diagnostic.py

+            self.buffer += textwrap.wrap(
+                f"{self.level_str(diag.level)}: {msg}",
+                self.MAX_MESSAGE_LINE_LEN,
+            )


Also we omit sub-diagnostics with spans. Is the assumption that there won't be any if the parent doesn't have a span? Ok if you are confident (like, the parent span gets filled in from the child spans), but otherwise you could check / print out all subdiagnostics after the if/else / etc.

Is the assumption that there won't be any if the parent doesn't have a span?

I think that's an ok assumption to make for now since basically 99% of diagnostics are going to come with a span. We can come back to that if it turns into an issue later.

I'll add a check though 👍

acl-cqc · 2024-10-10T11:49:04Z

guppylang/diagnostic.py

+                self.buffer += textwrap.wrap(
+                    f"{diag.rendered_long_message}", self.MAX_MESSAGE_LINE_LEN
+                )
+        # Finally, render all sub-diagnostics that have a non-span message


Is there something that says sub-diagnostics have either a message OR a span but not both??

The doc comment of SubDiagnostic.message says "Message that is printed if no span is provided."

acl-cqc · 2024-10-10T11:58:02Z

guppylang/diagnostic.py

+        if span.is_multiline:
+            [first, *middle, last] = span_lines
+            render_line(first, span.start.line)
+            # Compute the subspan that only covers the first line and render it's


super-nit: it's -> its

acl-cqc · 2024-10-10T11:59:40Z

guppylang/diagnostic.py

+            render_line(line, span.start.line - prefix_lines + i)
+        span_lines = all_lines[prefix_lines:]
+
+        if span.is_multiline:


You might want to only include a prefix for multiline spans, or something like that, I dunno, your call, I'm happy either way

Yeah, not sure either. This replicates the current behaviour in Guppy, but easy enough to change in the future by updating DiagnosticsRenderer.PREFIX_CONTEXT_LINES

acl-cqc · 2024-10-10T12:00:20Z

guppylang/diagnostic.py

+            )
+            render_line(first_highlight)
+            # Omit everything in the middle
+            if middle:


if len(middle)>1, maybe. It'd be silly to omit a single line to display a line ...

Oh, but I guess then you'd need to highlight that line as well, hmmm. Ok. Not sure about this, maybe I should just shut up and delete the parent comment ;)

acl-cqc · 2024-10-10T12:07:26Z

guppylang/diagnostic.py

+        if label:
+            [label_first, *label_rest] = textwrap.wrap(
+                label,
+                self.MAX_LABEL_LINE_LEN,


So this is the length of the label AFTER the highlight bar. Irrespective of how much space there is left on the line after said highlight bar. How about instead you ask textwap to layout highlight_char * len(last_span) + " " + label with initial indent last_span.start.column and subsequent indents, hmmm, you might want to go backwards ?

^^^^ there is | some problem here | that won't fit on the| rest of the line

(| to indicate line end boundary, not printed)

Think if your test_long_label has very little space after the ==...

I think the current alignment looks nicer, but I agree that the spacing might become problematic.

In a previous version I implemented a layout algorithm that abbreviates the start/end of the line and/or the middle of the span with ... ro ensure that the ^^^ doesn't go too far to the right and everything fits into a specified width

| 42 | ...else foo(looo...ng_id).more_stuff(... | ^^^^^^^^^^^^ Span label with | line break

I think that is the best solution but would have made this PR 150-200 lines longer 😅

I created #556 as a followup

acl-cqc · 2024-10-10T12:08:18Z

guppylang/diagnostic.py

+            render_line(last_highlight)
+
+    @staticmethod
+    def level_str(level: DiagnosticLevel) -> str:


This isn't DiagnosticLevel.to_string / format / something ?

This is the development branch for our new diagnostics infrastructure. Tracked by #535. * #548 * #551 * #552 * #553 * #586 * #588 * #587 * #589 * #590 * #600 * #601 * #604 * #605 * #606

mark-koch added 2 commits October 9, 2024 14:25

feat: Cache accessed source files

40e8cf2

feat: Render diagnostics

ff201cf

mark-koch requested a review from a team as a code owner October 9, 2024 13:41

mark-koch requested review from acl-cqc and removed request for a team October 9, 2024 13:41

mark-koch mentioned this pull request Oct 9, 2024

feat: Improve compiler diagnostics #547

Merged

Base automatically changed from diag/sources to feat/diagnostics October 10, 2024 09:07

Merge remote-tracking branch 'origin/feat/diagnostics' into diag/rend…

85bbf62

…ering

acl-cqc approved these changes Oct 10, 2024

View reviewed changes

mark-koch added 6 commits October 10, 2024 13:33

Overload _render

a2ccdfb

Fix doc comment

df1f836

Refactor render_line

6d1d08c

Right justify line numbers

013cc2f

Add test for line number justification

3fa9572

Fix comment typo

083884a

mark-koch mentioned this pull request Oct 10, 2024

Abbreviate diagnostics spans if they get too long #556

Open

mark-koch added 3 commits October 10, 2024 13:54

Get rid of match in level_str

a4262f5

Turn Diagnostic into subclass of SubDiagnostic

2cae47e

Ensure that span-less diagnostics don't have children

6dfc336

mark-koch merged commit f9709ae into feat/diagnostics Oct 10, 2024
2 checks passed

mark-koch deleted the diag/rendering branch October 10, 2024 13:13

mark-koch mentioned this pull request Oct 10, 2024

Implement diagnostics rendering #537

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Render diagnostics #552

feat: Render diagnostics #552

mark-koch commented Oct 9, 2024

codecov-commenter commented Oct 9, 2024 •

edited

Loading

acl-cqc left a comment

acl-cqc Oct 10, 2024

acl-cqc Oct 10, 2024

mark-koch Oct 10, 2024

acl-cqc Oct 10, 2024

acl-cqc Oct 10, 2024

mark-koch Oct 10, 2024

acl-cqc Oct 10, 2024

mark-koch Oct 10, 2024

acl-cqc Oct 10, 2024

acl-cqc Oct 10, 2024

mark-koch Oct 10, 2024

acl-cqc Oct 10, 2024

acl-cqc Oct 10, 2024

acl-cqc Oct 10, 2024

mark-koch Oct 10, 2024

acl-cqc Oct 10, 2024

feat: Render diagnostics #552

feat: Render diagnostics #552

Conversation

mark-koch commented Oct 9, 2024

codecov-commenter commented Oct 9, 2024 • edited Loading

Codecov Report

acl-cqc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 9, 2024 •

edited

Loading