add file_id to diagnostic RelatedInformation

alanz · meta-codesync[bot] · commit dcf4e3a4ffca · 2025-11-19T03:56:10.000-08:00
Summary:
A given diagnostic can have related information in it, which carries relevant context to the provided diagnostic.
For example, for a head mismatch diagnostic, it reports the specific head location(s) that do not match.

The LSP spec uses a `Location` for these, which includes a file URL, meaning the related info can be in a different file from where the diagnostic is reported.

In ELP, we currently do not provide a way of populating this information, so can only refer to related information within the same file.  Thic can result in erroneous outcomes.

## This diff
We include a `FileId` in our internal `RelatedInformation` structure, and map it to a URL when serializing for transmission to the LSP client

Reviewed By: robertoaloi

Differential Revision: D87316542

fbshipit-source-id: 5f94eb4fe5bb73e7bf1fff7cad458231c51bd6f0
diff --git a/crates/elp/src/bin/elp_parse_cli.rs b/crates/elp/src/bin/elp_parse_cli.rs
@@ -24,7 +24,6 @@ use elp::cli::Cli;
 use elp::convert;
 use elp::memory_usage::MemoryUsage;
 use elp::otp_file_to_ignore;
-use elp::server::file_id_to_url;
 use elp_eqwalizer::Mode;
 use elp_ide::Analysis;
 use elp_ide::diagnostics;
@@ -132,8 +131,7 @@ pub fn parse_all(
         (None, _, true) => do_parse_all_seq(cli, &loaded, &cfg, &args.to)?,
         (None, _, false) => do_parse_all_par(cli, &loaded, &cfg, &args.to)?,
         (Some(file_id), Some(name), _) => {
-            do_parse_one(&analysis, &loaded.vfs, &cfg, &args.to, file_id, &name)?
-                .map_or(vec![], |x| vec![x])
+            do_parse_one(&analysis, &cfg, &args.to, file_id, &name)?.map_or(vec![], |x| vec![x])
         }
         (Some(file_id), _, _) => panic!("Could not get name from file_id for {file_id:?}"),
     };
@@ -144,10 +142,6 @@ pub fn parse_all(
 
     let db = loaded.analysis_host.raw_database();
 
-    // We need a `Url` for converting to the lsp_types::Diagnostic for
-    // printing, but do not print it out. So just create a dummy value
-    let url = lsp_types::Url::parse("file:///unused_url").ok().unwrap();
-
     telemetry::report_elapsed_time("parse-elp operational", start_time);
 
     let memory_end = MemoryUsage::now();
@@ -197,7 +191,7 @@ pub fn parse_all(
                             cli,
                         )?;
                     } else {
-                        print_diagnostic(&diag, &line_index, &url, &mut err_in_diag, cli)?;
+                        print_diagnostic(&diag, &line_index, &mut err_in_diag, cli)?;
                     }
                 }
             }
