Introduce related spans into tsserver protocol #24548
Conversation
src/compiler/types.ts
Outdated
| file: SourceFile | undefined; | ||
| start: number | undefined; | ||
| length: number | undefined; | ||
| messageText: string | DiagnosticMessageChain; |
There was a problem hiding this comment.
messageText: string | DiagnosticMessageChain; [](start = 8, length = 45)
I think I need more information about how these are intended to be used. Roslyn diagnostics support additional locations, but those locations don't have messages. We could create separate diagnostics but we would need a new code and category and I don't believe we'd be able to link them together. I think Roslyn more or less dropped this functionality (vs the old native compiler).
There was a problem hiding this comment.
Secondary code locations and more information pertaining to specifically that location in the context of the diagnostic. (eg, "Defined here", "Declared here", "Replicated here", and so on. The additional spans also allow us to use those spans when calculating quick fixes - so we can, for example, know what we can suggest a cast at the site of an assignability error, but also suggest modifying the type the assignment is failing against (because it is mentioned as a related span). The only diagnostic set we currently do a similar thing with is the esModuleInterop import error, which we currently issue a secondary diagnostic for (so there's no obvious link between the two).
There was a problem hiding this comment.
For reference, this isn't a new concept and there is prior art, which is why vscode has basic support for them (and is looking to iterate on their UI) - rust makes good use of related spans in it's errors.
There was a problem hiding this comment.
I'm not saying that it's either novel or bad, just that it goes against the grain of the API we're using.
There was a problem hiding this comment.
We need to sit down and design this feature.. there seems to be two uses cases,
- additional locations, e.g. duplicate identifier errors, you would list them all in the same error message
- related diagnostics, e.g. type of var
xis not assignable to string, and here is wherexwas declated
| function createDiagnostic(start: protocol.Location, end: protocol.Location, message: DiagnosticMessage, args: ReadonlyArray<string> = [], category = diagnosticCategoryName(message), reportsUnnecessary?: {}): protocol.Diagnostic { | ||
| return { start, end, text: formatStringFromArgs(message.message, args), code: message.code, category, reportsUnnecessary, source: undefined }; | ||
| function createDiagnostic(start: protocol.Location, end: protocol.Location, message: DiagnosticMessage, args: ReadonlyArray<string> = [], category = diagnosticCategoryName(message), reportsUnnecessary?: {}, relatedInformation?: protocol.DiagnosticRelatedInformation[]): protocol.Diagnostic { | ||
| return { start, end, text: formatStringFromArgs(message.message, args), code: message.code, category, reportsUnnecessary, relatedInformation, source: undefined }; |
There was a problem hiding this comment.
relatedInformation [](start = 130, length = 18)
Should this new property be validated in some way?
There was a problem hiding this comment.
Nothing has been modified to produce one yet, though we have a massive list courtesy of @DanielRosenwasser - this just makes it so we can start chipping that list away.
| relatedInformation: map(diag.relatedInformation, formatRelatedInformation), | ||
| }; | ||
| return includeFileName | ||
| ? { ...common, fileName: diag.file && diag.file.fileName } |
There was a problem hiding this comment.
no need to create an object just to inline it.. you can set fileName to undefined
|
//CC @mjbvz |
|
@weswigham we should also show the additional locations under |
weswigham
force-pushed
the
allow-relatedinfo-on-diag
branch
from
a51811c to
fb03d91
Compare
|
@mhegazy |
|
@mhegazy I've implemented the pretty output (as above) as well as the |
|
@DanielRosenwasser Wanna open your massive list of error-updating backlog issues now that this is in? |
DanielRosenwasser
added
the
Domain: Related Error Spans
This just introduces related information spans into our protocol, this way we can start adding them to diagnostics and seeing them light up in the editor - needs a small change in vscode once the protocol element has been published as well.
Ref #22789, #10489