fix(lsp): include all diagnosable documents on initialize #17979

dsherret · 2023-02-28T21:06:57Z

Closes denoland/vscode_deno#797
Closes #11190
Closes denoland/vscode_deno#811
Closes denoland/vscode_deno#761
Closes denoland/vscode_deno#585
Closes denoland/vscode_deno#561
Closes denoland/vscode_deno#410

aapoalas

Reviewed halfway. Left some nitpicks. This is a very anticipated feature! eg. I just ran into this with a repo of some hundreds of files where keeping open (or forgetting to close) all files was just not a possible thing.

cli/lsp/config.rs

cli/lsp/documents.rs

cli/lsp/config.rs

…cs_root

…oken.

…aration

…to fix_lsp_open_docs_root

dsherret · 2023-03-30T19:16:37Z

cli/cache/http_cache.rs

@@ -42,7 +41,7 @@ fn base_url_to_filename(url: &Url) -> Option<PathBuf> {
    }
    "data" | "blob" => (),
    scheme => {
-      error!("Don't know how to create cache name for scheme: {}", scheme);


Now this error is more likely to happen because it might load in some random files that use a different scheme (ex. "ext:" scheme in our case). This could be especially annoying in the repl. I downgraded this to debug because it didn't seem that important to surface at an "error" level.

dsherret · 2023-03-30T19:19:09Z

cli/tests/integration/repl_tests.rs

@@ -131,24 +131,43 @@ fn pty_complete_expression() {

 #[test]
 fn pty_complete_imports() {
-  util::with_pty(&["repl", "-A"], |mut console| {


I updated the pty tests to use a temporary cwd by defualt in util::with_pty because otherwise now it will load too many files in that may conflict with things such as completions. Actually, thinking about it more, I should probably do the same for the lsp tests (Edit: Done)

I agree, it makes more sense to use temp CWD here and in the LSP tests

dsherret · 2023-03-30T19:56:01Z

cli/tests/integration/lsp_tests.rs

 use test_util::testdata_path;
 use test_util::TestContextBuilder;
 use tower_lsp::lsp_types as lsp;

 #[test]
 fn lsp_startup_shutdown() {
-  let mut client = LspClientBuilder::new().build();
+  let context = TestContextBuilder::new().use_temp_cwd().build();


I think this should be the default actually and it should opt out of using a temp cwd. I will look into this in a future PR and it would allow me to revert a lot of this.

bartlomieju

LGTM, so exciting to have this finally fixed. I'm sure users will be very happy about this change.

bartlomieju · 2023-03-30T20:35:58Z

cli/lsp/config.rs

+    assert_eq!(
+      config.enabled_root_urls(),
+      vec![
+        root_dir1.join("sub_dir").unwrap(),
+        root_dir1.join("test.ts").unwrap(),
+        root_dir2.join("other.ts").unwrap(),
+        root_dir3
+      ]
+    );


bartlomieju · 2023-03-30T20:36:20Z

cli/tests/integration/lsp_tests.rs

@@ -7216,3 +7216,56 @@ Deno.test({

  client.shutdown();
 }
+
+#[test]
+fn lsp_closed_file_find_references() {


bartlomieju · 2023-03-30T20:44:45Z

cli/tests/integration/repl_tests.rs

@@ -131,24 +131,43 @@ fn pty_complete_expression() {

 #[test]
 fn pty_complete_imports() {
-  util::with_pty(&["repl", "-A"], |mut console| {


I agree, it makes more sense to use temp CWD here and in the LSP tests

Closes denoland/vscode_deno#797 Closes #11190 Closes denoland/vscode_deno#811 Closes denoland/vscode_deno#761 Closes denoland/vscode_deno#585 Closes denoland/vscode_deno#561 Closes denoland/vscode_deno#410

sepehrst · 2023-03-31T21:43:09Z

This seems to have introduced a regression as described in #18538

dsherret added 5 commits February 28, 2023 15:43

fix(lsp): add all documents to the lsp on initialize

6125671

Fix

f08a5bf

Take into account enabled_paths

ef6475c

Another one.

17b1f0b

More work.

67439dd

aapoalas reviewed Mar 14, 2023

View reviewed changes

cli/lsp/config.rs Outdated Show resolved Hide resolved

cli/lsp/documents.rs Outdated Show resolved Hide resolved

cli/lsp/documents.rs Outdated Show resolved Hide resolved

cli/lsp/documents.rs Outdated Show resolved Hide resolved

cli/lsp/documents.rs Outdated Show resolved Hide resolved

dsherret mentioned this pull request Mar 14, 2023

fix(lsp): avoid calling client while holding lock #18197

Merged

dsherret added 4 commits March 15, 2023 11:10

Merge branch 'main' into fix_lsp_open_docs_root

abedfff

Fix inverted logic.

459ce44

Compile

72bfa94

Ignore minified files

cdc4af0

dsherret commented Mar 23, 2023

View reviewed changes

cli/lsp/config.rs Outdated Show resolved Hide resolved

dsherret and others added 18 commits March 29, 2023 10:24

Merge branch 'main' into fix_lsp_open_docs_root

0129cf9

Handle files in enabled paths

8688718

Consolidate code

8b0ab70

refactor(lsp): remove boolean parameters on documents.documents(...)

4c9e102

Remove word "And"

d9c5e91

Merge branch 'refactor_lsp_documents_bool_param' into fix_lsp_open_do…

fc4d027

…cs_root

Working.

08ef6af

Failing test because our textDocument/references implementation is br…

2dc17b0

…oken.

fix(lsp): textDocument/references should respect includeDeclaration

bab99d7

Small refactor.

b7dee99

Fix codelens test

a8cb329

Merge branch 'main' into fix_lsp_find_references_respect_include_decl…

5894f85

…aration

Merge branch 'main' into fix_lsp_find_references_respect_include_decl…

c5cfb24

…aration

Add test and improve code

b7d7541

Merge branch 'main' into fix_lsp_find_references_respect_include_decl…

41b3004

…aration

Oops... Fix test

8dec2d3

Reduce flakiness of package_json_uncached_no_error

5a55a7e

Merge branch 'main' into fix_lsp_open_docs_root

74fb03f

dsherret and others added 6 commits March 30, 2023 12:09

Merge branch 'fix_lsp_find_references_respect_include_declaration' in…

818811e

…to fix_lsp_open_docs_root

Add enabled_root_urls tests

7b96f00

Merge branch 'main' into fix_lsp_open_docs_root

bc3c42e

Refactor document finder to be more easily unit testable

697ace6

Another test.

2216402

Merge branch 'main' into fix_lsp_open_docs_root

8fe2597

dsherret marked this pull request as ready for review March 30, 2023 18:17

dsherret requested review from bartlomieju and aapoalas March 30, 2023 18:17

dsherret marked this pull request as draft March 30, 2023 18:27

dsherret and others added 2 commits March 30, 2023 15:15

Fix tests

4d8a9b0

Merge branch 'main' into fix_lsp_open_docs_root

b416267

dsherret marked this pull request as ready for review March 30, 2023 19:15

dsherret commented Mar 30, 2023

View reviewed changes

dsherret added 2 commits March 30, 2023 15:54

Fix two failing tests.

055f94d

Update lsp tests to use a temp cwd

6cc5a97

dsherret commented Mar 30, 2023

View reviewed changes

bartlomieju approved these changes Mar 30, 2023

View reviewed changes

dsherret merged commit 7722014 into denoland:main Mar 30, 2023

dsherret deleted the fix_lsp_open_docs_root branch March 30, 2023 21:47

haschu mentioned this pull request Apr 16, 2023

Update imports when filename changes denoland/vscode_deno#410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lsp): include all diagnosable documents on initialize #17979

fix(lsp): include all diagnosable documents on initialize #17979

dsherret commented Feb 28, 2023 •

edited

Loading

aapoalas left a comment

dsherret Mar 30, 2023

dsherret Mar 30, 2023 •

edited

Loading

bartlomieju Mar 30, 2023

dsherret Mar 30, 2023

bartlomieju left a comment

bartlomieju Mar 30, 2023

bartlomieju Mar 30, 2023

bartlomieju Mar 30, 2023

sepehrst commented Mar 31, 2023

fix(lsp): include all diagnosable documents on initialize #17979

fix(lsp): include all diagnosable documents on initialize #17979

Conversation

dsherret commented Feb 28, 2023 • edited Loading

aapoalas left a comment

Choose a reason for hiding this comment

dsherret Mar 30, 2023

Choose a reason for hiding this comment

dsherret Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

bartlomieju Mar 30, 2023

Choose a reason for hiding this comment

dsherret Mar 30, 2023

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

bartlomieju Mar 30, 2023

Choose a reason for hiding this comment

bartlomieju Mar 30, 2023

Choose a reason for hiding this comment

bartlomieju Mar 30, 2023

Choose a reason for hiding this comment

sepehrst commented Mar 31, 2023

dsherret commented Feb 28, 2023 •

edited

Loading

dsherret Mar 30, 2023 •

edited

Loading