Moved diagnostic panel contributions from SessionBuffer to Session (#1868) #1881

htmue · 2021-10-24T12:13:54Z

As before, in-view diagnostics are handled by the SessionBuffers. Panel contributions are now managed directly and only by the Sessions, with help of the new DiagnosticsManager. (I bypassed the compressor!)

See more in the commit message.

It turned out that this was all that's needed. There is no need for any knowledge of session buffers in the diagnostics manager (#1868 (comment)).

For now I did only cursory testing with 'haskell-language-server'. It seems to work and I wanted to upload the fix as soon as possible (and get it out of my head). During the next work week there will be some heavy testing on a big multi-folder project with different language servers.

…ublimelsp#1868). * created DiagnosticsManager, owned by Session * Session.m_textDocument_publishDiagnostics() now does the following: - add incoming diagnostics to DiagnosticsManager (new) - notify WindowManager about diagnostics update via WindowManager.update_diagnostics_panel_async() (new) - if there is a SessionBuffer for this uri, notify it via SessionBuffer.on_diagnostics_async() (unchanged, for in-view diagnostics) * WindowManager.update_diagnostics_panel_async() now queries the Session DiagnosticsManagers for contributions (instead of DocumentSyncListeners) * removed sorting of diagnostics: the order in which diagnostics are reported by the server carries meaning (maybe add optional sorting controlled by user setting?) * file paths in diagnostics panel are now absolute (same as in Sublime's "Find Results"): in multi-folder projects the full path is needed; besides, WindowManager.get_project_path() appears to be buggy * removed "result_base_dir" from diagnostics panel settings: with absolute file paths it isn't needed anymore and it didn't work with multi-folder projects anyway * removed all diagnostics panel related code from SessionBuffer, DiagnosticSeverityData and DocumentSyncListener (handling of in-view diagnostics is unchanged)

rwols · 2021-10-24T13:12:10Z

The diags seem to be keyed by the string representation of their URI. Have you checked if things keep working as expected for case-insensitive file systems and symlinks?

plugin/core/diagnostics_manager.py

htmue · 2021-10-24T14:31:04Z

The diags seem to be keyed by the string representation of their URI. Have you checked if things keep working as expected for case-insensitive file systems and symlinks?

No, but the uri is taken directly as sent by the language server, so I think we can assume that the language server is taking care of it.

The same uri is used in LSP as key for the session buffers all the time, nothing new here.

dcyoung05 · 2021-10-24T18:37:27Z

The diags seem to be keyed by the string representation of their URI. Have you checked if things keep working as expected for case-insensitive file systems and symlinks?

No, but the uri is taken directly as sent by the language server, so I think we can assume that the language server is taking care of it.

The same uri is used in LSP as key for the session buffers all the time, nothing new here.

I've run into a problem with this using Pyright on Windows: it seems that the URI for the file in the focused view does not have the colon (in "c:/") percent-encoded, while the URIs for other files have it encoded as %3A. This means that if a diagnostic is generated for a file in the background and then fixed while that file is focused, it won't be removed from the diagnostic panel (until another diagnostic is generated while the file is in the background).

I'm not sure if this is LSP or the server doing something wrong, but in any event I'm working around it for now by passing the URI to parse_uri (from plugin/core/url.py) before using it as a dict key.

htmue · 2021-10-24T20:37:10Z

I've run into a problem with this using Pyright on Windows: it seems that the URI for the file in the focused view does not have the colon (in "c:/") percent-encoded, while the URIs for other files have it encoded as %3A. This means that if a diagnostic is generated for a file in the background and then fixed while that file is focused, it won't be removed from the diagnostic panel (until another diagnostic is generated while the file is in the background).

I'm not sure if this is LSP or the server doing something wrong, but in any event I'm working around it for now by passing the URI to parse_uri (from plugin/core/url.py) before using it as a dict key.

Could you maybe turn on log_server in the LSP settings and check which URIs are sent by Pyright and by LSP? That would help to locate the culprit. The diagnostics manager only uses the URIs exactly as received from the server, so this problem indicates that it receives both forms. The logs should reveal which side is inconsistent with the URIs.

plugin/core/diagnostics_manager.py

plugin/core/windows.py

plugin/session_buffer.py

plugin/core/windows.py

plugin/core/diagnostics_manager.py

htmue · 2021-10-24T21:56:38Z

I've run into a problem with this using Pyright on Windows: it seems that the URI for the file in the focused view does not have the colon (in "c:/") percent-encoded, while the URIs for other files have it encoded as %3A.

Maybe this is related to #1796?

This also adds some Windows path normalisation.

htmue · 2021-10-25T06:53:47Z

... but in any event I'm working around it for now by passing the URI to parse_uri (from plugin/core/url.py) before using it as a dict key.

On second thought, it's better to parse the URI early and use the path as key anyway.

dcyoung05 · 2021-10-25T07:48:44Z

I've run into a problem with this using Pyright on Windows: it seems that the URI for the file in the focused view does not have the colon (in "c:/") percent-encoded, while the URIs for other files have it encoded as %3A. This means that if a diagnostic is generated for a file in the background and then fixed while that file is focused, it won't be removed from the diagnostic panel (until another diagnostic is generated while the file is in the background).
I'm not sure if this is LSP or the server doing something wrong, but in any event I'm working around it for now by passing the URI to parse_uri (from plugin/core/url.py) before using it as a dict key.

Could you maybe turn on log_server in the LSP settings and check which URIs are sent by Pyright and by LSP? That would help to locate the culprit. The diagnostics manager only uses the URIs exactly as received from the server, so this problem indicates that it receives both forms. The logs should reveal which side is inconsistent with the URIs.

From the logs, it seems like Pyright is being inconsistent while LSP is sending non-RFC-compliant URIs, so I'm not sure which should be changed.

The second and fifth lines are reports for the same file with different URIs, and Pyright seems to be aware of that (or at least it's not sending any further reports with the escaped URI):

::  -> LSP-pyright textDocument/didOpen: {'textDocument': {'uri': 'file:///C:/Users/dcyoung/file1.py'...
:: <-  LSP-pyright textDocument/publishDiagnostics: {'uri': 'file:///c%3A/Users/dcyoung/file2.py', 'diagnostics': [{'severity': 4, 'tags': [1], 'message': '"jt" is not accessed', 'source': 'Pyright', 'range': {'end': {'character': 26, 'line': 44}, 'start': {'character': 24, 'line': 44}}}], 'version': 0}
:: <-  LSP-pyright textDocument/publishDiagnostics: {'uri': 'file:///C:/Users/dcyoung/file1.py', 'diagnostics': [], 'version': 0}
::  -> LSP-pyright textDocument/didOpen: {'textDocument': {'uri': 'file:///C:/Users/dcyoung/file2.py'...
:: <-  LSP-pyright textDocument/publishDiagnostics: {'uri': 'file:///C:/Users/dcyoung/file2.py', 'diagnostics': [], 'version': 3}

And LSP is not escaping the colon because pathname2url on Windows never has (originally it replaced the colon with a pipe, which would not be any better in this situation). (https://github.com/python/cpython/blob/51ed2c56a1852cd6b09c85ba81312dc9782772ce/Lib/nturl2path.py#L77).

htmue · 2021-10-25T09:28:58Z

It looks like Pyright uses URIs unchanged if they were received from the client before and creates new URIs percent-encoded itself. If that is desirable or not could be debated but is beyond the scope of this PR, I would say. (I brougt it up myself, I know.)

As I read the RFC, the unescaped colon in an absolute path preceded by a scheme is valid (https://www.rfc-editor.org/rfc/rfc3986#section-3.3). Percent-encoding safe characters is allowed too.

So, what to say, both Pyright and LSP are probably right. Parsing the URI and normalising the path before using it to key the diagnostics is the thing to do.

Could you please check if this PR works properly for you after my last commit based on your workaround?

rwols

I'm sure you'll stick around if any bugs appear @htmue

dcyoung05 · 2021-10-25T21:32:38Z

It looks like Pyright uses URIs unchanged if they were received from the client before and creates new URIs percent-encoded itself. If that is desirable or not could be debated but is beyond the scope of this PR, I would say. (I brougt it up myself, I know.)

As I read the RFC, the unescaped colon in an absolute path preceded by a scheme is valid (https://www.rfc-editor.org/rfc/rfc3986#section-3.3). Percent-encoding safe characters is allowed too.

So, what to say, both Pyright and LSP are probably right. Parsing the URI and normalising the path before using it to key the diagnostics is the thing to do.

Could you please check if this PR works properly for you after my last commit based on your workaround?

Yes, works for me with the latest commit. Thanks!

htmue · 2021-10-26T11:15:13Z

Thanks for your contribution!

dcyoung05 reviewed Oct 24, 2021

View reviewed changes

plugin/core/diagnostics_manager.py Outdated Show resolved Hide resolved

Handle clear diagnostics messages safely (sublimelsp#1868).

b6d2c65

Added function signature (sublimelsp#1868).

c6cb4f9

Merge branch 'main' into fix/session-diagnostics-manager

9c7a575

rchl reviewed Oct 24, 2021

View reviewed changes

Removed unused code and some newlines (sublimelsp#1868).

87ee7fe

htmue added 3 commits October 25, 2021 00:09

Added _async suffix (sublimelsp#1868).

728ccf5

Filter empty contributions properly (sublimelsp#1868).

943f10e

Use path as key (sublimelsp#1868).

305bc21

This also adds some Windows path normalisation.

rchl approved these changes Oct 25, 2021

View reviewed changes

rwols approved these changes Oct 25, 2021

View reviewed changes

rwols merged commit f41c102 into sublimelsp:main Oct 25, 2021

rchl mentioned this pull request Nov 1, 2021

"Goto Diagnostic" quick panels. #1884

Merged

rchl mentioned this pull request Nov 17, 2021

refactor: decouple diagnostics from handlers into a hook hashicorp/terraform-ls#714

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved diagnostic panel contributions from SessionBuffer to Session (#1868) #1881

Moved diagnostic panel contributions from SessionBuffer to Session (#1868) #1881

htmue commented Oct 24, 2021 •

edited

Loading

rwols commented Oct 24, 2021

htmue commented Oct 24, 2021

dcyoung05 commented Oct 24, 2021

htmue commented Oct 24, 2021

htmue commented Oct 24, 2021

htmue commented Oct 25, 2021

dcyoung05 commented Oct 25, 2021

htmue commented Oct 25, 2021 •

edited

Loading

rwols left a comment

dcyoung05 commented Oct 25, 2021

htmue commented Oct 26, 2021

Moved diagnostic panel contributions from SessionBuffer to Session (#1868) #1881

Moved diagnostic panel contributions from SessionBuffer to Session (#1868) #1881

Conversation

htmue commented Oct 24, 2021 • edited Loading

rwols commented Oct 24, 2021

htmue commented Oct 24, 2021

dcyoung05 commented Oct 24, 2021

htmue commented Oct 24, 2021

htmue commented Oct 24, 2021

htmue commented Oct 25, 2021

dcyoung05 commented Oct 25, 2021

htmue commented Oct 25, 2021 • edited Loading

rwols left a comment

Choose a reason for hiding this comment

dcyoung05 commented Oct 25, 2021

htmue commented Oct 26, 2021

htmue commented Oct 24, 2021 •

edited

Loading

htmue commented Oct 25, 2021 •

edited

Loading