Commit ee47726d authored by Alex Crichton's avatar Alex Crichton

Refactor explicit production of errors

I was reading over the gnome-class macro crate recently and I was having
a difficult time getting up to speed on the error handling of the crate
as it was using a number of patterns I hadn't seen before. I was also
wondering how only one `Span` was necessary to create an error when
multi-token spans were needed sometimes, which led me in the end to
start a refactor here.

The first point of refactoring was to ensure that multi-token errors
were handled. Although the `syn` crate has a `Spanned` trait it doesn't
actually work on stable Rust, so the errors coming out of gnome-class
today weren't compatible with being spanned over multiple tokens. I
added a convenience API in https://github.com/dtolnay/syn/pull/538 and
have started using that here to construct syn `Error` instances.

Next I opted to lift similar error handling from the `failure` crate
here as well. This was primarily the `bail!` macro as well as the
`format_err!` macro, used to quickly return and create errors with
formatted messages.

Using this macro I was able to replace most of the support traits in
`src/errors.rs` with local error handling and/or various other idioms.
Afterwards the crate (to me at least) feels much more idiomatic in terms
of error handling where `?` is still maximally used, returning an error
is easy to do, and there aren't too many traits/methods to discover when
learning how to work with errors.
parent 3e982566
......@@ -184,7 +184,7 @@ dependencies = [
"proc-macro2 0.4.19 (registry+https://github.com/rust-lang/crates.io-index)",
"quote 0.6.8 (registry+https://github.com/rust-lang/crates.io-index)",
"rustfmt 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)",
"syn 0.15.3 (registry+https://github.com/rust-lang/crates.io-index)",
"syn 0.15.21 (git+https://github.com/dtolnay/syn)",
"unicode-xid 0.0.4 (registry+https://github.com/rust-lang/crates.io-index)",
"xml-rs 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)",
]
......@@ -497,7 +497,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
"proc-macro2 0.4.19 (registry+https://github.com/rust-lang/crates.io-index)",
"quote 0.6.8 (registry+https://github.com/rust-lang/crates.io-index)",
"syn 0.15.3 (registry+https://github.com/rust-lang/crates.io-index)",
"syn 0.15.21 (git+https://github.com/dtolnay/syn)",
]
[[package]]
......@@ -530,8 +530,8 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]]
name = "syn"
version = "0.15.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
version = "0.15.21"
source = "git+https://github.com/dtolnay/syn#6bd3e03c3d1407d0b77f93844bea1ff26ba3a28c"
dependencies = [
"proc-macro2 0.4.19 (registry+https://github.com/rust-lang/crates.io-index)",
"quote 0.6.8 (registry+https://github.com/rust-lang/crates.io-index)",
......@@ -803,7 +803,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
"checksum shell-words 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "39acde55a154c4cd3ae048ac78cc21c25f3a0145e44111b523279113dce0d94a"
"checksum strings 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "aa481ee1bc42fc3df8195f91f7cb43cf8f2b71b48bac40bf5381cfaf7e481f3c"
"checksum strsim 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)" = "bb4f380125926a99e52bc279241539c018323fab05ad6368b56f93d9369ff550"
"checksum syn 0.15.3 (registry+https://github.com/rust-lang/crates.io-index)" = "e5c1514eb7bb4216fc722b3cd08783d326d7de0d62f6d5e48a774f610bc97cb6"
"checksum syn 0.15.21 (git+https://github.com/dtolnay/syn)" = "<none>"
"checksum syntex_errors 0.59.1 (registry+https://github.com/rust-lang/crates.io-index)" = "3133289179676c9f5c5b2845bf5a2e127769f4889fcbada43035ef6bd662605e"
"checksum syntex_pos 0.59.1 (registry+https://github.com/rust-lang/crates.io-index)" = "30ab669fa003d208c681f874bbc76d91cc3d32550d16b5d9d2087cf477316470"
"checksum syntex_syntax 0.59.1 (registry+https://github.com/rust-lang/crates.io-index)" = "03815b9f04d95828770d9c974aa39c6e1f6ef3114eb77a3ce09008a0d15dd142"
......
......@@ -28,6 +28,10 @@ unicode-xid = "0.0.4"
xml-rs = "0.8.0"
difference = "2.0" #Used to compare a generated content with a file on disk
[patch.crates-io]
# Needed for now until https://github.com/dtolnay/syn/pull/538 is published
syn = { git = 'https://github.com/dtolnay/syn' }
[build-dependencies]
cc = "1.0"
......
// use lalrpop_intern::InternedString;
use crate::parser::keywords;
use proc_macro2::Ident;
use quote::ToTokens;
use syn::punctuated::Punctuated;
use syn::spanned::Spanned;
use syn::token;
use syn::{Attribute, Lit};
use syn::{Block, FieldsNamed, FnArg, ImplItemType, Path, ReturnType, Type};
......@@ -92,12 +92,12 @@ pub enum ImplItemKind {
}
impl ImplItemKind {
pub fn ident_span(&self) -> ::proc_macro2::Span {
pub fn tokens(&self) -> &ToTokens {
match self {
ImplItemKind::Method(ImplItemMethod { ref name, .. }) => name.span(),
ImplItemKind::Prop(ImplProp { ref name, .. }) => name.span(),
ImplItemKind::ReserveSlots(ref p, ..) => p.span,
ImplItemKind::TraitType(ref l) => l.span(),
ImplItemKind::Method(m) => &m.name,
ImplItemKind::Prop(ImplProp { name, .. }) => name,
ImplItemKind::ReserveSlots(_, lit) => lit,
ImplItemKind::TraitType(l) => l,
}
}
}
......
use proc_macro2::{Span, TokenStream};
use proc_macro2::TokenStream;
use quote::ToTokens;
use std::fmt::Display;
use std::result::Result as StdResult;
pub use syn::parse::Error;
use syn::parse::Result as SynResult;
use syn::spanned::Spanned;
use syn::Lit;
/// Result type alias for an error type which allows for multiple errors
pub type Result<T> = StdResult<T, Errors>;
/// Convenient extension trait to generate an error for a given Span.
pub trait ToError {
/// Make an error object for a span and a input
fn to_error<E: Display>(&self, input: E) -> Error;
/// Make an result object in error state for a span and a input
fn to_err<T, E: Display>(&self, input: E) -> SynResult<T> {
Err(self.to_error(input))
macro_rules! bail {
($($args:tt)*) => {
return Err(format_err!($($args)*).into())
}
// Make an errors object (error object which can contain multiple errors) for a span and an
// input
fn to_errors<E: Display>(&self, input: E) -> Errors {
self.to_error(input).into()
}
// Make an result object in error state with an error type which can contain multiple errors
fn to_errs<T, E: Display>(&self, input: E) -> Result<T> {
Err(self.to_errors(input))
}
}
/// Extension trait for a result.
pub trait ErrorAppend {
fn extend_error<E: Into<Error>>(&mut self, err: E);
fn append_error<TOther, E: Into<Error>>(
&mut self,
other: StdResult<TOther, E>,
) -> Option<TOther>;
fn extend_errors<E: Into<Errors>>(&mut self, err: E);
/// Modifies the self-result. If other contains errors, those errors are append to the result
/// discarting to result if any.
fn append_errors<TOther, E: Into<Errors>>(
&mut self,
other: StdResult<TOther, E>,
) -> Option<TOther>;
}
impl<T> ErrorAppend for Result<T> {
fn extend_error<E: Into<Error>>(&mut self, err: E) {
match self {
Err(Errors(ref mut errvec)) => errvec.push(err.into()),
this @ Ok(_) => {
::std::mem::replace(this, Err(Errors::from(err.into()))).ok();
}
}
}
fn append_error<TOther, E: Into<Error>>(
&mut self,
other: StdResult<TOther, E>,
) -> Option<TOther> {
match other {
Ok(ok) => Some(ok),
Err(err) => {
self.extend_error(err);
None
}
}
}
fn extend_errors<E: Into<Errors>>(&mut self, err: E) {
match self {
Err(Errors(ref mut errvec)) => errvec.extend(err.into().0),
this @ Ok(_) => {
::std::mem::replace(this, Err(err.into())).ok();
}
}
}
fn append_errors<TOther, E: Into<Errors>>(
&mut self,
other: StdResult<TOther, E>,
) -> Option<TOther> {
match other {
Ok(ok) => Some(ok),
Err(err) => {
self.extend_errors(err);
None
macro_rules! format_err {
($tokens:expr, $($msg:tt)*) => {
match &$tokens {
t => {
::syn::parse::Error::new_spanned(t, format_args!($($msg)*))
}
}
}
}
impl ToError for Spanned {
fn to_error<E: Display>(&self, input: E) -> Error {
Error::new(self.span(), input)
}
}
impl ToError for ::proc_macro2::Ident {
fn to_error<E: Display>(&self, input: E) -> Error {
Error::new(self.span(), input)
}
}
impl<'ast> ToError for ::syn::Type {
fn to_error<E: Display>(&self, input: E) -> Error {
Error::new(self.span(), input)
}
}
impl<'ast> ToError for ::syn::Path {
fn to_error<E: Display>(&self, input: E) -> Error {
Error::new(self.span(), input)
}
}
impl<'ast> ToError for ::syn::LitStr {
fn to_error<E: Display>(&self, input: E) -> Error {
Error::new(self.span(), input)
}
}
impl ToError for Lit {
fn to_error<E: Display>(&self, val: E) -> Error {
Error::new(
match self {
Lit::Bool(s) => s.span,
Lit::Str(s) => s.span(),
Lit::ByteStr(s) => s.span(),
Lit::Byte(s) => s.span(),
Lit::Char(s) => s.span(),
Lit::Int(s) => s.span(),
Lit::Float(s) => s.span(),
Lit::Verbatim(s) => s.span(),
},
val,
)
}
}
impl ToError for Span {
fn to_error<E: Display>(&self, val: E) -> Error {
Error::new(*self, val)
}
}
#[derive(Debug)]
pub struct Errors(Vec<Error>);
......@@ -154,77 +31,11 @@ impl From<Error> for Errors {
}
}
impl Errors {
/// Create a new empty errors.
pub fn new() -> Self {
Errors(Vec::new())
}
/// Append errors to the list
pub fn append<Err: Into<Errors>>(&mut self, err: Err) {
self.0.extend(err.into().0);
}
/// Append the errors of the result to the current errors.
/// If the result was succesful, return the succesfull value as option
pub fn append_result<T, Err: Into<Errors>>(&mut self, result: StdResult<T, Err>) -> Option<T> {
match result {
Err(err) => {
self.0.extend(err.into().0);
None
}
Ok(ok) => Some(ok),
}
}
/// Add an error
pub fn push<Err: Into<Error>>(&mut self, err: Err) {
self.0.push(err.into());
}
/// Convert to a result
pub fn ok_if_empty(self) -> Result<()> {
if self.0.len() == 0 {
Ok(())
} else {
Err(self)
}
}
}
pub trait IterCollect<Item> {
fn try_for_each_errs<F>(self, func: F) -> Result<()>
where
F: FnMut(Item) -> Result<()>;
}
impl<Iter> IterCollect<Iter::Item> for Iter
where
Iter: Iterator,
{
fn try_for_each_errs<F>(self, mut func: F) -> Result<()>
where
F: FnMut(Iter::Item) -> Result<()>,
{
self.fold(Ok(()), |mut res: Result<()>, item: Iter::Item| {
res.append_errors(func(item));
res
})
}
}
pub trait TrySet<T> {
fn try_set(&mut self, set: T, err: Error) -> Result<()>;
}
impl<T> TrySet<T> for Option<T> {
fn try_set(&mut self, set: T, err: Error) -> Result<()> {
if self.is_none() {
::std::mem::replace(self, Some(set));
Ok(())
} else {
Err(err.into())
}
impl From<Vec<Errors>> for Errors {
fn from(e: Vec<Errors>) -> Errors {
let result = e.into_iter().flat_map(|v| v.0).collect::<Vec<_>>();
assert!(result.len() > 0);
Errors(result)
}
}
......
......@@ -20,8 +20,10 @@ pub fn generate(program: &Program) -> Result<()> {
}
pub fn generate_gir(class: &Class) -> Result<()> {
let mut writer = GeneratorWrite::from(&class.attrs.gir);
generate_gir_xml(class, &mut writer).map_err(|err| class.names.span().to_errors(err))?;
let mut writer = GeneratorWrite::from(&class.attrs.gir)?;
if let Err(err) = generate_gir_xml(class, &mut writer) {
bail!(class.names.instance(), "{}", err)
}
writer.collect_output()
}
......
use crate::errors::{ErrorAppend, Errors, Result, ToError};
use crate::errors::{Errors, Result};
use crate::hir::generatorattrs::{Generator, GeneratorAttributes};
use difference::{Changeset, Difference};
use proc_macro2::{Span, TokenStream};
use quote::ToTokens;
use rustfmt;
use std::fs::File;
use std::io::{Read, Result as IoResult, Write};
use std::fs::{self, File};
use std::io::{Result as IoResult, Write};
use std::iter::FromIterator;
use std::rc::Rc;
use syn::LitStr;
......@@ -15,34 +15,32 @@ use syn::LitStr;
pub struct GeneratorWrite {
generator: Option<File>,
comparer: Option<(Vec<u8>, LitStr)>,
output: Result<()>,
span: Span,
lit: LitStr,
}
impl GeneratorWrite {
/// Initialize from a Generator. The generator is normally constructed as part of an attribute
pub fn from(gen: &Generator) -> Self {
let span = gen
pub fn from(gen: &Generator) -> Result<Self> {
let lit = gen
.generate
.as_ref()
.or(gen.compare.as_ref())
.map(|lit| lit.span())
.unwrap_or(Span::call_site());
let mut output = Ok(());
.cloned()
.unwrap_or(LitStr::new("foo", Span::call_site()));
let generator = match &gen.generate {
Some(path) => {
output.append_errors(File::create(path.value()).map_err(|err| span.to_error(err)))
}
Some(path) => match File::create(path.value()) {
Ok(f) => Some(f),
Err(e) => bail!(lit, "{}", e),
},
None => None,
};
let comparer = gen.compare.as_ref().map(|path| (Vec::new(), path.clone()));
GeneratorWrite {
Ok(GeneratorWrite {
generator,
comparer,
output: Ok(()),
span,
}
lit,
})
}
/// Interpretet the difference and write tje diff-file to String.
......@@ -116,57 +114,39 @@ impl GeneratorWrite {
/// Until here, the content is prepared to be compared.
/// This code actually compares the code.
pub fn collect_output(mut self) -> Result<()> {
fn compare(span: Span, data: Vec<u8>, path: LitStr) -> Result<()> {
let data_str = String::from_utf8(data).map_err(|err| span.to_error(err))?;
let mut file = File::open(path.value()).map_err(|err| span.to_error(err))?;
let mut file_str = String::new();
file.read_to_string(&mut file_str)
.map_err(|err| span.to_error(err))?;
if file_str != data_str {
let Changeset { diffs, .. } = Changeset::new(&file_str, &data_str, "\n");
span.to_errs(GeneratorWrite::collect_difference(diffs))
} else {
Ok(())
}
}
if let Some((data, path)) = self.comparer {
self.output.append_errors(compare(self.span, data, path));
pub fn collect_output(self) -> Result<()> {
let lit = self.lit;
let (data, path) = match self.comparer {
Some(p) => p,
None => return Ok(()),
};
let file_str = fs::read_to_string(path.value()).map_err(|e| format_err!(lit, "{}", e))?;
if file_str.as_bytes() == &data[..] {
return Ok(());
}
self.output
let data_str = String::from_utf8(data).map_err(|e| format_err!(lit, "{}", e))?;
let Changeset { diffs, .. } = Changeset::new(&file_str, &data_str, "\n");
let diff = GeneratorWrite::collect_difference(diffs);
bail!(lit, "{}", diff)
}
}
/// Append data to the file or to the content to be compared
impl Write for GeneratorWrite {
fn write(&mut self, mut buf: &[u8]) -> IoResult<usize> {
let writeresult = self
.generator
.as_ref()
.map(|mut generator| generator.write(buf));
match writeresult {
Some(Ok(size)) => {
buf = &buf[0..size];
}
Some(Err(err)) => {
self.output.append_errors::<(), _>(self.span.to_errs(err));
self.generator = None;
}
None => {}
if let Some(generator) = &mut self.generator {
let amt = generator.write(buf)?;
buf = &buf[..amt];
}
if let Some((ref mut data, _)) = &mut self.comparer {
if let Some((data, _)) = &mut self.comparer {
data.extend(buf);
}
Ok(buf.len())
}
fn flush(&mut self) -> IoResult<()> {
if let Some(ref mut generator) = self.generator {
if let Err(err) = generator.flush() {
self.output.append_errors::<(), _>(self.span.to_errs(err));
}
if let Some(generator) = &mut self.generator {
generator.flush()?;
}
Ok(())
}
......@@ -176,7 +156,7 @@ impl Write for GeneratorWrite {
/// the final TokenStream
pub struct RustWriter {
output: Vec<Rc<TokenStream>>,
errors: Errors,
errors: Vec<Errors>,
generate_files: Vec<RustWriterEntry>,
compare_files: Vec<RustWriterEntry>,
}
......@@ -192,7 +172,6 @@ pub struct RustWriterFile<'parent> {
parent_output: Option<&'parent mut Vec<Rc<TokenStream>>>,
write_entry: Option<&'parent mut RustWriterEntry>,
compare_entry: Option<&'parent mut RustWriterEntry>,
errors: &'parent mut Errors,
}
impl RustWriter {
......@@ -200,7 +179,7 @@ impl RustWriter {
pub fn new() -> Self {
RustWriter {
output: Vec::new(),
errors: Errors::new(),
errors: Vec::new(),
compare_files: Vec::new(),
generate_files: Vec::new(),
}
......@@ -243,7 +222,8 @@ impl RustWriter {
Some(index)
} else {
self.errors.push(
path.to_error("File cannot be used for both compare and generate"),
format_err!(path, "File cannot be used for both compare and generate")
.into(),
);
None
}
......@@ -277,7 +257,8 @@ impl RustWriter {
Some(index)
} else {
self.errors.push(
path.to_error("File cannot be used for both compare and generate"),
format_err!(path, "File cannot be used for both compare and generate")
.into(),
);
None
}
......@@ -295,62 +276,75 @@ impl RustWriter {
} else {
Some(&mut self.output)
},
errors: &mut self.errors,
}
}
/// Apply rustfmt on a TokenStream
fn format_stream(span: Span, tokens: Vec<Rc<TokenStream>>) -> Result<Vec<u8>> {
fn format_stream(path: &LitStr, tokens: Vec<Rc<TokenStream>>) -> Result<Vec<u8>> {
let mut config: rustfmt::config::Config = Default::default();
let mut out: Vec<u8> = vec![];
config.set().write_mode(rustfmt::config::WriteMode::Plain);
config.set().error_on_line_overflow(false);
let stream: String = tokens.iter().map(|token| token.to_string()).collect();
rustfmt::format_input(rustfmt::Input::Text(stream), &config, Some(&mut out))
.map_err(|e| span.to_errors(e.0))?;
.map_err(|e| format_err!(path, "{}", e.0))?;
Ok(out)
}
/// Generate a file from the TokenStream
fn generate(path: LitStr, tokens: Vec<Rc<TokenStream>>) -> Result<()> {
let mut file = File::create(path.value()).map_err(|err| path.to_errors(err))?;
let data = Self::format_stream(path.span(), tokens)?;
file.write_all(&data).map_err(|err| path.to_errors(err))?;
let data = Self::format_stream(&path, tokens)?;
fs::write(path.value(), data).map_err(|e| format_err!(path, "{}", e))?;
Ok(())
}
/// Compare with a file from the TokenStream
fn compare(path: LitStr, tokens: Vec<Rc<TokenStream>>) -> Result<()> {
let gen_data = Self::format_stream(path.span(), tokens)?;
let gen_data = Self::format_stream(&path, tokens)?;
let comparer = GeneratorWrite {
generator: None,
span: path.span(),
lit: path.clone(),
comparer: Some((gen_data, path)),
output: Ok(()),
};
comparer.collect_output()
}
fn compare_all(entries: Vec<RustWriterEntry>) -> Result<()> {
entries.into_iter().fold(Ok(()), |mut prev, cur| {
prev.append_errors(Self::compare(cur.span, cur.content));
prev
})
let mut errors = Vec::new();
for entry in entries {
if let Err(e) = Self::compare(entry.span, entry.content) {
errors.push(e);
}
}
if errors.len() == 0 {
Ok(())
} else {
Err(errors.into())
}
}
fn generate_all(entries: Vec<RustWriterEntry>) -> Result<()> {
entries.into_iter().fold(Ok(()), |mut prev, cur| {
prev.append_errors(Self::generate(cur.span, cur.content));
prev
})
let mut errors = Vec::new();
for entry in entries {
if let Err(e) = Self::generate(entry.span, entry.content) {
errors.push(e);
}
}
if errors.len() == 0 {
Ok(())
} else {
Err(errors.into())
}
}
/// Create a tokenstream of an error
fn into_tokenstream_inner(self) -> Result<TokenStream> {
// Check if there are earlier errors. If so, do not write and add the errors to the token
// stream
self.errors.ok_if_empty()?;
if self.errors.len() > 0 {
return Err(self.errors.into());
}
// Then, compare all the files. Do not generate anything if one compare fails
Self::compare_all(self.compare_files)?;
Self::generate_all(self.generate_files)?;
......@@ -380,15 +374,4 @@ impl<'parent> RustWriterFile<'parent> {
entry.content.push(rc.clone());