Show code snippets on demand by ilevkivskyi · Pull Request #7440 · python/mypy

ilevkivskyi · 2019-08-31T22:34:41Z

This essentially fixes #7411

Couple ideas I think may make the much larger amount of output more readable are:

Use dim color for the source code snippet
Wrap long error messages so that they are not messed up with file names etc.

An example output (dark terminal on Linux):

ilevkivskyi · 2019-09-01T14:56:56Z

I just discovered that this doesn't work well with blocking errors and/or daemon. I have a fix but it requires mypy.errors depend on mypy.fscache. Which is probably not so bad.

JukkaL · 2019-09-02T13:09:01Z

Some general comments, most of which are copied from #7411:

I think that we should fix at least the most obvious problems with column numbers before documenting this as a feature (it's okay to have it as a hidden option though). I have a half-finished PR that fixes column numbers of arguments and may be able to finish that up soon. Also, assignments should probably point to the rvalue.

Other comments:

I don't like the new location for the error code. I'd keep that in the original location after the message.
It might look better if we'd only underline the target token, i.e. the length of the underlining would vary. Maybe the underlining should cover a simple member expression (say, underline foo.bar in foo.bar.zar).
If the column number is missing, we should perhaps point to the first non-space token on the line. There should be a test for this.
The indentation when splitting error message may be too much, since the path can be very long. Maybe use a fixed level of indent (8 spaces?).

JukkaL · 2019-09-02T13:09:40Z

I have a fix but it requires mypy.errors depend on mypy.fscache.

Interesting, why does is need fscache?

ilevkivskyi · 2019-09-02T13:26:53Z

@JukkaL

I don't like the new location for the error code. I'd keep that in the original location after the message.

I actually like the new position more, since we are already taking much more vertical space, why not save a bit of horizontal space? Otherwise the line with wiggly arrow will have few information on it.

It might look better if we'd only underline the target token, i.e. the length of the underlining would vary.

It is hard to define what is "token". It seems to me you don't realize how hard would be implementing this to a reasonable degree of reliability and how misleading the positions can be if we will do this in ad-hoc manner. Like we may end up underlining the first item of a tuple while the problem is actually in the type of the second one. I would rather focus on making the start column better first.

If the column number is missing, we should perhaps point to the first non-space token on the line. There should be a test for this.

OK.

The indentation when splitting error message may be too much, since the path can be very long. Maybe use a fixed level of indent (8 spaces?).

OK.

Interesting, why does is need fscache?

I want to pass the cache as an argument to the Errors constructor (literally once), rather that "threading" the error line content to the every place it is needed. Therefore I need to annotate the constructor argument. I however just realized I can use a tiny protocol.

JukkaL

Here is a more detailed review of the code (my previous comments were about the formatting of the output). I'm excited that this will be improve usability by making it easier to figure out why mypy is complaining about something.

JukkaL · 2019-09-02T13:19:46Z

+    decode_python_encoding, DecodeError, trim_source_line, DEFAULT_SOURCE_OFFSET,
+    WIGGLY_LINE, DEFAULT_COLUMNS, MINIMUM_WIDTH
+)
+from mypy.fscache import FileSystemCache


I think it would be nice if there was no import from mypy.fscache in this file, and instead we'd accept a callback that reads file contents, for example (dependency injection). We could probably also get rid of the decode_python_encoding and DecodeError imports.

OK, I was thinking about a protocol, but callback is even better.

JukkaL · 2019-09-02T13:21:57Z

-        self.show_column_numbers = show_column_numbers
-        self.show_error_codes = show_error_codes
+    def __init__(self, fscache: Optional[FileSystemCache] = None,
+                 options: Optional[Options] = None) -> None:


This adds a big dependency, which we previously avoided by providing the required information explicitly. It would arguably be cleaner if you'd just add the required options as arguments here instead of passing the whole Options object.

OK, I will split this back.

JukkaL · 2019-09-02T13:23:33Z

+        source_lines = None
+        if self.options and self.options.show_source_code:
+            if self.fscache:
+                try:


This try statement could be provided as a callback to avoid a depending on fscache.

JukkaL · 2019-09-02T13:24:03Z

 ENCODING_RE = \
    re.compile(br'([ \t\v]*#.*(\r\n?|\n))??[ \t\v]*#.*coding[:=][ \t]*([-\w.]+)')  # type: Final

+PLAIN_ANSI_DIM = '\x1b[2m'  # type: Final


Is this portable? Could we get this from curses or something?

