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

Always use complete URI for store key #891

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Aug 15, 2023

Motivation

There are scenarios where we can receive the same file path with different schemes from VS Code. For example, when someone is looking at the diff editor, we receive the same path twice with two different schemes:

file:///foo/bar.rb
git:///foo/bar.rb

Because we were using the file path as the storage key, these two URIs would conflict and result in inconsistent internal state in the LSP. If you closed the git version, it would delete the file version too and that file would no longer exist (from the LSP's perspective).

Implementation

The switch to use file paths and untitled names as the storage key was a mistake. We should always use the complete URI so that there are no conflicts and each scheme has a separate version of the document. VS Code sends duplicate requests for each scheme version, so we should respect that and treat them as separate documents.

Automated tests

Added some examples that catch the issue.

@vinistock vinistock added this to the 2023-Q3 milestone Aug 15, 2023
@vinistock vinistock self-assigned this Aug 15, 2023
@vinistock vinistock requested a review from a team as a code owner August 15, 2023 17:55
@vinistock vinistock requested review from Morriar and egiurleo August 15, 2023 17:55
@github-actions
Copy link
Contributor

github-actions bot commented Aug 15, 2023

Benchmark results in seconds (slowest at top)

          textDocument/completion average: 0.266364 std_dev: 0.00487
          textDocument/diagnostic average: 0.042425 std_dev: 0.010273
          textDocument/definition average: 0.005436 std_dev: 0.003135
      textDocument/selectionRange average: 0.00422 std_dev: 0.000672
   textDocument/documentHighlight average: 0.002604 std_dev: 0.000184
 textDocument/semanticTokens/full average: 0.00259 std_dev: 0.00035
            textDocument/codeLens average: 0.002517 std_dev: 0.000191
      textDocument/documentSymbol average: 0.002514 std_dev: 0.000192
        textDocument/documentLink average: 0.0025 std_dev: 0.000222
        textDocument/foldingRange average: 0.002479 std_dev: 0.00022
textDocument/semanticTokens/range average: 0.001705 std_dev: 0.000152
               codeAction/resolve average: 0.001443 std_dev: 0.000115
               textDocument/hover average: 0.001408 std_dev: 0.000106
           textDocument/inlayHint average: 0.001383 std_dev: 0.000145
    textDocument/onTypeFormatting average: 0.000846 std_dev: 8.5e-05
          textDocument/formatting average: 0.000845 std_dev: 0.000308
          textDocument/codeAction average: 0.000819 std_dev: 7.3e-05


================================================================================
Comparison with main branch:

 textDocument/semanticTokens/full unchanged
textDocument/semanticTokens/range unchanged
      textDocument/documentSymbol unchanged
        textDocument/foldingRange unchanged
          textDocument/formatting unchanged
          textDocument/diagnostic unchanged
        textDocument/documentLink unchanged
           textDocument/inlayHint unchanged
      textDocument/selectionRange unchanged
   textDocument/documentHighlight unchanged
               textDocument/hover unchanged
          textDocument/codeAction unchanged
    textDocument/onTypeFormatting unchanged
               codeAction/resolve unchanged
          textDocument/completion unchanged
            textDocument/codeLens unchanged
          textDocument/definition unchanged


================================================================================
Missing benchmarks:

RubyLsp::Requests::ShowSyntaxTree

@vinistock vinistock force-pushed the vs/remove_unescape_duplication branch from 4336333 to 38fd267 Compare August 15, 2023 20:29
@vinistock vinistock changed the title Remove CGI.unescape duplication in Store#get Always use complete URI for store key Aug 15, 2023
@vinistock vinistock added the bugfix This PR will fix an existing bug label Aug 15, 2023
Copy link
Contributor

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

You said we're receiving the same path twice but with two different schemes. Are we sure we want to act upon requests on a git://?

Can't we just ignore all requests that are not on a file://?

@vinistock
Copy link
Member Author

I think it's worth responding to requests on git schemes, since it's useful to have consistent highlighting and navigation available when comparing two versions of the same file. If we only support file, then we'd have different features available on each diff side.

@vinistock vinistock requested review from Morriar and andyw8 August 16, 2023 20:36
Copy link
Contributor

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

I agree with Andy, the repeated to_s is unfortunate.

Related to #896, I wonder if we should create our own abstraction on top of URI that would behaves as we need when performing the hash lookup and that could be used to read/write files from other requests.

@vinistock
Copy link
Member Author

That's a good point, I added that to the issue. Maybe we can build our own URI class on top of the default one, abstracting some aspects to make it work the way we want.

@vinistock vinistock merged commit 428c909 into main Aug 17, 2023
@vinistock vinistock deleted the vs/remove_unescape_duplication branch August 17, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants