From c6055126d23441ea097e9692c86c16e6108eae24 Mon Sep 17 00:00:00 2001 From: Pierre Tardy Date: Mon, 12 Aug 2024 17:06:14 +0200 Subject: [PATCH 1/3] fix: add spans for import errors --- protox/src/compile/mod.rs | 10 ++- protox/src/error.rs | 78 +++++++++++++++++-- protox/tests/compiler.rs | 6 +- .../snapshots/compiler__import_not_found.snap | 7 +- 4 files changed, 87 insertions(+), 14 deletions(-) diff --git a/protox/src/compile/mod.rs b/protox/src/compile/mod.rs index be51484..3e1057c 100644 --- a/protox/src/compile/mod.rs +++ b/protox/src/compile/mod.rs @@ -148,8 +148,9 @@ impl Compiler { } let mut import_stack = vec![name.clone()]; - for import in &file.descriptor.dependency { - self.add_import(import, &mut import_stack)?; + for (i, import) in (&file.descriptor.dependency).iter().enumerate() { + self.add_import(import, &mut import_stack) + .map_err(|e| e.into_import_error(&file, i))?; } drop(import_stack); @@ -269,8 +270,9 @@ impl Compiler { let file = self.resolver.open_file(file_name)?; import_stack.push(file_name.to_owned()); - for import in &file.descriptor.dependency { - self.add_import(import, import_stack)?; + for (i, import) in (&file.descriptor.dependency).iter().enumerate() { + self.add_import(import, import_stack) + .map_err(|e| e.into_import_error(&file, i))?; } import_stack.pop(); diff --git a/protox/src/error.rs b/protox/src/error.rs index 5515ebc..b9345fd 100644 --- a/protox/src/error.rs +++ b/protox/src/error.rs @@ -1,10 +1,12 @@ use std::{fmt, io, path::PathBuf}; -use miette::Diagnostic; +use miette::{Diagnostic, NamedSource, SourceCode, SourceOffset, SourceSpan}; use prost_reflect::DescriptorError; use protox_parse::ParseError; use thiserror::Error; +use crate::file::File; + /// An error that can occur when compiling protobuf files. #[derive(Diagnostic, Error)] #[error(transparent)] @@ -33,8 +35,16 @@ pub(crate) enum ErrorKind { FileTooLarge { name: String }, #[error("file '{name}' is not valid utf-8")] FileInvalidUtf8 { name: String }, + #[error("file '{name}' not found")] + FileNotFound { name: String }, #[error("import '{name}' not found")] - ImportNotFound { name: String }, + ImportNotFound { + #[label("imported here")] + span: Option, + #[source_code] + source_code: NamedSource, + name: String, + }, #[error("import cycle detected: {cycle}")] CircularImport { name: String, cycle: String }, #[error("file '{path}' is not in any include path")] @@ -63,7 +73,7 @@ impl Error { /// /// This error should be returned by [`FileResolver`](crate::file::FileResolver) instances if a file is not found. pub fn file_not_found(name: &str) -> Self { - Error::from_kind(ErrorKind::ImportNotFound { + Error::from_kind(ErrorKind::FileNotFound { name: name.to_owned(), }) } @@ -76,11 +86,12 @@ impl Error { ErrorKind::OpenFile { name, .. } | ErrorKind::FileTooLarge { name } | ErrorKind::FileInvalidUtf8 { name } - | ErrorKind::ImportNotFound { name } + | ErrorKind::FileNotFound { name } | ErrorKind::CircularImport { name, .. } | ErrorKind::FileShadowed { name, .. } => Some(name), ErrorKind::FileNotIncluded { .. } => None, ErrorKind::Custom(_) => None, + ErrorKind::ImportNotFound { source_code, .. } => Some(source_code.name()), } } @@ -99,7 +110,9 @@ impl Error { pub fn is_file_not_found(&self) -> bool { matches!( &*self.kind, - ErrorKind::ImportNotFound { .. } | ErrorKind::FileNotIncluded { .. } + ErrorKind::FileNotFound { .. } + | ErrorKind::ImportNotFound { .. } + | ErrorKind::FileNotIncluded { .. } ) } @@ -121,6 +134,43 @@ impl Error { _ => false, } } + + pub(crate) fn into_import_error(self, file: &File, import_idx: usize) -> Self { + fn find_span(file: &File, import_idx: usize) -> Option { + if let Some(sci) = &file.descriptor.source_code_info { + if let Some(source) = file.source() { + for location in &sci.location { + if location.path == vec![3, import_idx as i32] { + if location.span.len() != 3 { + continue; + } + let start_line = *location.span.get(0)? as usize + 1; + let start_col = *location.span.get(1)? as usize + 1 ; + let end_col = *location.span.get(2)? as usize + 1; + return Some(SourceSpan::new( + SourceOffset::from_location(source, start_line, start_col), + end_col - start_col, + )); + } + } + } + } + None + } + match *self.kind { + ErrorKind::FileNotFound { name } => { + let source_code: NamedSource = + NamedSource::new(file.name(), file.source().or(Some("")).unwrap().to_string()); + let span = find_span(file, import_idx); + Error::from_kind(ErrorKind::ImportNotFound { + span, + source_code, + name, + }) + } + _ => self, + } + } } impl From for Error { @@ -149,11 +199,27 @@ impl fmt::Debug for Error { ErrorKind::OpenFile { err, .. } => write!(f, "{}: {}", self, err), ErrorKind::FileTooLarge { .. } | ErrorKind::FileInvalidUtf8 { .. } - | ErrorKind::ImportNotFound { .. } + | ErrorKind::FileNotFound { .. } | ErrorKind::CircularImport { .. } | ErrorKind::FileNotIncluded { .. } | ErrorKind::FileShadowed { .. } => write!(f, "{}", self), ErrorKind::Custom(err) => err.fmt(f), + ErrorKind::ImportNotFound { + span, source_code, .. + } => { + write!(f, "{}:", source_code.name())?; + if let Some(span) = span { + if let Ok(span_contents) = source_code.read_span(span.into(), 0, 0) { + write!( + f, + "{}:{}: ", + span_contents.line() + 1, + span_contents.column() + 1 + )?; + } + } + write!(f, "{}", self) + } } } } diff --git a/protox/tests/compiler.rs b/protox/tests/compiler.rs index 48514fb..3e01f51 100644 --- a/protox/tests/compiler.rs +++ b/protox/tests/compiler.rs @@ -309,7 +309,7 @@ fn pass_through_extension_options() { fn error_fmt_debug() { let parse_err = check(&[("root.proto", "message {")]).unwrap_err(); let check_err = check(&[("root.proto", "message Foo {} service Foo {}")]).unwrap_err(); - let import_err = check(&[("root.proto", "import 'notfound.proto';")]).unwrap_err(); + let import_err = check(&[("root.proto", "// comment \nimport 'notfound.proto';")]).unwrap_err(); let open_err = check(&[("root.proto", "import 'customerror.proto';")]).unwrap_err(); assert!(parse_err.is_parse()); @@ -332,11 +332,11 @@ fn error_fmt_debug() { ); assert!(import_err.is_file_not_found()); - assert_eq!(import_err.file(), Some("notfound.proto")); + assert_eq!(import_err.file(), Some("root.proto")); assert_eq!(import_err.to_string(), "import 'notfound.proto' not found"); assert_eq!( format!("{:?}", import_err), - "import 'notfound.proto' not found" + "root.proto:2:1: import 'notfound.proto' not found" ); assert!(open_err.is_io()); diff --git a/protox/tests/snapshots/compiler__import_not_found.snap b/protox/tests/snapshots/compiler__import_not_found.snap index 248c101..b7767e1 100644 --- a/protox/tests/snapshots/compiler__import_not_found.snap +++ b/protox/tests/snapshots/compiler__import_not_found.snap @@ -3,7 +3,12 @@ source: protox/tests/compiler.rs expression: "check_err(&[(\"root.proto\", \"import 'notfound.proto';\")])" --- causes: [] -labels: [] +filename: root.proto +labels: + - label: imported here + span: + length: 24 + offset: 0 message: "import 'notfound.proto' not found" related: [] severity: error From f03364ebb2eb0fc6360e28d01cbafddce710d52f Mon Sep 17 00:00:00 2001 From: Andrew Hickman Date: Thu, 15 Aug 2024 22:54:37 +0100 Subject: [PATCH 2/3] cargo fmt --- protox/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protox/src/error.rs b/protox/src/error.rs index b9345fd..f0f2cec 100644 --- a/protox/src/error.rs +++ b/protox/src/error.rs @@ -145,7 +145,7 @@ impl Error { continue; } let start_line = *location.span.get(0)? as usize + 1; - let start_col = *location.span.get(1)? as usize + 1 ; + let start_col = *location.span.get(1)? as usize + 1; let end_col = *location.span.get(2)? as usize + 1; return Some(SourceSpan::new( SourceOffset::from_location(source, start_line, start_col), From c596c6eaaab3bed0005e8fbe7c35479feaff27c0 Mon Sep 17 00:00:00 2001 From: Andrew Hickman Date: Thu, 15 Aug 2024 23:19:12 +0100 Subject: [PATCH 3/3] Fix lints --- protox/src/compile/mod.rs | 4 ++-- protox/src/error.rs | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/protox/src/compile/mod.rs b/protox/src/compile/mod.rs index 3e1057c..b1d6db7 100644 --- a/protox/src/compile/mod.rs +++ b/protox/src/compile/mod.rs @@ -148,7 +148,7 @@ impl Compiler { } let mut import_stack = vec![name.clone()]; - for (i, import) in (&file.descriptor.dependency).iter().enumerate() { + for (i, import) in file.descriptor.dependency.iter().enumerate() { self.add_import(import, &mut import_stack) .map_err(|e| e.into_import_error(&file, i))?; } @@ -270,7 +270,7 @@ impl Compiler { let file = self.resolver.open_file(file_name)?; import_stack.push(file_name.to_owned()); - for (i, import) in (&file.descriptor.dependency).iter().enumerate() { + for (i, import) in file.descriptor.dependency.iter().enumerate() { self.add_import(import, import_stack) .map_err(|e| e.into_import_error(&file, i))?; } diff --git a/protox/src/error.rs b/protox/src/error.rs index f0f2cec..8a31736 100644 --- a/protox/src/error.rs +++ b/protox/src/error.rs @@ -144,9 +144,9 @@ impl Error { if location.span.len() != 3 { continue; } - let start_line = *location.span.get(0)? as usize + 1; - let start_col = *location.span.get(1)? as usize + 1; - let end_col = *location.span.get(2)? as usize + 1; + let start_line = location.span[0] as usize + 1; + let start_col = location.span[1] as usize + 1; + let end_col = location.span[2] as usize + 1; return Some(SourceSpan::new( SourceOffset::from_location(source, start_line, start_col), end_col - start_col, @@ -160,7 +160,7 @@ impl Error { match *self.kind { ErrorKind::FileNotFound { name } => { let source_code: NamedSource = - NamedSource::new(file.name(), file.source().or(Some("")).unwrap().to_string()); + NamedSource::new(file.name(), file.source().unwrap_or_default().to_owned()); let span = find_span(file, import_idx); Error::from_kind(ErrorKind::ImportNotFound { span, @@ -209,7 +209,7 @@ impl fmt::Debug for Error { } => { write!(f, "{}:", source_code.name())?; if let Some(span) = span { - if let Ok(span_contents) = source_code.read_span(span.into(), 0, 0) { + if let Ok(span_contents) = source_code.read_span(span, 0, 0) { write!( f, "{}:{}: ",