Add DiagnosticChangeEvent.reason property for extension development?

[karlhorky]

Currently, the DiagnosticChangeEvent passed on to listeners via vscode.languages.onDidChangeDiagnostics only contains the uris property.

It would be useful for extension development to know where a diagnostic change came from. Eg. it would allow for callback-based approach instead of a timeout in situations like this one in error-lens:

usernamehw/vscode-error-lens#55 (comment)

Would the VS Code project be open for exposing a reason that a DiagnosticChangeEvent happened?

In the example below, I've also added a textDocumentSaveEvent, for additional information about the save event, which could also be useful.

I'm also not sure if there are other potential reasons for a DiagnosticChangeEvent to exist.

enum DiagnosticChangeEventReason {
  TextDocumentSave,
  TextDocumentChange
}

DiagnosticChangeEvent = {
  uris: ReadonlyArray<Uri>;
+  reason: DiagnosticChangeEventReason;
+  textDocumentSaveEvent: TextDocumentWillSaveEvent;
}

The naming is inspired by the TextDocumentSaveReason API.

[jrieken]

VS Code isn't asking for diagnostics, extensions simply push them onto us. That means we have no clue why diagnostics have changed... So, I don't know where such information should be coming from

[karlhorky]

Hm, ok. My assumptions were this:

1. An extension (such as the TypeScript diagnostics engine) knows where the original event comes from
2. The extension could push this information into the diagnostics that they are firing

Am I missing something? Or is there something untenable there?

[jrieken]

The extension could push this information into the diagnostics that they are firing

That's not how extensions are delivering diagnostics. They manage a diagnostics collection for which they can update, per file, what diagnostics there are. So, you can have diagnostics delivered for one file due to save and then also have diagnostics delivered for other files due to something else.

Also, the event represents a "joined" view onto all diagnostics in the system which means there is potential for conflict when extension A says TextDocumentSave and extension B says TextDocumentChange.

[karlhorky]

Got it, so it would require a more granular / complex system to pass along this information, eg. a Diagnostic.reason property:

// in extension
diagnosticCollection.set(uri, {
  message: `Object is possibly 'undefined'.`,
  range: range,
  severity: 0, // Error
  source: 'typescript',
  reason: 0, // TextDocumentSave
})

What are your thoughts on this instead? I can change the original description.

[jrieken]

Sorry, I am not yet sold. This needs buy-in/adoption from extensions and I am not sure they would be able to provide good values. E.g when you change file A you need to re-check all other files. That happens usually async and in the mean time saving of A might have happened and it's unclear what reason for file B should be taken. Also, it feels like not a good match in the domain of diagnostics.

Take a step back. What is it you are trying to achieve? You say extension development?

[karlhorky]

Yeah, extension development: the error-lens extension has a bit of code like this:

vscode.languages.onDidChangeDiagnostics(e => {
  if (Date.now() - Global.lastSavedTimestamp < 1000) {
    onChangedDiagnostics(e);
  }
})

This code is trying to match a diagnostic change with a file save event. Sometimes the 1000ms timeout is not enough (when the TS diagnostics are slow for whatever reason).

It would be nice to be able to make this code work in a non-timeout way. Some ways that come to mind:

1. an event-based pattern (this issue)
2. a callback-based pattern (probably a new API? something like onDidChangeDiagnosticsAfterSave or onDidChangeDiagnostics(callback, {reason: 'TextDocumentSave'})) - but these feel like less elegant options

[jrieken]

This code is trying to match a diagnostic change with a file save event.

Why does it need to do that? That sounds like a wrong assumption because most extensions validate as you type but with certain strategies and priorities, e.g syntax check the changed file, semantic check the changed file, check all other files etc.

[karlhorky]

Because the extension offers the option to only update the diagnostic lenses on save - to avoid eagerly showing (potentially annoying) error lenses until the developer is ready.

[jrieken]

Because the extension offers the option to only update the diagnostic lenses on save

Well, don't do that... Saving and diagnostics changes don't correlate at all. How do you handle the case of changes in file A produce error in file B. B will never be saved and hence never show error lenses?

[karlhorky]

Upon switching to File B, the diagnostics will be queried again and any lenses shown.

[jrieken]

Upon switching to File B, the diagnostics will be queried again and any lenses shown.

I have them side by side, no switching

[karlhorky]

Upon focus into the file (so that File B becomes the active editor).

[jrieken]

Upon focus into the file (so that File B becomes the active editor).

I have them side by side, no switching, no focus, only 👀 on the code

Sorry, closing without action. What you are asking for is very hard to provide, doesn't fit into the diagnostics model and is flawed: diagnostics are unrelated to saving or changing files. This is actually the reason why there is no "DiagnosticsProvider" - VS Code has no clue when to ask for diagnostics because files are dependent on each other in unknown ways. That's why there is the free push model of the diagnostics collection and extension are free to submit their validation results at any time for whatever reason.

[karlhorky]

I have them side by side, no switching, no focus, only 👀 on the code

Yes, so if you don't switch focus, then there would not be a change to the lenses.

However, you would see the feedback of squiggly underlines still appearing on File B. Once you click into the file and it becomes active, you will see the error lenses.

[karlhorky]

I suppose an alternative would be if TS diagnostics could be reported on save only, similar to this issue in atom/ide-typescript: atom/ide-typescript#123

However, drawing a smart line for what should be eager and what should be lazy diagnostics without annoying the user is difficult.

Much harder than the typical Form UX on webpages:

1. If the field is empty, validate first on blur
2. If the field is non-empty and has an error, validate eagerly

Following these rules in a code editor would be hard, because almost every case would be state number 2.

It would have to be heuristic-based I guess - trying to figure out whether the user is writing new code or addressing an error. Maybe something as simple as the following could be a start:

1. When a user is writing on a new line with only whitespace before and no characters afterwards, validate on save
2. When a user is in the middle of a line or has deleted something, validate eagerly

But I guess editors are probably not going in this direction yet. Either have to deal with annoying eager validation or turn it off completely.

[karlhorky]

Sorry, closing without action

Ok, no worries. Thanks for the answer at least. And now it's documented, in case anyone else runs into a similar line of reasoning.