I tried it in couple terminals and it works (because it is ANSI standard). The problem is that although it is a basic ANSI "feature", terminfo files for most default terminals don't have dim termcap entry, so curses doesn't report it. I was thinking about choosing a grey color that would look good on both white and black background, but it is actually not easy, and again most default terminals are 8-color, not 256-color, so we can't get the color code from curses.

Ok, sounds reasonable. Maybe add a comment about this?

Since this is not on by default, we can iterate on this afterwards even if some things don't work in every possible environment.

JukkaL · 2019-09-02T13:25:30Z

    return source_text


+def trim_source_line(line: str, max_len: int, col: int, min_width: int) -> Tuple[str, int]:


This function is a great candidate for regular unit tests.

JukkaL · 2019-09-02T13:30:43Z

            loc, msg = error.split('error:', maxsplit=1)
-            if not self.show_error_codes:
+            if self.show_source_code:
+                # Improve readability by wrapping lines when showing source code.


It's can be surprising that showing source code also affects how long messages are wrapped. Maybe the option should be named --pretty or something, as it controls a few things.

JukkaL · 2019-09-02T13:32:17Z

        elif ': note:' in error:
            loc, msg = error.split('note:', maxsplit=1)
            return loc + self.style('note:', 'blue') + self.underline_link(msg)
+        elif self.show_source_code and error.startswith(' ' * DEFAULT_SOURCE_OFFSET):


Detecting source code highlights through an indent prefix can be surprising. I wonder if there's a more general way to do this. If there's no alternative simple approach, this should be documented carefully somewhere.

JukkaL · 2019-09-02T13:36:00Z

+    return res
+
+
+def get_term_columns() -> int:


What about renaming this to get_terminal_width?

JukkaL · 2019-09-02T13:37:21Z

+            if add_snippets:
+                if severity == 'error' and source_lines and line > 0:
+                    source_line, offset = trim_source_line(source_lines[line - 1],
+                                                           DEFAULT_COLUMNS, column, MINIMUM_WIDTH)


Should this use the terminal width?

JukkaL · 2019-09-02T13:38:45Z

+            if self.show_source_code:
+                # Improve readability by wrapping lines when showing source code.
+                pad = len(loc) + len('error: ')
+                max_len = get_term_columns() - pad - 1  # compensate for space after 'error:'


This is kind of suspicious -- how will this work in the daemon? Should the client pass the terminal width to the server as part of each request, since the terminal may change between invocations?

Anyway, it might be better if this was provided by the caller.

This is kind of suspicious -- how will this work in the daemon?

I was going to fix this after the other PR lands. Essentially yes, this should be passed together with is_tty from the other PR.

ilevkivskyi · 2019-09-02T14:09:33Z

@JukkaL thanks for review, I agree with all comments I didn't reply above.

JukkaL · 2019-09-02T14:17:27Z

I actually like the new position more, since we are already taking much more vertical space, why not save a bit of horizontal space? Otherwise the line with wiggly arrow will have few information on it.

I think it's okay that the wiggly arrow is on its own line without anything else. The savings in horizontal space will be pretty minor, since error codes tend to be short.

Here's some reasoning why I'd rather not add the error code after the wiggly arrow (besides that it subjectively looks odd to me):

It makes the error code more prominent, even though most of the time it's the least interesting bit of information in the output. Now it kind of jumps at you, since it's the only alpha-numeric information on the line with the wiggly arrow.
It arguably changed the error formatting more than is necessary from the default mode. If we keep the error code in the original location, the new mode primarily adds some extra lines to the output but otherwise doesn't change things, which I like since it's simple and consistent. (Okay, it also does line wrapping, but that can be argued for on the basis of making it easier to see where the source line starts.)
Other tools tend to have the underlining/caret alone on a separate line, at least most of the time, so this seems to be a popular way of doing things. We don't need to follow what others do, but if we are unsure about something, falling back to common conventions may be a reasonable default.
We might want to put some other information in the location after/below the underlining in the future. See https://blog.rust-lang.org/2016/08/10/Shape-of-errors-to-come.html for some relevant Rust examples. Clang can also add some notes near the underlining (https://clang.llvm.org/diagnostics.html). I'd prefer to leave the space empty for future experiments. We could always move the error code back in the future, but it's a bit awkward if we have to move back and forth on an issue.

It is hard to define what is "token". [...]

