From 66fc7c8fc0dbc5f95c75feaec957b03ead6473be Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 24 Jun 2025 17:54:28 +0530 Subject: [PATCH] Consider virtual path for various server actions (#18910) ## Summary Ref: https://github.com/astral-sh/ruff/issues/14820#issuecomment-2996690681 This PR fixes a bug where virtual paths or any paths that doesn't exists on the file system weren't being considered for checking inclusion / exclusion. This was because the logic used `file_path` which returns `None` for those path. This PR fixes that by using the `virtual_file_path` method that returns a `Path` corresponding to the actual file on disk or any kind of virtual path. This should ideally just fix the above linked issue by way of excluding the documents representing the interactive window because they aren't in the inclusion set. It failed only on Windows previously because the file path construction would fail and then Ruff would default to including all the files. ## Test Plan On my machine, the `.interactive` paths are always excluded so I'm using the inclusion set instead: ```json { "ruff.nativeServer": "on", "ruff.path": ["/Users/dhruv/work/astral/ruff/target/debug/ruff"], "ruff.configuration": { "extend-include": ["*.interactive"] } } ``` The diagnostics are shown for both the file paths and the interactive window: Screenshot 2025-06-24 at 14 56 40 And, the logs: ``` 2025-06-24 14:56:26.478275000 DEBUG notification{method="notebookDocument/didChange"}: Included path via `extend-include`: /Interactive-1.interactive ``` And, when using `ruff.exclude` via: ```json { "ruff.exclude": ["*.interactive"] } ``` With logs: ``` 2025-06-24 14:58:41.117743000 DEBUG notification{method="notebookDocument/didChange"}: Ignored path via `exclude`: /Interactive-1.interactive ``` --- crates/ruff_server/src/fix.rs | 25 +++++----- crates/ruff_server/src/format.rs | 16 +++---- crates/ruff_server/src/lint.rs | 46 ++++++++----------- .../src/server/api/requests/format.rs | 20 ++++---- .../src/server/api/requests/format_range.rs | 20 ++++---- 5 files changed, 58 insertions(+), 69 deletions(-) diff --git a/crates/ruff_server/src/fix.rs b/crates/ruff_server/src/fix.rs index a427b44228..0c910f6755 100644 --- a/crates/ruff_server/src/fix.rs +++ b/crates/ruff_server/src/fix.rs @@ -28,21 +28,22 @@ pub(crate) fn fix_all( ) -> crate::Result { let source_kind = query.make_source_kind(); let settings = query.settings(); - let document_path = query.file_path(); + let document_path = query.virtual_file_path(); // If the document is excluded, return an empty list of fixes. - let package = if let Some(document_path) = document_path.as_ref() { - if is_document_excluded_for_linting( - document_path, - &settings.file_resolver, - linter_settings, - query.text_document_language_id(), - ) { - return Ok(Fixes::default()); - } + if is_document_excluded_for_linting( + &document_path, + &settings.file_resolver, + linter_settings, + query.text_document_language_id(), + ) { + return Ok(Fixes::default()); + } + let file_path = query.file_path(); + let package = if let Some(file_path) = &file_path { detect_package_root( - document_path + file_path .parent() .expect("a path to a document should have a parent path"), &linter_settings.namespace_packages, @@ -65,7 +66,7 @@ pub(crate) fn fix_all( result, .. } = ruff_linter::linter::lint_fix( - &query.virtual_file_path(), + &document_path, package, flags::Noqa::Enabled, settings.unsafe_fixes, diff --git a/crates/ruff_server/src/format.rs b/crates/ruff_server/src/format.rs index c2398ce020..d63d94f5ec 100644 --- a/crates/ruff_server/src/format.rs +++ b/crates/ruff_server/src/format.rs @@ -12,10 +12,10 @@ pub(crate) fn format( document: &TextDocument, source_type: PySourceType, formatter_settings: &FormatterSettings, - path: Option<&Path>, + path: &Path, ) -> crate::Result> { let format_options = - formatter_settings.to_format_options(source_type, document.contents(), path); + formatter_settings.to_format_options(source_type, document.contents(), Some(path)); match format_module_source(document.contents(), format_options) { Ok(formatted) => { let formatted = formatted.into_code(); @@ -40,10 +40,10 @@ pub(crate) fn format_range( source_type: PySourceType, formatter_settings: &FormatterSettings, range: TextRange, - path: Option<&Path>, + path: &Path, ) -> crate::Result> { let format_options = - formatter_settings.to_format_options(source_type, document.contents(), path); + formatter_settings.to_format_options(source_type, document.contents(), Some(path)); match ruff_python_formatter::format_range(document.contents(), range, format_options) { Ok(formatted) => { @@ -97,7 +97,7 @@ with open("a_really_long_foo") as foo, open("a_really_long_bar") as bar, open("a per_file_target_version, ..Default::default() }, - Some(Path::new("test.py")), + Path::new("test.py"), ) .expect("Expected no errors when formatting") .expect("Expected formatting changes"); @@ -119,7 +119,7 @@ with open("a_really_long_foo") as foo, open("a_really_long_bar") as bar, open("a unresolved_target_version: PythonVersion::PY38, ..Default::default() }, - Some(Path::new("test.py")), + Path::new("test.py"), ) .expect("Expected no errors when formatting") .expect("Expected formatting changes"); @@ -167,7 +167,7 @@ sys.exit( ..Default::default() }, range, - Some(Path::new("test.py")), + Path::new("test.py"), ) .expect("Expected no errors when formatting") .expect("Expected formatting changes"); @@ -190,7 +190,7 @@ sys.exit( ..Default::default() }, range, - Some(Path::new("test.py")), + Path::new("test.py"), ) .expect("Expected no errors when formatting") .expect("Expected formatting changes"); diff --git a/crates/ruff_server/src/lint.rs b/crates/ruff_server/src/lint.rs index 9b11ebdaf3..3e49295e97 100644 --- a/crates/ruff_server/src/lint.rs +++ b/crates/ruff_server/src/lint.rs @@ -69,37 +69,29 @@ pub(crate) fn check( ) -> DiagnosticsMap { let source_kind = query.make_source_kind(); let settings = query.settings(); - let document_path = query.file_path(); + let document_path = query.virtual_file_path(); // If the document is excluded, return an empty list of diagnostics. - let package = if let Some(document_path) = document_path.as_ref() { - if is_document_excluded_for_linting( - document_path, - &settings.file_resolver, - &settings.linter, - query.text_document_language_id(), - ) { - return DiagnosticsMap::default(); - } + if is_document_excluded_for_linting( + &document_path, + &settings.file_resolver, + &settings.linter, + query.text_document_language_id(), + ) { + return DiagnosticsMap::default(); + } - detect_package_root( - document_path - .parent() - .expect("a path to a document should have a parent path"), - &settings.linter.namespace_packages, - ) - .map(PackageRoot::root) - } else { - None - }; + let package = detect_package_root( + document_path + .parent() + .expect("a path to a document should have a parent path"), + &settings.linter.namespace_packages, + ) + .map(PackageRoot::root); let source_type = query.source_type(); - let target_version = if let Some(path) = &document_path { - settings.linter.resolve_target_version(path) - } else { - settings.linter.unresolved_target_version - }; + let target_version = settings.linter.resolve_target_version(&document_path); let parse_options = ParseOptions::from(source_type).with_target_version(target_version.parser_version()); @@ -123,7 +115,7 @@ pub(crate) fn check( // Generate checks. let diagnostics = check_path( - &query.virtual_file_path(), + &document_path, package, &locator, &stylist, @@ -138,7 +130,7 @@ pub(crate) fn check( ); let noqa_edits = generate_noqa_edits( - &query.virtual_file_path(), + &document_path, &diagnostics, &locator, indexer.comment_ranges(), diff --git a/crates/ruff_server/src/server/api/requests/format.rs b/crates/ruff_server/src/server/api/requests/format.rs index 2203d5381d..9066111217 100644 --- a/crates/ruff_server/src/server/api/requests/format.rs +++ b/crates/ruff_server/src/server/api/requests/format.rs @@ -83,18 +83,16 @@ fn format_text_document( is_notebook: bool, ) -> Result { let settings = query.settings(); + let file_path = query.virtual_file_path(); // If the document is excluded, return early. - let file_path = query.file_path(); - if let Some(file_path) = &file_path { - if is_document_excluded_for_formatting( - file_path, - &settings.file_resolver, - &settings.formatter, - text_document.language_id(), - ) { - return Ok(None); - } + if is_document_excluded_for_formatting( + &file_path, + &settings.file_resolver, + &settings.formatter, + text_document.language_id(), + ) { + return Ok(None); } let source = text_document.contents(); @@ -102,7 +100,7 @@ fn format_text_document( text_document, query.source_type(), &settings.formatter, - file_path.as_deref(), + &file_path, ) .with_failure_code(lsp_server::ErrorCode::InternalError)?; let Some(mut formatted) = formatted else { diff --git a/crates/ruff_server/src/server/api/requests/format_range.rs b/crates/ruff_server/src/server/api/requests/format_range.rs index e8cb4d56fc..4e4cddae1a 100644 --- a/crates/ruff_server/src/server/api/requests/format_range.rs +++ b/crates/ruff_server/src/server/api/requests/format_range.rs @@ -47,18 +47,16 @@ fn format_text_document_range( encoding: PositionEncoding, ) -> Result { let settings = query.settings(); + let file_path = query.virtual_file_path(); // If the document is excluded, return early. - let file_path = query.file_path(); - if let Some(file_path) = &file_path { - if is_document_excluded_for_formatting( - file_path, - &settings.file_resolver, - &settings.formatter, - text_document.language_id(), - ) { - return Ok(None); - } + if is_document_excluded_for_formatting( + &file_path, + &settings.file_resolver, + &settings.formatter, + text_document.language_id(), + ) { + return Ok(None); } let text = text_document.contents(); @@ -69,7 +67,7 @@ fn format_text_document_range( query.source_type(), &settings.formatter, range, - file_path.as_deref(), + &file_path, ) .with_failure_code(lsp_server::ErrorCode::InternalError)?;