-
Notifications
You must be signed in to change notification settings - Fork 211
feat: add workspace diagnostic mode support (Issue #397) #1415
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
base: main
Are you sure you want to change the base?
Changes from 12 commits
67b56b7
0ae7e0d
35b80e0
946ad22
c2d3667
761937a
64dd663
a71a9c4
88cd870
32c4cc2
2f74a46
01bf25b
99c3ec1
d4ee92b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -195,6 +195,7 @@ use crate::lsp::non_wasm::queue::HeavyTaskQueue; | |
| use crate::lsp::non_wasm::queue::LspEvent; | ||
| use crate::lsp::non_wasm::queue::LspQueue; | ||
| use crate::lsp::non_wasm::transaction_manager::TransactionManager; | ||
| use crate::lsp::non_wasm::workspace::DiagnosticMode; | ||
| use crate::lsp::non_wasm::workspace::LspAnalysisConfig; | ||
| use crate::lsp::non_wasm::workspace::Workspace; | ||
| use crate::lsp::non_wasm::workspace::Workspaces; | ||
|
|
@@ -923,6 +924,7 @@ impl Server { | |
| self.validate_in_memory_and_commit_if_possible(ide_transaction_manager); | ||
| let transaction = | ||
| ide_transaction_manager.non_committable_transaction(&self.state); | ||
|
|
||
| self.send_response(new_response( | ||
| x.id, | ||
| Ok(self.document_diagnostics(&transaction, params)), | ||
|
|
@@ -1121,9 +1123,24 @@ impl Server { | |
| .state | ||
| .config_finder() | ||
| .python_file(ModuleName::unknown(), e.path()); | ||
| if open_files.contains_key(&path) | ||
| && config.project_includes.covers(&path) | ||
| && !config.project_excludes.covers(&path) | ||
|
|
||
| // Get diagnostic mode for this file's workspace | ||
| let diagnostic_mode = self.workspaces.get_diagnostic_mode(&path); | ||
|
|
||
| // File must be in project (not excluded) to show diagnostics | ||
| let is_in_project = | ||
| config.project_includes.covers(&path) && !config.project_excludes.covers(&path); | ||
|
|
||
| // Then check based on diagnostic mode | ||
| let is_open = open_files.contains_key(&path); | ||
| let should_show = match diagnostic_mode { | ||
| // Workspace mode: show if in project (open or closed files) | ||
| DiagnosticMode::Workspace => is_in_project, | ||
| // OpenFilesOnly mode: show if open AND in project | ||
| DiagnosticMode::OpenFilesOnly => is_open && is_in_project, | ||
| }; | ||
|
|
||
| if should_show | ||
| && self | ||
| .type_error_display_status(e.path().as_path()) | ||
| .is_enabled() | ||
|
|
@@ -1215,19 +1232,84 @@ impl Server { | |
| let handles = | ||
| Self::validate_in_memory_for_transaction(&self.state, &self.open_files, transaction); | ||
|
|
||
| // Check if any workspace is in workspace diagnostic mode | ||
| let has_workspace_mode = self.workspaces.roots().iter().any(|root| { | ||
| matches!( | ||
| self.workspaces.get_diagnostic_mode(root), | ||
| DiagnosticMode::Workspace | ||
| ) | ||
| }); | ||
|
|
||
| // In workspace mode, analyze all project files so get_all_errors() includes unopened files | ||
| if has_workspace_mode { | ||
| let open_file_paths: std::collections::HashSet<_> = | ||
| self.open_files.read().keys().cloned().collect(); | ||
| if let Some(first_open_file) = open_file_paths.iter().next() { | ||
| let module_path = ModulePath::filesystem(first_open_file.clone()); | ||
| let config = self | ||
| .state | ||
| .config_finder() | ||
| .python_file(ModuleName::unknown(), &module_path); | ||
| let project_path_blobs = config.get_filtered_globs(None); | ||
| if let Ok(paths) = project_path_blobs.files() { | ||
| let project_handles: Vec<_> = paths | ||
| .into_iter() | ||
| .filter_map(|path| { | ||
| // Skip files that are already open (already in handles) | ||
| if open_file_paths.contains(&path) { | ||
| return None; | ||
| } | ||
| let module_path = ModulePath::filesystem(path.clone()); | ||
| let path_config = self | ||
| .state | ||
| .config_finder() | ||
| .python_file(ModuleName::unknown(), &module_path); | ||
| if config == path_config { | ||
| Some(handle_from_module_path(&self.state, module_path)) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect(); | ||
| // Analyze only for errors, not full indexing | ||
| transaction.run(&project_handles, Require::Errors); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let publish = |transaction: &Transaction| { | ||
| let mut diags: SmallMap<PathBuf, Vec<Diagnostic>> = SmallMap::new(); | ||
| let open_files = self.open_files.read(); | ||
|
|
||
| // Pre-populate with empty arrays for all open files to ensure we send | ||
| // publishDiagnostics notifications even when errors are cleared | ||
| for x in open_files.keys() { | ||
| diags.insert(x.as_path().to_owned(), Vec::new()); | ||
| } | ||
| for e in transaction.get_errors(&handles).collect_errors().shown { | ||
|
|
||
| // In workspace mode, use get_all_errors() to get errors from all project files. | ||
| // In open-files-only mode, use get_errors(&handles) to only get errors from open files. | ||
| // The filtering by diagnostic mode and project includes/excludes is handled in get_diag_if_shown. | ||
| let errors = if has_workspace_mode { | ||
| transaction.get_all_errors() | ||
AryanBagade marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } else { | ||
| transaction.get_errors(&handles) | ||
| }; | ||
|
|
||
| for e in errors.collect_errors().shown { | ||
| if let Some((path, diag)) = self.get_diag_if_shown(&e, &open_files) { | ||
| diags.entry(path.to_owned()).or_default().push(diag); | ||
| } | ||
| } | ||
|
|
||
| for (path, diagnostics) in diags.iter_mut() { | ||
| let handle = make_open_handle(&self.state, path); | ||
| // Use appropriate handle type: memory handle for open files, filesystem for others | ||
| let is_open = open_files.contains_key(path); | ||
| let handle = if is_open { | ||
| make_open_handle(&self.state, path) | ||
| } else { | ||
| handle_from_module_path(&self.state, ModulePath::filesystem(path.clone())) | ||
| }; | ||
| Self::append_unreachable_diagnostics(transaction, &handle, diagnostics); | ||
| } | ||
| self.connection.publish_diagnostics(diags); | ||
|
|
@@ -2256,10 +2338,17 @@ impl Server { | |
| transaction: &Transaction<'_>, | ||
| params: DocumentDiagnosticParams, | ||
| ) -> DocumentDiagnosticReport { | ||
| let handle = make_open_handle( | ||
|
||
| &self.state, | ||
| ¶ms.text_document.uri.to_file_path().unwrap(), | ||
| ); | ||
| let file_path = params.text_document.uri.to_file_path().unwrap(); | ||
| let is_file_open = self.open_files.read().contains_key(&file_path); | ||
|
|
||
| // When diagnostics are explicitly requested, always return them regardless of mode | ||
| let handle = if is_file_open { | ||
| make_open_handle(&self.state, &file_path) | ||
| } else { | ||
| let module_path = ModulePath::filesystem(file_path.clone()); | ||
| handle_from_module_path(&self.state, module_path) | ||
| }; | ||
|
|
||
| let mut items = Vec::new(); | ||
| let open_files = &self.open_files.read(); | ||
| for e in transaction.get_errors(once(&handle)).collect_errors().shown { | ||
|
|
@@ -2268,6 +2357,7 @@ impl Server { | |
| } | ||
| } | ||
| Self::append_unreachable_diagnostics(transaction, &handle, &mut items); | ||
|
|
||
| DocumentDiagnosticReport::Full(RelatedFullDocumentDiagnosticReport { | ||
| full_document_diagnostic_report: FullDocumentDiagnosticReport { | ||
| items, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # File with intentional type error for testing workspace diagnostic mode | ||
|
|
||
| def add_numbers(x: int, y: int) -> int: | ||
| return x + y | ||
|
|
||
| # This should cause a type error: passing string to int parameter | ||
| result = add_numbers("hello", "world") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # File that will be opened in tests (should always show diagnostics) | ||
|
|
||
| def greet(name: str) -> str: | ||
| return f"Hello, {name}" | ||
|
|
||
| # Type error: passing int to str parameter | ||
| message = greet(123) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| # Pyrefly config for workspace diagnostic mode tests | ||
| project-includes = ["**/*.py"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,147 @@ | ||
| /* | ||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| use lsp_types::Url; | ||
|
|
||
| use crate::test::lsp::lsp_interaction::object_model::InitializeSettings; | ||
| use crate::test::lsp::lsp_interaction::object_model::LspInteraction; | ||
| use crate::test::lsp::lsp_interaction::util::get_test_files_root; | ||
|
|
||
| /// Test that workspace mode shows errors from unopened files via publishDiagnostics | ||
| #[test] | ||
| fn test_workspace_mode_shows_unopened_file_errors() { | ||
| let test_files_root = get_test_files_root(); | ||
| let root_path = test_files_root.path().join("workspace_diagnostic_mode"); | ||
| let scope_uri = Url::from_file_path(&root_path).unwrap(); | ||
| let mut interaction = LspInteraction::new(); | ||
| interaction.set_root(root_path.clone()); | ||
| interaction.initialize(InitializeSettings { | ||
| workspace_folders: Some(vec![( | ||
| "workspace_diagnostic_mode".to_owned(), | ||
| scope_uri.clone(), | ||
| )]), | ||
| configuration: Some(Some(serde_json::json!([{ | ||
| "pyrefly": {"displayTypeErrors": "force-on"}, | ||
| "analysis": {"diagnosticMode": "workspace"} | ||
| }]))), | ||
| ..Default::default() | ||
| }); | ||
|
|
||
| // Open file_with_error.py (has 2 errors) | ||
| // In workspace mode, this should trigger publishDiagnostics for ALL workspace files with errors | ||
| interaction.server.did_open("file_with_error.py"); | ||
|
|
||
| // The opened file should show its 2 errors | ||
| interaction | ||
| .client | ||
| .expect_publish_diagnostics_error_count(root_path.join("file_with_error.py"), 2); | ||
|
|
||
| // The UNOPENED file (opened_file.py) should ALSO show its 1 error via publishDiagnostics | ||
| // This is the key test - workspace mode publishes diagnostics for unopened files! | ||
| interaction | ||
| .client | ||
| .expect_publish_diagnostics_error_count(root_path.join("opened_file.py"), 1); | ||
|
|
||
| interaction.shutdown(); | ||
| } | ||
|
|
||
| /// Test that openFilesOnly mode only publishes diagnostics for open files | ||
| #[test] | ||
| fn test_open_files_only_mode_filters_correctly() { | ||
| let test_files_root = get_test_files_root(); | ||
| let root_path = test_files_root.path().join("workspace_diagnostic_mode"); | ||
| let scope_uri = Url::from_file_path(&root_path).unwrap(); | ||
| let mut interaction = LspInteraction::new(); | ||
| interaction.set_root(root_path.clone()); | ||
| interaction.initialize(InitializeSettings { | ||
| workspace_folders: Some(vec![( | ||
| "workspace_diagnostic_mode".to_owned(), | ||
| scope_uri.clone(), | ||
| )]), | ||
| configuration: Some(Some(serde_json::json!([{ | ||
| "pyrefly": {"displayTypeErrors": "force-on"}, | ||
| "analysis": {"diagnosticMode": "openFilesOnly"} | ||
| }]))), | ||
| ..Default::default() | ||
| }); | ||
|
|
||
| // Open file with errors | ||
| interaction.server.did_open("file_with_error.py"); | ||
|
|
||
| // Should show errors for the opened file | ||
| interaction | ||
| .client | ||
| .expect_publish_diagnostics_error_count(root_path.join("file_with_error.py"), 2); | ||
|
|
||
| interaction.shutdown(); | ||
| } | ||
|
|
||
| /// Test default behavior (should be openFilesOnly) | ||
| #[test] | ||
| fn test_default_mode_is_open_files_only() { | ||
| let test_files_root = get_test_files_root(); | ||
| let root_path = test_files_root.path().join("workspace_diagnostic_mode"); | ||
| let scope_uri = Url::from_file_path(&root_path).unwrap(); | ||
| let mut interaction = LspInteraction::new(); | ||
| interaction.set_root(root_path.clone()); | ||
| interaction.initialize(InitializeSettings { | ||
| workspace_folders: Some(vec![( | ||
| "workspace_diagnostic_mode".to_owned(), | ||
| scope_uri.clone(), | ||
| )]), | ||
| configuration: Some(Some( | ||
| serde_json::json!([{"pyrefly": {"displayTypeErrors": "force-on"}}]), | ||
| )), | ||
| ..Default::default() | ||
| }); | ||
|
|
||
| // Open file with errors | ||
| interaction.server.did_open("file_with_error.py"); | ||
|
|
||
| // Default mode is openFilesOnly, should show errors for opened file only | ||
| interaction | ||
| .client | ||
| .expect_publish_diagnostics_error_count(root_path.join("file_with_error.py"), 2); | ||
|
|
||
| interaction.shutdown(); | ||
| } | ||
|
|
||
| /// Test that workspace mode filters out errors from stdlib/dependencies | ||
| #[test] | ||
| fn test_workspace_mode_excludes_files_outside_workspace() { | ||
| let test_files_root = get_test_files_root(); | ||
| let root_path = test_files_root.path().join("workspace_diagnostic_mode"); | ||
| let scope_uri = Url::from_file_path(&root_path).unwrap(); | ||
| let mut interaction = LspInteraction::new(); | ||
| interaction.set_root(root_path.clone()); | ||
| interaction.initialize(InitializeSettings { | ||
| workspace_folders: Some(vec![( | ||
| "workspace_diagnostic_mode".to_owned(), | ||
| scope_uri.clone(), | ||
| )]), | ||
| configuration: Some(Some(serde_json::json!([{ | ||
| "pyrefly": {"displayTypeErrors": "force-on"}, | ||
| "analysis": {"diagnosticMode": "workspace"} | ||
| }]))), | ||
| ..Default::default() | ||
| }); | ||
|
|
||
| // Open file with errors | ||
| interaction.server.did_open("file_with_error.py"); | ||
|
|
||
| // Should show errors for workspace files only, not stdlib/dependencies | ||
| interaction | ||
| .client | ||
| .expect_publish_diagnostics_error_count(root_path.join("file_with_error.py"), 2); | ||
|
|
||
| // Unopened file errors should also be published | ||
| interaction | ||
| .client | ||
| .expect_publish_diagnostics_error_count(root_path.join("opened_file.py"), 1); | ||
|
|
||
| interaction.shutdown(); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.