diff --git a/pyrefly/lib/error/error.rs b/pyrefly/lib/error/error.rs index 79d655d1ca..3d495800b1 100644 --- a/pyrefly/lib/error/error.rs +++ b/pyrefly/lib/error/error.rs @@ -14,7 +14,9 @@ use std::path::Path; use itertools::Itertools; use lsp_types::CodeDescription; use lsp_types::Diagnostic; +use lsp_types::DiagnosticRelatedInformation; use lsp_types::DiagnosticTag; +use lsp_types::Location; use lsp_types::Url; use pyrefly_python::ignore::Tool; use pyrefly_python::module::Module; @@ -35,6 +37,7 @@ use yansi::Paint; use crate::config::error_kind::ErrorKind; use crate::config::error_kind::Severity; +use crate::lsp::module_helpers::to_real_path; /// A secondary annotation that labels a span in the same file as the primary error. /// Used to show additional context, e.g. the types of both operands in a binary operation. @@ -322,8 +325,24 @@ impl Error { let code_description = Url::parse(&self.error_kind().docs_url()) .ok() .map(|href| CodeDescription { href }); - // TODO: Map secondary_annotations to DiagnosticRelatedInformation for LSP clients. - // This requires constructing a Url from the module path, which may not always succeed. + // Secondary annotations live in the same file as the primary error, so we can reuse this + // module's path for their locations. `Url::from_file_path` requires an absolute path and + // may fail; in that case we omit the related information rather than dropping the diagnostic. + let related_information = to_real_path(self.module.path()) + .and_then(|path| Url::from_file_path(path).ok()) + .map(|uri| { + self.secondary_annotations + .iter() + .map(|ann| DiagnosticRelatedInformation { + location: Location { + uri: uri.clone(), + range: self.module.to_lsp_range(ann.range), + }, + message: ann.label.to_string(), + }) + .collect::>() + }) + .filter(|related| !related.is_empty()); Diagnostic { range: self.module.to_lsp_range(self.range()), severity: Some(match self.severity() { @@ -342,6 +361,7 @@ impl Error { } else { None }, + related_information, ..Default::default() } } @@ -636,6 +656,71 @@ mod tests { ); } + #[test] + fn test_to_diagnostic_maps_secondary_annotations() { + // Use an absolute path so `Url::from_file_path` succeeds. + let source = "val * 2"; + let module_info = Module::new( + ModuleName::from_str("test"), + ModulePath::filesystem(PathBuf::from("/test.py")), + Arc::new(source.to_owned()), + ); + let error = Error::new( + module_info, + TextRange::new(TextSize::new(0), TextSize::new(7)), + "`*` is not supported between `int | str` and `int`".to_owned(), + Vec::new(), + ErrorKind::UnsupportedOperation, + ) + .with_annotation( + TextRange::new(TextSize::new(0), TextSize::new(3)), + "has type `int | str`".to_owned(), + ) + .with_annotation( + TextRange::new(TextSize::new(6), TextSize::new(7)), + "has type `int`".to_owned(), + ); + + let related = error + .to_diagnostic() + .related_information + .expect("secondary annotations should map to related information"); + assert_eq!(related.len(), 2); + assert_eq!(related[0].message, "has type `int | str`"); + assert_eq!(related[1].message, "has type `int`"); + assert_eq!( + related[0].location.uri, related[1].location.uri, + "annotations in the same file share one uri" + ); + assert!( + related[0].location.uri.as_str().ends_with("/test.py"), + "uri should point at the error's file, got {}", + related[0].location.uri + ); + // The first annotation starts where the primary error does (byte 0). + assert_eq!( + related[0].location.range.start, + error.to_diagnostic().range.start + ); + } + + #[test] + fn test_to_diagnostic_without_annotations_has_no_related_information() { + let module_info = Module::new( + ModuleName::from_str("test"), + ModulePath::filesystem(PathBuf::from("/test.py")), + Arc::new("def f(x: int) -> str:\n return x".to_owned()), + ); + let error = Error::new( + module_info, + TextRange::new(TextSize::new(26), TextSize::new(34)), + "bad return".to_owned(), + Vec::new(), + ErrorKind::BadReturn, + ); + assert!(error.to_diagnostic().related_information.is_none()); + } + /// Integration test: verify that binary operator errors from the type checker /// produce secondary annotations labeling both operands with their types. #[test] diff --git a/pyrefly/lib/test/lsp/lsp_interaction/configuration.rs b/pyrefly/lib/test/lsp/lsp_interaction/configuration.rs index 474fca6a2a..b68dc12856 100644 --- a/pyrefly/lib/test/lsp/lsp_interaction/configuration.rs +++ b/pyrefly/lib/test/lsp/lsp_interaction/configuration.rs @@ -638,10 +638,16 @@ fn test_did_change_workspace_folder() { interaction.shutdown().expect("Failed to shutdown"); } -fn get_diagnostics_result() -> serde_json::Value { +fn get_diagnostics_result(file_uri: &Url) -> serde_json::Value { + let uri = file_uri.as_str(); json!({"items": [ {"code":"unsupported-operation","codeDescription":{"href":"https://pyrefly.org/en/docs/error-kinds/#unsupported-operation"},"message":"`+` is not supported between `Literal[1]` and `Literal['']`\n Argument `Literal['']` is not assignable to parameter `value` with type `int` in function `int.__add__`", - "range":{"end":{"character":6,"line":5},"start":{"character":0,"line":5}},"severity":1,"source":"Pyrefly"}],"kind":"full" + "range":{"end":{"character":6,"line":5},"start":{"character":0,"line":5}}, + "relatedInformation":[ + {"location":{"range":{"end":{"character":1,"line":5},"start":{"character":0,"line":5}},"uri":uri},"message":"has type `Literal[1]`"}, + {"location":{"range":{"end":{"character":6,"line":5},"start":{"character":4,"line":5}},"uri":uri},"message":"has type `Literal['']`"} + ], + "severity":1,"source":"Pyrefly"}],"kind":"full" }) } @@ -952,10 +958,11 @@ fn test_diagnostics_default_workspace_with_config() { interaction.client.did_open("type_errors.py"); + let type_errors_uri = Url::from_file_path(root.join("type_errors.py")).unwrap(); interaction .client .diagnostic("type_errors.py") - .expect_response(get_diagnostics_result()) + .expect_response(get_diagnostics_result(&type_errors_uri)) .expect("Failed to receive expected response"); interaction.client.did_change_configuration(); @@ -1040,10 +1047,15 @@ fn test_diagnostics_file_not_in_includes() { .did_open("diagnostics_file_not_in_includes/type_errors_include.py"); // prove that it works for a project included + let include_uri = Url::from_file_path( + root.path() + .join("diagnostics_file_not_in_includes/type_errors_include.py"), + ) + .unwrap(); interaction .client .diagnostic("diagnostics_file_not_in_includes/type_errors_include.py") - .expect_response(get_diagnostics_result()) + .expect_response(get_diagnostics_result(&include_uri)) .expect("Failed to receive expected response"); // prove that it ignores a file not in project includes @@ -1078,10 +1090,15 @@ fn test_diagnostics_file_in_excludes() { .did_open("diagnostics_file_in_excludes/type_errors_include.py"); // prove that it works for a project included + let include_uri = Url::from_file_path( + root.path() + .join("diagnostics_file_in_excludes/type_errors_include.py"), + ) + .unwrap(); interaction .client .diagnostic("diagnostics_file_in_excludes/type_errors_include.py") - .expect_response(get_diagnostics_result()) + .expect_response(get_diagnostics_result(&include_uri)) .expect("Failed to receive expected response"); // prove that it ignores a file not in project includes diff --git a/pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs b/pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs index 2cacb68b4b..94e29ef3eb 100644 --- a/pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs +++ b/pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs @@ -676,6 +676,8 @@ fn test_error_documentation_links() { interaction.client.did_open("error_docs_test.py"); + let error_docs_uri = + Url::from_file_path(test_files_root.path().join("error_docs_test.py")).unwrap(); interaction .client .diagnostic("error_docs_test.py") @@ -691,6 +693,16 @@ fn test_error_documentation_links() { "end": {"character": 11, "line": 9}, "start": {"character": 9, "line": 9} }, + "relatedInformation": [{ + "location": { + "range": { + "end": {"character": 6, "line": 9}, + "start": {"character": 3, "line": 9} + }, + "uri": error_docs_uri.as_str() + }, + "message": "declared type" + }], "severity": 1, "source": "Pyrefly" }, @@ -1108,6 +1120,7 @@ fn test_shows_stdlib_type_errors_with_force_on() { interaction.client.did_open(stdlib_filepath); + let stdlib_uri = Url::from_file_path(test_files_root.path().join(stdlib_filepath)).unwrap(); interaction .client .diagnostic(stdlib_filepath) @@ -1123,6 +1136,16 @@ fn test_shows_stdlib_type_errors_with_force_on() { "end": {"character": 12, "line": 5}, "start": {"character": 9, "line": 5} }, + "relatedInformation": [{ + "location": { + "range": { + "end": {"character": 6, "line": 5}, + "start": {"character": 3, "line": 5} + }, + "uri": stdlib_uri.as_str() + }, + "message": "declared type" + }], "severity": 1, "source": "Pyrefly" } @@ -1160,14 +1183,9 @@ fn test_shows_stdlib_errors_for_multiple_versions_and_paths_with_force_on() { .unwrap() .send_configuration_response(json!([{"pyrefly": {"displayTypeErrors": "force-on"}}])); - interaction - .client - .did_open("filtering_stdlib_errors/usr/local/lib/python3.12/stdlib_file.py"); - - interaction - .client - .diagnostic("filtering_stdlib_errors/usr/local/lib/python3.12/stdlib_file.py") - .expect_response(json!({ + let expected_stdlib_diagnostic = |file_path: &str| { + let uri = Url::from_file_path(test_files_root.path().join(file_path)).unwrap(); + json!({ "items": [ { "code": "bad-assignment", @@ -1179,12 +1197,30 @@ fn test_shows_stdlib_errors_for_multiple_versions_and_paths_with_force_on() { "end": {"character": 12, "line": 5}, "start": {"character": 9, "line": 5} }, + "relatedInformation": [{ + "location": { + "range": { + "end": {"character": 6, "line": 5}, + "start": {"character": 3, "line": 5} + }, + "uri": uri.as_str() + }, + "message": "declared type" + }], "severity": 1, "source": "Pyrefly" } ], "kind": "full" - })) + }) + }; + + let path = "filtering_stdlib_errors/usr/local/lib/python3.12/stdlib_file.py"; + interaction.client.did_open(path); + interaction + .client + .diagnostic(path) + .expect_response(expected_stdlib_diagnostic(path)) .unwrap(); register_stdlib_paths(vec![ @@ -1193,58 +1229,20 @@ fn test_shows_stdlib_errors_for_multiple_versions_and_paths_with_force_on() { .join("filtering_stdlib_errors/usr/lib/python3.8"), ]); + let path = "filtering_stdlib_errors/usr/local/lib/python3.8/stdlib_file.py"; + interaction.client.did_open(path); interaction .client - .did_open("filtering_stdlib_errors/usr/local/lib/python3.8/stdlib_file.py"); - - interaction - .client - .diagnostic("filtering_stdlib_errors/usr/local/lib/python3.8/stdlib_file.py") - .expect_response(json!({ - "items": [ - { - "code": "bad-assignment", - "codeDescription": { - "href": "https://pyrefly.org/en/docs/error-kinds/#bad-assignment" - }, - "message": "`Literal['1']` is not assignable to `int`", - "range": { - "end": {"character": 12, "line": 5}, - "start": {"character": 9, "line": 5} - }, - "severity": 1, - "source": "Pyrefly" - } - ], - "kind": "full" - })) + .diagnostic(path) + .expect_response(expected_stdlib_diagnostic(path)) .unwrap(); + let path = "filtering_stdlib_errors/usr/lib/python3.12/stdlib_file.py"; + interaction.client.did_open(path); interaction .client - .did_open("filtering_stdlib_errors/usr/lib/python3.12/stdlib_file.py"); - - interaction - .client - .diagnostic("filtering_stdlib_errors/usr/lib/python3.12/stdlib_file.py") - .expect_response(json!({ - "items": [ - { - "code": "bad-assignment", - "codeDescription": { - "href": "https://pyrefly.org/en/docs/error-kinds/#bad-assignment" - }, - "message": "`Literal['1']` is not assignable to `int`", - "range": { - "end": {"character": 12, "line": 5}, - "start": {"character": 9, "line": 5} - }, - "severity": 1, - "source": "Pyrefly" - } - ], - "kind": "full" - })) + .diagnostic(path) + .expect_response(expected_stdlib_diagnostic(path)) .unwrap(); register_stdlib_paths(vec![ @@ -1253,31 +1251,12 @@ fn test_shows_stdlib_errors_for_multiple_versions_and_paths_with_force_on() { .join("filtering_stdlib_errors/usr/lib64/python3.12"), ]); + let path = "filtering_stdlib_errors/usr/lib64/python3.12/stdlib_file.py"; + interaction.client.did_open(path); interaction .client - .did_open("filtering_stdlib_errors/usr/lib64/python3.12/stdlib_file.py"); - - interaction - .client - .diagnostic("filtering_stdlib_errors/usr/lib64/python3.12/stdlib_file.py") - .expect_response(json!({ - "items": [ - { - "code": "bad-assignment", - "codeDescription": { - "href": "https://pyrefly.org/en/docs/error-kinds/#bad-assignment" - }, - "message": "`Literal['1']` is not assignable to `int`", - "range": { - "end": {"character": 12, "line": 5}, - "start": {"character": 9, "line": 5} - }, - "severity": 1, - "source": "Pyrefly" - } - ], - "kind": "full" - })) + .diagnostic(path) + .expect_response(expected_stdlib_diagnostic(path)) .unwrap(); interaction.shutdown().unwrap(); @@ -1350,6 +1329,7 @@ fn test_shows_stdlib_errors_when_explicitly_included_in_project_includes() { interaction.client.did_open(stdlib_filepath); + let stdlib_uri = Url::from_file_path(test_files_root.path().join(stdlib_filepath)).unwrap(); interaction .client .diagnostic(stdlib_filepath) @@ -1365,6 +1345,16 @@ fn test_shows_stdlib_errors_when_explicitly_included_in_project_includes() { "end": {"character": 12, "line": 5}, "start": {"character": 9, "line": 5} }, + "relatedInformation": [{ + "location": { + "range": { + "end": {"character": 6, "line": 5}, + "start": {"character": 3, "line": 5} + }, + "uri": stdlib_uri.as_str() + }, + "message": "declared type" + }], "severity": 1, "source": "Pyrefly" } diff --git a/pyrefly/lib/test/lsp/lsp_interaction/notebook_sync.rs b/pyrefly/lib/test/lsp/lsp_interaction/notebook_sync.rs index 8c9e596e95..44d177fac9 100644 --- a/pyrefly/lib/test/lsp/lsp_interaction/notebook_sync.rs +++ b/pyrefly/lib/test/lsp/lsp_interaction/notebook_sync.rs @@ -113,6 +113,7 @@ fn test_notebook_did_open() { .expect_response(json!({"items": [], "kind": "full"})) .unwrap(); // Cell 3 has an error + let notebook_uri = Url::from_file_path(root.path().join("notebook.ipynb")).unwrap(); interaction .diagnostic_for_cell("notebook.ipynb", "cell3") .expect_response(json!({ @@ -126,6 +127,16 @@ fn test_notebook_did_open() { "start": {"line": 1, "character": 4}, "end": {"line": 1, "character": 5} }, + "relatedInformation": [{ + "location": { + "range": { + "end": {"character": 6, "line": 0}, + "start": {"character": 3, "line": 0} + }, + "uri": notebook_uri.as_str() + }, + "message": "declared type" + }], "severity": 1, "source": "Pyrefly" }],