From 293dffcb8a064b44d762a9b5dfde3a9e862ab9a8 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Wed, 6 Sep 2023 20:06:49 +0100 Subject: [PATCH] chore: Remove reliance on `FileManager` in `noirc_errors` (#2580) --- crates/fm/src/file_map.rs | 28 +++++++++++++++----- crates/fm/src/lib.rs | 4 +++ crates/lsp/src/lib.rs | 19 +++++++------- crates/nargo_cli/src/cli/compile_cmd.rs | 4 +-- crates/nargo_cli/src/cli/execute_cmd.rs | 2 +- crates/nargo_cli/src/cli/test_cmd.rs | 2 +- crates/noirc_errors/src/reporter.rs | 34 +++++++++++++++---------- 7 files changed, 59 insertions(+), 34 deletions(-) diff --git a/crates/fm/src/file_map.rs b/crates/fm/src/file_map.rs index 22ac6d4e179..199723a28b5 100644 --- a/crates/fm/src/file_map.rs +++ b/crates/fm/src/file_map.rs @@ -1,7 +1,6 @@ -use crate::FileManager; -use codespan_reporting::files::{SimpleFile, SimpleFiles}; +use codespan_reporting::files::{Error, Files, SimpleFile, SimpleFiles}; use serde::{Deserialize, Serialize}; -use std::path::PathBuf; +use std::{ops::Range, path::PathBuf}; // XXX: File and FileMap serve as opaque types, so that the rest of the library does not need to import the dependency // or worry about when we change the dep @@ -74,9 +73,24 @@ impl Default for FileMap { } } -impl FileManager { - // Needed as code_span dep requires underlying SimpleFiles - pub fn as_simple_files(&self) -> &SimpleFiles { - &self.file_map.0 +impl<'a> Files<'a> for FileMap { + type FileId = FileId; + type Name = PathString; + type Source = &'a str; + + fn name(&self, file_id: Self::FileId) -> Result { + Ok(self.0.get(file_id.as_usize())?.name().clone()) + } + + fn source(&'a self, file_id: Self::FileId) -> Result { + Ok(self.0.get(file_id.as_usize())?.source().as_ref()) + } + + fn line_index(&self, file_id: Self::FileId, byte_index: usize) -> Result { + self.0.get(file_id.as_usize())?.line_index((), byte_index) + } + + fn line_range(&self, file_id: Self::FileId, line_index: usize) -> Result, Error> { + self.0.get(file_id.as_usize())?.line_range((), line_index) } } diff --git a/crates/fm/src/lib.rs b/crates/fm/src/lib.rs index ef976a80dbd..41a9d9ed0a8 100644 --- a/crates/fm/src/lib.rs +++ b/crates/fm/src/lib.rs @@ -37,6 +37,10 @@ impl FileManager { } } + pub fn as_file_map(&self) -> &FileMap { + &self.file_map + } + pub fn add_file(&mut self, file_name: &Path) -> Option { // Handle both relative file paths and std/lib virtual paths. let resolved_path: PathBuf = if is_stdlib_asset(file_name) { diff --git a/crates/lsp/src/lib.rs b/crates/lsp/src/lib.rs index a1bef82e4ca..a67b8a93dc8 100644 --- a/crates/lsp/src/lib.rs +++ b/crates/lsp/src/lib.rs @@ -195,7 +195,7 @@ fn on_code_lens_request( let _ = check_crate(&mut context, crate_id, false); let fm = &context.file_manager; - let files = fm.as_simple_files(); + let files = fm.as_file_map(); let tests = context .get_all_test_functions_in_crate_matching(&crate_id, FunctionNameMatch::Anything); @@ -211,8 +211,8 @@ fn on_code_lens_request( continue; } - let range = byte_span_to_range(files, file_id.as_usize(), location.span.into()) - .unwrap_or_default(); + let range = + byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); let test_command = Command { title: format!("{ARROW} {TEST_CODELENS_TITLE}"), @@ -245,8 +245,8 @@ fn on_code_lens_request( continue; } - let range = byte_span_to_range(files, file_id.as_usize(), location.span.into()) - .unwrap_or_default(); + let range = + byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); let compile_command = Command { title: format!("{ARROW} {COMPILE_CODELENS_TITLE}"), @@ -294,8 +294,8 @@ fn on_code_lens_request( continue; } - let range = byte_span_to_range(files, file_id.as_usize(), location.span.into()) - .unwrap_or_default(); + let range = + byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); let compile_command = Command { title: format!("{ARROW} {COMPILE_CODELENS_TITLE}"), @@ -417,7 +417,7 @@ fn on_did_save_text_document( if !file_diagnostics.is_empty() { let fm = &context.file_manager; - let files = fm.as_simple_files(); + let files = fm.as_file_map(); for FileDiagnostic { file_id, diagnostic, call_stack: _ } in file_diagnostics { // Ignore diagnostics for any file that wasn't the file we saved @@ -433,8 +433,7 @@ fn on_did_save_text_document( // TODO: Should this be the first item in secondaries? Should we bail when we find a range? for sec in diagnostic.secondaries { // Not using `unwrap_or_default` here because we don't want to overwrite a valid range with a default range - if let Some(r) = byte_span_to_range(files, file_id.as_usize(), sec.span.into()) - { + if let Some(r) = byte_span_to_range(files, file_id, sec.span.into()) { range = r } } diff --git a/crates/nargo_cli/src/cli/compile_cmd.rs b/crates/nargo_cli/src/cli/compile_cmd.rs index 4ceefdaf8b8..dca8c3a89d8 100644 --- a/crates/nargo_cli/src/cli/compile_cmd.rs +++ b/crates/nargo_cli/src/cli/compile_cmd.rs @@ -229,9 +229,9 @@ pub(crate) fn report_errors( deny_warnings: bool, ) -> Result { let (t, warnings) = result.map_err(|errors| { - noirc_errors::reporter::report_all(file_manager, &errors, deny_warnings) + noirc_errors::reporter::report_all(file_manager.as_file_map(), &errors, deny_warnings) })?; - noirc_errors::reporter::report_all(file_manager, &warnings, deny_warnings); + noirc_errors::reporter::report_all(file_manager.as_file_map(), &warnings, deny_warnings); Ok(t) } diff --git a/crates/nargo_cli/src/cli/execute_cmd.rs b/crates/nargo_cli/src/cli/execute_cmd.rs index 26662dc31cf..0c9ff31f3fb 100644 --- a/crates/nargo_cli/src/cli/execute_cmd.rs +++ b/crates/nargo_cli/src/cli/execute_cmd.rs @@ -173,7 +173,7 @@ fn report_error_with_opcode_locations( CustomDiagnostic::simple_error(message, String::new(), location.span) .in_file(location.file) .with_call_stack(source_locations) - .report(file_manager, false); + .report(file_manager.as_file_map(), false); } } } diff --git a/crates/nargo_cli/src/cli/test_cmd.rs b/crates/nargo_cli/src/cli/test_cmd.rs index 1b4f37a4528..78e3b4c16a0 100644 --- a/crates/nargo_cli/src/cli/test_cmd.rs +++ b/crates/nargo_cli/src/cli/test_cmd.rs @@ -117,7 +117,7 @@ fn run_tests( } TestStatus::CompileError(err) => { noirc_errors::reporter::report_all( - &context.file_manager, + context.file_manager.as_file_map(), &[err], compile_options.deny_warnings, ); diff --git a/crates/noirc_errors/src/reporter.rs b/crates/noirc_errors/src/reporter.rs index 65cd0a93279..0a021af61f8 100644 --- a/crates/noirc_errors/src/reporter.rs +++ b/crates/noirc_errors/src/reporter.rs @@ -1,5 +1,6 @@ use crate::{FileDiagnostic, Location, Span}; use codespan_reporting::diagnostic::{Diagnostic, Label}; +use codespan_reporting::files::Files; use codespan_reporting::term; use codespan_reporting::term::termcolor::{ColorChoice, StandardStream}; @@ -110,8 +111,8 @@ impl CustomLabel { /// Writes the given diagnostics to stderr and returns the count /// of diagnostics that were errors. -pub fn report_all( - files: &fm::FileManager, +pub fn report_all<'files>( + files: &'files impl Files<'files, FileId = fm::FileId>, diagnostics: &[FileDiagnostic], deny_warnings: bool, ) -> ReportedErrors { @@ -126,14 +127,18 @@ pub fn report_all( } impl FileDiagnostic { - pub fn report(&self, files: &fm::FileManager, deny_warnings: bool) -> bool { + pub fn report<'files>( + &self, + files: &'files impl Files<'files, FileId = fm::FileId>, + deny_warnings: bool, + ) -> bool { report(files, &self.diagnostic, Some(self.file_id), &self.call_stack, deny_warnings) } } /// Report the given diagnostic, and return true if it was an error -pub fn report( - files: &fm::FileManager, +pub fn report<'files>( + files: &'files impl Files<'files, FileId = fm::FileId>, custom_diagnostic: &CustomDiagnostic, file: Option, call_stack: &[Location], @@ -144,7 +149,7 @@ pub fn report( let stack_trace = stack_trace(files, call_stack); let diagnostic = convert_diagnostic(custom_diagnostic, file, stack_trace, deny_warnings); - term::emit(&mut writer.lock(), &config, files.as_simple_files(), &diagnostic).unwrap(); + term::emit(&mut writer.lock(), &config, files, &diagnostic).unwrap(); deny_warnings || custom_diagnostic.is_error() } @@ -154,7 +159,7 @@ fn convert_diagnostic( file: Option, stack_trace: String, deny_warnings: bool, -) -> Diagnostic { +) -> Diagnostic { let diagnostic = match (cd.kind, deny_warnings) { (DiagnosticKind::Warning, false) => Diagnostic::warning(), _ => Diagnostic::error(), @@ -166,7 +171,7 @@ fn convert_diagnostic( .map(|sl| { let start_span = sl.span.start() as usize; let end_span = sl.span.end() as usize; - Label::secondary(file_id.as_usize(), start_span..end_span).with_message(&sl.message) + Label::secondary(file_id, start_span..end_span).with_message(&sl.message) }) .collect() } else { @@ -179,7 +184,10 @@ fn convert_diagnostic( diagnostic.with_message(&cd.message).with_labels(secondary_labels).with_notes(notes) } -fn stack_trace(files: &fm::FileManager, call_stack: &[Location]) -> String { +fn stack_trace<'files>( + files: &'files impl Files<'files, FileId = fm::FileId>, + call_stack: &[Location], +) -> String { if call_stack.is_empty() { return String::new(); } @@ -187,11 +195,11 @@ fn stack_trace(files: &fm::FileManager, call_stack: &[Location]) -> String { let mut result = "Call stack:\n".to_string(); for (i, call_item) in call_stack.iter().enumerate() { - let path = files.path(call_item.file); - let source = files.fetch_file(call_item.file).source(); + let path = files.name(call_item.file).expect("should get file path"); + let source = files.source(call_item.file).expect("should get file source"); - let (line, column) = location(source, call_item.span.start()); - result += &format!("{}. {}.nr:{}:{}\n", i + 1, path.display(), line, column); + let (line, column) = location(source.as_ref(), call_item.span.start()); + result += &format!("{}. {}.nr:{}:{}\n", i + 1, path.to_string(), line, column); } result