Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add spans for import errors #80

Merged
merged 3 commits into from
Aug 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions protox/src/compile/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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();

Expand Down
78 changes: 72 additions & 6 deletions protox/src/error.rs
Original file line number Diff line number Diff line change
@@ -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)]
Expand Down Expand Up @@ -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<SourceSpan>,
#[source_code]
source_code: NamedSource<String>,
name: String,
},
#[error("import cycle detected: {cycle}")]
CircularImport { name: String, cycle: String },
#[error("file '{path}' is not in any include path")]
Expand Down Expand Up @@ -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(),
})
}
Expand All @@ -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()),
}
}

Expand All @@ -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 { .. }
)
}

Expand All @@ -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<SourceSpan> {
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[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,
));
}
}
}
}
None
}
match *self.kind {
ErrorKind::FileNotFound { name } => {
let source_code: NamedSource<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,
source_code,
name,
})
}
_ => self,
}
}
}

impl From<DescriptorError> for Error {
Expand Down Expand Up @@ -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, 0, 0) {
write!(
f,
"{}:{}: ",
span_contents.line() + 1,
span_contents.column() + 1
)?;
}
}
write!(f, "{}", self)
}
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions protox/tests/compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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());
Expand Down
7 changes: 6 additions & 1 deletion protox/tests/snapshots/compiler__import_not_found.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading