Context messages

[johnpryan]

Adds "Open Documentation" links, diagnostic messages, and corrections in the analysis panel

Diagnostic messages often show line numbers, so #317 would be nice to have soon.

depends on dart-archive/dart-services#663
fixes #1676

[johnpryan]

cc: @bwilkerson @devoncarew

[bwilkerson]

This is super exciting to see. Thank you!

[RedBrogdon]

LGTM!

Will this need a simultaneous deploy of both ends in order to make the proto changes work, or do things degrade gracefully?

EDIT: Just saw @domesticmouse's comment on dart-archive/dart-services#663 (comment). Is it possible/necessary to use optional fields or default values to make the deploy easier?

[RedBrogdon]

I'm kind of surprised there isn't already a class with these properties in the codebase, but I can't find one. There's Position in /lib/editing/editor.dart, but it doesn't include length.

[bwilkerson]

Or, given that Position is identical in structure to the interface Position from LSP, consider using the LSP interface Range instead. Although it might not be as convenient for your purposes.

[johnpryan]

The Location type is used in AnalysisError and DiagnosticMessage classes, so dart-services adds the line, charStart, and charLength to an AnalysisIssue protocol buffer message that the client gets. The client doesn't actually use the analysis_server_lib package at the moment.

We could add two new Postition and Range messages to our protocol buffer, but that might be out of scope here.

[RedBrogdon]

You may want to consider a slightly more specific name, like TextRange or SelectionRange or something. Just a thought.

[devoncarew]

This looks great! Some thoughts on the presentation:

• I'd try having the message, correction, and context all be full sentences. This might be as simple as removing the _stripPeriod() and endsWith('.') logic.
• You might try having the message and any correction be part of the same span (wrap as one bit of text) rather than be separate divs
• perhaps swap the documentation url and the context message positions? So the doc url would be in the part of the error message w/ the slightly different backgroun

[bwilkerson]

I'd try having the message, correction, and context all be full sentences.

I agree. We actually don't guarantee that we won't write any messages in the future that consist of multiple sentences, in which case removing the period from the last sentence would look odd.

perhaps wrap the documentation url and the context message positions? So the doc url would be in the part of the error message w/ the slightly different background

Wow. I couldn't even tell that the background was different. (Dark mode doesn't work well for my eyes. :-/)

It seems kind of odd to me that the background is different, as if the context messages weren't part of the description of the diagnostic. I think of them as being details that further explain the issue. I'd strongly prefer that we didn't imply otherwise to users.

Also, I noticed something earlier and decided not to mention it because I couldn't think of a concrete example where the situation would arise, but I've just thought of one. What I noticed is that you're displaying the line number for the context message, but the context message is allowed to point to a location in a different file than the one with which the diagnostic is associated, so displaying only the line number in those cases will be confusing.

That situation can't happen using the example you're showing above, but we also create context messages when the issue involves types that have the same name but are actually different. Situations like those cause diagnostics that read something like "The argument type List<int> can't be assigned to a parameter of type List<int>.", which is confusing unless you realize that the two types are from different libraries. So we added text in the message (for clients that can't handle context messages) and added context messages that link to the declarations of the types. To see this in action, try

main() {
  f([0, 1, 2]);
}

void f(List<int> counts) {}

class List<E> {}

Note that this causes the full file paths to be displayed, which is kind of ugly in this context because the user doesn't control the file paths, and will likely lead to confusing context message line numbers.

I suspect that the probability of cases like this will increase when we start supporting package imports.

[devoncarew]

What I noticed is that you're displaying the line number for the context message, but the context message is allowed to point to a location in a different file than the one with which the diagnostic is associated ...

This may be less of an issue for DartPad - there's only ever one dart file (and, the user doesn't ever see or control that file name).

[bwilkerson]

This may be less of an issue for DartPad - there's only ever one dart file (and, the user doesn't ever see or control that file name).

That's what I thought too and why I didn't mention it before. But paste my example into dartpad and you'll see the file paths displayed today, and in the future one of the context messages will have a line number in core.dart.

[johnpryan]

