From 2e14a0ecc58f691837ae979ba7bf5f154547415a Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Thu, 12 May 2022 12:55:00 -0700 Subject: [PATCH] ISLE: provide locations in errors in basic non-miette mode. (#4151) In #4143 we made ISLE compilation part of the normal build flow again, to avoid the issues with the checked-in source. To make this acceptably fast, we cut down dependencies of the ISLE compiler, so the "fancy" error printing is now optional. When not included, it just prints error messages to stderr in a list. However, this did not include file locations. It might be nice to have this without enabling the "fancy printing" and waiting for that to build. Fortunately most of the plumbing for this was already present (we had it at one point before switching to miette). This PR adds back locations to the basic error output. It now looks like: ``` Error building ISLE files: ISLE errors: src/isa/aarch64/inst.isle:1:1: parse error: Unexpected token Symbol("asdf") ``` --- cranelift/isle/isle/src/error.rs | 44 ++++++++++++++++++++++--- cranelift/isle/isle/src/error_miette.rs | 2 +- cranelift/isle/isle/src/lexer.rs | 9 +++-- cranelift/isle/isle/src/parser.rs | 2 +- cranelift/isle/isle/src/sema.rs | 2 +- 5 files changed, 49 insertions(+), 10 deletions(-) diff --git a/cranelift/isle/isle/src/error.rs b/cranelift/isle/isle/src/error.rs index 89c0ae4341..39b6adcf87 100644 --- a/cranelift/isle/isle/src/error.rs +++ b/cranelift/isle/isle/src/error.rs @@ -2,6 +2,8 @@ use std::sync::Arc; +use crate::lexer::Pos; + /// Either `Ok(T)` or `Err(isle::Error)`. pub type Result = std::result::Result; @@ -82,8 +84,30 @@ impl std::fmt::Display for Error { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { match self { Error::IoError { context, .. } => write!(f, "{}", context), + + // Include locations directly in the `Display` output when + // we're not wrapping errors with miette (which provides + // its own way of showing locations and context). + #[cfg(not(feature = "miette-errors"))] + Error::ParseError { src, span, msg, .. } => write!( + f, + "{}: parse error: {}", + span.from.pretty_print_with_filename(&*src.name), + msg + ), + #[cfg(not(feature = "miette-errors"))] + Error::TypeError { src, span, msg, .. } => write!( + f, + "{}: type error: {}", + span.from.pretty_print_with_filename(&*src.name), + msg + ), + + #[cfg(feature = "miette-errors")] Error::ParseError { msg, .. } => write!(f, "parse error: {}", msg), + #[cfg(feature = "miette-errors")] Error::TypeError { msg, .. } => write!(f, "type error: {}", msg), + Error::Errors(_) => write!( f, "found {} errors:\n\n{}", @@ -143,17 +167,27 @@ impl Source { #[derive(Clone, Debug)] pub struct Span { /// The byte offset of the start of the span. - pub from: usize, + pub from: Pos, /// The byte offset of the end of the span. - pub to: usize, + pub to: Pos, } impl Span { /// Create a new span that covers one character at the given offset. - pub fn new_single(offset: usize) -> Span { + pub fn new_single(pos: Pos) -> Span { Span { - from: offset, - to: offset + 1, + from: pos, + // This is a slight hack (we don't actually look at the + // file to find line/col of next char); but the span + // aspect, vs. just the starting point, is only relevant + // for miette and when miette is enabled we use only the + // `offset` here to provide its SourceSpans. + to: Pos { + file: pos.file, + offset: pos.offset + 1, + line: pos.line, + col: pos.col + 1, + }, } } } diff --git a/cranelift/isle/isle/src/error_miette.rs b/cranelift/isle/isle/src/error_miette.rs index 4050b065ad..555b4e4aca 100644 --- a/cranelift/isle/isle/src/error_miette.rs +++ b/cranelift/isle/isle/src/error_miette.rs @@ -8,7 +8,7 @@ use miette::{SourceCode, SourceSpan}; impl From for SourceSpan { fn from(span: Span) -> Self { - SourceSpan::new(span.from.into(), span.to.into()) + SourceSpan::new(span.from.offset.into(), span.to.offset.into()) } } diff --git a/cranelift/isle/isle/src/lexer.rs b/cranelift/isle/isle/src/lexer.rs index 996356c568..9a8a07e880 100644 --- a/cranelift/isle/isle/src/lexer.rs +++ b/cranelift/isle/isle/src/lexer.rs @@ -45,12 +45,17 @@ pub struct Pos { impl Pos { /// Print this source position as `file.isle:12:34`. pub fn pretty_print(&self, filenames: &[Arc]) -> String { - format!("{}:{}:{}", filenames[self.file], self.line, self.col) + self.pretty_print_with_filename(&filenames[self.file]) } /// Print this source position as `file.isle line 12`. pub fn pretty_print_line(&self, filenames: &[Arc]) -> String { format!("{} line {}", filenames[self.file], self.line) } + /// As above for `pretty_print`, but with the specific filename + /// already provided. + pub fn pretty_print_with_filename(&self, filename: &str) -> String { + format!("{}:{}:{}", filename, self.line, self.col) + } } /// A token of ISLE source. @@ -167,7 +172,7 @@ impl<'a> Lexer<'a> { self.filenames[pos.file].clone(), self.file_texts[pos.file].clone(), ), - span: Span::new_single(self.pos().offset), + span: Span::new_single(self.pos()), } } diff --git a/cranelift/isle/isle/src/parser.rs b/cranelift/isle/isle/src/parser.rs index 1427cdb2c4..c6b518f00a 100644 --- a/cranelift/isle/isle/src/parser.rs +++ b/cranelift/isle/isle/src/parser.rs @@ -39,7 +39,7 @@ impl<'a> Parser<'a> { self.lexer.filenames[pos.file].clone(), self.lexer.file_texts[pos.file].clone(), ), - span: Span::new_single(pos.offset), + span: Span::new_single(pos), } } diff --git a/cranelift/isle/isle/src/sema.rs b/cranelift/isle/isle/src/sema.rs index 07c861e3b5..db075d25f3 100644 --- a/cranelift/isle/isle/src/sema.rs +++ b/cranelift/isle/isle/src/sema.rs @@ -727,7 +727,7 @@ impl TypeEnv { self.filenames[pos.file].clone(), self.file_texts[pos.file].clone(), ), - span: Span::new_single(pos.offset), + span: Span::new_single(pos), }; log!("{}", e); e