@@ -242,11 +236,10 @@ fn print_diagnostic_json(
 fn print_diagnostic(
     diag: &diagnostics::Diagnostic,
     line_index: &LineIndex,
-    url: &lsp_types::Url,
     err_in_diag: &mut bool,
     cli: &mut dyn Cli,
 ) -> Result<(), anyhow::Error> {
-    let diag = convert::ide_to_lsp_diagnostic(line_index, url, diag);
+    let diag = convert::ide_to_lsp_diagnostic(line_index, diag, |_file_id| None);
     let severity = match diag.severity {
         None => DiagnosticSeverity::ERROR,
         Some(sev) => {
@@ -289,7 +282,6 @@ fn do_parse_all_par(
 
     let pb = cli.progress(module_iter.len() as u64, "Parsing modules");
 
-    let vfs = &loaded.vfs;
     Ok(module_iter
         .par_bridge()
         .progress_with(pb)
@@ -300,7 +292,7 @@ fn do_parse_all_par(
                     && file_source == FileSource::Src
                     && db.file_app_type(file_id).ok() != Some(Some(AppType::Dep))
                 {
-                    do_parse_one(db, vfs, config, to, file_id, module_name.as_str()).unwrap()
+                    do_parse_one(db, config, to, file_id, module_name.as_str()).unwrap()
                 } else {
                     None
                 }
@@ -321,7 +313,6 @@ fn do_parse_all_seq(
 
     let pb = cli.progress(module_iter.len() as u64, "Parsing modules (sequential)");
 
-    let vfs = &loaded.vfs;
     let db = loaded.analysis();
     Ok(module_iter
         .progress_with(pb)
@@ -330,7 +321,7 @@ fn do_parse_all_seq(
                 && file_source == FileSource::Src
                 && db.file_app_type(file_id).ok() != Some(Some(AppType::Dep))
             {
-                do_parse_one(&db, vfs, config, to, file_id, module_name.as_str()).unwrap()
+                do_parse_one(&db, config, to, file_id, module_name.as_str()).unwrap()
             } else {
                 None
             }
@@ -340,13 +331,11 @@ fn do_parse_all_seq(
 
 fn do_parse_one(
     db: &Analysis,
-    vfs: &Vfs,
     config: &DiagnosticsConfig,
     to: &Option<PathBuf>,
     file_id: FileId,
     name: &str,
 ) -> Result<Option<ParseResult>> {
-    let url = file_id_to_url(vfs, file_id);
     let native = db.native_diagnostics(config, &vec![], file_id)?;
     let erlang_service_diagnostics =
         db.erlang_service_diagnostics(file_id, config, RemoveElpReported::Yes)?;
@@ -364,11 +353,13 @@ fn do_parse_one(
         let mut output = File::create(to_path)?;
 
         for diagnostic in native.iter() {
-            let diagnostic = convert::ide_to_lsp_diagnostic(&line_index, &url, diagnostic);
+            let diagnostic =
+                convert::ide_to_lsp_diagnostic(&line_index, diagnostic, |_file_id| None);
             writeln!(output, "{diagnostic:?}")?;
         }
         for diagnostic in erlang_service.iter() {
-            let diagnostic = convert::ide_to_lsp_diagnostic(&line_index, &url, diagnostic);
+            let diagnostic =
+                convert::ide_to_lsp_diagnostic(&line_index, diagnostic, |_file_id| None);
             writeln!(output, "{diagnostic:?}")?;
         }
     }
diff --git a/crates/elp/src/convert.rs b/crates/elp/src/convert.rs
@@ -26,6 +26,7 @@ use elp_ide::elp_ide_db::assists::AssistContextDiagnostic;
 use elp_ide::elp_ide_db::assists::AssistContextDiagnosticCode;
 use elp_ide::elp_ide_db::elp_base_db::AbsPath;
 use elp_ide::elp_ide_db::elp_base_db::AbsPathBuf;
+use elp_ide::elp_ide_db::elp_base_db::FileId;
 use elp_ide::elp_ide_db::elp_base_db::VfsPath;
 use lsp_types::DiagnosticRelatedInformation;
 use lsp_types::Location;
@@ -67,11 +68,14 @@ pub fn diagnostic_severity(severity: Severity) -> lsp_types::DiagnosticSeverity
     }
 }
 
-pub fn ide_to_lsp_diagnostic(
+pub fn ide_to_lsp_diagnostic<F>(
     line_index: &LineIndex,
-    url: &Url,
     d: &Diagnostic,
-) -> lsp_types::Diagnostic {
+    get_file_info: F,
+) -> lsp_types::Diagnostic
+where
+    F: Fn(FileId) -> Option<(LineIndex, Url)>,
+{
     let code_description = match &d.code_doc_uri {
         Some(uri) => match lsp_types::Url::parse(uri) {
             Ok(href) => Some(lsp_types::CodeDescription { href }),
@@ -90,7 +94,7 @@ pub fn ide_to_lsp_diagnostic(
         code_description,
         source,
         message: d.message.clone(),
-        related_information: from_related(line_index, url, &d.related_info),
+        related_information: from_related(get_file_info, &d.related_info),
         tags: d.tag.as_ref().map(lsp_diagnostic_tags),
         data: None,
     }
@@ -165,22 +169,26 @@ pub fn eqwalizer_to_arc_diagnostic(
     )
 }
 
-fn from_related(
-    line_index: &LineIndex,
-    url: &Url,
+fn from_related<F>(
+    get_file_info: F,
     r: &Option<Vec<RelatedInformation>>,
-) -> Option<Vec<DiagnosticRelatedInformation>> {
+) -> Option<Vec<DiagnosticRelatedInformation>>
+where
+    F: Fn(elp_ide::elp_ide_db::elp_base_db::FileId) -> Option<(LineIndex, Url)>,
+{
     r.as_ref().map(|ri| {
         ri.iter()
-            .map(|i| {
+            .filter_map(|i| {
+                // Get the line index and URL for the file that contains the related information
+                let (line_index, uri) = get_file_info(i.file_id)?;
                 let location = Location {
-                    range: range(line_index, i.range),
-                    uri: url.clone(),
+                    range: range(&line_index, i.range),
+                    uri,
                 };
-                DiagnosticRelatedInformation {
+                Some(DiagnosticRelatedInformation {
                     location,
                     message: i.message.clone(),
-                }
+                })
             })
             .collect()
     })
diff --git a/crates/elp/src/server.rs b/crates/elp/src/server.rs
@@ -624,7 +624,16 @@ impl Server {
                     .diagnostics
                     .diagnostics_for(*file_id)
                     .iter()
-                    .map(|d| ide_to_lsp_diagnostic(&line_index, &url, d))
+                    .map(|d| {
+                        let vfs = self.vfs.clone();
+                        ide_to_lsp_diagnostic(&line_index, d, |related_file_id| {
+                            // Get the line index and URL for the related file
+                            let url = file_id_to_url(&vfs.read(), related_file_id);
+                            let snapshot = self.snapshot();
+                            let line_index = snapshot.analysis.line_index(related_file_id).ok()?;
+                            Some(((*line_index).clone(), url))
+                        })
+                    })
                     .collect();
 
                 self.send_notification::<notification::PublishDiagnostics>(
@@ -828,11 +837,20 @@ impl Server {
                         Arc::make_mut(&mut this.diagnostics)
                             .clear(file_id);
                         if let Ok(line_index) = analysis.line_index(file_id) {
+                            let vfs = this.vfs.clone();
                             diagnostics = this
                                 .diagnostics
                                 .diagnostics_for(file_id)
                                 .iter()
-                                .map(|d| ide_to_lsp_diagnostic(&line_index, &url, d))
+                                .map(|d| {
+                                    let snapshot = this.snapshot();
+                                    ide_to_lsp_diagnostic(&line_index, d, |related_file_id| {
+                                        // Get the line index and URL for the related file
+                                        let url = file_id_to_url(&vfs.read(), related_file_id);
+                                        let line_index = snapshot.analysis.line_index(related_file_id).ok()?;
+                                        Some(((*line_index).clone(), url))
+                                    })
+                                })
                                 .collect()
                         }
                     }
diff --git a/crates/ide/src/diagnostics.rs b/crates/ide/src/diagnostics.rs
@@ -227,8 +227,9 @@ impl Diagnostic {
         self
     }
 
-    pub(crate) fn as_related(&self) -> RelatedInformation {
+    pub(crate) fn as_related(&self, file_id: FileId) -> RelatedInformation {
         RelatedInformation {
+            file_id,
             range: self.range,
             message: self.message.clone(),
         }
@@ -447,6 +448,7 @@ impl Diagnostic {
 
 #[derive(Debug, Clone)]
 pub struct RelatedInformation {
+    pub file_id: FileId,
     pub range: TextRange,
     pub message: String,
 }
@@ -2818,6 +2820,7 @@ fn combine_syntax_errors(native: &Labeled, erlang_service: &Labeled) -> Labeled
 /// Combine the ELP and erlang_service diagnostics.  In particular,
 /// flatten any cascading diagnostics if possible.
 pub fn attach_related_diagnostics(
+    file_id: FileId,
     native: LabeledDiagnostics,
     erlang_service: LabeledDiagnostics,
 ) -> Vec<Diagnostic> {
@@ -2853,7 +2856,7 @@ pub fn attach_related_diagnostics(
         .flat_map(|(mfa_label, syntax_error_diags)| {
             if let Some(related) = erlang_service.labeled_undefined_errors.get(mfa_label) {
                 undefineds_to_remove.insert(mfa_label);
-                let related_info = related.iter().map(|d| d.as_related()).collect_vec();
+                let related_info = related.iter().map(|d| d.as_related(file_id)).collect_vec();
                 syntax_error_diags
                     .iter()
                     .map(|d| d.clone().with_related(Some(related_info.clone())))
diff --git a/crates/ide/src/diagnostics/head_mismatch.rs b/crates/ide/src/diagnostics/head_mismatch.rs
@@ -270,6 +270,7 @@ impl Validate<String> for Name {
             attr_loc,
         )
         .with_related(Some(vec![RelatedInformation {
+            file_id,
             range: ref_loc,
             message: "Mismatched clause name".to_string(),
         }]))
@@ -293,7 +294,7 @@ impl Validate<usize> for Arity {
 
     fn make_diagnostic(
         self,
-        _file_id: FileId,
+        file_id: FileId,
         attr: &usize,
         hattr: &usize,
         attr_loc: TextRange,
@@ -305,6 +306,7 @@ impl Validate<usize> for Arity {
             attr_loc,
         )
         .with_related(Some(vec![RelatedInformation {
+            file_id,
             range: ref_loc,
             message: "Mismatched clause".to_string(),
         }]))
diff --git a/crates/ide/src/diagnostics/misspelled_attribute.rs b/crates/ide/src/diagnostics/misspelled_attribute.rs
@@ -133,6 +133,7 @@ fn make_diagnostic(
         attr_name_range,
     )
     .with_related(Some(vec![RelatedInformation {
+        file_id,
         range: attr_name_range,
         message: "Misspelled attribute".to_string(),
     }]))
diff --git a/crates/ide/src/diagnostics_collection.rs b/crates/ide/src/diagnostics_collection.rs
@@ -104,7 +104,7 @@ impl DiagnosticCollection {
         let native = self.native.get(&file_id).unwrap_or(&empty_diags);
         let erlang_service = self.erlang_service.get(&file_id).unwrap_or(&empty_diags);
         let mut combined: Vec<Diagnostic> =
-            attach_related_diagnostics(native.clone(), erlang_service.clone());
+            attach_related_diagnostics(file_id, native.clone(), erlang_service.clone());
         let eqwalizer = self.eqwalizer.get(&file_id).into_iter().flatten().cloned();
         let eqwalizer_project = self
             .eqwalizer_project
@@ -348,7 +348,7 @@ mod tests {
             let file_id = *file_id;
             let diagnostics = diagnostics::native_diagnostics(&db, &config, &vec![], file_id);
 
-            let combined = attach_related_diagnostics(diagnostics, extra_diags.clone());
+            let combined = attach_related_diagnostics(file_id, diagnostics, extra_diags.clone());
             let expected = fixture.annotations_by_file_id(&file_id);
             let mut actual = combined
                 .into_iter()
diff --git a/crates/ide/src/tests.rs b/crates/ide/src/tests.rs
@@ -513,7 +513,8 @@ pub(crate) fn check_diagnostics_with_config_and_extra(
     for file_id in &fixture.files {
         let file_id = *file_id;
         let diagnostics = diagnostics::native_diagnostics(&analysis.db, &config, &vec![], file_id);
-        let diagnostics = diagnostics::attach_related_diagnostics(diagnostics, extra_diags.clone());
+        let diagnostics =
+            diagnostics::attach_related_diagnostics(file_id, diagnostics, extra_diags.clone());
 
         let expected = fixture.annotations_by_file_id(&file_id);
         let actual = convert_diagnostics_to_annotations(diagnostics);

Original file line number	Diff line number	Diff line change
`@@ -133,6 +133,7 @@ fn make_diagnostic(`
`133`	`133`	`attr_name_range,`
`134`	`134`	`)`
`135`	`135`	`.with_related(Some(vec![RelatedInformation {`
	`136`	`+ file_id,`
`136`	`137`	`range: attr_name_range,`
`137`	`138`	`message: "Misspelled attribute".to_string(),`
`138`	`139`	`}]))`