Language-server-protocol: Proposal: related locations for Diagnostics

[ljw1004]

Lots of languages want to show "related locations" for diagnostics. I propose

interface Diagnostic {
    ...
    relatedLocations?: RelatedLocation[];
}

interface RelatedLocation {
    location: Location;
    message: string;
}

I think that we don't need capability flags for this. The language server should always blindly send related locations if it has any. If the client doesn't support the display of related locations, or if it does but the user has turned that display off, then only the primary Diagnostic will be displayed, and the language server should make sure that the primary Diagnostic makes sense on its own.

Here are some example languages which report related locations. I've added squiggles to show conveniently which are the related locations.

C

1: class Program {
2:   static void Main() { }
3:   static void f() { } // first
4:   static void f() { } // second
5: }

$ mcs Program.cs

Program.cs(4,15): error CS0111: A member `Program.f()' is already defined. Rename this member or use different parameter types
   4: static void f() {} // second
                  ^
Program.cs(3,15): (Location of the symbol related to previous error)
   3: static void f() {} // first
                  ^

Note that the C# plugin for VSCode doesn't currently display related locations. Actually, I know that the previous-generative native C# compiler used to report related locations, but I don't know if the current Roslyn-based C# compiler still does. I do know that AdditionalLocations are central to Roslyn analyzers: link.

Hack

1: <?hh
2: class Clio {
3:   public function fred(): void {
4:     $this->jones();
5:   }
6: }

$ hh

Clio.php:4:12,16: Could not find method jones in an object of type Clio (Typing[4053])
    4:     $this->jones();
                  ^^^^^

Clio.php:4:5,9: This is why I think it is an object of type Clio
    4:     $this->jones();
           ^^^^^

Clio.php:2:7,10: Declaration of Clio is here
    2: class Clio {
             ^^^^

Flow

1: function g(x) {
2:   const y: string = x;
3: }
4: 
5: function f() {
6:   g(1);
7: }

$ flow

a.js:2:21,22: number. This type is incompatible with
    2:   const y: string = x;
                           ^

a.js:2:12,18: string
    2:   const y: string = x;
                  ^^^^^^

a.js:6:3,7: See also: function call
    6:   g(1);
         ^^^^

SARIF

Microsoft's SARIF, the Static Analysis Results Interchange Format, also uses relatedLocations similarly:

results: [{
  "ruleId": "JS3056",
  "level": "error",
  "message": "Name 'index' cannot be used in this scope",
  "locations": [{"analysisTarget": [{"uri": "file:///C:/Code/a.js", "region": {"startLine": "6", "startColumn": "10"}}]}],

  "relatedLocations": [{
    "message": "The previous declaration of 'index' was here.",
    "physicalLocation": {"uri": "file:///C:/Code/a.js", "region": {"startLine": "2", "startColumn": "6"}}
  }]
}]

Discussion

I don't have good thoughts on how VSCode should best display related locations. Here are illustrations of how Atom/Nuclide displays related locations. Observe in the error list there's a checkbox to turn on or off the display of those additional locations. (Typically, users prefer to see just the single line report when they're working through lots of diagnostics, but they prefer to either hover or see multiple related locations when they're drilling down into a complicated diagnostic).

There is a related issue #250 which suggests that diagnostics should change to being markdown rather than plain text. That would theoretically allow related locations to be embedded into the markdown string, e.g.

Could not find method [jones](file:///project/Clio.php#4) in an object
of type Clio. [Here](file:///project/Clio.php#4) is why I think it is an object
of type Clio. The declaration of Clio is [here](file:///project/Clio.php#2)

Markdown may or may not be a good idea for formatting the diagnostics. But I think it would be bad to use this markdown approach as the vehicle for related locations because (1) it's information-overload, (2) it doesn't let users turn off the related locations in their error list, (3) there's nothing in the file URL that lets it specify a range, (4) the "hit box" of each location is much smaller and it no longer indicates easily which file the target is in.

C# doesn't have a unique message for each related location. That's why it merely says "(Location of the symbol related to previous error)". I understand the C# team don't really like this message. That's why I believe it's important that LSP should allow the language server to specify its own message, on a per-related-location basis.

I proposed that the message property in RelatedLocation should be mandatory. That's because I don't think the client should be in the business of synthesizing a default message along the lines "(Location of the symbol related to the previous error)". That should be the domain of the LSP server, and indeed the LSP server is the one that knows how to localize the message properly.

[hansonw]

❤️ cc @damieng

[ljw1004]

There's also a feature-request from Rust folks for VSCode to display related locations:
https://github.com/Microsoft/vscode/issues/10271

It's related to the VSCode feature request for "multiline errors". (although multiline errors are a different concept from related locations)
https://github.com/Microsoft/vscode/issues/1927

cc @sandy081

[dbaeumer]

I like the proposal, however I would change one thing. Since related locations are to 99% in the same file I would not reuse the location definition since it has a mandatory uri property. I would instead do:

interface RelatedDiagnosticInfo {
     uri?: string;
     range: Range;
     message: string;
}

For consistency reason I would add a client capability for it. It is easy to set by the clients.

@jrieken do you have any comments on this?

For completeness: SymbolInformation uses a location but the location can omitted the URI which is very unclearly speced. I opened https://github.com/Microsoft/language-server-protocol/issues/338. We should do better with related diagnostic information.

[jrieken]

Since related locations are to 99% in the same file I would not reuse the location definition since it has a mandatory uri property.

👎 it was a mistake to do that in the SymbolInformation which is used for document and workspace symbols. it did cause confusion and I'd go for clarity by enforcing the URI

[dbaeumer]

I see the problem when the structure is reused in two requests. But I also see the problem of sending lots of identical strings (which in this case might not be huge, I agree). May be such an optimization must be more explicit so that users need to provide a value.

[rcjsuen]

This has been added to the protocol. See 69028bf06f0ce5f1549e189e9810daece89d622b and e361dca587d6c852b401748c79a50ea86e86868b.

Microsoft/vscode-languageserver-node#324 was the original PR that introduced the change to the VS Code Language Server packages.

[jrieken]

Yep, this should now work end to end, LSP, VS Code Extension API, and VS Code UX

[dbaeumer]

Closing the issue.

[dbaeumer]

Publish new version of the LSP node libraries today which already implement the LSP protocol on top of the VS Code protocol.