I'd try having the message, correction, and context all be full sentences. This might be as simple as removing the _stripPeriod() and endsWith('.') logic.

Done 👍

You might try having the message and any correction be part of the same span (wrap as one bit of text) rather than be separate divs

I tested that out, but felt that it was easier to scan the error message when it was on it's own line. Happy to change it back if we want to try it that way.

perhaps swap the documentation url and the context message positions? So the doc url would be in the part of the error message w/ the slightly different background

The screenshot is misleading, the darker background only shows when you hover over it. Clicking on the error element changes your selection to the error, and clicking on the context message changes your selection to the location associated with that DiagnosticMessage object.

The documentation url is already part of the error message, but before the context messages, so I could move it below the context messages if that's what you mean?

Dark mode doesn't work well for my eyes

We've also gotten some feedback on this (we recently changed the editor theme to be more accessible #1767) but this hasn't been deployed yet. We could also look into adding a light theme (used by the embeds) to the main page too.

What I noticed is that you're displaying the line number for the context message, but the context message is allowed to point to a location in a different file than the one with which the diagnostic is associated ...

Oof, it looks we're showing the absolute paths (from dart-services' point of view.) Right now we just grab these from analysis_server_lib and add them to the protobuf message in dart-services:

The argument type 'List<int> (where List is defined in /Users/ryjohn/.dvm/darts/2.10.5/lib/core/list.dart)' can't be assigned to the parameter type 'List<int> (where List is defined in /var/folders/4p/y54w9nqj0_n6ryqwn7lxqz6800m6cw/T/DartAnalysisWrapperlm562s/main.dart)'. - line 2
Open Documentation
List is defined in /Users/ryjohn/.dvm/darts/2.10.5/lib/core/list.dart
int is defined in /Users/ryjohn/.dvm/darts/2.10.5/lib/core/int.dart
List is defined in /var/folders/4p/y54w9nqj0_n6ryqwn7lxqz6800m6cw/T/DartAnalysisWrapperlm562s/main.dart
int is defined in /Users/ryjohn/.dvm/darts/2.10.5/lib/core/int.dart

Is there a way to tell analysis_server_lib how to display these paths?

[bwilkerson]

Dark mode doesn't work well for my eyes

We've also gotten some feedback on this ...

To be clear, I wasn't actually requesting a change, just explaining why I hadn't noticed the difference in background colors. Based on other comments I'm guessing that that's kind of an alternate way of turning a region into a link, not an attempt to visually separate the two sets of messages, so I'm fine with it the way it is.

Is there a way to tell analysis_server_lib how to display these paths?

What is analysis_server_lib?

[devoncarew]

List is defined in /Users/ryjohn/.dvm/darts/2.10.5/lib/core/list.dart
int is defined in /Users/ryjohn/.dvm/darts/2.10.5/lib/core/int.dart
List is defined in /var/folders/4p/y54w9nqj0_n6ryqwn7lxqz6800m6cw/T/DartAnalysisWrapperlm562s/main.dart
int is defined in /Users/ryjohn/.dvm/darts/2.10.5/lib/core/int.dart

Is there a way to tell analysis_server_lib how to display these paths?

At a guess, the paths are actually part of the string messages in the contexts. We definitely don't want to display those absolute paths - they'll leak info about the filesystem the backend is running on.

We might have to catch a convert likely file paths (lib/core/int.dart ==> dart:core/int.dart, T/DartAnalysisWrapperlm562s/main.dart ==> main.dart).

[johnpryan]

analysis_server_lib is a package that dart-services currently uses to talk to the analysis server.

Will take a look at changing these paths before they're sent to the client.

[bwilkerson]

Oh, so probably similar to analysis_server_client.

Then, no, there isn't currently any support for changing the file paths. We could consider adding such support if it's easier than converting the paths on the front end. My guess is that converting them on the front end would be easier, though.

As a side not, I've been proposing for a while that we should remove the file paths from the primary message when clients start showing the context messages in a more first-class way. So thanks again for this work. It's getting us one step closer to being able to clean up the messages!

[johnpryan]

The paths are getting cleaned up in dart-archive/dart-services#664 now. This is ready for another look.