From 2e03fb34cb9efd3f287b9658a8d84df52ad660dd Mon Sep 17 00:00:00 2001 From: Mathias Fischler Date: Sat, 24 Jun 2023 14:18:21 +0200 Subject: [PATCH] Add infrastructure to enrich errors with hints (#1486) --- cli/src/main.rs | 9 +- library/src/meta/reference.rs | 9 +- src/diag.rs | 55 ++++++++++ tests/src/tests.rs | 196 ++++++++++++++++++++++++---------- tests/typ/compiler/hint.typ | 10 +- 5 files changed, 216 insertions(+), 63 deletions(-) diff --git a/cli/src/main.rs b/cli/src/main.rs index f8affa0dd..af509a92c 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -23,7 +23,7 @@ use std::cell::OnceCell; use termcolor::{ColorChoice, StandardStream, WriteColor}; use typst::diag::{bail, FileError, FileResult, SourceError, StrResult}; use typst::doc::Document; -use typst::eval::{Datetime, Library}; +use typst::eval::{eco_format, Datetime, Library}; use typst::font::{Font, FontBook, FontInfo, FontVariant}; use typst::geom::Color; use typst::syntax::{Source, SourceId}; @@ -436,6 +436,13 @@ fn print_diagnostics( let range = error.range(world); let diag = Diagnostic::error() .with_message(error.message) + .with_notes( + error + .hints + .iter() + .map(|e| (eco_format!("hint: {e}")).into()) + .collect(), + ) .with_labels(vec![Label::primary(error.span.source(), range)]); term::emit(&mut w, &config, world, &diag)?; diff --git a/library/src/meta/reference.rs b/library/src/meta/reference.rs index 96358ffa8..7f21a3ce8 100644 --- a/library/src/meta/reference.rs +++ b/library/src/meta/reference.rs @@ -1,3 +1,5 @@ +use typst::diag::Hint; + use super::{BibliographyElem, CiteElem, Counter, Figurable, Numbering}; use crate::prelude::*; use crate::text::TextElem; @@ -178,11 +180,14 @@ impl Show for RefElem { .numbering() .ok_or_else(|| { eco_format!( - "cannot reference {0} without numbering \ - - did you mean to use `#set {0}(numbering: \"1.\")`?", + "cannot reference {} without numbering", elem.func().name() ) }) + .hint(eco_format!( + "did you mean to use `#set {}(numbering: \"1.\")`?", + elem.func().name() + )) .at(span)?; let numbers = refable diff --git a/src/diag.rs b/src/diag.rs index 4fb9ecc06..0f669f64b 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -82,6 +82,8 @@ pub struct SourceError { pub message: EcoString, /// The trace of function calls leading to the error. pub trace: Vec>, + /// Additonal hints to the user, indicating how this error could be avoided or worked around. + pub hints: Vec, } impl SourceError { @@ -92,6 +94,7 @@ impl SourceError { pos: ErrorPos::Full, trace: vec![], message: message.into(), + hints: vec![], } } @@ -112,6 +115,12 @@ impl SourceError { ErrorPos::End => full.end..full.end, } } + + /// Adds a user-facing hint to the error. + pub fn with_hints(mut self, hints: &mut Vec) -> Self { + self.hints.append(hints); + self + } } /// A part of an error's [trace](SourceError::trace). @@ -194,6 +203,52 @@ where } } +pub type StrWithHintResult = Result; + +pub struct StrWithHint { + message: EcoString, + hints: Vec, +} + +impl At for Result { + fn at(self, span: Span) -> SourceResult { + self.map_err(|mut diags| { + Box::new(vec![ + SourceError::new(span, diags.message).with_hints(&mut diags.hints) + ]) + }) + } +} + +/// Allows adding a user-facing hint in addition to the error. +pub trait Hint +where + S: Into, +{ + fn hint(self, hint: S) -> StrWithHintResult; +} + +impl Hint for StrResult +where + S: Into, +{ + fn hint(self, hint: S) -> StrWithHintResult { + self.map_err(|message| StrWithHint { message, hints: vec![hint.into()] }) + } +} + +impl Hint for StrWithHintResult +where + S: Into, +{ + fn hint(self, hint: S) -> StrWithHintResult { + self.map_err(|mut diags| { + diags.hints.push(hint.into()); + diags + }) + } +} + /// A result type with a file-related error. pub type FileResult = Result; diff --git a/tests/src/tests.rs b/tests/src/tests.rs index 34070e9c9..ee87f3edd 100644 --- a/tests/src/tests.rs +++ b/tests/src/tests.rs @@ -1,14 +1,14 @@ #![allow(clippy::comparison_chain)] use std::cell::{RefCell, RefMut}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::ffi::OsStr; use std::fmt::Write as FmtWrite; -use std::fs; use std::io::Write; use std::ops::Range; use std::path::{Path, PathBuf}; use std::{env, io}; +use std::{fs, iter}; use clap::Parser; use comemo::{Prehashed, Track}; @@ -22,7 +22,7 @@ use walkdir::WalkDir; use typst::diag::{bail, FileError, FileResult, StrResult}; use typst::doc::{Document, Frame, FrameItem, Meta}; -use typst::eval::{func, Datetime, Library, NoneValue, Value}; +use typst::eval::{eco_format, func, Datetime, Library, NoneValue, Value}; use typst::font::{Font, FontBook}; use typst::geom::{Abs, Color, RgbaColor, Smart}; use typst::syntax::{Source, SourceId, Span, SyntaxNode}; @@ -372,7 +372,8 @@ fn test( let mut updated = false; let mut frames = vec![]; let mut line = 0; - let mut compare_ref = true; + let mut compare_ref = None; + let mut validate_hints = None; let mut compare_ever = false; let mut rng = LinearShift::new(); @@ -400,9 +401,8 @@ fn test( if is_header { for line in part.lines() { - if line.starts_with("// Ref: false") { - compare_ref = false; - } + compare_ref = get_flag_metadata(line, "Ref").or(compare_ref); + validate_hints = get_flag_metadata(line, "Hints").or(validate_hints); } } else { let (part_ok, compare_here, part_frames) = test_part( @@ -411,7 +411,8 @@ fn test( src_path, part.into(), i, - compare_ref, + compare_ref.unwrap_or(true), + validate_hints.unwrap_or(true), line, &mut rng, ); @@ -489,6 +490,14 @@ fn test( ok } +fn get_metadata<'a>(line: &'a str, key: &str) -> Option<&'a str> { + line.strip_prefix(eco_format!("// {key}: ").as_str()) +} + +fn get_flag_metadata(line: &str, key: &str) -> Option { + get_metadata(line, key).map(|value| value == "true") +} + fn update_image(png_path: &Path, ref_path: &Path) { oxipng::optimize( &InFile::Path(png_path.to_owned()), @@ -506,6 +515,7 @@ fn test_part( text: String, i: usize, compare_ref: bool, + validate_hints: bool, line: usize, rng: &mut LinearShift, ) -> (bool, bool, Vec) { @@ -517,8 +527,10 @@ fn test_part( writeln!(output, "Syntax Tree:\n{:#?}\n", source.root()).unwrap(); } - let (local_compare_ref, mut ref_errors) = parse_metadata(source); - let compare_ref = local_compare_ref.unwrap_or(compare_ref); + let metadata = parse_part_metadata(source); + let compare_ref = metadata.part_configuration.compare_ref.unwrap_or(compare_ref); + let validate_hints = + metadata.part_configuration.validate_hints.unwrap_or(validate_hints); ok &= test_spans(output, source.root()); ok &= test_reparse(output, world.source(id).text(), i, rng); @@ -543,92 +555,158 @@ fn test_part( } // Map errors to range and message format, discard traces and errors from - // other files. - let mut errors: Vec<_> = errors + // other files. Collect hints. + // + // This has one caveat: due to the format of the expected hints, we can not verify if a hint belongs + // to a error or not. That should be irrelevant however, as the line of the hint is still verified. + let actual_errors_and_hints: HashSet = errors .into_iter() .filter(|error| error.span.source() == id) - .map(|error| (error.range(world), error.message.replace('\\', "/"))) + .flat_map(|error| { + let output_error = + UserOutput::Error(error.range(world), error.message.replace('\\', "/")); + let hints = error + .hints + .iter() + .filter(|_| validate_hints) // No unexpected hints should be verified if disabled. + .map(|hint| UserOutput::Hint(error.range(world), hint.to_string())); + iter::once(output_error).chain(hints).collect::>() + }) .collect(); - errors.sort_by_key(|error| error.0.start); - ref_errors.sort_by_key(|error| error.0.start); + // Basically symmetric_difference, but we need to know where an item is coming from. + let mut unexpected_outputs = actual_errors_and_hints + .difference(&metadata.invariants) + .collect::>(); + let mut missing_outputs = metadata + .invariants + .difference(&actual_errors_and_hints) + .collect::>(); - if errors != ref_errors { + unexpected_outputs.sort_by_key(|&o| o.start()); + missing_outputs.sort_by_key(|&o| o.start()); + + // This prints all unexpected emits first, then all missing emits. + // Is this reasonable or subject to change? + if !(unexpected_outputs.is_empty() && missing_outputs.is_empty()) { writeln!(output, " Subtest {i} does not match expected errors.").unwrap(); ok = false; - let source = world.source(id); - for error in errors.iter() { - if !ref_errors.contains(error) { - write!(output, " Not annotated | ").unwrap(); - print_error(output, source, line, error); - } + for unexpected in unexpected_outputs { + write!(output, " Not annotated | ").unwrap(); + print_user_output(output, source, line, unexpected) } - for error in ref_errors.iter() { - if !errors.contains(error) { - write!(output, " Not emitted | ").unwrap(); - print_error(output, source, line, error); - } + for missing in missing_outputs { + write!(output, " Not emitted | ").unwrap(); + print_user_output(output, source, line, missing) } } (ok, compare_ref, frames) } -fn parse_metadata(source: &Source) -> (Option, Vec<(Range, String)>) { +fn print_user_output( + output: &mut String, + source: &Source, + line: usize, + user_output: &UserOutput, +) { + let (range, message) = match &user_output { + UserOutput::Error(r, m) => (r, m), + UserOutput::Hint(r, m) => (r, m), + }; + + let start_line = 1 + line + source.byte_to_line(range.start).unwrap(); + let start_col = 1 + source.byte_to_column(range.start).unwrap(); + let end_line = 1 + line + source.byte_to_line(range.end).unwrap(); + let end_col = 1 + source.byte_to_column(range.end).unwrap(); + let kind = match user_output { + UserOutput::Error(_, _) => "Error", + UserOutput::Hint(_, _) => "Hint", + }; + writeln!(output, "{kind}: {start_line}:{start_col}-{end_line}:{end_col}: {message}") + .unwrap(); +} + +struct TestConfiguration { + compare_ref: Option, + validate_hints: Option, +} + +struct TestPartMetadata { + part_configuration: TestConfiguration, + invariants: HashSet, +} + +#[derive(PartialEq, Eq, Debug, Hash)] +enum UserOutput { + Error(Range, String), + Hint(Range, String), +} + +impl UserOutput { + fn start(&self) -> usize { + match self { + UserOutput::Error(r, _) => r.start, + UserOutput::Hint(r, _) => r.start, + } + } + + fn error(range: Range, message: String) -> UserOutput { + UserOutput::Error(range, message) + } + + fn hint(range: Range, message: String) -> UserOutput { + UserOutput::Hint(range, message) + } +} + +fn parse_part_metadata(source: &Source) -> TestPartMetadata { let mut compare_ref = None; - let mut errors = vec![]; + let mut validate_hints = None; + let mut expectations = HashSet::default(); let lines: Vec<_> = source.text().lines().map(str::trim).collect(); for (i, line) in lines.iter().enumerate() { - if line.starts_with("// Ref: false") { - compare_ref = Some(false); - } - - if line.starts_with("// Ref: true") { - compare_ref = Some(true); - } + compare_ref = get_flag_metadata(line, "Ref").or(compare_ref); + validate_hints = get_flag_metadata(line, "Hints").or(validate_hints); fn num(s: &mut Scanner) -> usize { s.eat_while(char::is_numeric).parse().unwrap() } - let comments = + let comments_until_code = lines[i..].iter().take_while(|line| line.starts_with("//")).count(); let pos = |s: &mut Scanner| -> usize { let first = num(s) - 1; let (delta, column) = if s.eat_if(':') { (first, num(s) - 1) } else { (0, first) }; - let line = (i + comments) + delta; + let line = (i + comments_until_code) + delta; source.line_column_to_byte(line, column).unwrap() }; - let Some(rest) = line.strip_prefix("// Error: ") else { continue; }; - let mut s = Scanner::new(rest); - let start = pos(&mut s); - let end = if s.eat_if('-') { pos(&mut s) } else { start }; - let range = start..end; + let error_factory: fn(Range, String) -> UserOutput = UserOutput::error; + let hint_factory: fn(Range, String) -> UserOutput = UserOutput::hint; - errors.push((range, s.after().trim().to_string())); + let error_metadata = get_metadata(line, "Error").map(|s| (s, error_factory)); + let get_hint_metadata = || get_metadata(line, "Hint").map(|s| (s, hint_factory)); + + if let Some((expectation, factory)) = error_metadata.or_else(get_hint_metadata) { + let mut s = Scanner::new(expectation); + let start = pos(&mut s); + let end = if s.eat_if('-') { pos(&mut s) } else { start }; + let range = start..end; + + expectations.insert(factory(range, s.after().trim().to_string())); + }; } - (compare_ref, errors) -} - -fn print_error( - output: &mut String, - source: &Source, - line: usize, - (range, message): &(Range, String), -) { - let start_line = 1 + line + source.byte_to_line(range.start).unwrap(); - let start_col = 1 + source.byte_to_column(range.start).unwrap(); - let end_line = 1 + line + source.byte_to_line(range.end).unwrap(); - let end_col = 1 + source.byte_to_column(range.end).unwrap(); - writeln!(output, "Error: {start_line}:{start_col}-{end_line}:{end_col}: {message}") - .unwrap(); + TestPartMetadata { + part_configuration: TestConfiguration { compare_ref, validate_hints }, + invariants: expectations, + } } /// Pseudorandomly edit the source file and test whether a reparse produces the diff --git a/tests/typ/compiler/hint.typ b/tests/typ/compiler/hint.typ index 981c954f3..19d233d02 100644 --- a/tests/typ/compiler/hint.typ +++ b/tests/typ/compiler/hint.typ @@ -23,5 +23,13 @@ --- = Heading -// Error: 1:20-1:26 cannot reference heading without numbering - did you mean to use `#set heading(numbering: "1.")`? +// Error: 1:20-1:26 cannot reference heading without numbering +// Hint: 1:20-1:26 did you mean to use `#set heading(numbering: "1.")`? +Can not be used as @intro + +--- +// Hints: false +// This test is more of a tooling test. It checks if hint annotation validation can be turned off. += Heading +// Error: 1:20-1:26 cannot reference heading without numbering Can not be used as @intro