Yes, there is a risk of doing something confusing. However, I think that the current approach is also confusing sometimes, especially if the error is at the end of the line, or if the target is a name that is much longer/shorter than the squiggle. I initially thought that it underlines a span of code, and was confused when this wasn't the case. A conservative option would be to only show a caret (^) and no squiggles, similar to what gcc does (or did), in the first implementation. We can always iterate on this later.

Here are other heuristics that could be built on top of the "caret only" approach that seem reasonably safe bets to me (but I haven't thought about this very carefully):

If the target is an identifier or a keyword, underline the identifier/keyword.
If the target starts an int/float/string/bytes literal, underline the literal.
Otherwise just show a caret.

As long as we can't always find the end span of an AST node, we may want to allow some extra options to be passed along with error messages that may affect how the length of the underlining is determined. For example, if the target expression is an attribute expression foo.bar, we can pass this as a flag, and the highlighter can try to find such an expression at the location using some heuristics. Yes, this will get increasingly hard to do if we want this to work always.

ilevkivskyi · 2019-09-02T21:09:28Z

After addressing some comments it looks like this. I will continue tomorrow.

JukkaL · 2019-09-03T11:06:05Z

Thanks for the updates! I like how it looks now.

emmatyping · 2019-09-03T20:23:12Z

Wow! This looks really good. Thank you for working on it Ivan!

ilevkivskyi · 2019-09-03T20:30:16Z

@ethanhs I am glad you like it :-)

@JukkaL This is ready for another review.

ilevkivskyi · 2019-09-03T21:15:59Z

Opened follow-up issues #7453 and #7454

JukkaL

Looks great! There doesn't seem to be any tests for the dmypy client change -- maybe add at least one? Even if the change is trivial, it might get more complex in the future.

JukkaL · 2019-09-04T11:22:50Z

+            "Long[Type, Names]" are never split.
+        ^^^^--------------------------------------------------
+        num_indent           max_len
+    """


I like the detailed comment!

ilevkivskyi added 9 commits August 31, 2019 19:00

Start working on source code snippets

e437357

Make some cleanup

22d9abd

Add couple tests

1f3fb3a

Undo the dedicated error; update self-check to see these columns

4d5353f

Only do wrapping when showing source

316af1f

Minor fixes

29585dc

Fix tests on Python 3.5

52fd9a8

Support also blocking errors and daemon

281c655

Add couple tests

bd4987b

Fix bug

b257dde

ilevkivskyi requested a review from JukkaL September 2, 2019 11:20

Merge branch 'master' into show-code-snippet

5337ab0

JukkaL reviewed Sep 2, 2019

View reviewed changes

ilevkivskyi mentioned this pull request Sep 2, 2019

Colorize daemon output and add summary line #7441

Merged

Ivan Levkivskyi added 2 commits September 2, 2019 21:12

Merge remote-tracking branch 'upstream/master' into show-code-snippet

8ef3ff9

Address some CR

f477808

Ivan Levkivskyi added 5 commits September 3, 2019 13:53

More CR

69a859b

Separate fit in terminal from colorizing

a44c239

Don't mutate message lists in-place

4334320

Update tests

0c5b024

Fix couple of-by-one errors; add tests and comments

d898e8b

Small tweaks

ade9a38

ilevkivskyi requested a review from JukkaL September 3, 2019 20:30

This was referenced Sep 3, 2019

Apply soft word wrap also to notes when using --pretty #7453

Open

Underline error span instead of just caret (if possible) when using --pretty #7454

Closed

JukkaL approved these changes Sep 4, 2019

View reviewed changes

Add an end-to-end daemon test for --pretty

89e24dc

ilevkivskyi merged commit 91adf61 into python:master Sep 4, 2019

ilevkivskyi deleted the show-code-snippet branch September 4, 2019 16:56

		return source_text


		def trim_source_line(line: str, max_len: int, col: int, min_width: int) -> Tuple[str, int]:

Conversation

ilevkivskyi commented Aug 31, 2019

Uh oh!

ilevkivskyi commented Sep 1, 2019

Uh oh!

JukkaL commented Sep 2, 2019

Uh oh!

JukkaL commented Sep 2, 2019

Uh oh!

ilevkivskyi commented Sep 2, 2019

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Sep 2, 2019

Uh oh!

JukkaL commented Sep 2, 2019

Uh oh!

ilevkivskyi commented Sep 2, 2019

Uh oh!

JukkaL commented Sep 3, 2019

Uh oh!

emmatyping commented Sep 3, 2019

Uh oh!

ilevkivskyi commented Sep 3, 2019

Uh oh!

ilevkivskyi commented Sep 3, 2019

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants