Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort diagnostics according to severity for detailed diag responses #1728

Merged
merged 1 commit into from
Feb 17, 2024

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Jan 14, 2024

Users are more likely to be interested in errors than warnings in /detailed_diagnostic responses. That means we need to sort the diagnostics before finding the one nearest to the cursor.

For LSP completers, that's easy as the severity is numerical. For Omnisharp-roslyn and TSServer, the severity is a word and we can't just say sort( key = severity ).

Since LSP severity was convenient for comparison, I have opted for sorting C# and TS diags by the equivalent LSP severity.


This change is Reviewable

Copy link

codecov bot commented Jan 14, 2024

Codecov Report

Merging #1728 (5fb0927) into master (0ce532e) will increase coverage by 0.40%.
The diff coverage is 66.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1728      +/-   ##
==========================================
+ Coverage   95.05%   95.46%   +0.40%     
==========================================
  Files          51       83      +32     
  Lines        6711     8194    +1483     
  Branches        0      163     +163     
==========================================
+ Hits         6379     7822    +1443     
+ Misses        332      322      -10     
- Partials        0       50      +50     

@bstaletic
Copy link
Collaborator Author

According to the coverage data, we're only ever dealing with errors in our tests, at least for C# and TS.

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM, but could we at least add a regression test for LSP ? Seems like something that we could easily forget about.

Reviewable status: 0 of 2 LGTMs obtained

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to, but I couldn't make a test case without pulling in the entirety of ranges-v3. That just feels like a massive dependency for our need.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

@puremourning
Copy link
Member

Don't we just need some code which produces a warning and error on the same line.

I think clangd does this. maybe something like:

int x;
if ( x = 2 )
{
}

With -Wno-error -Werror=uninitialized

@bstaletic bstaletic force-pushed the detailed-diag-priority branch 2 times, most recently from 376cec2 to da493ad Compare January 15, 2024 10:06
Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests added.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)


ycmd/completers/language_server/language_server_completer.py line 1666 at r2 (raw file):

    current_column = lsp.CodepointsToUTF16CodeUnits(
        GetFileLines( request_data, current_file )[ current_line_lsp ],
        request_data[ 'column_codepoint' ] - 1 )

I'm not sure about this....

the help doc for this func says the offset is in 1-based units:

def CodepointsToUTF16CodeUnits( line_value, codepoint_offset ):
  """Return the 1-based UTF-16 code unit offset equivalent to the 1-based
  unicode codepoint offset |codepoint_offset| in the Unicode string
  |line_value|"""

If current_column is 0-based then we should subtract 1 from the result of the call, not the input.

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)


ycmd/completers/language_server/language_server_completer.py line 1666 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I'm not sure about this....

the help doc for this func says the offset is in 1-based units:

def CodepointsToUTF16CodeUnits( line_value, codepoint_offset ):
  """Return the 1-based UTF-16 code unit offset equivalent to the 1-based
  unicode codepoint offset |codepoint_offset| in the Unicode string
  |line_value|"""

If current_column is 0-based then we should subtract 1 from the result of the call, not the input.

Without the -1, current_column is 1-based.
However, we are coparing that column to diagnostic[ 'range' ], later in the for loop, where diagnostics have 0-based character offsets. The comment I added above the call to CodepointsToUTF16CodeUnits() should explain that.

I remember going back and forward and not being sure if that -1 should go before of after ) of the function call.

Thanks for catching this! Fixed.

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

Users are more likely to be interested in errors than warnings in
/detailed_diagnostic responses. That means we need to sort the
diagnostics before finding the one nearest to the cursor.

For LSP completers, that's easy as the severity is numerical.
For Omnisharp-roslyn and TSServer, the severity is a word and we can't
just say `sort( key = severity )`.

Since LSP severity was convenient for comparison, I have opted for
sorting C# and TS diags by the equivalent LSP severity.
@bstaletic bstaletic added the Ship It! Manual override to merge a PR by maintainer label Feb 17, 2024
Copy link
Contributor

mergify bot commented Feb 17, 2024

Thanks for sending a PR!

@mergify mergify bot merged commit b63d2e8 into ycm-core:master Feb 17, 2024
16 of 18 checks passed
@bstaletic bstaletic deleted the detailed-diag-priority branch February 17, 2024 13:51
bstaletic added a commit to ycm-core/YouCompleteMe that referenced this pull request Feb 18, 2024
Changes since the last update:

ycm-core/ycmd#1728 YcmShowDetailedDiagnostic should now match what is echoed on the command line.
ycm-core/ycmd#1731 Add support for `workspace/didChangeWorkspaceFolders` LSP notification.
ycm-core/ycmd#1730 LSP servers are now updated with latest server state afer reset
ycm-core/ycmd#1727 Update JDT to version 1.31.0
ycm-core/ycmd#1726 C# symbol search filtering fix
ycm-core/ycmd#1724 C# GoToDocumentOutline consistency patch
ycm-core/ycmd#1723 Implement GoToDocumentOutline in C#
ycm-core/ycmd#1716 Display tsserver tags in detailed info and GetDoc

`workspace/didChangeWorkspaceFolders` seems to be working fine for java,
rust and go, but should be considered experimental. Definitely more
field experience is needed.
What it should do is allow LSP servers to understand multiple projects
in the same vim instance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants