Add extendedClientCapability to exclude diagnostics based on IMarker API

[mickaelistria]

No description provided.

[rgrunber]

We also collect JDT problem markers at https://github.com/eclipse/eclipse.jdt.ls/blob/57432947f5fe74ca43bd80079fc75614f5ebc8a7/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BaseDiagnosticsHandler.java#L67 . These are triggered when a Java source file is opened/changed. Should they also be excluded ?

Also more generally, I noticed that Diagnostic can store a source and tags. Would it help if we just marked the ones coming directly from Eclipse as such, so you could filter directly on client side without needing the special property ?

[mickaelistria]

So far, those IProblems have not caused issues in my IDE. I suspect they aren't immediately diagnostics.

Would it help if we just marked the ones coming directly from Eclipse as such, so you could filter directly on client side without needing the special property ?

It could, but at the moment LSP4E doesn't allow to filter diagnostics. But I'll think more about it and come back to you. We may actually be able to skip diagnostics in general in LSP4E is a marker already exists with the same description... I'm realizing that the current approach that just skips the diagnostic when it comes from marker makes that we probably miss the codeActions to resolve this marker... So I believe we do need both markers in the IDE: the marker from the workspace or JDT (to get Eclipse marker resolution used for this particular marker) and the diagnostic from the LS (to get the codeActions). I need to think a bit more about it... I'm converting it to draft.
Thanks for asking the questions, it brings us to better path (although not necessarily easier).

[rgrunber]

Alright.

I just looked at tagSupport.valueSet . I think that's what the client is meant to use in order to convey which tags are supported/wanted by the client. Unfortunately, JDT-LS doesn't filter diagnostics based on that. It's a bit vague because there's not really any agreed upon pre-defined tags other than https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnosticTag 😐

There's also textDocument/diagnostics (not a notification, like publishDiagnostics), which also doesn't support any filtering and we also don't support this currently.

[mickaelistria]

One other possibility would be to exclude some particular markers from being forwarded: allow to exclude some marker types with a clientCapability.
Something like

{
  "excludedMarkerTypes": { "org.eclipse.lsp4e.diagnostics" }
}

and in the code, we could place some markers.stream().filter(marker -> JavaLanguageServerPlugin.getPreferencesManager().getClientCapability().excludedMarkerTypes().stream().noneMatch(marker::isSubtypeOf)).
What do you think?

[mickaelistria]

This implements the proposal described above, allowing to skip some marker types in some clients (eg type org.eclipse.lsp4e.diagnostic in Eclipse client)

[rgrunber]

Looks good. I'll just need to document at https://github.com/eclipse/eclipse.jdt.ls/wiki/Language-Server-Settings-&-Capabilities#extended-client-capabilities after merging.

[rgrunber]

retest this please.

[rgrunber]

Change looks good to me, so I'll merge it.

[rgrunber]

Whoops, looks like the 3 test failures in WorkspaceDiagnosticsHandlerTest are from this change. Confirmed on local testing.

Changes needed

[mickaelistria]

looks like the 3 test failures in WorkspaceDiagnosticsHandlerTest are from this change.

OK, and what do you prefer: we make the WorkspaceDiagnosticsHandler.toDiagnosticsArray return a mutable list (previous behavior, but returning mutable list can be dangerous), or we change the test so they assume list is not mutable and they copy it?

[rgrunber]

Let's just preserve the mutable behaviour for now.

[mickaelistria]

OK, latest patch should restore mutable lists.

[rgrunber]

Another thing I noticed. You supported the exclusion list for toDiagnosticsArray(IDocument document, IMarker[] markers, boolean isDiagnosticTagSupported). However, there's another method used : toDiagnosticArray(Range range, Collection<IMarker> markers, boolean isDiagnosticTagSupported). This method is used to create project-level markers (it hacks the position to be at offset 0). won't you need to apply your filter logic here as well to ensure those can also get filtered out ? Or have you not had any issues with them when trying the API ?

[rgrunber]

Great. Once #2538 is good, I can merge+rebase.

[mickaelistria]

Thanks. I didn't notice an issue with this particular method so far, but I suspect it's because I never fall in the case of JDT-LS sending project markers. Still, it's more consistent and correct to filter there as well, and I'm almost certain that the same bug could have happened in eclipseide-jdtls with this method.