From 629375a98151f8b053be2e7939a78ea554858c92 Mon Sep 17 00:00:00 2001 From: PgBiel <9021226+PgBiel@users.noreply.github.com> Date: Mon, 15 Jul 2024 12:51:03 -0300 Subject: [PATCH] move around some sink functions --- crates/typst/src/diag.rs | 115 ++++++++++++++++++++++++++++++++++- crates/typst/src/engine.rs | 121 ++----------------------------------- crates/typst/src/lib.rs | 15 +---- 3 files changed, 122 insertions(+), 129 deletions(-) diff --git a/crates/typst/src/diag.rs b/crates/typst/src/diag.rs index 314efa1de..52f1c411c 100644 --- a/crates/typst/src/diag.rs +++ b/crates/typst/src/diag.rs @@ -1,5 +1,6 @@ //! Diagnostics. +use std::collections::HashSet; use std::fmt::{self, Display, Formatter}; use std::io; use std::path::{Path, PathBuf}; @@ -10,7 +11,7 @@ use comemo::Tracked; use ecow::{eco_vec, EcoVec}; use crate::syntax::package::{PackageSpec, PackageVersion}; -use crate::syntax::{Span, Spanned, SyntaxError}; +use crate::syntax::{ast, Span, Spanned, SyntaxError}; use crate::{World, WorldExt}; /// Early-return with a [`StrResult`] or [`SourceResult`]. @@ -476,6 +477,118 @@ impl Hint for HintedStrResult { } } +/// Deduplicate errors based on their spans and messages. +pub fn deduplicate_errors( + mut errors: EcoVec, +) -> EcoVec { + let mut unique = HashSet::new(); + errors.retain(|error| { + debug_assert!(error.severity == Severity::Error); + let hash = crate::utils::hash128(&(&error.span, &error.message)); + unique.insert(hash) + }); + errors +} + +/// Apply warning suppression, deduplication, and return the remaining +/// warnings. +/// +/// We deduplicate warnings which are identical modulo tracepoints. +/// This is so we can attempt to suppress each tracepoint separately, +/// without having one suppression discard all other attempts of raising +/// this same warning through a different set of tracepoints. +/// +/// For example, calling the same function twice but suppressing a warning +/// on the first call shouldn't suppress on the second, so each set of +/// tracepoints for a particular warning matters. If at least one instance +/// of a warning isn't suppressed, the warning will be returned, and the +/// remaining duplicates are discarded. +pub fn deduplicate_and_suppress_warnings( + warnings: &mut EcoVec, + world: &dyn World, +) { + let mut unsuppressed = HashSet::::default(); + + // Only retain warnings which weren't locally suppressed where they + // were emitted or at any of their tracepoints. + warnings.retain(|diag| { + debug_assert!(diag.severity == Severity::Warning); + let hash = crate::utils::hash128(&(&diag.span, &diag.identifier, &diag.message)); + if unsuppressed.contains(&hash) { + // This warning - with the same span, identifier and message - + // was already raised and not suppressed before, with a + // different set of tracepoints. Therefore, we should not raise + // it again, and checking for suppression is unnecessary. + return false; + } + + let Some(identifier) = &diag.identifier else { + // Can't suppress without an identifier. Therefore, retain the + // warning. It is not a duplicate due to the check above. + unsuppressed.insert(hash); + return true; + }; + + let should_raise = !is_warning_suppressed(diag.span, world, identifier) + && !diag.trace.iter().any(|tracepoint| { + is_warning_suppressed(tracepoint.span, world, identifier) + }); + + // If this warning wasn't suppressed, any further duplicates (with + // different tracepoints) should be removed. + should_raise && unsuppressed.insert(hash) + }); +} + +/// Checks if a given warning is suppressed given one span it has a tracepoint +/// in. If one of the ancestors of the node where the warning occurred has a +/// warning suppression decorator sibling right before it suppressing this +/// particular warning, the warning is considered suppressed. +fn is_warning_suppressed(span: Span, world: &dyn World, identifier: &Identifier) -> bool { + // Don't suppress detached warnings. + let Some(source) = span.id().and_then(|file| world.source(file).ok()) else { + return false; + }; + + let search_root = source.find(span); + let mut searched_node = search_root.as_ref(); + + // Walk the parent nodes to check for a warning suppression in the + // previous line. + while let Some(node) = searched_node { + let mut searched_decorator = node.prev_attached_decorator(); + while let Some(sibling) = searched_decorator { + let decorator = sibling.cast::().unwrap(); + if check_decorator_suppresses_warning(decorator, identifier) { + return true; + } + searched_decorator = sibling.prev_attached_decorator(); + } + searched_node = node.parent(); + } + + false +} + +/// Checks if an 'allow' decorator would cause a warning with a particular +/// identifier to be suppressed. +fn check_decorator_suppresses_warning( + decorator: ast::Decorator, + warning: &Identifier, +) -> bool { + if decorator.name().as_str() != "allow" { + return false; + } + + for argument in decorator.arguments() { + if warning.name() == argument.get() { + return true; + } + } + + false +} + /// A result type with a file-related error. pub type FileResult = Result; diff --git a/crates/typst/src/engine.rs b/crates/typst/src/engine.rs index b5cbe899d..d58a5ac5c 100644 --- a/crates/typst/src/engine.rs +++ b/crates/typst/src/engine.rs @@ -10,7 +10,7 @@ use rayon::iter::{IndexedParallelIterator, IntoParallelIterator, ParallelIterato use crate::diag::{self, SourceDiagnostic, SourceResult, Trace, Tracepoint}; use crate::foundations::{Styles, Value}; use crate::introspection::Introspector; -use crate::syntax::{ast, FileId, Span}; +use crate::syntax::{FileId, Span}; use crate::World; /// Holds all data needed during compilation. @@ -115,9 +115,9 @@ impl Engine<'_> { route: route.clone(), }; - // Trace errors immediately, followed by warnings on the sink. + // Trace errors and warnings on the sink immediately. let call_result = f(&mut engine).trace(world, make_point, span); - sink.trace_warnings(world, make_point, span); + sink.warnings = std::mem::take(&mut sink.warnings).trace(world, make_point, span); // Push the accumulated warnings and other fields back to the // original sink after we have modified them. This is needed so the @@ -208,69 +208,11 @@ impl Sink { self.values } - /// Apply warning suppression, deduplication, and return the remaining - /// warnings. - /// - /// We deduplicate warnings which are identical modulo tracepoints. - /// This is so we can attempt to suppress each tracepoint separately, - /// without having one suppression discard all other attempts of raising - /// this same warning through a different set of tracepoints. - /// - /// For example, calling the same function twice but suppressing a warning - /// on the first call shouldn't suppress on the second, so each set of - /// tracepoints for a particular warning matters. If at least one instance - /// of a warning isn't suppressed, the warning will be returned, and the - /// remaining duplicates are discarded. - pub fn suppress_and_deduplicate_warnings( - mut self, - world: &dyn World, - ) -> EcoVec { - let mut unsuppressed_warning_set = HashSet::::default(); - - // Only retain warnings which weren't locally suppressed where they - // were emitted or at any of their tracepoints. - self.warnings.retain(|diag| { - let hash = - crate::utils::hash128(&(&diag.span, &diag.identifier, &diag.message)); - if unsuppressed_warning_set.contains(&hash) { - // This warning - with the same span, identifier and message - - // was already raised and not suppressed before, with a - // different set of tracepoints. Therefore, we should not raise - // it again, and checking for suppression is unnecessary. - return false; - } - - let Some(identifier) = &diag.identifier else { - // Can't suppress without an identifier. Therefore, retain the - // warning. It is not a duplicate due to the check above. - unsuppressed_warning_set.insert(hash); - return true; - }; - - let should_raise = !is_warning_suppressed(diag.span, world, identifier) - && !diag.trace.iter().any(|tracepoint| { - is_warning_suppressed(tracepoint.span, world, identifier) - }); - - // If this warning wasn't suppressed, any further duplicates (with - // different tracepoints) should be removed. - should_raise && unsuppressed_warning_set.insert(hash) - }); - + /// Deduplicates and suppresses the stored warnings before returning them. + pub fn finish_warnings(mut self, world: &dyn World) -> EcoVec { + diag::deduplicate_and_suppress_warnings(&mut self.warnings, world); self.warnings } - - /// Adds a tracepoint to all warnings outside the given span. - pub fn trace_warnings( - &mut self, - world: Tracked, - make_point: F, - span: Span, - ) where - F: Fn() -> Tracepoint, - { - self.warnings = std::mem::take(&mut self.warnings).trace(world, make_point, span); - } } #[comemo::track] @@ -329,57 +271,6 @@ impl Sink { } } -/// Checks if a given warning is suppressed given one span it has a tracepoint -/// in. If one of the ancestors of the node where the warning occurred has a -/// warning suppression decorator sibling right before it suppressing this -/// particular warning, the warning is considered suppressed. -fn is_warning_suppressed( - span: Span, - world: &dyn World, - identifier: &diag::Identifier, -) -> bool { - // Don't suppress detached warnings. - let Some(source) = span.id().and_then(|file| world.source(file).ok()) else { - return false; - }; - - let search_root = source.find(span); - let mut searched_node = search_root.as_ref(); - - // Walk the parent nodes to check for a warning suppression in the - // previous line. - while let Some(node) = searched_node { - let mut searched_decorator = node.prev_attached_decorator(); - while let Some(sibling) = searched_decorator { - let decorator = sibling.cast::().unwrap(); - if check_decorator_suppresses_warning(decorator, identifier) { - return true; - } - searched_decorator = sibling.prev_attached_decorator(); - } - searched_node = node.parent(); - } - - false -} - -fn check_decorator_suppresses_warning( - decorator: ast::Decorator, - warning: &diag::Identifier, -) -> bool { - if decorator.name().as_str() != "allow" { - return false; - } - - for argument in decorator.arguments() { - if warning.name() == argument.get() { - return true; - } - } - - false -} - /// The route the engine took during compilation. This is used to detect /// cyclic imports and excessive nesting. pub struct Route<'a> { diff --git a/crates/typst/src/lib.rs b/crates/typst/src/lib.rs index f3f2a28f8..67bf3239d 100644 --- a/crates/typst/src/lib.rs +++ b/crates/typst/src/lib.rs @@ -56,7 +56,6 @@ pub use typst_syntax as syntax; #[doc(inline)] pub use typst_utils as utils; -use std::collections::HashSet; use std::ops::{Deref, Range}; use comemo::{Track, Tracked, Validate}; @@ -87,9 +86,9 @@ use crate::visualize::Color; pub fn compile(world: &dyn World) -> Warned> { let mut sink = Sink::new(); let output = compile_inner(world.track(), Traced::default().track(), &mut sink) - .map_err(deduplicate); + .map_err(diag::deduplicate_errors); - let warnings = sink.suppress_and_deduplicate_warnings(world); + let warnings = sink.finish_warnings(world); Warned { output, warnings } } @@ -178,16 +177,6 @@ fn compile_inner( Ok(document) } -/// Deduplicate diagnostics. -fn deduplicate(mut diags: EcoVec) -> EcoVec { - let mut unique = HashSet::new(); - diags.retain(|diag| { - let hash = crate::utils::hash128(&(&diag.span, &diag.message)); - unique.insert(hash) - }); - diags -} - /// The environment in which typesetting occurs. /// /// All loading functions (`main`, `source`, `file`, `font`) should perform